Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Stored call in multisig #6319

Merged
merged 31 commits into from
Jun 17, 2020
Merged

Stored call in multisig #6319

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c7584c0
Stored call in multisig
gavofyork Jun 10, 2020
0ddd158
Docs.
gavofyork Jun 10, 2020
c30f54f
Benchmarks.
gavofyork Jun 10, 2020
dbac9eb
Fix
gavofyork Jun 10, 2020
3669489
Update frame/multisig/src/lib.rs
gavofyork Jun 10, 2020
9f59d31
patch benchmarks
shawntabrizi Jun 11, 2020
69682e5
Minor grumbles.
gavofyork Jun 11, 2020
e48f3e4
Merge branch 'gav-multisig-stored-call' of github.com:paritytech/subs…
gavofyork Jun 11, 2020
a363d4b
Update as_multi weight
shawntabrizi Jun 11, 2020
1c69bb0
Merge branch 'gav-multisig-stored-call' of https://github.com/parityt…
shawntabrizi Jun 11, 2020
a8343cb
Fixes and refactoring.
gavofyork Jun 11, 2020
fdcab27
Merge branch 'gav-multisig-stored-call' of github.com:paritytech/subs…
gavofyork Jun 11, 2020
bd5f05e
Split out threshold=1 and opaquify Call.
gavofyork Jun 11, 2020
8b9aaae
Compiles, tests pass, weights are broken
shawntabrizi Jun 14, 2020
09da872
Update benchmarks, add working tests
shawntabrizi Jun 14, 2020
2b5c361
Add benchmark to threshold 1, add event too
shawntabrizi Jun 14, 2020
2cf2832
suppress warning for now
shawntabrizi Jun 14, 2020
9a2d25f
@xlc improvment nit
shawntabrizi Jun 14, 2020
db75543
Update weight and tests
shawntabrizi Jun 15, 2020
6275b9c
Test for weight check
shawntabrizi Jun 15, 2020
5c9a7af
Merge branch 'master' into gav-multisig-stored-call
shawntabrizi Jun 15, 2020
81195da
Fix line width
shawntabrizi Jun 15, 2020
7a75cf7
one more line width error
shawntabrizi Jun 15, 2020
9dd0585
Apply suggestions from code review
shawntabrizi Jun 15, 2020
7af9a83
Merge branch 'master' into gav-multisig-stored-call
shawntabrizi Jun 15, 2020
f64e26d
fix merge
shawntabrizi Jun 15, 2020
9becf6e
more @apopiak feedback
shawntabrizi Jun 15, 2020
5bd6514
Multisig handles no preimage
shawntabrizi Jun 16, 2020
5cc17da
Optimize return weight after dispatch
shawntabrizi Jun 16, 2020
a053de8
Merge remote-tracking branch 'origin/master' into gav-multisig-stored…
gavofyork Jun 16, 2020
9e2d5b6
Error on failed deposit.
gavofyork Jun 16, 2020
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
62 changes: 54 additions & 8 deletions frame/multisig/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,16 @@ benchmarks! {
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call)
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, false)

as_multi_create_store {
// Signatories, need at least 2 total people
let s in 2 .. T::MaxSignatories::get() as u32;
// Transaction Length
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let caller = signatories.pop().ok_or("signatories should have len 2 or more")?;
}: as_multi(RawOrigin::Signed(caller), s as u16, signatories, None, call, true)

as_multi_approve {
// Signatories, need at least 2 people
Expand All @@ -68,9 +77,9 @@ benchmarks! {
// 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())?;
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?;
let caller2 = signatories2.remove(0);
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call)
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false)

as_multi_complete {
// Signatories, need at least 2 people
Expand All @@ -83,16 +92,16 @@ benchmarks! {
// 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())?;
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?;
// 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())?;
Multisig::<T>::as_multi(o, s as u16, signatories_loop, Some(timepoint), call.clone(), false)?;
}
let caller2 = signatories2.remove(0);
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call)
}: as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call, false)

approve_as_multi_create {
// Signatories, need at least 2 people
Expand All @@ -117,7 +126,30 @@ benchmarks! {
// 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())?;
Multisig::<T>::as_multi(RawOrigin::Signed(caller).into(), s as u16, signatories, None, call.clone(), false)?;
let caller2 = signatories2.remove(0);
}: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash)

approve_as_multi_complete {
// Signatories, need at least 2 people
let s in 2 .. T::MaxSignatories::get() as u32;
// Transaction Length
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
let mut signatories2 = signatories.clone();
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.clone(), true)?;
// 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(), false)?;
}
let caller2 = signatories2.remove(0);
}: approve_as_multi(RawOrigin::Signed(caller2), s as u16, signatories2, Some(timepoint), call_hash)

Expand All @@ -132,8 +164,22 @@ benchmarks! {
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.clone())?;
Multisig::<T>::as_multi(o, s as u16, signatories.clone(), None, call.clone(), false)?;
}: _(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash)

cancel_as_multi_store {
// Signatories, need at least 2 people
let s in 2 .. T::MaxSignatories::get() as u32;
// Transaction Length
let z in 0 .. 10_000;
let (mut signatories, call) = setup_multi::<T>(s, z)?;
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.clone(), true)?;
}: cancel_as_multi(RawOrigin::Signed(caller), s as u16, signatories, timepoint, call_hash)
}

#[cfg(test)]
Expand Down
45 changes: 43 additions & 2 deletions frame/multisig/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ decl_storage! {
pub Multisigs: double_map
hasher(twox_64_concat) T::AccountId, hasher(blake2_128_concat) [u8; 32]
=> Option<Multisig<T::BlockNumber, BalanceOf<T>, T::AccountId>>;

pub Calls: map hasher(identity) [u8; 32] => Option<(Vec<u8>, T::AccountId, BalanceOf<T>)>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: maybe add descriptive type defs for [u8; 32] (CallHash?) and Vec<u8> (EncodedCall) ?

}
}

Expand Down Expand Up @@ -278,6 +280,7 @@ decl_module! {
other_signatories: Vec<T::AccountId>,
maybe_timepoint: Option<Timepoint<T::BlockNumber>>,
call: Box<<T as Trait>::Call>,
store_call: bool,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;
// We're now executing as a freshly authenticated new account, so the previous call
Expand All @@ -292,7 +295,12 @@ decl_module! {
let signatories = Self::ensure_sorted_and_insert(other_signatories, who.clone())?;

let id = Self::multi_account_id(&signatories, threshold);
let call_hash = call.using_encoded(blake2_256);

let encoded_call = call.encode();
let call_hash = blake2_256(&encoded_call);
if store_call {
Self::store_call(who.clone(), &call_hash, encoded_call)?;
gavofyork marked this conversation as resolved.
Show resolved Hide resolved
}

if let Some(mut m) = <Multisigs<T>>::get(&id, call_hash) {
let timepoint = maybe_timepoint.ok_or(Error::<T>::NoTimepoint)?;
Expand All @@ -315,6 +323,7 @@ decl_module! {
let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into());
let _ = T::Currency::unreserve(&m.depositor, m.deposit);
<Multisigs<T>>::remove(&id, call_hash);
Self::take_call(&call_hash);
Self::deposit_event(RawEvent::MultisigExecuted(
who, timepoint, id, call_hash, result.map(|_| ()).map_err(|e| e.error)
));
Expand Down Expand Up @@ -425,6 +434,17 @@ decl_module! {
ensure!(m.when == timepoint, Error::<T>::WrongTimepoint);
ensure!(m.approvals.len() < threshold as usize, Error::<T>::NoApprovalsNeeded);
if let Err(pos) = m.approvals.binary_search(&who) {
if m.approvals.len() + 1 == threshold as usize {
if let Some(call) = Self::take_call(&call_hash) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should change the weight calculation of the dispatchable.
Suggestion: Introduce a weight_of::take_call function (and corresponding weight_of::store_call).

let result = call.dispatch(frame_system::RawOrigin::Signed(id.clone()).into());
Copy link
Contributor

@apopiak apopiak Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also should change the weight. In my estimation you are running into the same issues I had in collective which also has stored calls that are (potentially) executed later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See weight_bound param here and the validate_and_get_proposal function here.
We might want to build some utilities around this use case of deferred calls so it's not as much effort to make it secure weight-wise every time.

let _ = T::Currency::unreserve(&m.depositor, m.deposit);
<Multisigs<T>>::remove(&id, call_hash);
Self::deposit_event(RawEvent::MultisigExecuted(
who, timepoint, id, call_hash, result.map(|_| ()).map_err(|e| e.error)
));
return Ok(());
}
}
m.approvals.insert(pos, who.clone());
<Multisigs<T>>::insert(&id, call_hash, m);
Self::deposit_event(RawEvent::MultisigApproval(who, timepoint, id, call_hash));
Expand Down Expand Up @@ -506,7 +526,8 @@ decl_module! {
ensure!(m.depositor == who, Error::<T>::NotOwner);

let _ = T::Currency::unreserve(&m.depositor, m.deposit);
<Multisigs<T>>::remove(&id, call_hash);
<Multisigs<T>>::remove(&id, &call_hash);
Self::take_call(&call_hash);

Self::deposit_event(RawEvent::MultisigCancelled(who, timepoint, id, call_hash));
Ok(())
Expand All @@ -524,6 +545,26 @@ impl<T: Trait> Module<T> {
T::AccountId::decode(&mut &entropy[..]).unwrap_or_default()
}

/// Place a call's encoded data in storage, reserving funds as appropriate.
fn store_call(who: T::AccountId, hash: &[u8; 32], data: Vec<u8>) -> DispatchResult {
if !Calls::<T>::contains_key(hash) {
let deposit = T::DepositBase::get()
+ T::DepositFactor::get() * BalanceOf::<T>::from(((data.len() + 31) / 32) as u32);
apopiak marked this conversation as resolved.
Show resolved Hide resolved
T::Currency::reserve(&who, deposit)?;
// we store `data` here because storing `call` would result in needing another `.encode`.
Calls::<T>::insert(&hash, (data, who, deposit));
}
Ok(())
}

/// Attempt to remove a call from storage, returning it and any deposit on it to the owner.
fn take_call(hash: &[u8; 32]) -> Option<<T as Trait>::Call> {
Calls::<T>::take(hash).and_then(|(data, who, deposit)| {
T::Currency::unreserve(&who, deposit);
Decode::decode(&mut &data[..]).ok()
})
}

/// The current `Timepoint`.
pub fn timepoint() -> Timepoint<T::BlockNumber> {
Timepoint {
Expand Down
Loading