Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove timepoint from multisig #2542

Closed
wants to merge 1 commit into from
Closed

remove timepoint from multisig #2542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 10 additions & 18 deletions substrate/frame/multisig/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,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());
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, Weight::zero())
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, call, Weight::zero())
verify {
assert!(Multisigs::<T>::contains_key(multi_account_id, call_hash));
}
Expand All @@ -94,15 +94,13 @@ benchmarks! {
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")?;
// before the call, get the timepoint
let timepoint = Multisig::<T>::timepoint();
// Create the multi
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), Weight::zero())?;
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, call.clone(), Weight::zero())?;
let caller2 = signatories2.remove(0);
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller2);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, Weight::zero())
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, call, Weight::zero())
verify {
let multisig = Multisigs::<T>::get(multi_account_id, call_hash).ok_or("multisig not created")?;
assert_eq!(multisig.approvals.len(), 2);
Expand All @@ -118,23 +116,21 @@ benchmarks! {
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")?;
// before the call, get the timepoint
let timepoint = Multisig::<T>::timepoint();
// Create the multi
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), Weight::zero())?;
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, call.clone(), Weight::zero())?;
// Everyone except the first person approves
for i in 1 .. s - 1 {
let mut signatories_loop = signatories2.clone();
let caller_loop = signatories_loop.remove(i as usize);
let o = RawOrigin::Signed(caller_loop).into();
Multisig::<T>::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), Weight::zero())?;
Multisig::<T>::as_multi(o, s as u16, signatories_loop, call.clone(), Weight::zero())?;
}
let caller2 = signatories2.remove(0);
assert!(Multisigs::<T>::contains_key(&multi_account_id, call_hash));
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller2);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, Weight::MAX)
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, call, Weight::MAX)
verify {
assert!(!Multisigs::<T>::contains_key(&multi_account_id, call_hash));
}
Expand All @@ -152,7 +148,7 @@ benchmarks! {
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
// Create the multi
}: approve_as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call_hash, Weight::zero())
}: approve_as_multi(RawOrigin::Signed(caller), s as u16, signatories, call_hash, Weight::zero())
verify {
assert!(Multisigs::<T>::contains_key(multi_account_id, call_hash));
}
Expand All @@ -167,22 +163,19 @@ benchmarks! {
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 = call.using_encoded(blake2_256);
// before the call, get the timepoint
let timepoint = Multisig::<T>::timepoint();
// Create the multi
Multisig::<T>::as_multi(
RawOrigin::Signed(caller).into(),
s as u16,
signatories,
None,
call,
Weight::zero()
)?;
let caller2 = signatories2.remove(0);
// Whitelist caller account from further DB operations.
let caller_key = frame_system::Account::<T>::hashed_key_for(&caller2);
frame_benchmarking::benchmarking::add_to_whitelist(caller_key.into());
}: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash, Weight::zero())
}: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, call_hash, Weight::zero())
verify {
let multisig = Multisigs::<T>::get(multi_account_id, call_hash).ok_or("multisig not created")?;
assert_eq!(multisig.approvals.len(), 2);
Expand All @@ -197,15 +190,14 @@ benchmarks! {
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 = call.using_encoded(blake2_256);
let timepoint = Multisig::<T>::timepoint();
// Create the multi
let o = RawOrigin::Signed(caller.clone()).into();
Multisig::<T>::as_multi(o, s as u16, signatories.clone(), None, call, Weight::zero())?;
Multisig::<T>::as_multi(o, s as u16, signatories.clone(), call, Weight::zero())?;
assert!(Multisigs::<T>::contains_key(&multi_account_id, call_hash));
// 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());
}: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash)
}: _(RawOrigin::Signed(caller), s as u16, signatories, call_hash)
verify {
assert!(!Multisigs::<T>::contains_key(multi_account_id, call_hash));
}
Expand Down
90 changes: 7 additions & 83 deletions substrate/frame/multisig/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ use frame_support::{
weights::Weight,
BoundedVec,
};
use frame_system::{self as system, pallet_prelude::BlockNumberFor, RawOrigin};
use frame_system::{pallet_prelude::BlockNumberFor, RawOrigin};
use scale_info::TypeInfo;
use sp_io::hashing::blake2_256;
use sp_runtime::{
Expand Down Expand Up @@ -88,28 +88,13 @@ macro_rules! log {
type BalanceOf<T> =
<<T as Config>::Currency as Currency<<T as frame_system::Config>::AccountId>>::Balance;

/// 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
/// composite was created to be uniquely identified.
#[derive(
Copy, Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug, TypeInfo, MaxEncodedLen,
)]
pub struct Timepoint<BlockNumber> {
/// The height of the chain at the point in time.
height: BlockNumber,
/// The index of the extrinsic at the point in time.
index: u32,
}

/// An open multisig operation.
#[derive(Clone, Eq, PartialEq, Encode, Decode, Default, RuntimeDebug, TypeInfo, MaxEncodedLen)]
#[scale_info(skip_type_params(MaxApprovals))]
pub struct Multisig<BlockNumber, Balance, AccountId, MaxApprovals>
pub struct Multisig<Balance, AccountId, MaxApprovals>
where
MaxApprovals: Get<u32>,
{
/// The extrinsic when the multisig operation was opened.
when: Timepoint<BlockNumber>,
/// The amount held in reserve of the `depositor`, to be returned once the operation ends.
deposit: Balance,
/// The account who opened it (i.e. the first to approve it).
Expand Down Expand Up @@ -183,7 +168,7 @@ pub mod pallet {
T::AccountId,
Blake2_128Concat,
[u8; 32],
Multisig<BlockNumberFor<T>, BalanceOf<T>, T::AccountId, T::MaxSignatories>,
Multisig<BalanceOf<T>, T::AccountId, T::MaxSignatories>,
>;

#[pallet::error]
Expand All @@ -206,12 +191,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.
UnexpectedTimepoint,
/// The maximum weight information provided was too low.
MaxWeightTooLow,
/// The data to be stored is already stored.
Expand All @@ -224,27 +203,16 @@ pub mod pallet {
/// A new multisig operation has begun.
NewMultisig { approving: T::AccountId, multisig: T::AccountId, call_hash: CallHash },
/// A multisig operation has been approved by someone.
MultisigApproval {
approving: T::AccountId,
timepoint: Timepoint<BlockNumberFor<T>>,
multisig: T::AccountId,
call_hash: CallHash,
},
MultisigApproval { approving: T::AccountId, multisig: T::AccountId, call_hash: CallHash },
/// A multisig operation has been executed.
MultisigExecuted {
approving: T::AccountId,
timepoint: 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>>,
multisig: T::AccountId,
call_hash: CallHash,
},
MultisigCancelled { cancelling: T::AccountId, multisig: T::AccountId, call_hash: CallHash },
}

#[pallet::hooks]
Expand Down Expand Up @@ -327,9 +295,6 @@ 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.
/// - `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.
/// - `call`: The call to be executed.
///
/// NOTE: Unless this is the final approval, you will generally want to use
Expand Down Expand Up @@ -366,19 +331,11 @@ pub mod pallet {
origin: OriginFor<T>,
threshold: u16,
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<BlockNumberFor<T>>>,
call: Box<<T as Config>::RuntimeCall>,
max_weight: Weight,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;
Self::operate(
who,
threshold,
other_signatories,
maybe_timepoint,
CallOrHash::Call(*call),
max_weight,
)
Self::operate(who, threshold, other_signatories, CallOrHash::Call(*call), max_weight)
}

/// Register approval for a dispatch to be made from a deterministic composite account if
Expand All @@ -393,9 +350,6 @@ 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.
/// - `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.
/// - `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.
Expand Down Expand Up @@ -423,7 +377,6 @@ pub mod pallet {
origin: OriginFor<T>,
threshold: u16,
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<BlockNumberFor<T>>>,
call_hash: [u8; 32],
max_weight: Weight,
) -> DispatchResultWithPostInfo {
Expand All @@ -432,7 +385,6 @@ pub mod pallet {
who,
threshold,
other_signatories,
maybe_timepoint,
CallOrHash::Hash(call_hash),
max_weight,
)
Expand All @@ -446,8 +398,6 @@ 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.
/// - `call_hash`: The hash of the call to be executed.
///
/// ## Complexity
Expand All @@ -465,7 +415,6 @@ pub mod pallet {
origin: OriginFor<T>,
threshold: u16,
other_signatories: Vec<T::AccountId>,
timepoint: Timepoint<BlockNumberFor<T>>,
call_hash: [u8; 32],
) -> DispatchResult {
let who = ensure_signed(origin)?;
Expand All @@ -478,7 +427,6 @@ 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);
ensure!(m.depositor == who, Error::<T>::NotOwner);

let err_amount = T::Currency::unreserve(&m.depositor, m.deposit);
Expand All @@ -487,7 +435,6 @@ pub mod pallet {

Self::deposit_event(Event::MultisigCancelled {
cancelling: who,
timepoint,
multisig: id,
call_hash,
});
Expand All @@ -511,7 +458,6 @@ impl<T: Config> Pallet<T> {
who: T::AccountId,
threshold: u16,
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<BlockNumberFor<T>>>,
call_or_hash: CallOrHash<T>,
max_weight: Weight,
) -> DispatchResultWithPostInfo {
Expand All @@ -535,10 +481,6 @@ 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);

// Ensure that either we have not yet signed or that it is at threshold.
let mut approvals = m.approvals.len() as u16;
// We only bother with the approval if we're below threshold.
Expand All @@ -564,7 +506,6 @@ impl<T: Config> Pallet<T> {
let result = call.dispatch(RawOrigin::Signed(id.clone()).into());
Self::deposit_event(Event::MultisigExecuted {
approving: who,
timepoint,
multisig: id,
call_hash,
result: result.map(|_| ()).map_err(|e| e.error),
Expand All @@ -590,7 +531,6 @@ impl<T: Config> Pallet<T> {
<Multisigs<T>>::insert(&id, call_hash, m);
Self::deposit_event(Event::MultisigApproval {
approving: who,
timepoint,
multisig: id,
call_hash,
});
Expand All @@ -606,9 +546,6 @@ impl<T: Config> Pallet<T> {
Ok(Some(final_weight).into())
}
} else {
// Not yet started; there should be no timepoint given.
ensure!(maybe_timepoint.is_none(), Error::<T>::UnexpectedTimepoint);

// Just start the operation by recording it in storage.
let deposit = T::DepositBase::get() + T::DepositFactor::get() * threshold.into();

Expand All @@ -620,12 +557,7 @@ impl<T: Config> Pallet<T> {
<Multisigs<T>>::insert(
&id,
call_hash,
Multisig {
when: Self::timepoint(),
deposit,
depositor: who.clone(),
approvals: initial_approvals,
},
Multisig { deposit, depositor: who.clone(), approvals: initial_approvals },
);
Self::deposit_event(Event::NewMultisig { approving: who, multisig: id, call_hash });

Expand All @@ -636,14 +568,6 @@ impl<T: Config> Pallet<T> {
}
}

/// The current `Timepoint`.
pub fn timepoint() -> Timepoint<BlockNumberFor<T>> {
Timepoint {
height: <system::Pallet<T>>::block_number(),
index: <system::Pallet<T>>::extrinsic_index().unwrap_or_default(),
}
}

/// Check that signatories is sorted and doesn't contain sender, then insert sender.
fn ensure_sorted_and_insert(
other_signatories: Vec<T::AccountId>,
Expand Down
Loading