rename & helper function

frame/multisig/src/benchmarking.rs

@@ -210,7 +210,7 @@ benchmarks! {
 		assert!(!Multisigs::<T>::contains_key(multi_account_id, call_hash));
 	}
 
-	clear_multi {
+	clear_expired_multi {
 		// Signatories, need at least 2 people
 		let s in 2 .. T::MaxSignatories::get();
 		// Transaction Length, not a component
@@ -229,7 +229,7 @@ benchmarks! {
 		// 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());
-		// For calling clear_multi all the signatories must be specified.
+		// For calling clear_expired_multi all the signatories must be specified.
 		signatories.push(caller.clone());
 	}: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash)
 	verify {

frame/multisig/src/lib.rs

@@ -507,9 +507,9 @@ pub mod pallet {
 			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
 			ensure!(m.depositor == who, Error::<T>::NotOwner);
 
-			let err_amount = T::Currency::unreserve(&m.depositor, m.deposit);
-			debug_assert!(err_amount.is_zero());
-			<Multisigs<T>>::remove(&id, &call_hash);
+			let maybe_expiry = <MultisigExpiries<T>>::get(&id, call_hash);
+
+			Self::remove_multisig(&id, call_hash, &m.depositor, m.deposit, maybe_expiry);
 
 			Self::deposit_event(Event::MultisigCancelled { timepoint, multisig: id, call_hash });
 			Ok(())
@@ -596,7 +596,7 @@ pub mod pallet {
 		/// - Storage: removes one item.
 		#[pallet::call_index(5)]
 		#[pallet::weight(T::WeightInfo::cancel_as_multi(signatories.len() as u32))]
-		pub fn clear_multi(
+		pub fn clear_expired_multi(
 			origin: OriginFor<T>,
 			threshold: u16,
 			signatories: Vec<T::AccountId>,
@@ -618,10 +618,7 @@ pub mod pallet {
 			);
 
 			// Clean up all the state that is not needed anymore since the multisig expired.
-			<Multisigs<T>>::remove(&id, call_hash);
-			<MultisigExpiries<T>>::remove(&id, call_hash);
-
-			T::Currency::unreserve(&m.depositor, m.deposit);
+			Self::remove_multisig(&id, call_hash, &m.depositor, m.deposit, Some(expiry));
 
 			Self::deposit_event(Event::MultisigCancelled { timepoint, multisig: id, call_hash });
 
@@ -675,12 +672,15 @@ impl<T: Config> Pallet<T> {
 			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
 
 			// Ensure that the mutlisig did not expire.
-			if let Some(expiry) = <MultisigExpiries<T>>::get(&id, call_hash) {
-				ensure!(
-					expiry >= <frame_system::Pallet<T>>::block_number(),
-					Error::<T>::MultisigExpired
-				);
-			}
+			match <MultisigExpiries<T>>::get(&id, call_hash) {
+				Some(expiry) if expiry < <frame_system::Pallet<T>>::block_number() => {
+					// If the multisig is expired we will take the chance to remove it now so that
+					// the multisig creator does not have to make a separate call to clean it up.
+					Self::remove_multisig(&id, call_hash, &m.depositor, m.deposit, maybe_expiry);
+					return Err(Error::<T>::MultisigExpired.into())
+				},
+				_ => {},
+			};
 
 			// Ensure that either we have not yet signed or that it is at threshold.
 			let mut approvals = m.approvals.len() as u16;
@@ -799,6 +799,26 @@ impl<T: Config> Pallet<T> {
 		}
 	}
 
+	/// Removes a multisig operation from the state.
+	///
+	/// If the multisig had an expiry it will also get removed from the
+	/// `MultisigExpiries` storage map.
+	fn remove_multisig(
+		id: &T::AccountId,
+		call_hash: [u8; 32],
+		depositor: &T::AccountId,
+		deposit: BalanceOf<T>,
+		expiry: Option<BlockNumberFor<T>>,
+	) {
+		let err_amount = T::Currency::unreserve(&depositor, deposit);
+		debug_assert!(err_amount.is_zero());
+		<Multisigs<T>>::remove(id, call_hash);
+
+		if expiry.is_some() {
+			<MultisigExpiries<T>>::remove(id, call_hash);
+		}
+	}
+
 	/// The current `Timepoint`.
 	pub fn timepoint() -> Timepoint<BlockNumberFor<T>> {
 		Timepoint {

frame/multisig/src/tests.rs

@@ -287,7 +287,13 @@ fn mutlisig_with_expiry_works() {
 
 		// Cannot clear the multisig operation since it has not expired yet.
 		assert_noop!(
-			Multisig::clear_multi(RuntimeOrigin::signed(42), 2, vec![1, 2, 3], timepoint, hash),
+			Multisig::clear_expired_multi(
+				RuntimeOrigin::signed(42),
+				2,
+				vec![1, 2, 3],
+				timepoint,
+				hash
+			),
 			Error::<Test>::MultisigNotExpired
 		);
 
@@ -355,7 +361,7 @@ fn mutlisig_wont_get_executed_when_expired() {
 			Error::<Test>::MultisigExpired
 		);
 
-		assert_ok!(Multisig::clear_multi(
+		assert_ok!(Multisig::clear_expired_multi(
 			RuntimeOrigin::signed(42),
 			2,
 			vec![1, 2, 3],
@@ -374,7 +380,13 @@ fn mutlisig_wont_get_executed_when_expired() {
 
 		// Cannot clear the multisig operation since it does not longer exist.
 		assert_noop!(
-			Multisig::clear_multi(RuntimeOrigin::signed(42), 2, vec![1, 2, 3], timepoint, hash),
+			Multisig::clear_expired_multi(
+				RuntimeOrigin::signed(42),
+				2,
+				vec![1, 2, 3],
+				timepoint,
+				hash
+			),
 			Error::<Test>::NotFound
 		);
 	})