From 17be6fd5e5dec2b358dbffa9ce07e4bc1d3e01e2 Mon Sep 17 00:00:00 2001 From: Gavin Wood Date: Wed, 17 Jun 2020 12:24:32 +0200 Subject: [PATCH] Stored call in multisig (#6319) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Stored call in multisig * Docs. * Benchmarks. * Fix * Update frame/multisig/src/lib.rs Co-authored-by: Bastian Köcher * patch benchmarks * Minor grumbles. * Update as_multi weight * Fixes and refactoring. * Split out threshold=1 and opaquify Call. * Compiles, tests pass, weights are broken * Update benchmarks, add working tests * Add benchmark to threshold 1, add event too * suppress warning for now * @xlc improvment nit * Update weight and tests * Test for weight check * Fix line width * one more line width error * Apply suggestions from code review Co-authored-by: Alexander Popiak * fix merge * more @apopiak feedback * Multisig handles no preimage * Optimize return weight after dispatch * Error on failed deposit. Co-authored-by: Bastian Köcher Co-authored-by: Shawn Tabrizi Co-authored-by: Alexander Popiak --- frame/multisig/src/benchmarking.rs | 176 +++++++++-- frame/multisig/src/lib.rs | 475 +++++++++++++++++++---------- frame/multisig/src/tests.rs | 295 ++++++++++++++---- 3 files changed, 708 insertions(+), 238 deletions(-) diff --git a/frame/multisig/src/benchmarking.rs b/frame/multisig/src/benchmarking.rs index fa2ec52e6b2c5..9479c16cb2b07 100644 --- a/frame/multisig/src/benchmarking.rs +++ b/frame/multisig/src/benchmarking.rs @@ -22,14 +22,15 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account}; -use sp_runtime::traits::Saturating; +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,36 +42,79 @@ 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! { _ { } + 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 { + // If the benchmark resolves, then the call was dispatched successfully. + } + as_multi_create { // 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 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) + }: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false, 0) + verify { + assert!(Multisigs::::contains_key(multi_account_id, call_hash)); + } - as_multi_approve { - // Signatories, need at least 2 people + 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 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, 0) + verify { + assert!(Multisigs::::contains_key(multi_account_id, call_hash)); + assert!(Calls::::contains_key(call_hash)); + } + + as_multi_approve { + // 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 let timepoint = Multisig::::timepoint(); - // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone())?; + // 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) + }: 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); + } as_multi_complete { // Signatories, need at least 2 people @@ -78,21 +122,27 @@ 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 let timepoint = Multisig::::timepoint(); - // Create the multi - Multisig::::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone())?; + // 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())?; + Multisig::::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false, 0)?; } let caller2 = signatories2.remove(0); - }: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call) + assert!(Multisigs::::contains_key(&multi_account_id, call_hash)); + }: 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)); + } approve_as_multi_create { // Signatories, need at least 2 people @@ -100,10 +150,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) + }: approve_as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call_hash, 0) + verify { + assert!(Multisigs::::contains_key(multi_account_id, call_hash)); + } approve_as_multi_approve { // Signatories, need at least 2 people @@ -112,14 +166,63 @@ 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())?; + 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); + } + + 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 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 = 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(), 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, 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, + Weight::max_value() + ) + verify { + assert!(!Multisigs::::contains_key(multi_account_id, call_hash)); + } cancel_as_multi { // Signatories, need at least 2 people @@ -127,13 +230,40 @@ 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())?; + 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 { + assert!(!Multisigs::::contains_key(multi_account_id, 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 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 = 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, 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) + verify { + assert!(!Multisigs::::contains_key(&multi_account_id, call_hash)); + assert!(!Calls::::contains_key(call_hash)); + } } #[cfg(test)] @@ -145,12 +275,16 @@ 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::()); 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::()); }); } } diff --git a/frame/multisig/src/lib.rs b/frame/multisig/src/lib.rs index 672e6bed205e2..50bd96aca3c53 100644 --- a/frame/multisig/src/lib.rs +++ b/frame/multisig/src/lib.rs @@ -51,11 +51,11 @@ 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}, - weights::{Weight, GetDispatchInfo, DispatchClass, Pays}, + weights::{Weight, GetDispatchInfo, constants::{WEIGHT_PER_NANOS, WEIGHT_PER_MICROS}}, dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo}, }; -use frame_system::{self as system, ensure_signed}; -use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable}; +use frame_system::{self as system, ensure_signed, RawOrigin}; +use sp_runtime::{DispatchError, DispatchResult, traits::{Dispatchable, Zero}}; mod tests; mod benchmarking; @@ -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. @@ -119,13 +121,15 @@ 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)>; } } 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. @@ -148,6 +152,10 @@ decl_error! { WrongTimepoint, /// A timepoint was given, yet no multisig operation is underway. UnexpectedTimepoint, + /// The maximum weight information provided was too low. + WeightTooLow, + /// The data to be stored is already stored. + AlreadyStored, } } @@ -176,22 +184,50 @@ decl_event! { mod weight_of { use super::*; + /// - 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: 46.55 + 0.089 * S µs - /// - Approve: 34.03 + .112 * S µs - /// - Complete: 40.36 + .225 * S µ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] - /// - Writes: Multisig Storage, [Caller Account] + /// - Reads: Multisig Storage, [Caller Account], Calls, Depositor Account + /// - Writes: Multisig Storage, [Caller Account], Calls, Depositor Account /// - Plus Call Weight - pub fn as_multi(other_sig_len: usize, call_weight: Weight) -> Weight { + pub fn as_multi( + sig_len: usize, + call_len: usize, + call_weight: Weight, + calls_write: bool, + refunded: bool, + ) -> 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)) + .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 } } +enum CallOrHash { + Call(Vec, bool), + Hash([u8; 32]), +} + decl_module! { pub struct Module for enum Call where origin: T::Origin { type Error = Error; @@ -210,6 +246,66 @@ decl_module! { 1_000_000_000 } + /// Immediately dispatch a multi-signature call using a single approval from the caller. + /// + /// The dispatch origin for this call must be _Signed_. + /// + /// - `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. + /// + /// Result is equivalent to the dispatched result. + /// + /// # + /// 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 + /// - Plus Call Weight + /// # + #[weight = ( + weight_of::as_multi_threshold_1::( + call.using_encoded(|c| c.len()), + call.get_dispatch_info().weight + ), + call.get_dispatch_info().class, + )] + fn as_multi_threshold_1(origin, + other_signatories: Vec, + 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(); + 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); + + let call_len = call.using_encoded(|c| c.len()); + 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::( + call_len, + actual_weight, + )) + .into() + ).map_err(|err| match err.post_info.actual_weight { + Some(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 } + }, + 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`. /// @@ -252,99 +348,32 @@ 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), - call.get_dispatch_info().class, - Pays::Yes, + #[weight = weight_of::as_multi::( + other_signatories.len(), + call.len(), + *max_weight, + true, // assume worst case: calls write + true, // assume worst case: refunded )] fn as_multi(origin, threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, - call: Box<::Call>, + call: Vec, + store_call: bool, + max_weight: Weight, ) -> 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); - 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 call_hash = call.using_encoded(blake2_256); - - 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); - 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::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()], - }); - 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), max_weight) } /// Register approval for a dispatch to be made from a deterministic composite account if @@ -386,57 +415,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, // assume worst case: calls write + true, // assume worst case: refunded )] fn approve_as_multi(origin, threshold: u16, other_signatories: Vec, maybe_timepoint: Option>, call_hash: [u8; 32], - ) -> DispatchResult { + max_weight: Weight, + ) -> 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) { - 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), max_weight) } /// Cancel a pre-existing, on-going multisig transaction. Any deposit reserved previously @@ -461,18 +455,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, @@ -480,7 +471,7 @@ decl_module! { call_hash: [u8; 32], ) -> DispatchResult { let who = ensure_signed(origin)?; - ensure!(threshold >= 1, 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); @@ -494,7 +485,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::clear_call(&call_hash); Self::deposit_event(RawEvent::MultisigCancelled(who, timepoint, id, call_hash)); Ok(()) @@ -512,6 +504,169 @@ 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, + max_weight: Weight, + ) -> DispatchResultWithPostInfo { + 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(); + 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); + + // Threshold > 1; this means it's a multi-step operation. We extract the `call_hash`. + let (call_hash, call_len, maybe_call, store) = match call_or_hash { + CallOrHash::Call(call, should_store) => { + let call_hash = blake2_256(&call); + let call_len = call.len(); + (call_hash, call_len, Some(call), should_store) + } + CallOrHash::Hash(h) => (h, 0, None, false), + }; + + // 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 { + Self::get_call(&call_hash, maybe_call.as_ref().map(|c| c.as_ref())) + } else { None }; + + if let Some(call) = maybe_approved_call { + // verify weight + ensure!(call.get_dispatch_info().weight <= max_weight, Error::::WeightTooLow); + + let result = call.dispatch(RawOrigin::Signed(id.clone()).into()); + 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(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. + + // Store the call if desired. + 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. + 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, + call_len, + 0, + stored, // Call stored? + false, // No refund + )).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(); + + // Store the call if desired. + 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(), + 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, + call_len, + 0, + stored, // Call stored? + false, // No refund + )).into()) + } + } + + /// Place a call's encoded data in storage, reserving funds as appropriate. + /// + /// 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_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. + fn get_call(hash: &[u8; 32], maybe_known: Option<&[u8]>) -> Option<::Call> { + 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. + 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 { @@ -541,3 +696,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, + } +} diff --git a/frame/multisig/src/tests.rs b/frame/multisig/src/tests.rs index 4b1395476db31..4911ca90cf33d 100644 --- a/frame/multisig/src/tests.rs +++ b/frame/multisig/src/tests.rs @@ -156,24 +156,79 @@ 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))); - assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone())); + 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)); + 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); }); } +#[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 = 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, call_weight)); + 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 = 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_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()), 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, call_weight)); + 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(|| { - 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_ok!(Multisig::approve_as_multi(Origin::signed(2), 3, vec![1, 3], Some(now()), hash.clone())); + 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(), 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!( @@ -192,28 +247,48 @@ 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()), + 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()), + 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()), + Multisig::as_multi(Origin::signed(2), 2, vec![1, 3], Some(later), call.clone(), false, 0), 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 = 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, call_weight)); + assert_eq!(Balances::free_balance(6), 15); + }); +} + #[test] fn multisig_2_of_3_works() { new_test_ext().execute_with(|| { @@ -222,12 +297,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 = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); - let hash = call.using_encoded(blake2_256); - 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)); + 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); }); } @@ -240,13 +317,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 = 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_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)); + 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); }); } @@ -254,10 +333,10 @@ 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); - 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)).encode(); + let hash = blake2_256(&call); + 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, @@ -268,6 +347,40 @@ fn cancel_multisig_works() { }); } +#[test] +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, 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(), 0)); + 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 = 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(), 0)); + assert_eq!(Balances::free_balance(1), 6); + 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); + assert_eq!(Balances::free_balance(2), 10); + }); +} + #[test] fn multisig_2_of_3_as_multi_works() { new_test_ext().execute_with(|| { @@ -276,11 +389,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 = Box::new(Call::Balances(BalancesCall::transfer(6, 15))); - assert_ok!(Multisig::as_multi(Origin::signed(1), 2, vec![2, 3], None, call.clone())); + 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)); + 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); }); } @@ -293,13 +408,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 = 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)); + 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())); - 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, 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); @@ -314,26 +433,33 @@ 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))); - 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())); + 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())); - 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, 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, call.using_encoded(blake2_256), Err(err))); + expect_event(RawEvent::MultisigExecuted(3, now(), multi, hash, Err(err))); }); } #[test] -fn zero_threshold_fails() { +fn minimum_threshold_check_works() { 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.clone(), false, 0), + Error::::MinimumThreshold, + ); assert_noop!( - Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call), - Error::::ZeroThreshold, + Multisig::as_multi(Origin::signed(1), 1, vec![2], None, call.clone(), false, 0), + Error::::MinimumThreshold, ); }); } @@ -341,9 +467,9 @@ 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()), + Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone(), false, 0), Error::::TooManySignatories, ); }); @@ -352,17 +478,17 @@ 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); - assert_ok!(Multisig::approve_as_multi(Origin::signed(1), 2, vec![2, 3], None, hash.clone())); + 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(), 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()), - Error::::NoApprovalsNeeded, + Multisig::approve_as_multi(Origin::signed(3), 2, vec![1, 2], Some(now()), hash.clone(), 0), + Error::::AlreadyApproved, ); }); } @@ -375,17 +501,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, + Multisig::approve_as_multi(Origin::signed(1), 1, vec![2, 3], None, hash.clone(), 0), + Error::::MinimumThreshold, ); assert_noop!( - Multisig::as_multi(Origin::signed(4), 1, vec![2, 3], None, call.clone()), - BalancesError::::InsufficientBalance, + Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call.clone(), false, 0), + Error::::MinimumThreshold, ); - assert_ok!(Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call)); + 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); }); @@ -396,8 +523,52 @@ 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()), + Multisig::as_multi_threshold_1(Origin::signed(1), vec![2], call.clone()), DispatchError::BadOrigin, ); }); } + +#[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, + ); + }); +} + +#[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); + }); +}