diff --git a/pallets/common/src/traits/multisig.rs b/pallets/common/src/traits/multisig.rs index 249e03aa89..fb7fed04a3 100644 --- a/pallets/common/src/traits/multisig.rs +++ b/pallets/common/src/traits/multisig.rs @@ -70,6 +70,9 @@ decl_event!( ProposalExecutionFailed(DispatchError), /// Scheduling of proposal fails. SchedulingFailed(DispatchError), + /// Event emitted when a proposal failed to execute. + /// Arguments: caller DID, multisig, proposal ID, error. + ProposalFailedToExecute(IdentityId, AccountId, u64, DispatchError), } ); diff --git a/pallets/multisig/src/lib.rs b/pallets/multisig/src/lib.rs index 3ae0315fe2..665012e809 100644 --- a/pallets/multisig/src/lib.rs +++ b/pallets/multisig/src/lib.rs @@ -885,6 +885,12 @@ impl Module { Err(e) => { update_proposal_status(ProposalStatus::ExecutionFailed); Self::deposit_event(RawEvent::ProposalExecutionFailed(e.error)); + Self::deposit_event(RawEvent::ProposalFailedToExecute( + multisig_did, + multisig.clone(), + proposal_id, + e.error, + )); false } }; @@ -905,11 +911,18 @@ impl Module { proposal_id: u64, ) -> DispatchResult { Self::ensure_ms_signer(&multisig, &signer)?; - ensure!( - !Self::votes((&multisig, proposal_id), &signer), - Error::::AlreadyVoted - ); + let mut proposal_details = Self::proposal_detail(&multisig, proposal_id); + + // Only allow the original proposer to change their vote if no one else has voted + let mut proposal_owner = false; + if Votes::::get((&multisig, proposal_id), &signer) { + if proposal_details.rejections != 0 || proposal_details.approvals != 1 { + return Err(Error::::AlreadyVoted.into()); + } + proposal_owner = true; + } + proposal_details.rejections += 1u64; let current_did = Context::current_identity::>().unwrap_or_default(); match proposal_details.status { @@ -929,7 +942,12 @@ impl Module { if proposal_details.auto_close { let approvals_needed = Self::ms_signs_required(multisig.clone()); let ms_signers = Self::number_of_signers(multisig.clone()); - if proposal_details.rejections > ms_signers.saturating_sub(approvals_needed) { + if proposal_details.rejections > ms_signers.saturating_sub(approvals_needed) + || proposal_owner + { + if proposal_owner { + proposal_details.approvals = 0; + } proposal_details.status = ProposalStatus::Rejected; Self::deposit_event(RawEvent::ProposalRejected( current_did, diff --git a/pallets/runtime/tests/src/multisig.rs b/pallets/runtime/tests/src/multisig.rs index 64e398ffb4..3873230dba 100644 --- a/pallets/runtime/tests/src/multisig.rs +++ b/pallets/runtime/tests/src/multisig.rs @@ -1,6 +1,6 @@ -use frame_support::{assert_noop, assert_ok, StorageMap}; +use frame_support::{assert_noop, assert_ok, StorageDoubleMap, StorageMap}; -use pallet_multisig::{self as multisig, LostCreatorPrivileges}; +use pallet_multisig::{self as multisig, LostCreatorPrivileges, ProposalDetail, Votes}; use polymesh_common_utilities::constants::currency::POLY; use polymesh_primitives::multisig::ProposalStatus; use polymesh_primitives::{AccountId, AuthorizationData, Permissions, SecondaryKey, Signatory}; @@ -1240,6 +1240,142 @@ fn remove_creator_controls_successfully() { }); } +#[test] +fn proposal_owner_rejection() { + ExtBuilder::default().build().execute_with(|| { + let eve: User = User::new(AccountKeyring::Eve); + let bob: User = User::new(AccountKeyring::Bob); + let dave: User = User::new(AccountKeyring::Dave); + let alice: User = User::new(AccountKeyring::Alice); + + // Creates a multi-signature + let ms_address = + MultiSig::get_next_multisig_address(AccountKeyring::Alice.to_account_id()).unwrap(); + setup_multisig( + alice.origin(), + 3, + vec![ + Signatory::from(alice.did), + Signatory::from(bob.did), + Signatory::from(dave.did), + Signatory::from(eve.did), + ], + ); + + // Creates a proposal + let call1 = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 4 }, + )); + assert_ok!(MultiSig::create_proposal_as_identity( + alice.origin(), + ms_address.clone(), + call1, + None, + true + )); + let proposal_id = MultiSig::ms_tx_done(ms_address.clone()) - 1; + + // The owner of the proposal should be able to reject it if no one else has voted + assert_ok!(MultiSig::reject_as_identity( + alice.origin(), + ms_address.clone(), + proposal_id + )); + + // The proposal status must be set to rejected + let proposal_details = ProposalDetail::::get(&ms_address, proposal_id); + assert_eq!(proposal_details.status, ProposalStatus::Rejected); + assert_eq!(proposal_details.approvals, 0); + assert_eq!(proposal_details.rejections, 1); + assert_eq!(proposal_details.auto_close, true); + assert_eq!( + Votes::::get((&ms_address, proposal_id), Signatory::from(alice.did)), + true + ); + + // The owner shouldn't be able to change their vote again + assert_noop!( + MultiSig::reject_as_identity(alice.origin(), ms_address.clone(), proposal_id), + Error::AlreadyVoted + ); + + // No other votes are allowed, since the proposal has been rejected + assert_noop!( + MultiSig::reject_as_identity(bob.origin(), ms_address.clone(), proposal_id), + Error::ProposalAlreadyRejected + ); + }); +} + +#[test] +fn proposal_owner_rejection_denied() { + ExtBuilder::default().build().execute_with(|| { + let eve: User = User::new(AccountKeyring::Eve); + let bob: User = User::new(AccountKeyring::Bob); + let dave: User = User::new(AccountKeyring::Dave); + let alice: User = User::new(AccountKeyring::Alice); + + // Creates a multi-signature + let ms_address = + MultiSig::get_next_multisig_address(AccountKeyring::Alice.to_account_id()).unwrap(); + setup_multisig( + alice.origin(), + 3, + vec![ + Signatory::from(alice.did), + Signatory::from(bob.did), + Signatory::from(dave.did), + Signatory::from(eve.did), + ], + ); + + // Creates a proposal + let call1 = Box::new(RuntimeCall::MultiSig( + multisig::Call::change_sigs_required { sigs_required: 4 }, + )); + assert_ok!(MultiSig::create_proposal_as_identity( + alice.origin(), + ms_address.clone(), + call1, + None, + true + )); + let proposal_id = MultiSig::ms_tx_done(ms_address.clone()) - 1; + + // The owner of the proposal shouldn't be able to reject it since bob has already voted + assert_ok!(MultiSig::reject_as_identity( + bob.origin(), + ms_address.clone(), + proposal_id + )); + assert_noop!( + MultiSig::reject_as_identity(alice.origin(), ms_address.clone(), proposal_id), + Error::AlreadyVoted + ); + + // The proposal status must be set to Active + let proposal_details = ProposalDetail::::get(&ms_address, proposal_id); + assert_eq!(proposal_details.status, ProposalStatus::ActiveOrExpired); + assert_eq!(proposal_details.approvals, 1); + assert_eq!(proposal_details.rejections, 1); + assert_eq!(proposal_details.auto_close, true); + assert_eq!( + Votes::::get((&ms_address, proposal_id), Signatory::from(alice.did)), + true + ); + assert_eq!( + Votes::::get((&ms_address, proposal_id), Signatory::from(bob.did)), + true + ); + + // No user should be able to change their vote + assert_noop!( + MultiSig::reject_as_identity(bob.origin(), ms_address.clone(), proposal_id), + Error::AlreadyVoted + ); + }); +} + fn expired_proposals() { ExtBuilder::default().build().execute_with(|| { let alice_did = register_keyring_account(AccountKeyring::Alice).unwrap();