relaxing timepoint constraint for multisig

substrate/frame/multisig/src/lib.rs

@@ -206,8 +206,6 @@ pub mod pallet {
 		NotFound,
 		/// Only the account that originally created the multisig is able to cancel it.
 		NotOwner,
-		/// No timepoint was given, yet the multisig operation is already underway.
-		NoTimepoint,
 		/// A different timepoint was given to the multisig operation that is underway.
 		WrongTimepoint,
 		/// A timepoint was given, yet no multisig operation is underway.
@@ -226,22 +224,22 @@ pub mod pallet {
 		/// A multisig operation has been approved by someone.
 		MultisigApproval {
 			approving: T::AccountId,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			multisig: T::AccountId,
 			call_hash: CallHash,
 		},
 		/// A multisig operation has been executed.
 		MultisigExecuted {
 			approving: T::AccountId,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			multisig: T::AccountId,
 			call_hash: CallHash,
 			result: DispatchResult,
 		},
 		/// A multisig operation has been cancelled.
 		MultisigCancelled {
 			cancelling: T::AccountId,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			multisig: T::AccountId,
 			call_hash: CallHash,
 		},
@@ -328,8 +326,9 @@ pub mod pallet {
 		/// - `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.
+		/// not the first approval, then it can be `Some`, with the timepoint (block number and
+		/// transaction index) of the first approval transaction. When provided, timepoint will serve
+		/// as an additional layer of security.
 		/// - `call`: The call to be executed.
 		///
 		/// NOTE: Unless this is the final approval, you will generally want to use
@@ -394,8 +393,9 @@ pub mod pallet {
 		/// - `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.
+		/// not the first approval, then it can be `Some`, with the timepoint (block number and
+		/// transaction index) of the first approval transaction. When provided, timepoint will serve
+		/// as an additional layer of security.
 		/// - `call_hash`: The hash of the call to be executed.
 		///
 		/// NOTE: If this is the final approval, you will want to use `as_multi` instead.
@@ -446,8 +446,9 @@ pub mod pallet {
 		/// - `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.
-		/// - `timepoint`: The timepoint (block number and transaction index) of the first approval
-		/// transaction for this dispatch.
+		/// - `maybe_timepoint`: The timepoint (block number and transaction index) of the first approval
+		/// transaction for this dispatch. When provided, timepoint will serve as an additional
+		/// layer of security.
 		/// - `call_hash`: The hash of the call to be executed.
 		///
 		/// ## Complexity
@@ -465,7 +466,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			threshold: u16,
 			other_signatories: Vec<T::AccountId>,
-			timepoint: Timepoint<BlockNumberFor<T>>,
+			maybe_timepoint: Option<Timepoint<BlockNumberFor<T>>>,
 			call_hash: [u8; 32],
 		) -> DispatchResult {
 			let who = ensure_signed(origin)?;
@@ -478,7 +479,9 @@ pub mod pallet {
 			let id = Self::multi_account_id(&signatories, threshold);
 
 			let m = <Multisigs<T>>::get(&id, call_hash).ok_or(Error::<T>::NotFound)?;
-			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
+			if let Some(timepoint) = maybe_timepoint.as_ref() {
+				ensure!(m.when == *timepoint, Error::<T>::WrongTimepoint);
+			}
 			ensure!(m.depositor == who, Error::<T>::NotOwner);
 
 			let err_amount = T::Currency::unreserve(&m.depositor, m.deposit);
@@ -487,7 +490,7 @@ pub mod pallet {
 
 			Self::deposit_event(Event::MultisigCancelled {
 				cancelling: who,
-				timepoint,
+				timepoint: maybe_timepoint,
 				multisig: id,
 				call_hash,
 			});
@@ -535,9 +538,10 @@ impl<T: Config> Pallet<T> {
 
 		// Branch on whether the operation has already started or not.
 		if let Some(mut m) = <Multisigs<T>>::get(&id, call_hash) {
-			// Yes; ensure that the timepoint exists and agrees.
-			let timepoint = maybe_timepoint.ok_or(Error::<T>::NoTimepoint)?;
-			ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
+			if let Some(timepoint) = maybe_timepoint.as_ref() {
+				// if the optional timepoint is supplied, it should be the correct one
+				ensure!(m.when == *timepoint, Error::<T>::WrongTimepoint);
+			}
 
 			// Ensure that either we have not yet signed or that it is at threshold.
 			let mut approvals = m.approvals.len() as u16;
@@ -564,7 +568,7 @@ impl<T: Config> Pallet<T> {
 				let result = call.dispatch(RawOrigin::Signed(id.clone()).into());
 				Self::deposit_event(Event::MultisigExecuted {
 					approving: who,
-					timepoint,
+					timepoint: maybe_timepoint,
 					multisig: id,
 					call_hash,
 					result: result.map(|_| ()).map_err(|e| e.error),
@@ -590,7 +594,7 @@ impl<T: Config> Pallet<T> {
 					<Multisigs<T>>::insert(&id, call_hash, m);
 					Self::deposit_event(Event::MultisigApproval {
 						approving: who,
-						timepoint,
+						timepoint: maybe_timepoint,
 						multisig: id,
 						call_hash,
 					});

substrate/frame/multisig/src/tests.rs

@@ -153,7 +153,13 @@ fn cancel_multisig_returns_deposit() {
 		));
 		assert_eq!(Balances::free_balance(1), 6);
 		assert_eq!(Balances::reserved_balance(1), 4);
-		assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash));
+		assert_ok!(Multisig::cancel_as_multi(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			Some(now()),
+			hash
+		));
 		assert_eq!(Balances::free_balance(1), 10);
 		assert_eq!(Balances::reserved_balance(1), 0);
 	});
@@ -168,6 +174,7 @@ fn timepoint_checking_works() {
 		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5));
 
 		let call = call_transfer(6, 15);
+		let call_weight = call.get_dispatch_info().weight;
 		let hash = blake2_256(&call.encode());
 
 		assert_noop!(
@@ -191,29 +198,29 @@ fn timepoint_checking_works() {
 			Weight::zero()
 		));
 
-		assert_noop!(
-			Multisig::as_multi(
-				RuntimeOrigin::signed(2),
-				2,
-				vec![1, 3],
-				None,
-				call.clone(),
-				Weight::zero()
-			),
-			Error::<Test>::NoTimepoint,
-		);
 		let later = Timepoint { index: 1, ..now() };
 		assert_noop!(
 			Multisig::as_multi(
 				RuntimeOrigin::signed(2),
 				2,
 				vec![1, 3],
 				Some(later),
-				call,
+				call.clone(),
 				Weight::zero()
 			),
 			Error::<Test>::WrongTimepoint,
 		);
+
+		assert_ok!(Multisig::as_multi(
+			RuntimeOrigin::signed(2),
+			2,
+			vec![1, 3],
+			None,
+			call,
+			call_weight
+		));
+
+		assert_eq!(Balances::free_balance(6), 15);
 	});
 }
 
@@ -250,6 +257,47 @@ fn multisig_2_of_3_works() {
 	});
 }
 
+#[test]
+fn multisig_3_of_3_without_timepoint_works() {
+	new_test_ext().execute_with(|| {
+		let multi = Multisig::multi_account_id(&[1, 2, 3][..], 3);
+		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(1), multi, 5));
+		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(2), multi, 5));
+		assert_ok!(Balances::transfer_allow_death(RuntimeOrigin::signed(3), multi, 5));
+
+		let call = call_transfer(6, 15);
+		let call_weight = call.get_dispatch_info().weight;
+		let hash = blake2_256(&call.encode());
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(3),
+			3,
+			vec![1, 2],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_eq!(Balances::free_balance(6), 0);
+
+		assert_ok!(Multisig::as_multi(
+			RuntimeOrigin::signed(2),
+			3,
+			vec![1, 3],
+			None,
+			call,
+			call_weight
+		));
+		assert_eq!(Balances::free_balance(6), 15);
+	});
+}
+
 #[test]
 fn multisig_3_of_3_works() {
 	new_test_ext().execute_with(|| {
@@ -313,10 +361,45 @@ fn cancel_multisig_works() {
 			Weight::zero()
 		));
 		assert_noop!(
-			Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], now(), hash),
+			Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], Some(now()), hash),
+			Error::<Test>::NotOwner,
+		);
+		assert_ok!(Multisig::cancel_as_multi(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			Some(now()),
+			hash
+		));
+	});
+}
+
+#[test]
+fn cancel_multisig_without_timepoint_works() {
+	new_test_ext().execute_with(|| {
+		let call = call_transfer(6, 15).encode();
+		let hash = blake2_256(&call);
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(1),
+			3,
+			vec![2, 3],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_ok!(Multisig::approve_as_multi(
+			RuntimeOrigin::signed(2),
+			3,
+			vec![1, 3],
+			None,
+			hash,
+			Weight::zero()
+		));
+		assert_noop!(
+			Multisig::cancel_as_multi(RuntimeOrigin::signed(2), 3, vec![1, 3], None, hash),
 			Error::<Test>::NotOwner,
 		);
-		assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], now(), hash),);
+		assert_ok!(Multisig::cancel_as_multi(RuntimeOrigin::signed(1), 3, vec![2, 3], None, hash));
 	});
 }
 
@@ -452,7 +535,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
 		System::assert_last_event(
 			pallet_multisig::Event::MultisigExecuted {
 				approving: 3,
-				timepoint: now(),
+				timepoint: Some(now()),
 				multisig: multi,
 				call_hash: hash,
 				result: Err(TokenError::FundsUnavailable.into()),