From c7584c064f0c02b0a06a267a4b04b1e669fe8f51 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Wed, 10 Jun 2020 13:48:45 +0200 Subject: [PATCH 01/25] Stored call in multisig --- frame/multisig/src/lib.rs | 46 +++++++++++- frame/multisig/src/tests.rs | 143 ++++++++++++++++++++++++++++++------ 2 files changed, 166 insertions(+), 23 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index bde0a06de60fb..4e11d6b04a786 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -122,6 +122,8 @@ decl_storage! { pub Multisigs: double_map hasher(twox_64_concat) T::AccountId, hasher(blake2_128_concat) [u8; 32] => Option, T::AccountId>>; + + pub Calls: map hasher(identity) [u8; 32] => Option<(Vec, T::AccountId, BalanceOf)>; } } @@ -278,6 +280,7 @@ decl_module! { other_signatories: Vec, maybe_timepoint: Option>, call: Box<::Call>, + store_call: bool, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; // We're now executing as a freshly authenticated new account, so the previous call @@ -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) = >::get(&id, call_hash) { let timepoint = maybe_timepoint.ok_or(Error::::NoTimepoint)?; @@ -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); >::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) )); @@ -425,6 +434,17 @@ decl_module! { ensure!(m.when == timepoint, Error::::WrongTimepoint); ensure!(m.approvals.len() < threshold as usize, Error::::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) { + let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); + let _ = T::Currency::unreserve(&m.depositor, m.deposit); + >::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()); >::insert(&id, call_hash, m); Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash)); @@ -506,7 +526,8 @@ decl_module! { ensure!(m.depositor == who, Error::::NotOwner); let _ = T::Currency::unreserve(&m.depositor, m.deposit); - >::remove(&id, call_hash); + >::remove(&id, &call_hash); + Self::take_call(&call_hash); Self::deposit_event(RawEvent::MultisigCancelled(who, timepoint, id, call_hash)); Ok(()) @@ -524,6 +545,27 @@ impl Module { 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) -> DispatchResult { + if !Calls::::contains_key(hash) { + let deposit = T::DepositBase::get() + + T::DepositFactor::get() * BalanceOf::::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::::insert(&hash, (data, who, deposit)); + } + Ok(()) + } + + fn take_call(hash: &[u8; 32]) -> Option<::Call> { + if let Some((data, who, deposit)) = Calls::::take(hash) { + T::Currency::unreserve(&who, deposit); + Decode::decode(&mut &data[..]).ok() + } else { + None + } + } + /// The current `Timepoint`. pub fn timepoint() -> Timepoint { Timepoint { diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index 067ef4cf98e68..c33cdabae5ad6 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -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(|| { @@ -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::::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::::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(|| { @@ -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); }); } @@ -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); }); } @@ -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::::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(|| { @@ -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); }); } @@ -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); @@ -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::::InsufficientBalance).stripped(); expect_event(RawEvent::MultisigExecuted(3, now(), multi, call.using_encoded(blake2_256), Err(err))); @@ -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::::ZeroThreshold, ); }); @@ -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::::TooManySignatories, ); }); @@ -389,10 +490,10 @@ fn multisig_1_of_3_works() { Error::::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::::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); }); @@ -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::::Uncallable, ); }); From 0ddd158b007bcf0fde49fb8e955b439ef6d5d6e8 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Wed, 10 Jun 2020 13:58:15 +0200 Subject: [PATCH 02/25] Docs. --- frame/multisig/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 4e11d6b04a786..e32137ae3a4d4 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -545,7 +545,7 @@ impl Module { T::AccountId::decode(&mut &entropy[..]).unwrap_or_default() } - /// Please a call in storage, reserving funds as appropriate. + /// Place a call's encoded data in storage, reserving funds as appropriate. fn store_call(who: T::AccountId, hash: &[u8; 32], data: Vec) -> DispatchResult { if !Calls::::contains_key(hash) { let deposit = T::DepositBase::get() @@ -557,6 +557,7 @@ impl Module { Ok(()) } + /// Attempt to remove a call from storage, returning it and any deposit on it to the owner. fn take_call(hash: &[u8; 32]) -> Option<::Call> { if let Some((data, who, deposit)) = Calls::::take(hash) { T::Currency::unreserve(&who, deposit); From c30f54f8cc292a7aac82f5af498516f6f2ae4f83 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Wed, 10 Jun 2020 15:03:34 +0200 Subject: [PATCH 03/25] Benchmarks. --- frame/multisig/src/benchmarking.rs | 62 ++++++++++++++++++++++++++---- 1 file changed, 54 insertions(+), 8 deletions(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index fa2ec52e6b2c5..5a12f66ad6303 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -55,7 +55,16 @@ benchmarks! { let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; - }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call) + }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false) + + as_multi_create_store { + // Signatories, need at least 2 total people + let s in 2 .. T::MaxSignatories::get() as u32; + // Transaction Length + let z in 0 .. 10_000; + let (mut signatories, call) = setup_multi::(s, z)?; + let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true) as_multi_approve { // Signatories, need at least 2 people @@ -68,9 +77,9 @@ benchmarks! { // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone())?; + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; let caller2 = signatories2.remove(0); - }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call) + }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false) as_multi_complete { // Signatories, need at least 2 people @@ -83,16 +92,16 @@ benchmarks! { // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone())?; + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; // Everyone except the first person approves for i in 1 .. s - 1 { let mut signatories_loop = signatories2.clone(); let caller_loop = signatories_loop.remove(i as usize); let o = RawOrigin::Signed(caller_loop).into(); - Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone())?; + Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?; } let caller2 = signatories2.remove(0); - }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call) + }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false) approve_as_multi_create { // Signatories, need at least 2 people @@ -117,7 +126,30 @@ benchmarks! { // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone())?; + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; + let caller2 = signatories2.remove(0); + }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash) + + approve_as_multi_complete { + // Signatories, need at least 2 people + let s in 2 .. T::MaxSignatories::get() as u32; + // Transaction Length + let z in 0 .. 10_000; + let (mut signatories, call) = setup_multi::(s, z)?; + let mut signatories2 = signatories.clone(); + let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + let call_hash = call.using_encoded(blake2_256); + // before the call, get the timepoint + let timepoint = Multisig::::timepoint(); + // Create the multi + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true)?; + // Everyone except the first person approves + for i in 1 .. s - 1 { + let mut signatories_loop = signatories2.clone(); + let caller_loop = signatories_loop.remove(i as usize); + let o = RawOrigin::Signed(caller_loop).into(); + Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?; + } let caller2 = signatories2.remove(0); }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash) @@ -132,7 +164,21 @@ benchmarks! { let timepoint = Multisig::::timepoint(); // Create the multi let o = RawOrigin::Signed(caller.clone()).into(); - Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone())?; + Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), false)?; + }: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) + + cancel_as_multi_store { + // Signatories, need at least 2 people + let s in 2 .. T::MaxSignatories::get() as u32; + // Transaction Length + let z in 0 .. 10_000; + let (mut signatories, call) = setup_multi::(s, z)?; + let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + let call_hash = call.using_encoded(blake2_256); + let timepoint = Multisig::::timepoint(); + // Create the multi + let o = RawOrigin::Signed(caller.clone()).into(); + Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), true)?; }: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) } From dbac9eb6ba5731edec5eb8b9fb96298296aa8856 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Wed, 10 Jun 2020 16:18:42 +0200 Subject: [PATCH 04/25] Fix --- frame/multisig/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index 5a12f66ad6303..55bb880fb5582 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -179,7 +179,7 @@ benchmarks! { // Create the multi let o = RawOrigin::Signed(caller.clone()).into(); Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), true)?; - }: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) + }: cancel_as_multi(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) } #[cfg(test)] From 3669489a6b1eee75803f1978a6b34321733d03dd Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Thu, 11 Jun 2020 00:40:05 +0200 Subject: [PATCH 05/25] Update frame/multisig/src/lib.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- frame/multisig/src/lib.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index e32137ae3a4d4..19597eac67d9e 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -559,12 +559,10 @@ impl Module { /// Attempt to remove a call from storage, returning it and any deposit on it to the owner. fn take_call(hash: &[u8; 32]) -> Option<::Call> { - if let Some((data, who, deposit)) = Calls::::take(hash) { + Calls::::take(hash).and_then(|(data, who, deposit)| { T::Currency::unreserve(&who, deposit); Decode::decode(&mut &data[..]).ok() - } else { - None - } + }) } /// The current `Timepoint`. From 9f59d316ea668130c76faf15943081b88a5fa608 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 11 Jun 2020 11:56:18 +0200 Subject: [PATCH 06/25] patch benchmarks --- frame/multisig/src/benchmarking.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index 55bb880fb5582..8b2006c1e63d0 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -22,7 +22,7 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account}; -use sp_runtime::traits::Saturating; +use sp_runtime::traits::{Bounded, Saturating}; use crate::Module as Multisig; @@ -64,6 +64,7 @@ benchmarks! { let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true) as_multi_approve { @@ -138,6 +139,7 @@ benchmarks! { let (mut signatories, call) = setup_multi::(s, z)?; let mut signatories2 = signatories.clone(); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); let call_hash = call.using_encoded(blake2_256); // before the call, get the timepoint let timepoint = Multisig::::timepoint(); @@ -174,6 +176,7 @@ benchmarks! { let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); let call_hash = call.using_encoded(blake2_256); let timepoint = Multisig::::timepoint(); // Create the multi @@ -192,11 +195,14 @@ mod tests { fn test_benchmarks() { new_test_ext().execute_with(|| { assert_ok!(test_benchmark_as_multi_create::()); + assert_ok!(test_benchmark_as_multi_create_store::()); assert_ok!(test_benchmark_as_multi_approve::()); assert_ok!(test_benchmark_as_multi_complete::()); assert_ok!(test_benchmark_approve_as_multi_create::()); assert_ok!(test_benchmark_approve_as_multi_approve::()); + assert_ok!(test_benchmark_approve_as_multi_complete::()); assert_ok!(test_benchmark_cancel_as_multi::()); + assert_ok!(test_benchmark_cancel_as_multi_store::()); }); } } From 69682e5a8aa5211679f9235485377522b01657f5 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 11 Jun 2020 12:29:11 +0200 Subject: [PATCH 07/25] Minor grumbles. --- frame/multisig/src/lib.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index e32137ae3a4d4..6f908f153949d 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -74,10 +74,12 @@ pub trait Trait: frame_system::Trait { /// The currency mechanism. type Currency: ReservableCurrency; - /// The base amount of currency needed to reserve for creating a multisig execution. + /// The base amount of currency needed to reserve for creating a multisig execution or to store + /// a dispatch call for later. /// /// This is held for an additional storage item whose value size is - /// `4 + sizeof((BlockNumber, Balance, AccountId))` bytes. + /// `4 + sizeof((BlockNumber, Balance, AccountId))` bytes and whose key size is + /// `32 + sizeof(AccountId)` bytes. type DepositBase: Get>; /// The amount of currency needed per unit threshold when creating a multisig execution. @@ -298,9 +300,6 @@ decl_module! { 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) = >::get(&id, call_hash) { let timepoint = maybe_timepoint.ok_or(Error::::NoTimepoint)?; @@ -310,6 +309,9 @@ decl_module! { if (m.approvals.len() as u16) < threshold - 1 { m.approvals.insert(pos, who.clone()); >::insert(&id, call_hash, m); + if store_call { + Self::store_call(who.clone(), &call_hash, encoded_call)?; + } Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash)); // Call is not made, so the actual weight does not include call return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) @@ -340,6 +342,9 @@ decl_module! { depositor: who.clone(), approvals: vec![who.clone()], }); + if store_call { + Self::store_call(who.clone(), &call_hash, encoded_call)?; + } Self::deposit_event(RawEvent::NewMultisig(who, id, call_hash)); // Call is not made, so we can return that weight return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) From a363d4b91bffd715b1d761eae855885530920d7e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 11 Jun 2020 13:17:16 +0200 Subject: [PATCH 08/25] Update as_multi weight --- frame/multisig/src/lib.rs | 86 +++++++++++++++++++++++++++++---------- 1 file changed, 64 insertions(+), 22 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 19597eac67d9e..305795a286c45 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -51,7 +51,7 @@ use codec::{Encode, Decode}; use sp_io::hashing::blake2_256; use frame_support::{decl_module, decl_event, decl_error, decl_storage, Parameter, ensure, RuntimeDebug}; use frame_support::{traits::{Get, ReservableCurrency, Currency, Filter, FilterStack, ClearFilterGuard}, - weights::{Weight, GetDispatchInfo, DispatchClass, Pays}, + weights::{Weight, GetDispatchInfo, DispatchClass, Pays, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}}, dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo}, }; use frame_system::{self as system, ensure_signed}; @@ -186,18 +186,33 @@ mod weight_of { use super::*; /// - Base Weight: - /// - Create: 46.55 + 0.089 * S µs - /// - Approve: 34.03 + .112 * S µs - /// - Complete: 40.36 + .225 * S µs + /// - Create: 41.89 + 0.118 * S + .002 * Z µs + /// - Create w/ Store: 53.57 + 0.119 * S + .003 * Z µs + /// - Approve: 31.39 + 0.136 * S + .002 * Z µs + /// - Complete: 39.94 + 0.26 * S + .002 * Z µs /// - DB Weight: - /// - Reads: Multisig Storage, [Caller Account] - /// - Writes: Multisig Storage, [Caller Account] + /// - Reads: Multisig Storage, [Caller Account], Calls (if `store_call`) + /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) /// - Plus Call Weight - pub fn as_multi(other_sig_len: usize, call_weight: Weight) -> Weight { - call_weight - .saturating_add(45_000_000) - .saturating_add((other_sig_len as Weight).saturating_mul(250_000)) - .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + pub fn as_multi( + sig_len: usize, + call_len: usize, + call_weight: Weight, + store_call: bool, + ) -> Weight { + if store_call { + return call_weight + .saturating_add(53 * WEIGHT_PER_MICROS) + .saturating_add((250 * WEIGHT_PER_NANOS).saturating_mul(sig_len as Weight)) + .saturating_add((3 * WEIGHT_PER_NANOS).saturating_mul(call_len as Weight)) + .saturating_add(T::DbWeight::get().reads_writes(2, 2)) + } else { + return call_weight + .saturating_add(42 * WEIGHT_PER_MICROS) + .saturating_add((250 * WEIGHT_PER_NANOS).saturating_mul(sig_len as Weight)) + .saturating_add((2 * WEIGHT_PER_NANOS).saturating_mul(call_len as Weight)) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) + } } } @@ -262,16 +277,22 @@ decl_module! { /// `DepositBase + threshold * DepositFactor`. /// ------------------------------- /// - Base Weight: - /// - Create: 46.55 + 0.089 * S µs - /// - Approve: 34.03 + .112 * S µs - /// - Complete: 40.36 + .225 * S µs + /// - Create: 41.89 + 0.118 * S + .002 * Z µs + /// - Create w/ Store: 53.57 + 0.119 * S + .003 * Z µs + /// - Approve: 31.39 + 0.136 * S + .002 * Z µs + /// - Complete: 39.94 + 0.26 * S + .002 * Z µs /// - DB Weight: - /// - Reads: Multisig Storage, [Caller Account] - /// - Writes: Multisig Storage, [Caller Account] + /// - Reads: Multisig Storage, [Caller Account], Calls (if `store_call`) + /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) /// - Plus Call Weight /// # #[weight = ( - weight_of::as_multi::(other_signatories.len(), call.get_dispatch_info().weight), + weight_of::as_multi::( + other_signatories.len(), + call.using_encoded(|x| x.len()), + call.get_dispatch_info().weight, + *store_call + ), call.get_dispatch_info().class, Pays::Yes, )] @@ -297,6 +318,7 @@ decl_module! { let id = Self::multi_account_id(&signatories, threshold); let encoded_call = call.encode(); + let call_len = encoded_call.len(); let call_hash = blake2_256(&encoded_call); if store_call { Self::store_call(who.clone(), &call_hash, encoded_call)?; @@ -311,8 +333,13 @@ decl_module! { m.approvals.insert(pos, who.clone()); >::insert(&id, call_hash, m); Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash)); - // Call is not made, so the actual weight does not include call - return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) + // Call is not made, so the actual weight does not include call weight + return Ok(Some(weight_of::as_multi::( + other_signatories_len, + call_len, + 0, + store_call, + )).into()) } } else { if (m.approvals.len() as u16) < threshold { @@ -342,20 +369,35 @@ decl_module! { }); Self::deposit_event(RawEvent::NewMultisig(who, id, call_hash)); // Call is not made, so we can return that weight - return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) + return Ok(Some(weight_of::as_multi::( + other_signatories_len, + call_len, + 0, + store_call, + )).into()) } else { let result = call.dispatch(frame_system::RawOrigin::Signed(id).into()); match result { Ok(post_dispatch_info) => { match post_dispatch_info.actual_weight { - Some(actual_weight) => return Ok(Some(weight_of::as_multi::(other_signatories_len, actual_weight)).into()), + Some(actual_weight) => return Ok(Some(weight_of::as_multi::( + other_signatories_len, + call_len, + actual_weight, + store_call, + )).into()), None => return Ok(None.into()), } }, Err(err) => { match err.post_info.actual_weight { Some(actual_weight) => { - let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); + let weight_used = weight_of::as_multi::( + other_signatories_len, + call_len, + actual_weight, + store_call, + ); return Err(DispatchErrorWithPostInfo { post_info: Some(weight_used).into(), error: err.error.into() }) }, None => { From a8343cbbbac4e2327d5a5ca643f5e960816acac0 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 11 Jun 2020 14:18:36 +0200 Subject: [PATCH 09/25] Fixes and refactoring. --- frame/multisig/src/lib.rs | 311 +++++++++++++++++++----------------- frame/multisig/src/tests.rs | 4 +- 2 files changed, 168 insertions(+), 147 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 2631ec376a69f..a8be986335ebe 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -203,6 +203,11 @@ mod weight_of { } } +enum CallOrHash { + Call(Call, bool), + Hash([u8; 32]), +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { type Error = Error; @@ -285,92 +290,7 @@ decl_module! { store_call: bool, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - // We're now executing as a freshly authenticated new account, so the previous call - // restrictions no longer apply. - let _guard = ClearFilterGuard::::Call>::new(); - ensure!(T::IsCallable::filter(call.as_ref()), Error::::Uncallable); - ensure!(threshold >= 1, Error::::ZeroThreshold); - let max_sigs = T::MaxSignatories::get() as usize; - ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); - let other_signatories_len = other_signatories.len(); - ensure!(other_signatories_len < max_sigs, Error::::TooManySignatories); - let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; - - let id = Self::multi_account_id(&signatories, threshold); - - let encoded_call = call.encode(); - let call_hash = blake2_256(&encoded_call); - - if let Some(mut m) = >::get(&id, call_hash) { - let timepoint = maybe_timepoint.ok_or(Error::::NoTimepoint)?; - ensure!(m.when == timepoint, Error::::WrongTimepoint); - if let Err(pos) = m.approvals.binary_search(&who) { - // we know threshold is greater than zero from the above ensure. - if (m.approvals.len() as u16) < threshold - 1 { - m.approvals.insert(pos, who.clone()); - >::insert(&id, call_hash, m); - if store_call { - Self::store_call(who.clone(), &call_hash, encoded_call)?; - } - Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash)); - // Call is not made, so the actual weight does not include call - return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) - } - } else { - if (m.approvals.len() as u16) < threshold { - Err(Error::::AlreadyApproved)? - } - } - - let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); - let _ = T::Currency::unreserve(&m.depositor, m.deposit); - >::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) - )); - return Ok(None.into()) - } else { - ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); - if threshold > 1 { - let deposit = T::DepositBase::get() - + T::DepositFactor::get() * threshold.into(); - T::Currency::reserve(&who, deposit)?; - >::insert(&id, call_hash, Multisig { - when: Self::timepoint(), - deposit, - depositor: who.clone(), - approvals: vec![who.clone()], - }); - if store_call { - Self::store_call(who.clone(), &call_hash, encoded_call)?; - } - Self::deposit_event(RawEvent::NewMultisig(who, id, call_hash)); - // Call is not made, so we can return that weight - return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) - } else { - let result = call.dispatch(frame_system::RawOrigin::Signed(id).into()); - match result { - Ok(post_dispatch_info) => { - match post_dispatch_info.actual_weight { - Some(actual_weight) => return Ok(Some(weight_of::as_multi::(other_signatories_len, actual_weight)).into()), - None => return Ok(None.into()), - } - }, - Err(err) => { - match err.post_info.actual_weight { - Some(actual_weight) => { - let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); - return Err(DispatchErrorWithPostInfo { post_info: Some(weight_used).into(), error: err.error.into() }) - }, - None => { - return Err(err) - } - } - } - } - } - } + Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Call(*call, store_call)) } /// Register approval for a dispatch to be made from a deterministic composite account if @@ -424,56 +344,9 @@ decl_module! { other_signatories: Vec, maybe_timepoint: Option>, call_hash: [u8; 32], - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - ensure!(threshold >= 1, Error::::ZeroThreshold); - let max_sigs = T::MaxSignatories::get() as usize; - ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); - ensure!(other_signatories.len() < max_sigs, Error::::TooManySignatories); - let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; - - let id = Self::multi_account_id(&signatories, threshold); - - if let Some(mut m) = >::get(&id, call_hash) { - let timepoint = maybe_timepoint.ok_or(Error::::NoTimepoint)?; - ensure!(m.when == timepoint, Error::::WrongTimepoint); - ensure!(m.approvals.len() < threshold as usize, Error::::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) { - let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); - let _ = T::Currency::unreserve(&m.depositor, m.deposit); - >::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()); - >::insert(&id, call_hash, m); - Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash)); - } else { - Err(Error::::AlreadyApproved)? - } - } else { - if threshold > 1 { - ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); - let deposit = T::DepositBase::get() - + T::DepositFactor::get() * threshold.into(); - T::Currency::reserve(&who, deposit)?; - >::insert(&id, call_hash, Multisig { - when: Self::timepoint(), - deposit, - depositor: who.clone(), - approvals: vec![who.clone()], - }); - Self::deposit_event(RawEvent::NewMultisig(who, id, call_hash)); - } else { - Err(Error::::NoApprovalsNeeded)? - } - } - Ok(()) + Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Hash(call_hash)) } /// Cancel a pre-existing, on-going multisig transaction. Any deposit reserved previously @@ -532,7 +405,7 @@ decl_module! { let _ = T::Currency::unreserve(&m.depositor, m.deposit); >::remove(&id, &call_hash); - Self::take_call(&call_hash); + Self::clear_call(&call_hash); Self::deposit_event(RawEvent::MultisigCancelled(who, timepoint, id, call_hash)); Ok(()) @@ -550,26 +423,174 @@ impl Module { T::AccountId::decode(&mut &entropy[..]).unwrap_or_default() } + fn operate( + who: T::AccountId, + threshold: u16, + other_signatories: Vec, + maybe_timepoint: Option>, + call_or_hash: CallOrHash<::Call>, + ) -> DispatchResultWithPostInfo { + ensure!(threshold >= 1, Error::::ZeroThreshold); + let max_sigs = T::MaxSignatories::get() as usize; + ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); + let other_signatories_len = other_signatories.len(); + ensure!(other_signatories_len < max_sigs, Error::::TooManySignatories); + let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; + + let id = Self::multi_account_id(&signatories, threshold); + + if threshold == 1 { + // Special case for 1 of N; we can always execute it directly, so it's a single-step + // operation. + ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); + let call = match call_or_hash { + CallOrHash::Call(c, _) => c, + _ => Err(Error::::NoApprovalsNeeded)?, + }; + // We're now executing as a freshly authenticated new account, so the previous call + // restrictions no longer apply. + let _guard = ClearFilterGuard::::Call>::new(); + ensure!(T::IsCallable::filter(&call), Error::::Uncallable); + + let result = call.dispatch(frame_system::RawOrigin::Signed(id).into()); + match result { + Ok(post_dispatch_info) => { + match post_dispatch_info.actual_weight { + Some(actual_weight) => return Ok(Some(weight_of::as_multi::(other_signatories_len, actual_weight)).into()), + None => return Ok(None.into()), + } + }, + Err(err) => { + match err.post_info.actual_weight { + Some(actual_weight) => { + let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); + return Err(DispatchErrorWithPostInfo { post_info: Some(weight_used).into(), error: err.error.into() }) + }, + None => { + return Err(err) + } + } + } + } + } + + // Threshold > 1; this means it's a multi-step operation. We extract the `call_hash`. + let (call_hash, maybe_call, maybe_store) = match call_or_hash { + CallOrHash::Call(call, should_store) => { + let encoded_call = call.encode(); + let call_hash = blake2_256(&encoded_call); + (call_hash, Some(call), if should_store { Some(encoded_call) } else { None }) + } + CallOrHash::Hash(h) => (h, None, None), + }; + + // Branch on whether the operation has already started or not. + if let Some(mut m) = >::get(&id, call_hash) { + // Yes; ensure that the timepoint exists and agrees. + let timepoint = maybe_timepoint.ok_or(Error::::NoTimepoint)?; + ensure!(m.when == timepoint, Error::::WrongTimepoint); + + // Ensure that either we have not yet signed or that it is at threshold. + let mut approvals = m.approvals.len() as u16; + // We only bother with the approval if we're below threshold. + let maybe_pos = m.approvals.binary_search(&who).err().filter(|_| approvals < threshold); + // Bump approvals if not yet voted and the vote is needed. + if maybe_pos.is_some() { approvals += 1; } + + // We only bother fetching/decoding call if we know that we're ready to execute. + let maybe_approved_call = if approvals >= threshold { + maybe_call.or_else(|| Self::get_call(&call_hash)) + } else { None }; + + if let Some(call) = maybe_approved_call { + // We're now executing as a freshly authenticated new account, so the previous call + // restrictions no longer apply. + let _guard = ClearFilterGuard::::Call>::new(); + ensure!(T::IsCallable::filter(&call), Error::::Uncallable); + + let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); + let _ = T::Currency::unreserve(&m.depositor, m.deposit); + >::remove(&id, call_hash); + Self::clear_call(&call_hash); + Self::deposit_event(RawEvent::MultisigExecuted( + who, timepoint, id, call_hash, result.map(|_| ()).map_err(|e| e.error) + )); + Ok(None.into()) + } else { + // We cannot dispatch the call now; either it isn't available, or it is, but we + // don't have threshold approvals even with our signature. + + // Store the call if desired. + let stored = maybe_store + .map_or(false, |data| Self::store_call(who.clone(), &call_hash, data)); + + if let Some(pos) = maybe_pos { + // Record approval. + m.approvals.insert(pos, who.clone()); + >::insert(&id, call_hash, m); + Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash)); + } else { + // If we already approved and didn't store the Call, then this was useless and + // we report an error. + ensure!(stored, Error::::AlreadyApproved); + } + + // Call is not made, so the actual weight does not include call + Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) + } + } else { + // Not yet started; there should be no timepoint given. + ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); + + // Just start the operation by recording it in storage. + let deposit = T::DepositBase::get() + + T::DepositFactor::get() * threshold.into(); + T::Currency::reserve(&who, deposit)?; + if let Some(data) = maybe_store { Self::store_call(who.clone(), &call_hash, data); }; + >::insert(&id, call_hash, Multisig { + when: Self::timepoint(), + deposit, + depositor: who.clone(), + approvals: vec![who.clone()], + }); + Self::deposit_event(RawEvent::NewMultisig(who, id, call_hash)); + // Call is not made, so we can return that weight + return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) + } + } + /// Place a call's encoded data in storage, reserving funds as appropriate. - fn store_call(who: T::AccountId, hash: &[u8; 32], data: Vec) -> DispatchResult { + /// + /// We store `data` here because storing `call` would result in needing another `.encode`. + /// + /// Returns a `bool` indicating whether the data did end up being stored. + fn store_call(who: T::AccountId, hash: &[u8; 32], data: Vec) -> bool { if !Calls::::contains_key(hash) { let deposit = T::DepositBase::get() + T::DepositFactor::get() * BalanceOf::::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::::insert(&hash, (data, who, deposit)); + if T::Currency::reserve(&who, deposit).is_ok() { + // Only store the data if the deposit was paid. + Calls::::insert(&hash, (data, who, deposit)); + return true + } } - Ok(()) + false } - /// Attempt to remove a call from storage, returning it and any deposit on it to the owner. - fn take_call(hash: &[u8; 32]) -> Option<::Call> { - Calls::::take(hash).and_then(|(data, who, deposit)| { - T::Currency::unreserve(&who, deposit); + /// Attempt to read a call from storage, returning it. + fn get_call(hash: &[u8; 32]) -> Option<::Call> { + Calls::::get(hash).and_then(|(data, ..)| { Decode::decode(&mut &data[..]).ok() }) } + /// Attempt to remove a call from storage, returning any deposit on it to the owner. + fn clear_call(hash: &[u8; 32]) { + if let Some((_, who, deposit)) = Calls::::take(hash) { + T::Currency::unreserve(&who, deposit); + } + } + /// The current `Timepoint`. pub fn timepoint() -> Timepoint { Timepoint { diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index c33cdabae5ad6..8abf72d78969f 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -470,7 +470,7 @@ fn duplicate_approvals_are_ignored() { assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash.clone())); assert_noop!( Multisig::approve_as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), hash.clone()), - Error::::NoApprovalsNeeded, + Error::::AlreadyApproved, ); }); } @@ -504,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(), false), + Multisig::as_multi(Origin::signed(1), 1, vec![2], None, call.clone(), false), Error::::Uncallable, ); }); From bd5f05ece778c83708e24f130290302dc9b02d7d Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Thu, 11 Jun 2020 14:54:46 +0200 Subject: [PATCH 10/25] Split out threshold=1 and opaquify Call. --- frame/multisig/src/lib.rs | 164 ++++++++++++++++++++++++++------------ 1 file changed, 111 insertions(+), 53 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index cc3f2c53433ce..8b1c93f037b3a 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -218,8 +218,8 @@ mod weight_of { } } -enum CallOrHash { - Call(Call, bool), +enum CallOrHash { + Call(Vec, bool), Hash([u8; 32]), } @@ -241,6 +241,100 @@ decl_module! { 1_000_000_000 } + /// Register approval for a dispatch to be made from a deterministic composite account if + /// approved by a total of `threshold - 1` of `other_signatories`. + /// + /// If there are enough, then dispatch the call. Calls must each fulfil the `IsCallable` + /// filter. + /// + /// Payment: `DepositBase` will be reserved if this is the first approval, plus + /// `threshold` times `DepositFactor`. It is returned once this dispatch happens or + /// is cancelled. + /// + /// The dispatch origin for this call must be _Signed_. + /// + /// - `threshold`: The total number of approvals for this dispatch before it is executed. + /// - `other_signatories`: The accounts (other than the sender) who can approve this + /// dispatch. May not be empty. + /// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is + /// not the first approval, then it must be `Some`, with the timepoint (block number and + /// transaction index) of the first approval transaction. + /// - `call`: The call to be executed. + /// + /// NOTE: Unless this is the final approval, you will generally want to use + /// `approve_as_multi` instead, since it only requires a hash of the call. + /// + /// Result is equivalent to the dispatched result if `threshold` is exactly `1`. Otherwise + /// on success, result is `Ok` and the result from the interior call, if it was executed, + /// may be found in the deposited `MultisigExecuted` event. + /// + /// # + /// - `O(S + Z + Call)`. + /// - Up to one balance-reserve or unreserve operation. + /// - One passthrough operation, one insert, both `O(S)` where `S` is the number of + /// signatories. `S` is capped by `MaxSignatories`, with weight being proportional. + /// - One call encode & hash, both of complexity `O(Z)` where `Z` is tx-len. + /// - One encode & hash, both of complexity `O(S)`. + /// - Up to one binary search and insert (`O(logS + S)`). + /// - I/O: 1 read `O(S)`, up to 1 mutate `O(S)`. Up to one remove. + /// - One event. + /// - The weight of the `call`. + /// - Storage: inserts one item, value size bounded by `MaxSignatories`, with a + /// deposit taken for its lifetime of + /// `DepositBase + threshold * DepositFactor`. + /// ------------------------------- + /// - Base Weight: + /// - Create: 41.89 + 0.118 * S + .002 * Z µs + /// - Create w/ Store: 53.57 + 0.119 * S + .003 * Z µs + /// - Approve: 31.39 + 0.136 * S + .002 * Z µs + /// - Complete: 39.94 + 0.26 * S + .002 * Z µs + /// - DB Weight: + /// - Reads: Multisig Storage, [Caller Account], Calls (if `store_call`) + /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) + /// - Plus Call Weight + /// # + #[weight = ( + weight_of::as_multi::( + other_signatories.len(), + call.using_encoded(|x| x.len()), + call.get_dispatch_info().weight, + *store_call + ), + call.get_dispatch_info().class, + Pays::Yes, + )] + fn as_multi_threshold_1(origin, + other_signatories: Vec, + call: T::Call, + ) -> DispatchResultWithPostInfo { + let max_sigs = T::MaxSignatories::get() as usize; + ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); + let other_signatories_len = other_signatories.len(); + ensure!(other_signatories_len < max_sigs, Error::::TooManySignatories); + let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?; + + let id = Self::multi_account_id(&signatories, 1); + + // We're now executing as a freshly authenticated new account, so the previous call + // restrictions no longer apply. + let _guard = ClearFilterGuard::::Call>::new(); + ensure!(T::IsCallable::filter(&call), Error::::Uncallable); + + call.dispatch(frame_system::RawOrigin::Signed(id).into()) + .map(|post_dispatch_info| post_dispatch_info.actual_weight + .map(|actual_weight| weight_of::as_multi::(other_signatories_len, actual_weight)) + .into() + ).map_err(|err| match err.post_info.actual_weight { + Some(actual_weight) => { + let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); + let post_info = Some(weight_used).into(); + let error = err.error.into(); + DispatchErrorWithPostInfo { post_info, error } + }, + None => err, + }) + } + /// Register approval for a dispatch to be made from a deterministic composite account if /// approved by a total of `threshold - 1` of `other_signatories`. /// @@ -307,11 +401,11 @@ decl_module! { threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, - call: Box<::Call>, + call: Vec, store_call: bool, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Call(*call, store_call)) + Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Call(call, store_call)) } /// Register approval for a dispatch to be made from a deterministic composite account if @@ -411,7 +505,7 @@ decl_module! { call_hash: [u8; 32], ) -> DispatchResult { let who = ensure_signed(origin)?; - ensure!(threshold >= 1, Error::::ZeroThreshold); + ensure!(threshold >= 2, Error::::ZeroThreshold); let max_sigs = T::MaxSignatories::get() as usize; ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); ensure!(other_signatories.len() < max_sigs, Error::::TooManySignatories); @@ -449,9 +543,9 @@ impl Module { threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, - call_or_hash: CallOrHash<::Call>, + call_or_hash: CallOrHash, ) -> DispatchResultWithPostInfo { - ensure!(threshold >= 1, Error::::ZeroThreshold); + ensure!(threshold >= 2, Error::::ZeroThreshold); let max_sigs = T::MaxSignatories::get() as usize; ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); let other_signatories_len = other_signatories.len(); @@ -460,49 +554,13 @@ impl Module { let id = Self::multi_account_id(&signatories, threshold); - if threshold == 1 { - // Special case for 1 of N; we can always execute it directly, so it's a single-step - // operation. - ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); - let call = match call_or_hash { - CallOrHash::Call(c, _) => c, - _ => Err(Error::::NoApprovalsNeeded)?, - }; - // We're now executing as a freshly authenticated new account, so the previous call - // restrictions no longer apply. - let _guard = ClearFilterGuard::::Call>::new(); - ensure!(T::IsCallable::filter(&call), Error::::Uncallable); - - let result = call.dispatch(frame_system::RawOrigin::Signed(id).into()); - match result { - Ok(post_dispatch_info) => { - match post_dispatch_info.actual_weight { - Some(actual_weight) => return Ok(Some(weight_of::as_multi::(other_signatories_len, actual_weight)).into()), - None => return Ok(None.into()), - } - }, - Err(err) => { - match err.post_info.actual_weight { - Some(actual_weight) => { - let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); - return Err(DispatchErrorWithPostInfo { post_info: Some(weight_used).into(), error: err.error.into() }) - }, - None => { - return Err(err) - } - } - } - } - } - // Threshold > 1; this means it's a multi-step operation. We extract the `call_hash`. - let (call_hash, maybe_call, maybe_store) = match call_or_hash { + let (call_hash, maybe_call, store) = match call_or_hash { CallOrHash::Call(call, should_store) => { - let encoded_call = call.encode(); - let call_hash = blake2_256(&encoded_call); - (call_hash, Some(call), if should_store { Some(encoded_call) } else { None }) + let call_hash = blake2_256(&call); + (call_hash, Some(call), should_store) } - CallOrHash::Hash(h) => (h, None, None), + CallOrHash::Hash(h) => (h, None, false), }; // Branch on whether the operation has already started or not. @@ -520,7 +578,7 @@ impl Module { // We only bother fetching/decoding call if we know that we're ready to execute. let maybe_approved_call = if approvals >= threshold { - maybe_call.or_else(|| Self::get_call(&call_hash)) + Self::get_call(&call_hash, maybe_call.as_ref().map(Into::into)) } else { None }; if let Some(call) = maybe_approved_call { @@ -542,7 +600,8 @@ impl Module { // don't have threshold approvals even with our signature. // Store the call if desired. - let stored = maybe_store + let stored = maybe_call + .filter(|| store) .map_or(false, |data| Self::store_call(who.clone(), &call_hash, data)); if let Some(pos) = maybe_pos { @@ -564,10 +623,9 @@ impl Module { ensure!(maybe_timepoint.is_none(), Error::::UnexpectedTimepoint); // Just start the operation by recording it in storage. - let deposit = T::DepositBase::get() - + T::DepositFactor::get() * threshold.into(); + let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into(); T::Currency::reserve(&who, deposit)?; - if let Some(data) = maybe_store { Self::store_call(who.clone(), &call_hash, data); }; + call.filter(|| store).map(|d| Self::store_call(who.clone(), &call_hash, d)); >::insert(&id, call_hash, Multisig { when: Self::timepoint(), deposit, @@ -599,7 +657,7 @@ impl Module { } /// Attempt to read a call from storage, returning it. - fn get_call(hash: &[u8; 32]) -> Option<::Call> { + fn get_call(hash: &[u8; 32], maybe_known: Option<&[u8]>) -> Option<::Call> { Calls::::get(hash).and_then(|(data, ..)| { Decode::decode(&mut &data[..]).ok() }) From 8b9aaae4df1ae3a52aaf5a2316bd993910dce521 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 14 Jun 2020 09:54:23 +0200 Subject: [PATCH 11/25] Compiles, tests pass, weights are broken --- frame/multisig/src/lib.rs | 49 +++++++++++++----------- frame/multisig/src/tests.rs | 74 +++++++++++++++++++------------------ 2 files changed, 66 insertions(+), 57 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 8b1c93f037b3a..dac39ab369f89 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -187,6 +187,13 @@ decl_event! { mod weight_of { use super::*; + pub fn as_multi( + sig_len: usize, + call_weight: Weight, + ) -> Weight { + 0 + } + /// - Base Weight: /// - Create: 41.89 + 0.118 * S + .002 * Z µs /// - Create w/ Store: 53.57 + 0.119 * S + .003 * Z µs @@ -196,7 +203,7 @@ mod weight_of { /// - Reads: Multisig Storage, [Caller Account], Calls (if `store_call`) /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) /// - Plus Call Weight - pub fn as_multi( + pub fn as_multi_new( sig_len: usize, call_len: usize, call_weight: Weight, @@ -294,19 +301,15 @@ decl_module! { /// - Plus Call Weight /// # #[weight = ( - weight_of::as_multi::( - other_signatories.len(), - call.using_encoded(|x| x.len()), - call.get_dispatch_info().weight, - *store_call - ), + 0, call.get_dispatch_info().class, Pays::Yes, )] fn as_multi_threshold_1(origin, other_signatories: Vec, - call: T::Call, + call: Box<::Call>, ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; let max_sigs = T::MaxSignatories::get() as usize; ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); let other_signatories_len = other_signatories.len(); @@ -387,16 +390,14 @@ decl_module! { /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) /// - Plus Call Weight /// # - #[weight = ( - weight_of::as_multi::( + #[weight = + weight_of::as_multi_new::( other_signatories.len(), call.using_encoded(|x| x.len()), - call.get_dispatch_info().weight, + 0, *store_call - ), - call.get_dispatch_info().class, - Pays::Yes, - )] + ) + ] fn as_multi(origin, threshold: u16, other_signatories: Vec, @@ -578,7 +579,7 @@ impl Module { // We only bother fetching/decoding call if we know that we're ready to execute. let maybe_approved_call = if approvals >= threshold { - Self::get_call(&call_hash, maybe_call.as_ref().map(Into::into)) + Self::get_call(&call_hash, maybe_call.as_ref().map(|c| c.as_ref())) } else { None }; if let Some(call) = maybe_approved_call { @@ -601,7 +602,7 @@ impl Module { // Store the call if desired. let stored = maybe_call - .filter(|| store) + .filter(|_| store) .map_or(false, |data| Self::store_call(who.clone(), &call_hash, data)); if let Some(pos) = maybe_pos { @@ -625,7 +626,7 @@ impl Module { // Just start the operation by recording it in storage. let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into(); T::Currency::reserve(&who, deposit)?; - call.filter(|| store).map(|d| Self::store_call(who.clone(), &call_hash, d)); + maybe_call.filter(|_| store).map(|d| Self::store_call(who.clone(), &call_hash, d)); >::insert(&id, call_hash, Multisig { when: Self::timepoint(), deposit, @@ -658,9 +659,15 @@ impl Module { /// Attempt to read a call from storage, returning it. fn get_call(hash: &[u8; 32], maybe_known: Option<&[u8]>) -> Option<::Call> { - Calls::::get(hash).and_then(|(data, ..)| { - Decode::decode(&mut &data[..]).ok() - }) + if maybe_known.is_some() { + maybe_known.and_then(|data| { + Decode::decode(&mut &data[..]).ok() + }) + } else { + Calls::::get(hash).and_then(|(data, ..)| { + Decode::decode(&mut &data[..]).ok() + }) + } } /// Attempt to remove a call from storage, returning any deposit on it to the owner. diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index 8abf72d78969f..ce71fe7413609 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -163,7 +163,7 @@ fn multisig_deposit_is_taken_and_returned() { 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 call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); 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); @@ -182,8 +182,8 @@ fn multisig_deposit_is_taken_and_returned_with_call_storage() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); 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); @@ -202,8 +202,8 @@ fn multisig_deposit_is_taken_and_returned_with_alt_call_storage() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); assert_eq!(Balances::free_balance(1), 1); @@ -226,8 +226,8 @@ fn multisig_deposit_is_taken_and_returned_with_alt_call_storage() { #[test] fn cancel_multisig_returns_deposit() { new_test_ext().execute_with(|| { - let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); - let hash = call.using_encoded(blake2_256); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); assert_eq!(Balances::free_balance(1), 6); @@ -248,8 +248,8 @@ fn timepoint_checking_works() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_noop!( Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash.clone()), @@ -278,8 +278,8 @@ fn multisig_2_of_3_works_with_call_storing() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call, true)); assert_eq!(Balances::free_balance(6), 0); @@ -296,8 +296,8 @@ fn multisig_2_of_3_works() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash)); assert_eq!(Balances::free_balance(6), 0); @@ -314,8 +314,8 @@ fn multisig_3_of_3_works() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); assert_eq!(Balances::free_balance(6), 0); @@ -328,8 +328,8 @@ fn multisig_3_of_3_works() { #[test] fn cancel_multisig_works() { new_test_ext().execute_with(|| { - let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); - let hash = call.using_encoded(blake2_256); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); assert_noop!( @@ -345,8 +345,8 @@ 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); 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())); @@ -364,8 +364,8 @@ fn cancel_multisig_with_call_storage_works() { #[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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); 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)); @@ -384,7 +384,7 @@ fn multisig_2_of_3_as_multi_works() { 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 call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false)); assert_eq!(Balances::free_balance(6), 0); @@ -401,8 +401,8 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call1 = Box::new(Call::Balances(BalancesCall::transfer(6, 10))); - let call2 = Box::new(Call::Balances(BalancesCall::transfer(7, 5))); + let call1 = Call::Balances(BalancesCall::transfer(6, 10)).encode(); + let call2 = Call::Balances(BalancesCall::transfer(7, 5)).encode(); 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)); @@ -422,7 +422,8 @@ fn multisig_2_of_3_cannot_reissue_same_call() { 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, 10))); + let call = Call::Balances(BalancesCall::transfer(6, 10)).encode(); + let hash = blake2_256(&call); 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); @@ -431,14 +432,14 @@ fn multisig_2_of_3_cannot_reissue_same_call() { assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), call.clone(), false)); let err = DispatchError::from(BalancesError::::InsufficientBalance).stripped(); - expect_event(RawEvent::MultisigExecuted(3, now(), multi, call.using_encoded(blake2_256), Err(err))); + expect_event(RawEvent::MultisigExecuted(3, now(), multi, hash, Err(err))); }); } #[test] fn zero_threshold_fails() { new_test_ext().execute_with(|| { - let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); assert_noop!( Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call, false), Error::::ZeroThreshold, @@ -449,7 +450,7 @@ fn zero_threshold_fails() { #[test] fn too_many_signatories_fails() { new_test_ext().execute_with(|| { - let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); assert_noop!( Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone(), false), Error::::TooManySignatories, @@ -460,8 +461,8 @@ fn too_many_signatories_fails() { #[test] fn duplicate_approvals_are_ignored() { new_test_ext().execute_with(|| { - let call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); - let hash = call.using_encoded(blake2_256); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash.clone())); assert_noop!( Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], Some(now()), hash.clone()), @@ -483,17 +484,18 @@ fn multisig_1_of_3_works() { 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); + let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); + let hash = blake2_256(&call); assert_noop!( Multisig::approve_as_multi(Origin::signed(1), 1, vec![2, 3], None, hash.clone()), - Error::::NoApprovalsNeeded, + Error::::ZeroThreshold, ); assert_noop!( Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone(), false), - BalancesError::::InsufficientBalance, + Error::::ZeroThreshold, ); - assert_ok!(Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call, false)); + let boxed_call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); + assert_ok!(Multisig::as_multi_threshold_1(Origin::signed(1), vec![2, 3], boxed_call)); assert_eq!(Balances::free_balance(6), 15); }); @@ -504,7 +506,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![2], None, call.clone(), false), + Multisig::as_multi_threshold_1(Origin::signed(1), vec![2], call), Error::::Uncallable, ); }); From 09da8720ddc8036250c41a7d08377c2bcd170793 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 00:21:29 +0200 Subject: [PATCH 12/25] Update benchmarks, add working tests --- frame/multisig/src/benchmarking.rs | 74 +++++++++++++++++++++++++----- frame/multisig/src/lib.rs | 2 +- 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index 8b2006c1e63d0..ea077a757d3f0 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -23,13 +23,14 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account}; use sp_runtime::traits::{Bounded, Saturating}; +use core::convert::TryInto; use crate::Module as Multisig; const SEED: u32 = 0; fn setup_multi(s: u32, z: u32) - -> Result<(Vec, Box<::Call>), &'static str> + -> Result<(Vec, Vec), &'static str> { let mut signatories: Vec = Vec::new(); for i in 0 .. s { @@ -41,8 +42,10 @@ fn setup_multi(s: u32, z: u32) signatories.push(signatory); } signatories.sort(); - let call: Box<::Call> = Box::new(frame_system::Call::remark(vec![0; z as usize]).into()); - return Ok((signatories, call)) + // Must first convert to outer call type. + let call: ::Call = frame_system::Call::::remark(vec![0; z as usize]).into(); + let call_data = call.encode(); + return Ok((signatories, call_data)) } benchmarks! { @@ -54,8 +57,13 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let call_hash = blake2_256(&call); + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false) + verify { + assert!(Multisigs::::contains_key(multi_account_id, call_hash)); + } as_multi_create_store { // Signatories, need at least 2 total people @@ -63,16 +71,24 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let call_hash = blake2_256(&call); + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true) + verify { + assert!(Multisigs::::contains_key(multi_account_id, call_hash)); + assert!(Calls::::contains_key(call_hash)); + } as_multi_approve { - // Signatories, need at least 2 people - let s in 2 .. T::MaxSignatories::get() as u32; + // Signatories, need at least 3 people (so we don't complete the multisig) + let s in 3 .. T::MaxSignatories::get() as u32; // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let call_hash = blake2_256(&call); + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let mut signatories2 = signatories.clone(); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; // before the call, get the timepoint @@ -81,6 +97,10 @@ benchmarks! { Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; let caller2 = signatories2.remove(0); }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false) + verify { + let multisig = Multisigs::::get(multi_account_id, call_hash).ok_or("multisig not created")?; + assert_eq!(multisig.approvals.len(), 2); + } as_multi_complete { // Signatories, need at least 2 people @@ -88,6 +108,8 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let call_hash = blake2_256(&call); + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let mut signatories2 = signatories.clone(); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; // before the call, get the timepoint @@ -102,7 +124,11 @@ benchmarks! { Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?; } let caller2 = signatories2.remove(0); + assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false) + verify { + assert!(!Multisigs::::contains_key(&multi_account_id, call_hash)); + } approve_as_multi_create { // Signatories, need at least 2 people @@ -110,10 +136,14 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; - let call_hash = call.using_encoded(blake2_256); + let call_hash = blake2_256(&call); // Create the multi }: approve_as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call_hash) + verify { + assert!(Multisigs::::contains_key(multi_account_id, call_hash)); + } approve_as_multi_approve { // Signatories, need at least 2 people @@ -122,14 +152,19 @@ benchmarks! { let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; let mut signatories2 = signatories.clone(); + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; - let call_hash = call.using_encoded(blake2_256); + let call_hash = blake2_256(&call); // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; + Multisig::::as_multi(RawOrigin::Signed(caller.clone()).into(), s as u16, signatories, None, call.clone(), false)?; let caller2 = signatories2.remove(0); }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash) + verify { + let multisig = Multisigs::::get(multi_account_id, call_hash).ok_or("multisig not created")?; + assert_eq!(multisig.approvals.len(), 2); + } approve_as_multi_complete { // Signatories, need at least 2 people @@ -137,10 +172,11 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let mut signatories2 = signatories.clone(); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - let call_hash = call.using_encoded(blake2_256); + let call_hash = blake2_256(&call); // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi @@ -153,7 +189,11 @@ benchmarks! { Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?; } let caller2 = signatories2.remove(0); + assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash) + verify { + assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); + } cancel_as_multi { // Signatories, need at least 2 people @@ -161,13 +201,18 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; - let call_hash = call.using_encoded(blake2_256); + let call_hash = blake2_256(&call); let timepoint = Multisig::::timepoint(); // Create the multi let o = RawOrigin::Signed(caller.clone()).into(); Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), false)?; + assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); }: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) + verify { + assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); + } cancel_as_multi_store { // Signatories, need at least 2 people @@ -175,14 +220,21 @@ benchmarks! { // Transaction Length let z in 0 .. 10_000; let (mut signatories, call) = setup_multi::(s, z)?; + let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - let call_hash = call.using_encoded(blake2_256); + let call_hash = blake2_256(&call); let timepoint = Multisig::::timepoint(); // Create the multi let o = RawOrigin::Signed(caller.clone()).into(); Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), true)?; + assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); + assert!(Calls::::contains_key(call_hash)); }: cancel_as_multi(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) + verify { + assert!(!Multisigs::::contains_key(&multi_account_id, call_hash)); + assert!(!Calls::::contains_key(call_hash)); + } } #[cfg(test)] diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index dac39ab369f89..c0e52f8459644 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -657,7 +657,7 @@ impl Module { false } - /// Attempt to read a call from storage, returning it. + /// Attempt to decode and return the call, provided by the user or from storage. fn get_call(hash: &[u8; 32], maybe_known: Option<&[u8]>) -> Option<::Call> { if maybe_known.is_some() { maybe_known.and_then(|data| { From 2b5c361770e35d87a7e44f4780ed437e12b53756 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 01:09:13 +0200 Subject: [PATCH 13/25] Add benchmark to threshold 1, add event too --- frame/multisig/src/benchmarking.rs | 28 +++++++++++++++++++++++++- frame/multisig/src/lib.rs | 32 ++++++++++++++++++------------ 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index ea077a757d3f0..f97d3abd7ae3d 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_system::RawOrigin; +use frame_system::{Module as System, RawOrigin, EventRecord}; use frame_benchmarking::{benchmarks, account}; use sp_runtime::traits::{Bounded, Saturating}; use core::convert::TryInto; @@ -29,6 +29,14 @@ use crate::Module as Multisig; const SEED: u32 = 0; +fn assert_last_event(generic_event: ::Event) { + let events = System::::events(); + let system_event: ::Event = generic_event.into(); + // compare to the last event record + let EventRecord { event, .. } = &events[events.len() - 1]; + assert_eq!(event, &system_event); +} + fn setup_multi(s: u32, z: u32) -> Result<(Vec, Vec), &'static str> { @@ -51,6 +59,23 @@ fn setup_multi(s: u32, z: u32) benchmarks! { _ { } + as_multi_threshold_1 { + // Transaction Length + let z in 0 .. 10_000; + let max_signatories = T::MaxSignatories::get().into(); + let (mut signatories, _) = setup_multi::(max_signatories, z)?; + let call: ::Call = frame_system::Call::::remark(vec![0; z as usize]).into(); + let call_hash = call.using_encoded(blake2_256); + let multi_account_id = Multisig::::multi_account_id(&signatories, 1); + let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; + }: _(RawOrigin::Signed(caller.clone()), signatories, Box::new(call)) + verify { + let timepoint = Multisig::::timepoint(); + assert_last_event::( + RawEvent::MultisigExecuted(caller, timepoint, multi_account_id, call_hash, Ok(())).into() + ); + } + as_multi_create { // Signatories, need at least 2 total people let s in 2 .. T::MaxSignatories::get() as u32; @@ -246,6 +271,7 @@ mod tests { #[test] fn test_benchmarks() { new_test_ext().execute_with(|| { + assert_ok!(test_benchmark_as_multi_threshold_1::()); assert_ok!(test_benchmark_as_multi_create::()); assert_ok!(test_benchmark_as_multi_create_store::()); assert_ok!(test_benchmark_as_multi_approve::()); diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index c0e52f8459644..ee251f5683532 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -323,19 +323,25 @@ decl_module! { let _guard = ClearFilterGuard::::Call>::new(); ensure!(T::IsCallable::filter(&call), Error::::Uncallable); - call.dispatch(frame_system::RawOrigin::Signed(id).into()) - .map(|post_dispatch_info| post_dispatch_info.actual_weight - .map(|actual_weight| weight_of::as_multi::(other_signatories_len, actual_weight)) - .into() - ).map_err(|err| match err.post_info.actual_weight { - Some(actual_weight) => { - let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); - let post_info = Some(weight_used).into(); - let error = err.error.into(); - DispatchErrorWithPostInfo { post_info, error } - }, - None => err, - }) + let call_hash = call.using_encoded(blake2_256); + let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); + + Self::deposit_event(RawEvent::MultisigExecuted( + who, Self::timepoint(), id, call_hash, result.map(|_| ()).map_err(|e| e.error) + )); + + result.map(|post_dispatch_info| post_dispatch_info.actual_weight + .map(|actual_weight| weight_of::as_multi::(other_signatories_len, actual_weight)) + .into() + ).map_err(|err| match err.post_info.actual_weight { + Some(actual_weight) => { + let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); + let post_info = Some(weight_used).into(); + let error = err.error.into(); + DispatchErrorWithPostInfo { post_info, error } + }, + None => err, + }) } /// Register approval for a dispatch to be made from a deterministic composite account if From 2cf28328ec72a0b95aa2181dc9fab635cc5d75b5 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 01:21:49 +0200 Subject: [PATCH 14/25] suppress warning for now --- frame/multisig/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index ee251f5683532..44182b99dd9a8 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -188,8 +188,8 @@ mod weight_of { use super::*; pub fn as_multi( - sig_len: usize, - call_weight: Weight, + _sig_len: usize, + _call_weight: Weight, ) -> Weight { 0 } From 9a2d25f647cf1f566c18e3f735b7c16911fffde9 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 01:53:40 +0200 Subject: [PATCH 15/25] @xlc improvment nit --- frame/multisig/src/lib.rs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 44182b99dd9a8..789d7a0615bc6 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -665,15 +665,13 @@ impl Module { /// Attempt to decode and return the call, provided by the user or from storage. fn get_call(hash: &[u8; 32], maybe_known: Option<&[u8]>) -> Option<::Call> { - if maybe_known.is_some() { - maybe_known.and_then(|data| { - Decode::decode(&mut &data[..]).ok() - }) - } else { + maybe_known.map_or_else(|| { Calls::::get(hash).and_then(|(data, ..)| { Decode::decode(&mut &data[..]).ok() }) - } + }, |data| { + Decode::decode(&mut &data[..]).ok() + }) } /// Attempt to remove a call from storage, returning any deposit on it to the owner. From db7554331fe6cd6168266c52a91481a42ab0d6ec Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 08:51:16 +0200 Subject: [PATCH 16/25] Update weight and tests --- frame/multisig/src/benchmarking.rs | 34 ++--- frame/multisig/src/lib.rs | 207 ++++++++++++++--------------- frame/multisig/src/tests.rs | 140 ++++++++++--------- 3 files changed, 198 insertions(+), 183 deletions(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index f97d3abd7ae3d..7b0fc3bb6e6b9 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -85,7 +85,7 @@ benchmarks! { let call_hash = blake2_256(&call); let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; - }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false) + }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false, 0) verify { assert!(Multisigs::::contains_key(multi_account_id, call_hash)); } @@ -100,7 +100,7 @@ benchmarks! { let multi_account_id = Multisig::::multi_account_id(&signatories, s.try_into().unwrap()); let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true) + }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true, 0) verify { assert!(Multisigs::::contains_key(multi_account_id, call_hash)); assert!(Calls::::contains_key(call_hash)); @@ -118,10 +118,10 @@ benchmarks! { let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; // before the call, get the timepoint let timepoint = Multisig::::timepoint(); - // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; + // Create the multi, storing for worst case + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true, 0)?; let caller2 = signatories2.remove(0); - }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false) + }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false, 0) verify { let multisig = Multisigs::::get(multi_account_id, call_hash).ok_or("multisig not created")?; assert_eq!(multisig.approvals.len(), 2); @@ -139,18 +139,18 @@ benchmarks! { let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; // before the call, get the timepoint let timepoint = Multisig::::timepoint(); - // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?; + // Create the multi, storing it for worst case + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true, 0)?; // Everyone except the first person approves for i in 1 .. s - 1 { let mut signatories_loop = signatories2.clone(); let caller_loop = signatories_loop.remove(i as usize); let o = RawOrigin::Signed(caller_loop).into(); - Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?; + Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false, 0)?; } let caller2 = signatories2.remove(0); assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); - }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false) + }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false, Weight::max_value()) verify { assert!(!Multisigs::::contains_key(&multi_account_id, call_hash)); } @@ -165,7 +165,7 @@ benchmarks! { let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; let call_hash = blake2_256(&call); // Create the multi - }: approve_as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call_hash) + }: approve_as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call_hash, 0) verify { assert!(Multisigs::::contains_key(multi_account_id, call_hash)); } @@ -183,9 +183,9 @@ benchmarks! { // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller.clone()).into(), s as u16, signatories, None, call.clone(), false)?; + Multisig::::as_multi(RawOrigin::Signed(caller.clone()).into(), s as u16, signatories, None, call.clone(), false, 0)?; let caller2 = signatories2.remove(0); - }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash) + }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash, 0) verify { let multisig = Multisigs::::get(multi_account_id, call_hash).ok_or("multisig not created")?; assert_eq!(multisig.approvals.len(), 2); @@ -205,17 +205,17 @@ benchmarks! { // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true)?; + Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true, 0)?; // Everyone except the first person approves for i in 1 .. s - 1 { let mut signatories_loop = signatories2.clone(); let caller_loop = signatories_loop.remove(i as usize); let o = RawOrigin::Signed(caller_loop).into(); - Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?; + Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false, 0)?; } let caller2 = signatories2.remove(0); assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); - }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash) + }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash, Weight::max_value()) verify { assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); } @@ -232,7 +232,7 @@ benchmarks! { let timepoint = Multisig::::timepoint(); // Create the multi let o = RawOrigin::Signed(caller.clone()).into(); - Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), false)?; + Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), true, 0)?; assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); }: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) verify { @@ -252,7 +252,7 @@ benchmarks! { let timepoint = Multisig::::timepoint(); // Create the multi let o = RawOrigin::Signed(caller.clone()).into(); - Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), true)?; + Multisig::::as_multi(o, s as u16, signatories.clone(), None, call.clone(), true, 0)?; assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); assert!(Calls::::contains_key(call_hash)); }: cancel_as_multi(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 789d7a0615bc6..823776bf5c293 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -51,7 +51,7 @@ use codec::{Encode, Decode}; use sp_io::hashing::blake2_256; use frame_support::{decl_module, decl_event, decl_error, decl_storage, Parameter, ensure, RuntimeDebug}; use frame_support::{traits::{Get, ReservableCurrency, Currency, Filter, FilterStack, ClearFilterGuard}, - weights::{Weight, GetDispatchInfo, DispatchClass, Pays, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}}, + weights::{Weight, GetDispatchInfo, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}}, dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo}, }; use frame_system::{self as system, ensure_signed}; @@ -157,6 +157,8 @@ decl_error! { UnexpectedTimepoint, /// A call with a `false` `IsCallable` filter was attempted. Uncallable, + /// The maximum weight information was provided was too low. + WeightTooLow, } } @@ -187,41 +189,42 @@ decl_event! { mod weight_of { use super::*; - pub fn as_multi( - _sig_len: usize, - _call_weight: Weight, - ) -> Weight { - 0 + /// - Base Weight: 33.72 + 0.002 * Z µs + /// - DB Weight: None + /// - Plus Call Weight + pub fn as_multi_threshold_1( + call_len: usize, + call_weight: Weight, + ) -> Weight { + (34 * WEIGHT_PER_MICROS) + .saturating_add((2 * WEIGHT_PER_NANOS).saturating_mul(call_len as Weight)) + .saturating_add(call_weight) } /// - Base Weight: - /// - Create: 41.89 + 0.118 * S + .002 * Z µs - /// - Create w/ Store: 53.57 + 0.119 * S + .003 * Z µs - /// - Approve: 31.39 + 0.136 * S + .002 * Z µs - /// - Complete: 39.94 + 0.26 * S + .002 * Z µs + /// - Create: 38.82 + 0.121 * S + .001 * Z µs + /// - Create w/ Store: 54.22 + 0.120 * S + .003 * Z µs + /// - Approve: 29.86 + 0.143 * S + .001 * Z µs + /// - Complete: 39.55 + 0.267 * S + .002 * Z µs /// - DB Weight: - /// - Reads: Multisig Storage, [Caller Account], Calls (if `store_call`) - /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) + /// - Reads: Multisig Storage, [Caller Account], Calls, Depositor Account + /// - Writes: Multisig Storage, [Caller Account], Calls, Depositor Account /// - Plus Call Weight - pub fn as_multi_new( + pub fn as_multi( sig_len: usize, call_len: usize, call_weight: Weight, - store_call: bool, + calls_write: bool, + refunded: bool, ) -> Weight { - if store_call { - return call_weight - .saturating_add(53 * WEIGHT_PER_MICROS) - .saturating_add((250 * WEIGHT_PER_NANOS).saturating_mul(sig_len as Weight)) - .saturating_add((3 * WEIGHT_PER_NANOS).saturating_mul(call_len as Weight)) - .saturating_add(T::DbWeight::get().reads_writes(2, 2)) - } else { - return call_weight - .saturating_add(42 * WEIGHT_PER_MICROS) - .saturating_add((250 * WEIGHT_PER_NANOS).saturating_mul(sig_len as Weight)) - .saturating_add((2 * WEIGHT_PER_NANOS).saturating_mul(call_len as Weight)) - .saturating_add(T::DbWeight::get().reads_writes(1, 1)) - } + call_weight + .saturating_add(55 * WEIGHT_PER_MICROS) + .saturating_add((250 * WEIGHT_PER_NANOS).saturating_mul(sig_len as Weight)) + .saturating_add((3 * WEIGHT_PER_NANOS).saturating_mul(call_len as Weight)) + .saturating_add(T::DbWeight::get().reads_writes(1, 1)) // Multisig read/write + .saturating_add(T::DbWeight::get().reads(1)) // Calls read + .saturating_add(T::DbWeight::get().writes(calls_write.into())) // Calls write + .saturating_add(T::DbWeight::get().reads_writes(refunded.into(), refunded.into())) // Deposit refunded } } @@ -248,62 +251,29 @@ decl_module! { 1_000_000_000 } - /// Register approval for a dispatch to be made from a deterministic composite account if - /// approved by a total of `threshold - 1` of `other_signatories`. - /// - /// If there are enough, then dispatch the call. Calls must each fulfil the `IsCallable` - /// filter. - /// - /// Payment: `DepositBase` will be reserved if this is the first approval, plus - /// `threshold` times `DepositFactor`. It is returned once this dispatch happens or - /// is cancelled. + /// Immediately dispatch a multi-signature call using a single approval from the caller. /// /// The dispatch origin for this call must be _Signed_. /// - /// - `threshold`: The total number of approvals for this dispatch before it is executed. - /// - `other_signatories`: The accounts (other than the sender) who can approve this - /// dispatch. May not be empty. - /// - `maybe_timepoint`: If this is the first approval, then this must be `None`. If it is - /// not the first approval, then it must be `Some`, with the timepoint (block number and - /// transaction index) of the first approval transaction. + /// - `other_signatories`: The accounts (other than the sender) who are part of the + /// multi-signature, but do not participate in the approval process. /// - `call`: The call to be executed. /// - /// NOTE: Unless this is the final approval, you will generally want to use - /// `approve_as_multi` instead, since it only requires a hash of the call. - /// - /// Result is equivalent to the dispatched result if `threshold` is exactly `1`. Otherwise - /// on success, result is `Ok` and the result from the interior call, if it was executed, - /// may be found in the deposited `MultisigExecuted` event. + /// Result is equivalent to the dispatched result. /// /// # - /// - `O(S + Z + Call)`. - /// - Up to one balance-reserve or unreserve operation. - /// - One passthrough operation, one insert, both `O(S)` where `S` is the number of - /// signatories. `S` is capped by `MaxSignatories`, with weight being proportional. - /// - One call encode & hash, both of complexity `O(Z)` where `Z` is tx-len. - /// - One encode & hash, both of complexity `O(S)`. - /// - Up to one binary search and insert (`O(logS + S)`). - /// - I/O: 1 read `O(S)`, up to 1 mutate `O(S)`. Up to one remove. - /// - One event. - /// - The weight of the `call`. - /// - Storage: inserts one item, value size bounded by `MaxSignatories`, with a - /// deposit taken for its lifetime of - /// `DepositBase + threshold * DepositFactor`. + /// O(Z) where Z is the length of the call. /// ------------------------------- - /// - Base Weight: - /// - Create: 41.89 + 0.118 * S + .002 * Z µs - /// - Create w/ Store: 53.57 + 0.119 * S + .003 * Z µs - /// - Approve: 31.39 + 0.136 * S + .002 * Z µs - /// - Complete: 39.94 + 0.26 * S + .002 * Z µs - /// - DB Weight: - /// - Reads: Multisig Storage, [Caller Account], Calls (if `store_call`) - /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) + /// - Base Weight: 33.72 + 0.002 * Z µs + /// - DB Weight: None /// - Plus Call Weight /// # #[weight = ( - 0, + weight_of::as_multi_threshold_1::( + call.using_encoded(|c| c.len()), + call.get_dispatch_info().weight + ), call.get_dispatch_info().class, - Pays::Yes, )] fn as_multi_threshold_1(origin, other_signatories: Vec, @@ -323,7 +293,7 @@ decl_module! { let _guard = ClearFilterGuard::::Call>::new(); ensure!(T::IsCallable::filter(&call), Error::::Uncallable); - let call_hash = call.using_encoded(blake2_256); + let (call_len, call_hash) = call.using_encoded(|c| (c.len(), blake2_256(&c))); let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); Self::deposit_event(RawEvent::MultisigExecuted( @@ -331,11 +301,17 @@ decl_module! { )); result.map(|post_dispatch_info| post_dispatch_info.actual_weight - .map(|actual_weight| weight_of::as_multi::(other_signatories_len, actual_weight)) + .map(|actual_weight| weight_of::as_multi_threshold_1::( + call_len, + actual_weight, + )) .into() ).map_err(|err| match err.post_info.actual_weight { Some(actual_weight) => { - let weight_used = weight_of::as_multi::(other_signatories_len, actual_weight); + let weight_used = weight_of::as_multi_threshold_1::( + call_len, + actual_weight, + ); let post_info = Some(weight_used).into(); let error = err.error.into(); DispatchErrorWithPostInfo { post_info, error } @@ -396,23 +372,23 @@ decl_module! { /// - Writes: Multisig Storage, [Caller Account], Calls (if `store_call`) /// - Plus Call Weight /// # - #[weight = - weight_of::as_multi_new::( - other_signatories.len(), - call.using_encoded(|x| x.len()), - 0, - *store_call - ) - ] + #[weight = weight_of::as_multi::( + other_signatories.len(), + call.len(), + *max_weight, + true, + true, + )] fn as_multi(origin, threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, call: Vec, store_call: bool, + max_weight: Weight, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Call(call, store_call)) + Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Call(call, store_call), max_weight) } /// Register approval for a dispatch to be made from a deterministic composite account if @@ -454,21 +430,22 @@ decl_module! { /// - Read: Multisig Storage, [Caller Account] /// - Write: Multisig Storage, [Caller Account] /// # - #[weight = ( - T::DbWeight::get().reads_writes(1, 1) - .saturating_add(45_000_000) - .saturating_add((other_signatories.len() as Weight).saturating_mul(120_000)), - DispatchClass::Normal, - Pays::Yes, + #[weight = weight_of::as_multi::( + other_signatories.len(), + 0, // call_len is zero in this case + *max_weight, + true, + true, )] fn approve_as_multi(origin, threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, call_hash: [u8; 32], + max_weight: Weight, ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Hash(call_hash)) + Self::operate(who, threshold, other_signatories, maybe_timepoint, CallOrHash::Hash(call_hash), max_weight) } /// Cancel a pre-existing, on-going multisig transaction. Any deposit reserved previously @@ -493,18 +470,15 @@ decl_module! { /// - I/O: 1 read `O(S)`, one remove. /// - Storage: removes one item. /// ---------------------------------- - /// - Base Weight: 37.6 + 0.084 * S + /// - Base Weight: 36.07 + 0.124 * S /// - DB Weight: - /// - Read: Multisig Storage, [Caller Account] - /// - Write: Multisig Storage, [Caller Account] + /// - Read: Multisig Storage, [Caller Account], Refund Account, Calls + /// - Write: Multisig Storage, [Caller Account], Refund Account, Calls /// # - #[weight = ( - T::DbWeight::get().reads_writes(1, 1) - .saturating_add(40_000_000) - .saturating_add((other_signatories.len() as Weight).saturating_mul(100_000)), - DispatchClass::Normal, - Pays::Yes, - )] + #[weight = T::DbWeight::get().reads_writes(3, 3) + .saturating_add(36 * WEIGHT_PER_MICROS) + .saturating_add((other_signatories.len() as Weight).saturating_mul(100 * WEIGHT_PER_NANOS)) + ] fn cancel_as_multi(origin, threshold: u16, other_signatories: Vec, @@ -551,6 +525,7 @@ impl Module { other_signatories: Vec, maybe_timepoint: Option>, call_or_hash: CallOrHash, + max_weight: Weight, ) -> DispatchResultWithPostInfo { ensure!(threshold >= 2, Error::::ZeroThreshold); let max_sigs = T::MaxSignatories::get() as usize; @@ -562,12 +537,13 @@ impl Module { let id = Self::multi_account_id(&signatories, threshold); // Threshold > 1; this means it's a multi-step operation. We extract the `call_hash`. - let (call_hash, maybe_call, store) = match call_or_hash { + let (call_hash, call_len, maybe_call, store) = match call_or_hash { CallOrHash::Call(call, should_store) => { let call_hash = blake2_256(&call); - (call_hash, Some(call), should_store) + let call_len = call.len(); + (call_hash, call_len, Some(call), should_store) } - CallOrHash::Hash(h) => (h, None, false), + CallOrHash::Hash(h) => (h, 0, None, false), }; // Branch on whether the operation has already started or not. @@ -589,6 +565,8 @@ impl Module { } else { None }; if let Some(call) = maybe_approved_call { + // verify weight + ensure!(call.get_dispatch_info().weight <= max_weight, Error::::WeightTooLow); // We're now executing as a freshly authenticated new account, so the previous call // restrictions no longer apply. let _guard = ClearFilterGuard::::Call>::new(); @@ -623,7 +601,13 @@ impl Module { } // Call is not made, so the actual weight does not include call - Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) + Ok(Some(weight_of::as_multi::( + other_signatories_len, + call_len, + 0, + stored, // Call stored? + false, // No refund + )).into()) } } else { // Not yet started; there should be no timepoint given. @@ -632,7 +616,12 @@ impl Module { // Just start the operation by recording it in storage. let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into(); T::Currency::reserve(&who, deposit)?; - maybe_call.filter(|_| store).map(|d| Self::store_call(who.clone(), &call_hash, d)); + + // Store the call if desired. + let stored = maybe_call + .filter(|_| store) + .map_or(false, |data| Self::store_call(who.clone(), &call_hash, data)); + >::insert(&id, call_hash, Multisig { when: Self::timepoint(), deposit, @@ -641,7 +630,13 @@ impl Module { }); Self::deposit_event(RawEvent::NewMultisig(who, id, call_hash)); // Call is not made, so we can return that weight - return Ok(Some(weight_of::as_multi::(other_signatories_len, 0)).into()) + return Ok(Some(weight_of::as_multi::( + other_signatories_len, + call_len, + 0, + stored, // Call stored? + false, // No refund + )).into()) } } diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index ce71fe7413609..0d0a3bb33cdbf 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -163,12 +163,14 @@ fn multisig_deposit_is_taken_and_returned() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false)); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data.clone(), false, 0)); 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, false)); + assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), data, false, call_weight)); assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::reserved_balance(1), 0); }); @@ -182,13 +184,15 @@ fn multisig_deposit_is_taken_and_returned_with_call_storage() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - let hash = blake2_256(&call); - assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call, true)); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data, true, 0)); 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_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash, call_weight)); assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::reserved_balance(1), 0); }); @@ -202,20 +206,22 @@ fn multisig_deposit_is_taken_and_returned_with_alt_call_storage() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - let hash = blake2_256(&call); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone(), 0)); 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_ok!(Multisig::as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), data, true, 0)); 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_ok!(Multisig::approve_as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), hash, call_weight)); assert_eq!(Balances::free_balance(1), 5); assert_eq!(Balances::reserved_balance(1), 0); assert_eq!(Balances::free_balance(2), 5); @@ -228,8 +234,8 @@ fn cancel_multisig_returns_deposit() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); let hash = blake2_256(&call); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); - assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone(), 0)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone(), 0)); assert_eq!(Balances::free_balance(1), 6); assert_eq!(Balances::reserved_balance(1), 4); assert_ok!( @@ -252,19 +258,19 @@ fn timepoint_checking_works() { let hash = blake2_256(&call); assert_noop!( - Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash.clone()), + Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash.clone(), 0), Error::::UnexpectedTimepoint, ); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash, 0)); assert_noop!( - Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, call.clone(), false), + Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, call.clone(), false, 0), Error::::NoTimepoint, ); let later = Timepoint { index: 1, .. now() }; assert_noop!( - Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(later), call.clone(), false), + Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(later), call.clone(), false, 0), Error::::WrongTimepoint, ); }); @@ -278,12 +284,14 @@ fn multisig_2_of_3_works_with_call_storing() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - let hash = blake2_256(&call); - assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call, true)); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data, true, 0)); assert_eq!(Balances::free_balance(6), 0); - assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash, call_weight)); assert_eq!(Balances::free_balance(6), 15); }); } @@ -296,12 +304,14 @@ fn multisig_2_of_3_works() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - let hash = blake2_256(&call); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash)); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash, 0)); assert_eq!(Balances::free_balance(6), 0); - assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call, false)); + assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), data, false, call_weight)); assert_eq!(Balances::free_balance(6), 15); }); } @@ -314,13 +324,15 @@ fn multisig_3_of_3_works() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - let hash = blake2_256(&call); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); - assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone(), 0)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone(), 0)); assert_eq!(Balances::free_balance(6), 0); - assert_ok!(Multisig::as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), call, false)); + assert_ok!(Multisig::as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), data, false, call_weight)); assert_eq!(Balances::free_balance(6), 15); }); } @@ -330,8 +342,8 @@ fn cancel_multisig_works() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); let hash = blake2_256(&call); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); - assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone(), 0)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone(), 0)); assert_noop!( Multisig::cancel_as_multi(Origin::signed(2), 3, vec![1, 3], now(), hash.clone()), Error::::NotOwner, @@ -347,9 +359,9 @@ fn cancel_multisig_with_call_storage_works() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); let hash = blake2_256(&call); - assert_ok!(Multisig::as_multi(Origin::signed(1), 3, vec![2, 3], None, call, true)); + assert_ok!(Multisig::as_multi(Origin::signed(1), 3, vec![2, 3], None, call, true, 0)); 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_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone(), 0)); assert_noop!( Multisig::cancel_as_multi(Origin::signed(2), 3, vec![1, 3], now(), hash.clone()), Error::::NotOwner, @@ -366,9 +378,9 @@ fn cancel_multisig_with_alt_call_storage_works() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); let hash = blake2_256(&call); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone())); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone(), 0)); assert_eq!(Balances::free_balance(1), 6); - assert_ok!(Multisig::as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), call, true)); + assert_ok!(Multisig::as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), call, true, 0)); 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); @@ -384,11 +396,13 @@ fn multisig_2_of_3_as_multi_works() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); - assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone(), false)); + let call = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data.clone(), false, 0)); assert_eq!(Balances::free_balance(6), 0); - assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), call, false)); + assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), data, false, call_weight)); assert_eq!(Balances::free_balance(6), 15); }); } @@ -401,13 +415,17 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call1 = Call::Balances(BalancesCall::transfer(6, 10)).encode(); - let call2 = Call::Balances(BalancesCall::transfer(7, 5)).encode(); + let call1 = Call::Balances(BalancesCall::transfer(6, 10)); + let call1_weight = call1.get_dispatch_info().weight; + let data1 = call1.encode(); + let call2 = Call::Balances(BalancesCall::transfer(7, 5)); + let call2_weight = call2.get_dispatch_info().weight; + let data2 = call2.encode(); - 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_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data1.clone(), false, 0)); + assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], None, data2.clone(), false, 0)); + assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), data1, false, call1_weight)); + assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), data2, false, call2_weight)); assert_eq!(Balances::free_balance(6), 10); assert_eq!(Balances::free_balance(7), 5); @@ -422,14 +440,16 @@ fn multisig_2_of_3_cannot_reissue_same_call() { assert_ok!(Balances::transfer(Origin::signed(2), multi, 5)); assert_ok!(Balances::transfer(Origin::signed(3), multi, 5)); - let call = Call::Balances(BalancesCall::transfer(6, 10)).encode(); - let hash = blake2_256(&call); - 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)); + let call = Call::Balances(BalancesCall::transfer(6, 10)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data.clone(), false, 0)); + assert_ok!(Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), data.clone(), false, call_weight)); assert_eq!(Balances::free_balance(multi), 5); - 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)); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data.clone(), false, 0)); + assert_ok!(Multisig::as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), data.clone(), false, call_weight)); let err = DispatchError::from(BalancesError::::InsufficientBalance).stripped(); expect_event(RawEvent::MultisigExecuted(3, now(), multi, hash, Err(err))); @@ -441,7 +461,7 @@ fn zero_threshold_fails() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); assert_noop!( - Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call, false), + Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call, false, 0), Error::::ZeroThreshold, ); }); @@ -452,7 +472,7 @@ fn too_many_signatories_fails() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); assert_noop!( - Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone(), false), + Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone(), false, 0), Error::::TooManySignatories, ); }); @@ -463,14 +483,14 @@ fn duplicate_approvals_are_ignored() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); let hash = blake2_256(&call); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash.clone())); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash.clone(), 0)); assert_noop!( - Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], Some(now()), hash.clone()), + Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], Some(now()), hash.clone(), 0), Error::::AlreadyApproved, ); - assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash.clone())); + assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), hash.clone(), 0)); assert_noop!( - Multisig::approve_as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), hash.clone()), + Multisig::approve_as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), hash.clone(), 0), Error::::AlreadyApproved, ); }); @@ -487,11 +507,11 @@ fn multisig_1_of_3_works() { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); let hash = blake2_256(&call); assert_noop!( - Multisig::approve_as_multi(Origin::signed(1), 1, vec![2, 3], None, hash.clone()), + Multisig::approve_as_multi(Origin::signed(1), 1, vec![2, 3], None, hash.clone(), 0), Error::::ZeroThreshold, ); assert_noop!( - Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone(), false), + Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone(), false, 0), Error::::ZeroThreshold, ); let boxed_call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); From 6275b9ca987384d146d67861a1cb200b0e64a9a8 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 09:45:52 +0200 Subject: [PATCH 17/25] Test for weight check --- frame/multisig/src/tests.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index 0d0a3bb33cdbf..ccec76f60f7e1 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -531,3 +531,23 @@ fn multisig_filters() { ); }); } + +#[test] +fn weight_check_works() { + 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 = Call::Balances(BalancesCall::transfer(6, 15)); + let data = call.encode(); + assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, data.clone(), false, 0)); + assert_eq!(Balances::free_balance(6), 0); + + assert_noop!( + Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(now()), data, false, 0), + Error::::WeightTooLow, + ); + }); +} From 81195da8d3e5b44ce5afd0da99ac770b455b023e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 10:26:41 +0200 Subject: [PATCH 18/25] Fix line width --- frame/multisig/src/benchmarking.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index 7b0fc3bb6e6b9..c913344933c83 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -183,7 +183,15 @@ benchmarks! { // before the call, get the timepoint let timepoint = Multisig::::timepoint(); // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller.clone()).into(), s as u16, signatories, None, call.clone(), false, 0)?; + Multisig::::as_multi( + RawOrigin::Signed(caller.clone()).into(), + s as u16, + signatories, + None, + call.clone(), + false, + 0 + )?; let caller2 = signatories2.remove(0); }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash, 0) verify { From 7a75cf7f89484089f7a3fdc5aedf4b0595fbead5 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 12:30:39 +0200 Subject: [PATCH 19/25] one more line width error --- frame/multisig/src/benchmarking.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index c913344933c83..799a5a89fd51d 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -223,7 +223,14 @@ benchmarks! { } let caller2 = signatories2.remove(0); assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); - }: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash, Weight::max_value()) + }: approve_as_multi( + RawOrigin::Signed(caller2), + s as u16, + signatories2, + Some(timepoint), + call_hash, + Weight::max_value() + ) verify { assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); } From 9dd05851f2d3e332ddbd5e54a02dfa6af103638c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 17:30:52 +0200 Subject: [PATCH 20/25] Apply suggestions from code review Co-authored-by: Alexander Popiak --- frame/multisig/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 823776bf5c293..4a69780ba0c68 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -157,7 +157,7 @@ decl_error! { UnexpectedTimepoint, /// A call with a `false` `IsCallable` filter was attempted. Uncallable, - /// The maximum weight information was provided was too low. + /// The maximum weight information provided was too low. WeightTooLow, } } @@ -262,7 +262,7 @@ decl_module! { /// Result is equivalent to the dispatched result. /// /// # - /// O(Z) where Z is the length of the call. + /// O(Z + C) where Z is the length of the call and C its execution weight. /// ------------------------------- /// - Base Weight: 33.72 + 0.002 * Z µs /// - DB Weight: None @@ -376,8 +376,8 @@ decl_module! { other_signatories.len(), call.len(), *max_weight, - true, - true, + true, // assume worst case: calls write + true, // assume worst case: refunded )] fn as_multi(origin, threshold: u16, @@ -434,8 +434,8 @@ decl_module! { other_signatories.len(), 0, // call_len is zero in this case *max_weight, - true, - true, + true, // assume worst case: calls write + true, // assume worst case: refunded )] fn approve_as_multi(origin, threshold: u16, From f64e26d216fdcfb0859ce787a6a45bf5918f77c2 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 18:27:37 +0200 Subject: [PATCH 21/25] fix merge --- frame/multisig/src/benchmarking.rs | 15 ++------------- frame/multisig/src/lib.rs | 6 +----- 2 files changed, 3 insertions(+), 18 deletions(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index 799a5a89fd51d..9479c16cb2b07 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -20,7 +20,7 @@ #![cfg(feature = "runtime-benchmarks")] use super::*; -use frame_system::{Module as System, RawOrigin, EventRecord}; +use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account}; use sp_runtime::traits::{Bounded, Saturating}; use core::convert::TryInto; @@ -29,14 +29,6 @@ use crate::Module as Multisig; const SEED: u32 = 0; -fn assert_last_event(generic_event: ::Event) { - let events = System::::events(); - let system_event: ::Event = generic_event.into(); - // compare to the last event record - let EventRecord { event, .. } = &events[events.len() - 1]; - assert_eq!(event, &system_event); -} - fn setup_multi(s: u32, z: u32) -> Result<(Vec, Vec), &'static str> { @@ -70,10 +62,7 @@ benchmarks! { let caller = signatories.pop().ok_or("signatories should have len 2 or more")?; }: _(RawOrigin::Signed(caller.clone()), signatories, Box::new(call)) verify { - let timepoint = Multisig::::timepoint(); - assert_last_event::( - RawEvent::MultisigExecuted(caller, timepoint, multi_account_id, call_hash, Ok(())).into() - ); + // If the benchmark resolves, then the call was dispatched successfully. } as_multi_create { diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index a37c969b03717..db6d402a7f430 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -281,13 +281,9 @@ decl_module! { let id = Self::multi_account_id(&signatories, 1); - let (call_len, call_hash) = call.using_encoded(|c| (c.len(), blake2_256(&c))); + let call_len = call.using_encoded(|c| c.len()); let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); - Self::deposit_event(RawEvent::MultisigExecuted( - who, Self::timepoint(), id, call_hash, result.map(|_| ()).map_err(|e| e.error) - )); - result.map(|post_dispatch_info| post_dispatch_info.actual_weight .map(|actual_weight| weight_of::as_multi_threshold_1::( call_len, From 9becf6e98e4c05a4d59bc11f7d3d207b90fa1ba5 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 15 Jun 2020 18:37:30 +0200 Subject: [PATCH 22/25] more @apopiak feedback --- frame/multisig/src/lib.rs | 14 +++++++------- frame/multisig/src/tests.rs | 16 ++++++++++------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index db6d402a7f430..9814fa92c34ac 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -54,7 +54,7 @@ use frame_support::{traits::{Get, ReservableCurrency, Currency}, weights::{Weight, GetDispatchInfo, constants::{WEIGHT_PER_NANOS, WEIGHT_PER_MICROS}}, dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo}, }; -use frame_system::{self as system, ensure_signed}; +use frame_system::{self as system, ensure_signed, RawOrigin}; use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable}; mod tests; @@ -128,8 +128,8 @@ decl_storage! { decl_error! { pub enum Error for Module { - /// Threshold is too low (zero). - ZeroThreshold, + /// Threshold must be 2 or greater. + MinimumThreshold, /// Call is already approved by this signatory. AlreadyApproved, /// Call doesn't need any (more) approvals. @@ -282,7 +282,7 @@ decl_module! { let id = Self::multi_account_id(&signatories, 1); let call_len = call.using_encoded(|c| c.len()); - let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); + let result = call.dispatch(RawOrigin::Signed(id.clone()).into()); result.map(|post_dispatch_info| post_dispatch_info.actual_weight .map(|actual_weight| weight_of::as_multi_threshold_1::( @@ -469,7 +469,7 @@ decl_module! { call_hash: [u8; 32], ) -> DispatchResult { let who = ensure_signed(origin)?; - ensure!(threshold >= 2, Error::::ZeroThreshold); + ensure!(threshold >= 2, Error::::MinimumThreshold); let max_sigs = T::MaxSignatories::get() as usize; ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); ensure!(other_signatories.len() < max_sigs, Error::::TooManySignatories); @@ -510,7 +510,7 @@ impl Module { call_or_hash: CallOrHash, max_weight: Weight, ) -> DispatchResultWithPostInfo { - ensure!(threshold >= 2, Error::::ZeroThreshold); + ensure!(threshold >= 2, Error::::MinimumThreshold); let max_sigs = T::MaxSignatories::get() as usize; ensure!(!other_signatories.is_empty(), Error::::TooFewSignatories); let other_signatories_len = other_signatories.len(); @@ -551,7 +551,7 @@ impl Module { // verify weight ensure!(call.get_dispatch_info().weight <= max_weight, Error::::WeightTooLow); - let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into()); + let result = call.dispatch(RawOrigin::Signed(id.clone()).into()); let _ = T::Currency::unreserve(&m.depositor, m.deposit); >::remove(&id, call_hash); Self::clear_call(&call_hash); diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index 626e3b473ff4a..cdacfe4f588ca 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -450,12 +450,16 @@ fn multisig_2_of_3_cannot_reissue_same_call() { } #[test] -fn zero_threshold_fails() { +fn minimum_threshold_check_works() { new_test_ext().execute_with(|| { let call = Call::Balances(BalancesCall::transfer(6, 15)).encode(); assert_noop!( - Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call, false, 0), - Error::::ZeroThreshold, + Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call.clone(), false, 0), + Error::::MinimumThreshold, + ); + assert_noop!( + Multisig::as_multi(Origin::signed(1), 1, vec![2], None, call.clone(), false, 0), + Error::::MinimumThreshold, ); }); } @@ -501,11 +505,11 @@ fn multisig_1_of_3_works() { let hash = blake2_256(&call); assert_noop!( Multisig::approve_as_multi(Origin::signed(1), 1, vec![2, 3], None, hash.clone(), 0), - Error::::ZeroThreshold, + Error::::MinimumThreshold, ); assert_noop!( - Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone(), false, 0), - Error::::ZeroThreshold, + Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call.clone(), false, 0), + Error::::MinimumThreshold, ); let boxed_call = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); assert_ok!(Multisig::as_multi_threshold_1(Origin::signed(1), vec![2, 3], boxed_call)); From 5bd6514ff5dcbb1a054c0615331477db6b3d3b9c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 16 Jun 2020 13:51:16 +0200 Subject: [PATCH 23/25] Multisig handles no preimage --- frame/multisig/src/tests.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index cdacfe4f588ca..4911ca90cf33d 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -548,3 +548,27 @@ fn weight_check_works() { ); }); } + +#[test] +fn multisig_handles_no_preimage_after_all_approve() { + // This test checks the situation where everyone approves a multi-sig, but no-one provides the call data. + // In the end, any of the multisig callers can approve again with the call data and the call will go through. + 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 = Call::Balances(BalancesCall::transfer(6, 15)); + let call_weight = call.get_dispatch_info().weight; + let data = call.encode(); + let hash = blake2_256(&data); + assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 3, vec![2, 3], None, hash.clone(), 0)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone(), 0)); + assert_ok!(Multisig::approve_as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), hash.clone(), 0)); + assert_eq!(Balances::free_balance(6), 0); + + assert_ok!(Multisig::as_multi(Origin::signed(3), 3, vec![1, 2], Some(now()), data, false, call_weight)); + assert_eq!(Balances::free_balance(6), 15); + }); +} From 5cc17da77c3b20667cd010e1a81c9783afc1e812 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Tue, 16 Jun 2020 15:26:39 +0200 Subject: [PATCH 24/25] Optimize return weight after dispatch --- frame/multisig/src/lib.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 9814fa92c34ac..caf2d10edcb8e 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -558,7 +558,13 @@ impl Module { Self::deposit_event(RawEvent::MultisigExecuted( who, timepoint, id, call_hash, result.map(|_| ()).map_err(|e| e.error) )); - Ok(None.into()) + Ok(get_result_weight(result).map(|actual_weight| weight_of::as_multi::( + other_signatories_len, + call_len, + actual_weight, + true, // Call is removed + true, // User is refunded + )).into()) } else { // We cannot dispatch the call now; either it isn't available, or it is, but we // don't have threshold approvals even with our signature. @@ -684,3 +690,13 @@ impl Module { Ok(signatories) } } + +/// Return the weight of a dispatch call result as an `Option`. +/// +/// Will return the weight regardless of what the state of the result is. +fn get_result_weight(result: DispatchResultWithPostInfo) -> Option { + match result { + Ok(post_info) => post_info.actual_weight, + Err(err) => err.post_info.actual_weight, + } +} From 9e2d5b673bdf53d11dd9724e50be69f915d4ef90 Mon Sep 17 00:00:00 2001 From: Gav Wood Date: Tue, 16 Jun 2020 22:46:23 +0200 Subject: [PATCH 25/25] Error on failed deposit. --- frame/multisig/src/lib.rs | 46 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 20 deletions(-) diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index caf2d10edcb8e..50bd96aca3c53 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -55,7 +55,7 @@ use frame_support::{traits::{Get, ReservableCurrency, Currency}, dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo}, }; use frame_system::{self as system, ensure_signed, RawOrigin}; -use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable}; +use sp_runtime::{DispatchError, DispatchResult, traits::{Dispatchable, Zero}}; mod tests; mod benchmarking; @@ -154,6 +154,8 @@ decl_error! { UnexpectedTimepoint, /// The maximum weight information provided was too low. WeightTooLow, + /// The data to be stored is already stored. + AlreadyStored, } } @@ -552,7 +554,7 @@ impl Module { ensure!(call.get_dispatch_info().weight <= max_weight, Error::::WeightTooLow); let result = call.dispatch(RawOrigin::Signed(id.clone()).into()); - let _ = T::Currency::unreserve(&m.depositor, m.deposit); + T::Currency::unreserve(&m.depositor, m.deposit); >::remove(&id, call_hash); Self::clear_call(&call_hash); Self::deposit_event(RawEvent::MultisigExecuted( @@ -570,9 +572,12 @@ impl Module { // don't have threshold approvals even with our signature. // Store the call if desired. - let stored = maybe_call - .filter(|_| store) - .map_or(false, |data| Self::store_call(who.clone(), &call_hash, data)); + let stored = if let Some(data) = maybe_call.filter(|_| store) { + Self::store_call_and_reserve(who.clone(), &call_hash, data, BalanceOf::::zero())?; + true + } else { + false + }; if let Some(pos) = maybe_pos { // Record approval. @@ -600,12 +605,15 @@ impl Module { // Just start the operation by recording it in storage. let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into(); - T::Currency::reserve(&who, deposit)?; // Store the call if desired. - let stored = maybe_call - .filter(|_| store) - .map_or(false, |data| Self::store_call(who.clone(), &call_hash, data)); + let stored = if let Some(data) = maybe_call.filter(|_| store) { + Self::store_call_and_reserve(who.clone(), &call_hash, data, deposit)?; + true + } else { + T::Currency::reserve(&who, deposit)?; + false + }; >::insert(&id, call_hash, Multisig { when: Self::timepoint(), @@ -630,17 +638,15 @@ impl Module { /// We store `data` here because storing `call` would result in needing another `.encode`. /// /// Returns a `bool` indicating whether the data did end up being stored. - fn store_call(who: T::AccountId, hash: &[u8; 32], data: Vec) -> bool { - if !Calls::::contains_key(hash) { - let deposit = T::DepositBase::get() - + T::DepositFactor::get() * BalanceOf::::from(((data.len() + 31) / 32) as u32); - if T::Currency::reserve(&who, deposit).is_ok() { - // Only store the data if the deposit was paid. - Calls::::insert(&hash, (data, who, deposit)); - return true - } - } - false + fn store_call_and_reserve(who: T::AccountId, hash: &[u8; 32], data: Vec, other_deposit: BalanceOf) + -> DispatchResult + { + ensure!(!Calls::::contains_key(hash), Error::::AlreadyStored); + let deposit = other_deposit + T::DepositBase::get() + + T::DepositFactor::get() * BalanceOf::::from(((data.len() + 31) / 32) as u32); + T::Currency::reserve(&who, deposit)?; + Calls::::insert(&hash, (data, who, deposit)); + Ok(()) } /// Attempt to decode and return the call, provided by the user or from storage.