Skip to content

Commit

Permalink
pallet-multisig: Improve opaque call handling (paritytech#10060)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@bkchr @ark0f
bkchr authored and ark0f committed Feb 27, 2023
1 parent 8c1f9c7 commit 6bd50bd
Show file tree
Hide file tree
Showing 5 changed files with 241 additions and 63 deletions.
25 changes: 14 additions & 11 deletions frame/multisig/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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))
}

Expand Down Expand Up @@ -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.
Expand All @@ -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());
Expand All @@ -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")?;
Expand All @@ -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")?;
Expand All @@ -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")?;
Expand Down Expand Up @@ -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());
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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();
Expand Down
44 changes: 22 additions & 22 deletions frame/multisig/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand All @@ -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
Expand All @@ -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]),
}

Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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))
Expand All @@ -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 {
Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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);
Expand All @@ -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),
Expand All @@ -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
};
Expand Down Expand Up @@ -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(())
Expand All @@ -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())),
)
}

Expand Down
Loading

0 comments on commit 6bd50bd

Please sign in to comment.