Skip to content

Commit

Permalink
MESH-2111 improve multisig storage all (#1543)
Browse files Browse the repository at this point in the history 
* Improve MultiSig proposals storage.

* Improve docs.

* Improve MultiSig details and votes storage.

* Fix tests.

* Improve Votes storage.
  • Loading branch information
@Neopallium
Neopallium authored Oct 6, 2023
1 parent 03cd84f commit 54ae63a
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 36 deletions.
4 changes: 2 additions & 2 deletions pallets/multisig/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ macro_rules! assert_proposal_created {

macro_rules! assert_vote_cast {
($proposal_id:ident, $multisig:ident, $signatory:expr) => {
assert!(<MultiSig<T>>::votes(($multisig, $signatory, $proposal_id)));
assert!(<MultiSig<T>>::votes(($multisig, $proposal_id), $signatory));
};
}

Expand Down Expand Up @@ -361,7 +361,7 @@ benchmarks! {
let ephemeral_proposal_id = proposal_id.clone();
}: _(RawOrigin::Root, ephemeral_multisig, ephemeral_proposal_id, alice.did(), Weight::zero())
verify {
assert!(<ProposalDetail<T>>::get((&multisig, proposal_id)).status == ProposalStatus::ExecutionSuccessful);
assert!(<ProposalDetail<T>>::get(&multisig, proposal_id).status == ProposalStatus::ExecutionSuccessful);
}

change_sigs_required_via_creator {
Expand Down
52 changes: 29 additions & 23 deletions pallets/multisig/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,17 +148,26 @@ decl_storage! {
pub MultiSigSignsRequired get(fn ms_signs_required): map hasher(identity) T::AccountId => u64;
/// Number of transactions proposed in a multisig. Used as tx id; starts from 0.
pub MultiSigTxDone get(fn ms_tx_done): map hasher(identity) T::AccountId => u64;
/// Proposals presented for voting to a multisig (multisig, proposal id) => Option<T::Proposal>.
pub Proposals get(fn proposals): map hasher(twox_64_concat) (T::AccountId, u64) => Option<T::Proposal>;
/// Proposals presented for voting to a multisig.
///
/// multisig -> proposal id => Option<T::Proposal>.
pub Proposals get(fn proposals):
double_map hasher(twox_64_concat) T::AccountId, hasher(twox_64_concat) u64 => Option<T::Proposal>;
/// A mapping of proposals to their IDs.
pub ProposalIds get(fn proposal_ids):
double_map hasher(identity) T::AccountId, hasher(blake2_128_concat) T::Proposal => Option<u64>;
/// Individual multisig signer votes. (multi sig, signer, proposal) => vote.
pub Votes get(fn votes): map hasher(twox_64_concat) (T::AccountId, Signatory<T::AccountId>, u64) => bool;
/// Individual multisig signer votes.
///
/// (multisig, proposal_id) -> signer => vote.
pub Votes get(fn votes):
double_map hasher(twox_64_concat) (T::AccountId, u64), hasher(twox_64_concat) Signatory<T::AccountId> => bool;
/// Maps a multisig account to its identity.
pub MultiSigToIdentity get(fn ms_to_identity): map hasher(identity) T::AccountId => IdentityId;
/// Details of a multisig proposal
pub ProposalDetail get(fn proposal_detail): map hasher(twox_64_concat) (T::AccountId, u64) => ProposalDetails<T::Moment>;
///
/// multisig -> proposal id => ProposalDetails.
pub ProposalDetail get(fn proposal_detail):
double_map hasher(twox_64_concat) T::AccountId, hasher(twox_64_concat) u64 => ProposalDetails<T::Moment>;
/// Tracks creators who are no longer allowed to call via_creator extrinsics.
pub LostCreatorPrivileges get(fn lost_creator_privileges): map hasher(identity) IdentityId => bool;

Expand Down Expand Up @@ -732,13 +741,14 @@ impl<T: Config> Module<T> {
.unwrap_or_else(|_| <MultiSigToIdentity<T>>::get(&multisig)),
};
let proposal_id = Self::ms_tx_done(multisig.clone());
<Proposals<T>>::insert((multisig.clone(), proposal_id), proposal.clone());
<Proposals<T>>::insert(multisig.clone(), proposal_id, proposal.clone());
if proposal_to_id {
// Only use the `Proposal` -> id map for `create_or_approve_proposal` calls.
<ProposalIds<T>>::insert(multisig.clone(), *proposal, proposal_id);
}
<ProposalDetail<T>>::insert(
(multisig.clone(), proposal_id),
&multisig,
proposal_id,
ProposalDetails::new(expiry, auto_close),
);
// Since proposal_ids are always only incremented by 1, they can not overflow.
Expand Down Expand Up @@ -778,18 +788,16 @@ impl<T: Config> Module<T> {
proposal_id: u64,
) -> DispatchResult {
Self::ensure_ms_signer(&multisig, &signer)?;
let multisig_signer_proposal = (multisig.clone(), signer.clone(), proposal_id);
let multisig_proposal = (multisig.clone(), proposal_id);
ensure!(
!Self::votes(&multisig_signer_proposal),
!Self::votes((&multisig, proposal_id), &signer),
Error::<T>::AlreadyVoted
);
ensure!(
<Proposals<T>>::contains_key(&multisig_proposal),
<Proposals<T>>::contains_key(&multisig, proposal_id),
Error::<T>::ProposalMissing
);

let mut proposal_details = Self::proposal_detail(&multisig_proposal);
let mut proposal_details = Self::proposal_detail(&multisig, proposal_id);
proposal_details.approvals += 1u64;
let multisig_did = <MultiSigToIdentity<T>>::get(&multisig);
match proposal_details.status {
Expand All @@ -807,7 +815,7 @@ impl<T: Config> Module<T> {
);
}
if proposal_details.approvals >= Self::ms_signs_required(&multisig) {
if let Some(proposal) = Self::proposals((multisig.clone(), proposal_id)) {
if let Some(proposal) = Self::proposals(&multisig, proposal_id) {
let execution_at = frame_system::Pallet::<T>::block_number() + One::one();
let call = Call::<T>::execute_scheduled_proposal {
multisig: multisig.clone(),
Expand All @@ -832,8 +840,8 @@ impl<T: Config> Module<T> {
}
}
// Update storage
<Votes<T>>::insert(&multisig_signer_proposal, true);
<ProposalDetail<T>>::insert(&multisig_proposal, proposal_details);
<Votes<T>>::insert((&multisig, proposal_id), &signer, true);
<ProposalDetail<T>>::insert(&multisig, proposal_id, proposal_details);
// emit proposal approved event
Self::deposit_event(RawEvent::ProposalApproved(
multisig_did,
Expand All @@ -857,9 +865,9 @@ impl<T: Config> Module<T> {
T::CddHandler::set_current_identity(&multisig_did);
T::CddHandler::set_payer_context(Identity::<T>::get_primary_key(multisig_did));

if let Some(proposal) = Self::proposals((multisig.clone(), proposal_id)) {
if let Some(proposal) = Self::proposals(&multisig, proposal_id) {
let update_proposal_status = |status| {
<ProposalDetail<T>>::mutate((&multisig, proposal_id), |proposal_details| {
<ProposalDetail<T>>::mutate(&multisig, proposal_id, |proposal_details| {
proposal_details.status = status
})
};
Expand Down Expand Up @@ -893,13 +901,11 @@ impl<T: Config> Module<T> {
proposal_id: u64,
) -> DispatchResult {
Self::ensure_ms_signer(&multisig, &signer)?;
let multisig_signer_proposal = (multisig.clone(), signer.clone(), proposal_id);
let multisig_proposal = (multisig.clone(), proposal_id);
ensure!(
!Self::votes(&multisig_signer_proposal),
!Self::votes((&multisig, proposal_id), &signer),
Error::<T>::AlreadyVoted
);
let mut proposal_details = Self::proposal_detail(&multisig_proposal);
let mut proposal_details = Self::proposal_detail(&multisig, proposal_id);
proposal_details.rejections += 1u64;
let current_did = Context::current_identity::<Identity<T>>().unwrap_or_default();
match proposal_details.status {
Expand Down Expand Up @@ -931,8 +937,8 @@ impl<T: Config> Module<T> {
}
}
// Update storage
<Votes<T>>::insert(&multisig_signer_proposal, true);
<ProposalDetail<T>>::insert(&multisig_proposal, proposal_details);
<Votes<T>>::insert((&multisig, proposal_id), &signer, true);
<ProposalDetail<T>>::insert(&multisig, proposal_id, proposal_details);
// emit proposal rejected event
Self::deposit_event(RawEvent::ProposalRejectionVote(
current_did,
Expand Down
4 changes: 2 additions & 2 deletions pallets/runtime/tests/src/bridge.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ fn signers_approve_bridge_tx(tx: BridgeTx, signers: &[AccountId]) -> BridgeTx {
// Fetch proposal ID if unknown.
let p_id = proposal_id
.get_or_insert_with(|| {
MultiSig::proposal_ids(&controller.clone(), proposal.clone()).unwrap_or_default()
MultiSig::proposal_ids(&controller, proposal.clone()).unwrap_or_default()
})
.clone();
assert_eq!(
MultiSig::proposal_detail(&(controller.clone(), p_id)).approvals,
MultiSig::proposal_detail(&controller, p_id).approvals,
(i + 1) as u64
);
}
Expand Down
17 changes: 8 additions & 9 deletions pallets/runtime/tests/src/multisig.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,7 @@ fn change_multisig_sigs_required() {

assert_eq!(MultiSig::ms_signs_required(ms_address.clone()), 2);

let proposal = (ms_address.clone(), 0);
let proposal_details = MultiSig::proposal_detail(&proposal);
let proposal_details = MultiSig::proposal_detail(&ms_address, 0);
assert_eq!(proposal_details.status, ProposalStatus::ActiveOrExpired);

set_curr_did(Some(alice_did));
Expand Down Expand Up @@ -860,7 +859,7 @@ fn check_for_approval_closure() {
after_extra_approval_multi_purpose_nonce
);
assert_eq!(
MultiSig::proposal_detail(&(ms_address.clone(), proposal_id)).approvals,
MultiSig::proposal_detail(&ms_address, proposal_id).approvals,
1
);
});
Expand Down Expand Up @@ -947,7 +946,7 @@ fn reject_proposals() {
ms_address.clone(),
proposal_id1
));
let proposal_details1 = MultiSig::proposal_detail(&(ms_address.clone(), proposal_id1));
let proposal_details1 = MultiSig::proposal_detail(&ms_address, proposal_id1);
assert_eq!(proposal_details1.approvals, 2);
assert_eq!(proposal_details1.rejections, 3);
assert_eq!(proposal_details1.status, ProposalStatus::ActiveOrExpired);
Expand Down Expand Up @@ -977,7 +976,7 @@ fn reject_proposals() {
Error::ProposalAlreadyRejected
);

let proposal_details2 = MultiSig::proposal_detail(&(ms_address.clone(), proposal_id2));
let proposal_details2 = MultiSig::proposal_detail(&ms_address, proposal_id2);
next_block();
assert_eq!(proposal_details2.approvals, 1);
assert_eq!(proposal_details2.rejections, 3);
Expand Down Expand Up @@ -1263,7 +1262,7 @@ fn expired_proposals() {
));

let proposal_id = MultiSig::ms_tx_done(ms_address.clone()) - 1;
let mut proposal_details = MultiSig::proposal_detail(&(ms_address.clone(), proposal_id));
let mut proposal_details = MultiSig::proposal_detail(&ms_address, proposal_id);
assert_eq!(proposal_details.approvals, 1);
assert_eq!(proposal_details.status, ProposalStatus::ActiveOrExpired);

Expand All @@ -1274,7 +1273,7 @@ fn expired_proposals() {
proposal_id
));

proposal_details = MultiSig::proposal_detail(&(ms_address.clone(), proposal_id));
proposal_details = MultiSig::proposal_detail(&ms_address, proposal_id);
assert_eq!(proposal_details.approvals, 2);
assert_eq!(proposal_details.status, ProposalStatus::ActiveOrExpired);

Expand All @@ -1286,7 +1285,7 @@ fn expired_proposals() {
Error::ProposalExpired
);

proposal_details = MultiSig::proposal_detail(&(ms_address.clone(), proposal_id));
proposal_details = MultiSig::proposal_detail(&ms_address, proposal_id);
assert_eq!(proposal_details.approvals, 2);
assert_eq!(proposal_details.status, ProposalStatus::ActiveOrExpired);

Expand All @@ -1298,7 +1297,7 @@ fn expired_proposals() {
proposal_id
));

proposal_details = MultiSig::proposal_detail(&(ms_address.clone(), proposal_id));
proposal_details = MultiSig::proposal_detail(&ms_address, proposal_id);
assert_eq!(proposal_details.approvals, 3);
assert_eq!(proposal_details.status, ProposalStatus::ExecutionSuccessful);
});
Expand Down

0 comments on commit 54ae63a

Please sign in to comment.