Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Stored call in multisig #6319

Merged
gavofyork merged 31 commits into from
Jun 17, 2020
Merged

Stored call in multisig #6319

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c7584c0
Stored call in multisig
gavofyork Jun 10, 2020
0ddd158
Docs.
gavofyork Jun 10, 2020
c30f54f
Benchmarks.
gavofyork Jun 10, 2020
dbac9eb
Fix
gavofyork Jun 10, 2020
3669489
Update frame/multisig/src/lib.rs
gavofyork Jun 10, 2020
9f59d31
patch benchmarks
shawntabrizi Jun 11, 2020
69682e5
Minor grumbles.
gavofyork Jun 11, 2020
e48f3e4
Merge branch 'gav-multisig-stored-call' of github.com:paritytech/subs…
gavofyork Jun 11, 2020
a363d4b
Update as_multi weight
shawntabrizi Jun 11, 2020
1c69bb0
Merge branch 'gav-multisig-stored-call' of https://github.com/parityt…
shawntabrizi Jun 11, 2020
a8343cb
Fixes and refactoring.
gavofyork Jun 11, 2020
fdcab27
Merge branch 'gav-multisig-stored-call' of github.com:paritytech/subs…
gavofyork Jun 11, 2020
bd5f05e
Split out threshold=1 and opaquify Call.
gavofyork Jun 11, 2020
8b9aaae
Compiles, tests pass, weights are broken
shawntabrizi Jun 14, 2020
09da872
Update benchmarks, add working tests
shawntabrizi Jun 14, 2020
2b5c361
Add benchmark to threshold 1, add event too
shawntabrizi Jun 14, 2020
2cf2832
suppress warning for now
shawntabrizi Jun 14, 2020
9a2d25f
@xlc improvment nit
shawntabrizi Jun 14, 2020
db75543
Update weight and tests
shawntabrizi Jun 15, 2020
6275b9c
Test for weight check
shawntabrizi Jun 15, 2020
5c9a7af
Merge branch 'master' into gav-multisig-stored-call
shawntabrizi Jun 15, 2020
81195da
Fix line width
shawntabrizi Jun 15, 2020
7a75cf7
one more line width error
shawntabrizi Jun 15, 2020
9dd0585
Apply suggestions from code review
shawntabrizi Jun 15, 2020
7af9a83
Merge branch 'master' into gav-multisig-stored-call
shawntabrizi Jun 15, 2020
f64e26d
fix merge
shawntabrizi Jun 15, 2020
9becf6e
more @apopiak feedback
shawntabrizi Jun 15, 2020
5bd6514
Multisig handles no preimage
shawntabrizi Jun 16, 2020
5cc17da
Optimize return weight after dispatch
shawntabrizi Jun 16, 2020
a053de8
Merge remote-tracking branch 'origin/master' into gav-multisig-stored…
gavofyork Jun 16, 2020
9e2d5b6
Error on failed deposit.
gavofyork Jun 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Stored call in multisig
  • Loading branch information
@gavofyork
gavofyork committed Jun 10, 2020
commit c7584c064f0c02b0a06a267a4b04b1e669fe8f51
46 changes: 44 additions & 2 deletions frame/multisig/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ decl_storage! {
pub Multisigs: double_map
hasher(twox_64_concat) T::AccountId, hasher(blake2_128_concat) [u8; 32]
=> Option<Multisig<T::BlockNumber, BalanceOf<T>, T::AccountId>>;

pub Calls: map hasher(identity) [u8; 32] => Option<(Vec<u8>, T::AccountId, BalanceOf<T>)>;
Copy link
Contributor

@apopiak apopiak Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe add descriptive type defs for [u8; 32] (CallHash?) and Vec<u8> (EncodedCall) ?

}
}

Expand Down Expand Up @@ -278,6 +280,7 @@ decl_module! {
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<T::BlockNumber>>,
call: Box<<T as Trait>::Call>,
store_call: bool,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;
// We're now executing as a freshly authenticated new account, so the previous call
Expand All @@ -292,7 +295,12 @@ decl_module! {
let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?;

let id = Self::multi_account_id(&signatories, threshold);
let call_hash = call.using_encoded(blake2_256);

let encoded_call = call.encode();
let call_hash = blake2_256(&encoded_call);
if store_call {
Self::store_call(who.clone(), &call_hash, encoded_call)?;
}

if let Some(mut m) = <Multisigs<T>>::get(&id, call_hash) {
let timepoint = maybe_timepoint.ok_or(Error::<T>::NoTimepoint)?;
Expand All @@ -315,6 +323,7 @@ decl_module! {
let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into());
let _ = T::Currency::unreserve(&m.depositor, m.deposit);
<Multisigs<T>>::remove(&id, call_hash);
Self::take_call(&call_hash);
Self::deposit_event(RawEvent::MultisigExecuted(
who, timepoint, id, call_hash, result.map(|_| ()).map_err(|e| e.error)
));
Expand Down Expand Up @@ -425,6 +434,17 @@ decl_module! {
ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
ensure!(m.approvals.len() < threshold as usize, Error::<T>::NoApprovalsNeeded);
if let Err(pos) = m.approvals.binary_search(&who) {
if m.approvals.len() + 1 == threshold as usize {
if let Some(call) = Self::take_call(&call_hash) {
Copy link
Contributor

@apopiak apopiak Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should change the weight calculation of the dispatchable.
Suggestion: Introduce a weight_of::take_call function (and corresponding weight_of::store_call).

let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into());
Copy link
Contributor

@apopiak apopiak Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also should change the weight. In my estimation you are running into the same issues I had in collective which also has stored calls that are (potentially) executed later.

Copy link
Contributor

@apopiak apopiak Jun 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See weight_bound param here and the validate_and_get_proposal function here.
We might want to build some utilities around this use case of deferred calls so it's not as much effort to make it secure weight-wise every time.

let _ = T::Currency::unreserve(&m.depositor, m.deposit);
<Multisigs<T>>::remove(&id, call_hash);
Self::deposit_event(RawEvent::MultisigExecuted(
who, timepoint, id, call_hash, result.map(|_| ()).map_err(|e| e.error)
));
return Ok(());
}
}
m.approvals.insert(pos, who.clone());
<Multisigs<T>>::insert(&id, call_hash, m);
Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash));
Expand Down Expand Up @@ -506,7 +526,8 @@ decl_module! {
ensure!(m.depositor == who, Error::<T>::NotOwner);

let _ = T::Currency::unreserve(&m.depositor, m.deposit);
<Multisigs<T>>::remove(&id, call_hash);
<Multisigs<T>>::remove(&id, &call_hash);
Self::take_call(&call_hash);

Self::deposit_event(RawEvent::MultisigCancelled(who, timepoint, id, call_hash));
Ok(())
Expand All @@ -524,6 +545,27 @@ impl<T: Trait> Module<T> {
T::AccountId::decode(&mut &entropy[..]).unwrap_or_default()
}

/// Please a call in storage, reserving funds as appropriate.
fn store_call(who: T::AccountId, hash: &[u8; 32], data: Vec<u8>) -> DispatchResult {
if !Calls::<T>::contains_key(hash) {
let deposit = T::DepositBase::get()
+ T::DepositFactor::get() * BalanceOf::<T>::from(((data.len() + 31) / 32) as u32);
T::Currency::reserve(&who, deposit)?;
// we store `data` here because storing `call` would result in needing another `.encode`.
Calls::<T>::insert(&hash, (data, who, deposit));
}
Ok(())
}

fn take_call(hash: &[u8; 32]) -> Option<<T as Trait>::Call> {
if let Some((data, who, deposit)) = Calls::<T>::take(hash) {
T::Currency::unreserve(&who, deposit);
Decode::decode(&mut &data[..]).ok()
} else {
None
}
}

/// The current `Timepoint`.
pub fn timepoint() -> Timepoint<T::BlockNumber> {
Timepoint {
Expand Down
143 changes: 122 additions & 21 deletions frame/multisig/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,16 +164,65 @@ fn multisig_deposit_is_taken_and_returned() {
assert_ok!(Balances::transfer(Origin::signed(3), multi, 5));

let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false));
assert_eq!(Balances::free_balance(1), 2);
assert_eq!(Balances::reserved_balance(1), 3);

assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call, false));
assert_eq!(Balances::free_balance(1), 5);
assert_eq!(Balances::reserved_balance(1), 0);
});
}

#[test]
fn multisig_deposit_is_taken_and_returned_with_call_storage() {
new_test_ext().execute_with(|| {
let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2);
assert_ok!(Balances::transfer(Origin::signed(1), multi, 5));
assert_ok!(Balances::transfer(Origin::signed(2), multi, 5));
assert_ok!(Balances::transfer(Origin::signed(3), multi, 5));

let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
let hash = call.using_encoded(blake2_256);
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call, true));
assert_eq!(Balances::free_balance(1), 0);
assert_eq!(Balances::reserved_balance(1), 5);

assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash));
assert_eq!(Balances::free_balance(1), 5);
assert_eq!(Balances::reserved_balance(1), 0);
});
}

#[test]
fn multisig_deposit_is_taken_and_returned_with_alt_call_storage() {
new_test_ext().execute_with(|| {
let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3);
assert_ok!(Balances::transfer(Origin::signed(1), multi, 5));
assert_ok!(Balances::transfer(Origin::signed(2), multi, 5));
assert_ok!(Balances::transfer(Origin::signed(3), multi, 5));

let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
let hash = call.using_encoded(blake2_256);

assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone()));
assert_eq!(Balances::free_balance(1), 1);
assert_eq!(Balances::reserved_balance(1), 4);

assert_ok!(Multisig::as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), call, true));
assert_eq!(Balances::free_balance(2), 3);
assert_eq!(Balances::reserved_balance(2), 2);
assert_eq!(Balances::free_balance(1), 1);
assert_eq!(Balances::reserved_balance(1), 4);

assert_ok!(Multisig::approve_as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), hash));
assert_eq!(Balances::free_balance(1), 5);
assert_eq!(Balances::reserved_balance(1), 0);
assert_eq!(Balances::free_balance(2), 5);
assert_eq!(Balances::reserved_balance(2), 0);
});
}

#[test]
fn cancel_multisig_returns_deposit() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -210,17 +259,35 @@ fn timepoint_checking_works() {
assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash));

assert_noop!(
Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, call.clone()),
Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, call.clone(), false),
Error::<Test>::NoTimepoint,
);
let later = Timepoint { index: 1, .. now() };
assert_noop!(
Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(later), call.clone()),
Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(later), call.clone(), false),
Error::<Test>::WrongTimepoint,
);
});
}

#[test]
fn multisig_2_of_3_works_with_call_storing() {
new_test_ext().execute_with(|| {
let multi = Multisig::multi_account_id(&[1, 2, 3][..], 2);
assert_ok!(Balances::transfer(Origin::signed(1), multi, 5));
assert_ok!(Balances::transfer(Origin::signed(2), multi, 5));
assert_ok!(Balances::transfer(Origin::signed(3), multi, 5));

let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
let hash = call.using_encoded(blake2_256);
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call, true));
assert_eq!(Balances::free_balance(6), 0);

assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash));
assert_eq!(Balances::free_balance(6), 15);
});
}

#[test]
fn multisig_2_of_3_works() {
new_test_ext().execute_with(|| {
Expand All @@ -234,7 +301,7 @@ fn multisig_2_of_3_works() {
assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash));
assert_eq!(Balances::free_balance(6), 0);

assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call, false));
assert_eq!(Balances::free_balance(6), 15);
});
}
Expand All @@ -253,7 +320,7 @@ fn multisig_3_of_3_works() {
assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone()));
assert_eq!(Balances::free_balance(6), 0);

assert_ok!(Multisig::as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), call));
assert_ok!(Multisig::as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), call, false));
assert_eq!(Balances::free_balance(6), 15);
});
}
Expand All @@ -275,6 +342,40 @@ fn cancel_multisig_works() {
});
}

#[test]
fn cancel_multisig_with_call_storage_works() {
new_test_ext().execute_with(|| {
let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
let hash = call.using_encoded(blake2_256);
assert_ok!(Multisig::as_multi(Origin::signed(1), 3, vec![2, 3], None, call, true));
assert_eq!(Balances::free_balance(1), 4);
assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone()));
assert_noop!(
Multisig::cancel_as_multi(Origin::signed(2), 3, vec![1, 3], now(), hash.clone()),
Error::<Test>::NotOwner,
);
assert_ok!(
Multisig::cancel_as_multi(Origin::signed(1), 3, vec![2, 3], now(), hash.clone()),
);
assert_eq!(Balances::free_balance(1), 10);
});
}

#[test]
fn cancel_multisig_with_alt_call_storage_works() {
new_test_ext().execute_with(|| {
let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
let hash = call.using_encoded(blake2_256);
assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone()));
assert_eq!(Balances::free_balance(1), 6);
assert_ok!(Multisig::as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), call, true));
assert_eq!(Balances::free_balance(2), 8);
assert_ok!(Multisig::cancel_as_multi(Origin::signed(1), 3, vec![2, 3], now(), hash));
assert_eq!(Balances::free_balance(1), 10);
assert_eq!(Balances::free_balance(2), 10);
});
}

#[test]
fn multisig_2_of_3_as_multi_works() {
new_test_ext().execute_with(|| {
Expand All @@ -284,10 +385,10 @@ fn multisig_2_of_3_as_multi_works() {
assert_ok!(Balances::transfer(Origin::signed(3), multi, 5));

let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false));
assert_eq!(Balances::free_balance(6), 0);

assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call, false));
assert_eq!(Balances::free_balance(6), 15);
});
}
Expand All @@ -303,10 +404,10 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() {
let call1 = Box::new(Call::Balances(BalancesCall::transfer(6, 10)));
let call2 = Box::new(Call::Balances(BalancesCall::transfer(7, 5)));

assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call1.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, call2.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call2));
assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call1));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call1.clone(), false));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, call2.clone(), false));
assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call2, false));
assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call1, false));

assert_eq!(Balances::free_balance(6), 10);
assert_eq!(Balances::free_balance(7), 5);
Expand All @@ -322,12 +423,12 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
assert_ok!(Balances::transfer(Origin::signed(3), multi, 5));

let call = Box::new(Call::Balances(BalancesCall::transfer(6, 10)));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false));
assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call.clone(), false));
assert_eq!(Balances::free_balance(multi), 5);

assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call.clone()));
assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false));
assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call.clone(), false));

let err = DispatchError::from(BalancesError::<Test, _>::InsufficientBalance).stripped();
expect_event(RawEvent::MultisigExecuted(3, now(), multi, call.using_encoded(blake2_256), Err(err)));
Expand All @@ -339,7 +440,7 @@ fn zero_threshold_fails() {
new_test_ext().execute_with(|| {
let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
assert_noop!(
Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call),
Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call, false),
Error::<Test>::ZeroThreshold,
);
});
Expand All @@ -350,7 +451,7 @@ fn too_many_signatories_fails() {
new_test_ext().execute_with(|| {
let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15)));
assert_noop!(
Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone()),
Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone(), false),
Error::<Test>::TooManySignatories,
);
});
Expand Down Expand Up @@ -389,10 +490,10 @@ fn multisig_1_of_3_works() {
Error::<Test>::NoApprovalsNeeded,
);
assert_noop!(
Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone()),
Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone(), false),
BalancesError::<Test, _>::InsufficientBalance,
);
assert_ok!(Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call));
assert_ok!(Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call, false));

assert_eq!(Balances::free_balance(6), 15);
});
Expand All @@ -403,7 +504,7 @@ fn multisig_filters() {
new_test_ext().execute_with(|| {
let call = Box::new(Call::System(frame_system::Call::set_code(vec![])));
assert_noop!(
Multisig::as_multi(Origin::signed(1), 1, vec![], None, call.clone()),
Multisig::as_multi(Origin::signed(1), 1, vec![], None, call.clone(), false),
Error::<Test>::Uncallable,
);
});
Expand Down