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

Multisig: Fix tests and re-introduce reserve logic #12241

Merged
merged 4 commits into from
Sep 16, 2022
Merged

Multisig: Fix tests and re-introduce reserve logic #12241

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7bacd73
Fix tests and re-introduce reserve logic
ruseinov Sep 12, 2022
e9ab03f
fix benches
ruseinov Sep 13, 2022
d2776bd
add todo
ruseinov Sep 13, 2022
42acac2
remove irrelevant bench
ruseinov Sep 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 22 additions & 71 deletions frame/multisig/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ const SEED: u32 = 0;
fn setup_multi<T: Config>(
s: u32,
z: u32,
) -> Result<(Vec<T::AccountId>, OpaqueCall<T>), &'static str> {
) -> Result<(Vec<T::AccountId>, Box<<T as Config>::Call>), &'static str> {
let mut signatories: Vec<T::AccountId> = Vec::new();
for i in 0..s {
let signatory = account("signatory", i, SEED);
Expand All @@ -44,8 +44,7 @@ fn setup_multi<T: Config>(
// Must first convert to outer call type.
let call: <T as Config>::Call =
frame_system::Call::<T>::remark { remark: vec![0; z as usize] }.into();
let call_data = OpaqueCall::<T>::from_encoded(call.encode());
Ok((signatories, call_data))
Ok((signatories, Box::new(call)))
}

benchmarks! {
Expand Down Expand Up @@ -74,35 +73,15 @@ benchmarks! {
// Transaction Length
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let call_hash = blake2_256(call.encoded());
let multi_account_id = Multisig::<T>::multi_account_id(&signatories, s.try_into().unwrap());
let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false, Weight::zero())
verify {
assert!(Multisigs::<T>::contains_key(multi_account_id, call_hash));
assert!(!Calls::<T>::contains_key(call_hash));
}

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::<T>(s, z)?;
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
let multi_account_id = Multisig::<T>::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::<T>::max_value());
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true, Weight::zero())
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, Weight::zero())
verify {
assert!(Multisigs::<T>::contains_key(multi_account_id, call_hash));
assert!(Calls::<T>::contains_key(call_hash));
}

as_multi_approve {
Expand All @@ -111,49 +90,22 @@ benchmarks! {
// Transaction Length
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let call_hash = blake2_256(call.encoded());
let multi_account_id = Multisig::<T>::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::<T>::timepoint();
// Create the multi, storing for worst case
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true, Weight::zero())?;
assert!(Calls::<T>::contains_key(call_hash));
let caller2 = signatories2.remove(0);
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller2);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false, Weight::zero())
verify {
let multisig = Multisigs::<T>::get(multi_account_id, call_hash).ok_or("multisig not created")?;
assert_eq!(multisig.approvals.len(), 2);
}

as_multi_approve_store {
// 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::<T>(s, z)?;
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
let multi_account_id = Multisig::<T>::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::<T>::timepoint();
// Create the multi, not storing
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false, Weight::zero())?;
assert!(!Calls::<T>::contains_key(call_hash));
// Create the multi
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), Weight::zero())?;
let caller2 = signatories2.remove(0);
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller2);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, true, Weight::zero())
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, Weight::zero())
verify {
let multisig = Multisigs::<T>::get(multi_account_id, call_hash).ok_or("multisig not created")?;
assert_eq!(multisig.approvals.len(), 2);
assert!(Calls::<T>::contains_key(call_hash));
}

as_multi_complete {
Expand All @@ -162,27 +114,27 @@ benchmarks! {
// Transaction Length
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
let multi_account_id = Multisig::<T>::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::<T>::timepoint();
// Create the multi, storing it for worst case
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true, Weight::zero())?;
// Create the multi
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), Weight::zero())?;
// 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::<T>::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false, Weight::zero())?;
Multisig::<T>::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), Weight::zero())?;
}
let caller2 = signatories2.remove(0);
assert!(Multisigs::<T>::contains_key(&multi_account_id, call_hash));
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller2);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false, Weight::MAX)
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, Weight::MAX)
verify {
assert!(!Multisigs::<T>::contains_key(&multi_account_id, call_hash));
}
Expand All @@ -195,7 +147,7 @@ benchmarks! {
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let multi_account_id = Multisig::<T>::multi_account_id(&signatories, s.try_into().unwrap());
let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
Expand All @@ -214,7 +166,7 @@ benchmarks! {
let mut signatories2 = signatories.clone();
let multi_account_id = Multisig::<T>::multi_account_id(&signatories, s.try_into().unwrap());
let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
// before the call, get the timepoint
let timepoint = Multisig::<T>::timepoint();
// Create the multi
Expand All @@ -224,7 +176,6 @@ benchmarks! {
signatories,
None,
call,
false,
Weight::zero()
)?;
let caller2 = signatories2.remove(0);
Expand All @@ -237,6 +188,8 @@ benchmarks! {
assert_eq!(multisig.approvals.len(), 2);
}

/// TODO: in the documentation it's said that the last approval should be `as_multi` which
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
/// is clearly not the case here. Shall this be removed?
approve_as_multi_complete {
// Signatories, need at least 2 people
let s in 2 .. T::MaxSignatories::get() as u32;
Expand All @@ -247,17 +200,17 @@ benchmarks! {
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::<T>::max_value());
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
// before the call, get the timepoint
let timepoint = Multisig::<T>::timepoint();
// Create the multi
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), true, Weight::zero())?;
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), Weight::zero())?;
// 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::<T>::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false, Weight::zero())?;
Multisig::<T>::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), Weight::zero())?;
}
let caller2 = signatories2.remove(0);
assert!(Multisigs::<T>::contains_key(&multi_account_id, call_hash));
Expand All @@ -284,20 +237,18 @@ benchmarks! {
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let multi_account_id = Multisig::<T>::multi_account_id(&signatories, s.try_into().unwrap());
let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
let call_hash = blake2_256(call.encoded());
let call_hash = call.using_encoded(blake2_256);
let timepoint = Multisig::<T>::timepoint();
// Create the multi
let o = RawOrigin::Signed(caller.clone()).into();
Multisig::<T>::as_multi(o, s as u16, signatories.clone(), None, call, true, Weight::zero())?;
Multisig::<T>::as_multi(o, s as u16, signatories.clone(), None, call, Weight::zero())?;
assert!(Multisigs::<T>::contains_key(&multi_account_id, call_hash));
assert!(Calls::<T>::contains_key(call_hash));
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash)
verify {
assert!(!Multisigs::<T>::contains_key(multi_account_id, call_hash));
assert!(!Calls::<T>::contains_key(call_hash));
}

impl_benchmark_test_suite!(Multisig, crate::tests::new_test_ext(), crate::tests::Test);
Expand Down
10 changes: 7 additions & 3 deletions frame/multisig/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ use frame_support::{
DispatchErrorWithPostInfo, DispatchResult, DispatchResultWithPostInfo, PostDispatchInfo,
},
ensure,
traits::{Currency, Get, ReservableCurrency, WrapperKeepOpaque},
traits::{Currency, Get, ReservableCurrency},
weights::{GetDispatchInfo, Weight},
RuntimeDebug,
};
Expand Down Expand Up @@ -592,7 +592,8 @@ impl<T: Config> Pallet<T> {
Err(Error::<T>::AlreadyApproved)?
}

let final_weight = T::WeightInfo::as_multi_approve(other_signatories_len as u32, call_len as u32);
let final_weight =
T::WeightInfo::as_multi_approve(other_signatories_len as u32, call_len as u32);
// Call is not made, so the actual weight does not include call
Ok(Some(final_weight).into())
}
Expand All @@ -603,6 +604,8 @@ impl<T: Config> Pallet<T> {
// Just start the operation by recording it in storage.
let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into();

T::Currency::reserve(&who, deposit)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

only logical change of this PR, which seems correct to me.

Copy link
Member

Choose a reason for hiding this comment

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

agreed.


<Multisigs<T>>::insert(
&id,
call_hash,
Expand All @@ -615,7 +618,8 @@ impl<T: Config> Pallet<T> {
);
Self::deposit_event(Event::NewMultisig { approving: who, multisig: id, call_hash });

let final_weight = T::WeightInfo::as_multi_create(other_signatories_len as u32, call_len as u32);
let final_weight =
T::WeightInfo::as_multi_create(other_signatories_len as u32, call_len as u32);
// Call is not made, so the actual weight does not include call
Ok(Some(final_weight).into())
}
Expand Down
Loading