pallet-multisig: Improve opaque call handling (#10060)

* pallet-multisig: Improve opaque call handling

Before the opaque call was just a type redefinition of `Vec<u8>`. With metadata v14 that was
breaking external tools, as they stopped looking at the type name. To improve the situation the
`WrapperKeepOpaque` type is introduced that communicates to the outside the correct type info.

* Cleanup

* Fix benchmarks

* FMT

substrate/frame/multisig/src/benchmarking.rs

@@ -29,7 +29,10 @@ use crate::Pallet as Multisig;
 
 const SEED: u32 = 0;
 
-fn setup_multi<T: Config>(s: u32, z: u32) -> Result<(Vec<T::AccountId>, Vec<u8>), &'static str> {
+fn setup_multi<T: Config>(
+	s: u32,
+	z: u32,
+) -> Result<(Vec<T::AccountId>, OpaqueCall<T>), &'static str> {
 	let mut signatories: Vec<T::AccountId> = Vec::new();
 	for i in 0..s {
 		let signatory = account("signatory", i, SEED);
@@ -42,7 +45,7 @@ fn setup_multi<T: Config>(s: u32, z: u32) -> Result<(Vec<T::AccountId>, Vec<u8>)
 	// 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 = call.encode();
+	let call_data = OpaqueCall::<T>::from_encoded(call.encode());
 	return Ok((signatories, call_data))
 }
 
@@ -72,7 +75,7 @@ benchmarks! {
 		// Transaction Length
 		let z in 0 .. 10_000;
 		let (mut signatories, call) = setup_multi::<T>(s, z)?;
-		let call_hash = blake2_256(&call);
+		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.
@@ -90,7 +93,7 @@ benchmarks! {
 		// Transaction Length
 		let z in 0 .. 10_000;
 		let (mut signatories, call) = setup_multi::<T>(s, z)?;
-		let call_hash = blake2_256(&call);
+		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")?;
 		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
@@ -109,7 +112,7 @@ benchmarks! {
 		// Transaction Length
 		let z in 0 .. 10_000;
 		let (mut signatories, call) = setup_multi::<T>(s, z)?;
-		let call_hash = blake2_256(&call);
+		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")?;
@@ -134,7 +137,7 @@ benchmarks! {
 		// Transaction Length
 		let z in 0 .. 10_000;
 		let (mut signatories, call) = setup_multi::<T>(s, z)?;
-		let call_hash = blake2_256(&call);
+		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")?;
@@ -160,7 +163,7 @@ benchmarks! {
 		// Transaction Length
 		let z in 0 .. 10_000;
 		let (mut signatories, call) = setup_multi::<T>(s, z)?;
-		let call_hash = blake2_256(&call);
+		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")?;
@@ -193,7 +196,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);
+		let call_hash = blake2_256(&call.encoded());
 		// 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());
@@ -212,7 +215,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);
+		let call_hash = blake2_256(&call.encoded());
 		// before the call, get the timepoint
 		let timepoint = Multisig::<T>::timepoint();
 		// Create the multi
@@ -245,7 +248,7 @@ 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);
+		let call_hash = blake2_256(&call.encoded());
 		// before the call, get the timepoint
 		let timepoint = Multisig::<T>::timepoint();
 		// Create the multi
@@ -282,7 +285,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);
+		let call_hash = blake2_256(&call.encoded());
 		let timepoint = Multisig::<T>::timepoint();
 		// Create the multi
 		let o = RawOrigin::Signed(caller.clone()).into();

substrate/frame/multisig/src/lib.rs

@@ -56,7 +56,7 @@ use frame_support::{
 		DispatchErrorWithPostInfo, DispatchResult, DispatchResultWithPostInfo, PostDispatchInfo,
 	},
 	ensure,
-	traits::{Currency, Get, ReservableCurrency},
+	traits::{Currency, Get, ReservableCurrency, WrapperKeepOpaque},
 	weights::{GetDispatchInfo, Weight},
 	RuntimeDebug,
 };
@@ -74,8 +74,6 @@ pub use pallet::*;
 
 type BalanceOf<T> =
 	<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;
-/// Just a bunch of bytes, but they should decode to a valid `Call`.
-pub type OpaqueCall = Vec<u8>;
 
 /// A global extrinsic index, formed as the extrinsic index within a block, together with that
 /// block's height. This allows a transaction in which a multisig operation of a particular
@@ -101,10 +99,12 @@ pub struct Multisig<BlockNumber, Balance, AccountId> {
 	approvals: Vec<AccountId>,
 }
 
+type OpaqueCall<T> = WrapperKeepOpaque<<T as Config>::Call>;
+
 type CallHash = [u8; 32];
 
-enum CallOrHash {
-	Call(OpaqueCall, bool),
+enum CallOrHash<T: Config> {
+	Call(OpaqueCall<T>, bool),
 	Hash([u8; 32]),
 }
 
@@ -168,7 +168,7 @@ pub mod pallet {
 
 	#[pallet::storage]
 	pub type Calls<T: Config> =
-		StorageMap<_, Identity, [u8; 32], (OpaqueCall, T::AccountId, BalanceOf<T>)>;
+		StorageMap<_, Identity, [u8; 32], (OpaqueCall<T>, T::AccountId, BalanceOf<T>)>;
 
 	#[pallet::error]
 	pub enum Error<T> {
@@ -339,7 +339,7 @@ pub mod pallet {
 		/// # </weight>
 		#[pallet::weight({
 			let s = other_signatories.len() as u32;
-			let z = call.len() as u32;
+			let z = call.encoded_len() as u32;
 
 			T::WeightInfo::as_multi_create(s, z)
 			.max(T::WeightInfo::as_multi_create_store(s, z))
@@ -352,7 +352,7 @@ pub mod pallet {
 			threshold: u16,
 			other_signatories: Vec<T::AccountId>,
 			maybe_timepoint: Option<Timepoint<T::BlockNumber>>,
-			call: OpaqueCall,
+			call: OpaqueCall<T>,
 			store_call: bool,
 			max_weight: Weight,
 		) -> DispatchResultWithPostInfo {
@@ -406,9 +406,9 @@ pub mod pallet {
 			let s = other_signatories.len() as u32;
 
 			T::WeightInfo::approve_as_multi_create(s)
-			.max(T::WeightInfo::approve_as_multi_approve(s))
-			.max(T::WeightInfo::approve_as_multi_complete(s))
-			.saturating_add(*max_weight)
+				.max(T::WeightInfo::approve_as_multi_approve(s))
+				.max(T::WeightInfo::approve_as_multi_complete(s))
+				.saturating_add(*max_weight)
 		})]
 		pub fn approve_as_multi(
 			origin: OriginFor<T>,
@@ -502,7 +502,7 @@ impl<T: Config> Pallet<T> {
 		threshold: u16,
 		other_signatories: Vec<T::AccountId>,
 		maybe_timepoint: Option<Timepoint<T::BlockNumber>>,
-		call_or_hash: CallOrHash,
+		call_or_hash: CallOrHash<T>,
 		max_weight: Weight,
 	) -> DispatchResultWithPostInfo {
 		ensure!(threshold >= 2, Error::<T>::MinimumThreshold);
@@ -517,8 +517,8 @@ impl<T: Config> Pallet<T> {
 		// 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();
+				let call_hash = blake2_256(call.encoded());
+				let call_len = call.encoded_len();
 				(call_hash, call_len, Some(call), should_store)
 			},
 			CallOrHash::Hash(h) => (h, 0, None, false),
@@ -541,7 +541,7 @@ impl<T: Config> Pallet<T> {
 
 			// 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()))
+				Self::get_call(&call_hash, maybe_call.as_ref())
 			} else {
 				None
 			};
@@ -658,13 +658,14 @@ impl<T: Config> Pallet<T> {
 	fn store_call_and_reserve(
 		who: T::AccountId,
 		hash: &[u8; 32],
-		data: OpaqueCall,
+		data: OpaqueCall<T>,
 		other_deposit: BalanceOf<T>,
 	) -> DispatchResult {
 		ensure!(!Calls::<T>::contains_key(hash), Error::<T>::AlreadyStored);
 		let deposit = other_deposit +
 			T::DepositBase::get() +
-			T::DepositFactor::get() * BalanceOf::<T>::from(((data.len() + 31) / 32) as u32);
+			T::DepositFactor::get() *
+				BalanceOf::<T>::from(((data.encoded_len() + 31) / 32) as u32);
 		T::Currency::reserve(&who, deposit)?;
 		Calls::<T>::insert(&hash, (data, who, deposit));
 		Ok(())
@@ -673,15 +674,14 @@ impl<T: Config> Pallet<T> {
 	/// Attempt to decode and return the call, provided by the user or from storage.
 	fn get_call(
 		hash: &[u8; 32],
-		maybe_known: Option<&[u8]>,
+		maybe_known: Option<&OpaqueCall<T>>,
 	) -> Option<(<T as Config>::Call, usize)> {
 		maybe_known.map_or_else(
 			|| {
-				Calls::<T>::get(hash).and_then(|(data, ..)| {
-					Decode::decode(&mut &data[..]).ok().map(|d| (d, data.len()))
-				})
+				Calls::<T>::get(hash)
+					.and_then(|(data, ..)| Some((data.try_decode()?, data.encoded_len())))
 			},
-			|data| Decode::decode(&mut &data[..]).ok().map(|d| (d, data.len())),
+			|data| Some((data.try_decode()?, data.encoded_len())),
 		)
 	}
 

substrate/frame/multisig/src/tests.rs

@@ -31,6 +31,7 @@ use sp_runtime::{
 
 type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
 type Block = frame_system::mocking::MockBlock<Test>;
+type OpaqueCall = super::OpaqueCall<Test>;
 
 frame_support::construct_runtime!(
 	pub enum Test where
@@ -152,7 +153,7 @@ fn multisig_deposit_is_taken_and_returned() {
 			2,
 			vec![2, 3],
 			None,
-			data.clone(),
+			OpaqueCall::from_encoded(data.clone()),
 			false,
 			0
 		));
@@ -164,7 +165,7 @@ fn multisig_deposit_is_taken_and_returned() {
 			2,
 			vec![1, 3],
 			Some(now()),
-			data,
+			OpaqueCall::from_encoded(data),
 			false,
 			call_weight
 		));
@@ -185,7 +186,15 @@ fn multisig_deposit_is_taken_and_returned_with_call_storage() {
 		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_ok!(Multisig::as_multi(
+			Origin::signed(1),
+			2,
+			vec![2, 3],
+			None,
+			OpaqueCall::from_encoded(data),
+			true,
+			0
+		));
 		assert_eq!(Balances::free_balance(1), 0);
 		assert_eq!(Balances::reserved_balance(1), 5);
 
@@ -231,7 +240,7 @@ fn multisig_deposit_is_taken_and_returned_with_alt_call_storage() {
 			3,
 			vec![1, 3],
 			Some(now()),
-			data,
+			OpaqueCall::from_encoded(data),
 			true,
 			0
 		));
@@ -316,7 +325,15 @@ fn timepoint_checking_works() {
 		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(), false, 0),
+			Multisig::as_multi(
+				Origin::signed(2),
+				2,
+				vec![1, 3],
+				None,
+				OpaqueCall::from_encoded(call.clone()),
+				false,
+				0
+			),
 			Error::<Test>::NoTimepoint,
 		);
 		let later = Timepoint { index: 1, ..now() };
@@ -326,7 +343,7 @@ fn timepoint_checking_works() {
 				2,
 				vec![1, 3],
 				Some(later),
-				call.clone(),
+				OpaqueCall::from_encoded(call),
 				false,
 				0
 			),
@@ -347,7 +364,15 @@ fn multisig_2_of_3_works_with_call_storing() {
 		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_ok!(Multisig::as_multi(
+			Origin::signed(1),
+			2,
+			vec![2, 3],
+			None,
+			OpaqueCall::from_encoded(data),
+			true,
+			0
+		));
 		assert_eq!(Balances::free_balance(6), 0);
 
 		assert_ok!(Multisig::approve_as_multi(
@@ -382,7 +407,7 @@ fn multisig_2_of_3_works() {
 			2,
 			vec![1, 3],
 			Some(now()),
-			data,
+			OpaqueCall::from_encoded(data),
 			false,
 			call_weight
 		));
@@ -425,7 +450,7 @@ fn multisig_3_of_3_works() {
 			3,
 			vec![1, 2],
 			Some(now()),
-			data,
+			OpaqueCall::from_encoded(data),
 			false,
 			call_weight
 		));
@@ -473,7 +498,15 @@ fn cancel_multisig_with_call_storage_works() {
 	new_test_ext().execute_with(|| {
 		let call = call_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_ok!(Multisig::as_multi(
+			Origin::signed(1),
+			3,
+			vec![2, 3],
+			None,
+			OpaqueCall::from_encoded(call),
+			true,
+			0
+		));
 		assert_eq!(Balances::free_balance(1), 4);
 		assert_ok!(Multisig::approve_as_multi(
 			Origin::signed(2),
@@ -517,7 +550,7 @@ fn cancel_multisig_with_alt_call_storage_works() {
 			3,
 			vec![1, 3],
 			Some(now()),
-			call,
+			OpaqueCall::from_encoded(call),
 			true,
 			0
 		));
@@ -544,7 +577,7 @@ fn multisig_2_of_3_as_multi_works() {
 			2,
 			vec![2, 3],
 			None,
-			data.clone(),
+			OpaqueCall::from_encoded(data.clone()),
 			false,
 			0
 		));
@@ -555,7 +588,7 @@ fn multisig_2_of_3_as_multi_works() {
 			2,
 			vec![1, 3],
 			Some(now()),
-			data,
+			OpaqueCall::from_encoded(data),
 			false,
 			call_weight
 		));
@@ -583,7 +616,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() {
 			2,
 			vec![2, 3],
 			None,
-			data1.clone(),
+			OpaqueCall::from_encoded(data1.clone()),
 			false,
 			0
 		));
@@ -592,7 +625,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() {
 			2,
 			vec![1, 3],
 			None,
-			data2.clone(),
+			OpaqueCall::from_encoded(data2.clone()),
 			false,
 			0
 		));
@@ -601,7 +634,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() {
 			2,
 			vec![1, 2],
 			Some(now()),
-			data1,
+			OpaqueCall::from_encoded(data1),
 			false,
 			call1_weight
 		));
@@ -610,7 +643,7 @@ fn multisig_2_of_3_as_multi_with_many_calls_works() {
 			2,
 			vec![1, 2],
 			Some(now()),
-			data2,
+			OpaqueCall::from_encoded(data2),
 			false,
 			call2_weight
 		));
@@ -637,7 +670,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
 			2,
 			vec![2, 3],
 			None,
-			data.clone(),
+			OpaqueCall::from_encoded(data.clone()),
 			false,
 			0
 		));
@@ -646,7 +679,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
 			2,
 			vec![1, 3],
 			Some(now()),
-			data.clone(),
+			OpaqueCall::from_encoded(data.clone()),
 			false,
 			call_weight
 		));
@@ -657,7 +690,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
 			2,
 			vec![2, 3],
 			None,
-			data.clone(),
+			OpaqueCall::from_encoded(data.clone()),
 			false,
 			0
 		));
@@ -666,7 +699,7 @@ fn multisig_2_of_3_cannot_reissue_same_call() {
 			2,
 			vec![1, 2],
 			Some(now()),
-			data.clone(),
+			OpaqueCall::from_encoded(data),
 			false,
 			call_weight
 		));
@@ -683,11 +716,27 @@ fn minimum_threshold_check_works() {
 	new_test_ext().execute_with(|| {
 		let call = call_transfer(6, 15).encode();
 		assert_noop!(
-			Multisig::as_multi(Origin::signed(1), 0, vec![2], None, call.clone(), false, 0),
+			Multisig::as_multi(
+				Origin::signed(1),
+				0,
+				vec![2],
+				None,
+				OpaqueCall::from_encoded(call.clone()),
+				false,
+				0
+			),
 			Error::<Test>::MinimumThreshold,
 		);
 		assert_noop!(
-			Multisig::as_multi(Origin::signed(1), 1, vec![2], None, call.clone(), false, 0),
+			Multisig::as_multi(
+				Origin::signed(1),
+				1,
+				vec![2],
+				None,
+				OpaqueCall::from_encoded(call.clone()),
+				false,
+				0
+			),
 			Error::<Test>::MinimumThreshold,
 		);
 	});
@@ -698,7 +747,15 @@ fn too_many_signatories_fails() {
 	new_test_ext().execute_with(|| {
 		let call = call_transfer(6, 15).encode();
 		assert_noop!(
-			Multisig::as_multi(Origin::signed(1), 2, vec![2, 3, 4], None, call.clone(), false, 0),
+			Multisig::as_multi(
+				Origin::signed(1),
+				2,
+				vec![2, 3, 4],
+				None,
+				OpaqueCall::from_encoded(call),
+				false,
+				0
+			),
 			Error::<Test>::TooManySignatories,
 		);
 	});
@@ -765,7 +822,15 @@ fn multisig_1_of_3_works() {
 			Error::<Test>::MinimumThreshold,
 		);
 		assert_noop!(
-			Multisig::as_multi(Origin::signed(1), 1, vec![2, 3], None, call.clone(), false, 0),
+			Multisig::as_multi(
+				Origin::signed(1),
+				1,
+				vec![2, 3],
+				None,
+				OpaqueCall::from_encoded(call),
+				false,
+				0
+			),
 			Error::<Test>::MinimumThreshold,
 		);
 		let boxed_call = Box::new(call_transfer(6, 15));
@@ -801,14 +866,22 @@ fn weight_check_works() {
 			2,
 			vec![2, 3],
 			None,
-			data.clone(),
+			OpaqueCall::from_encoded(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),
+			Multisig::as_multi(
+				Origin::signed(2),
+				2,
+				vec![1, 3],
+				Some(now()),
+				OpaqueCall::from_encoded(data),
+				false,
+				0
+			),
 			Error::<Test>::MaxWeightTooLow,
 		);
 	});
@@ -860,7 +933,7 @@ fn multisig_handles_no_preimage_after_all_approve() {
 			3,
 			vec![1, 2],
 			Some(now()),
-			data,
+			OpaqueCall::from_encoded(data),
 			false,
 			call_weight
 		));