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

* Fix tests and re-introduce reserve logic

* fix benches

* add todo

* remove irrelevant bench

Author: ruseinov

frame/multisig/src/benchmarking.rs

@@ -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);
@@ -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! {
@@ -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 {
@@ -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 {
@@ -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));
 	}
@@ -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());
@@ -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
@@ -224,7 +176,6 @@ benchmarks! {
 			signatories,
 			None,
 			call,
-			false,
 			Weight::zero()
 		)?;
 		let caller2 = signatories2.remove(0);
@@ -237,45 +188,6 @@ benchmarks! {
 		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, not a component
-		let z = 10_000;
-		let (mut signatories, call) = setup_multi::<T>(s, z)?;
-		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")?;
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
-		let call_hash = blake2_256(call.encoded());
-		// 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())?;
-		// 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())?;
-		}
-		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());
-	}: approve_as_multi(
-		RawOrigin::Signed(caller2),
-		s as u16,
-		signatories2,
-		Some(timepoint),
-		call_hash,
-		Weight::MAX
-	)
-	verify {
-		assert!(!Multisigs::<T>::contains_key(multi_account_id, call_hash));
-	}
-
 	cancel_as_multi {
 		// Signatories, need at least 2 people
 		let s in 2 .. T::MaxSignatories::get() as u32;
@@ -284,20 +196,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);

frame/multisig/src/lib.rs

@@ -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,
 };
@@ -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())
 			}
@@ -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)?;
+
 			<Multisigs<T>>::insert(
 				&id,
 				call_hash,
@@ -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())
 		}