pallet-multisig: Improve opaque call handling

client/db/Cargo.toml

@@ -41,6 +41,7 @@ substrate-test-runtime-client = { version = "2.0.0", path = "../../test-utils/ru
 quickcheck = "1.0.3"
 kvdb-rocksdb = "0.14.0"
 tempfile = "3"
+criterion = "0.3.5"
 
 [features]
 default = []

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 {
@@ -370,9 +370,8 @@ pub mod pallet {
 		/// Register approval for a dispatch to be made from a deterministic composite account if
 		/// approved by a total of `threshold - 1` of `other_signatories`.
 		///
-		/// Payment: `DepositBase` will be reserved if this is the first approval, plus
-		/// `threshold` times `DepositFactor`. It is returned once this dispatch happens or
-		/// is cancelled.
+		/// Payment: `DepositBase` will be reserved if this is the first appromes `DepositFactor`.
+		/// It is returned once this dispatch happens or is cancelled.
 		///
 		/// The dispatch origin for this call must be _Signed_.
 		///
@@ -406,9 +405,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 +501,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 +516,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 +540,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 +657,13 @@ 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 +672,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())),
 		)
 	}