From 7d97b2628339cea3febb8c83660999d2a5a17f07 Mon Sep 17 00:00:00 2001 From: clearloop Date: Wed, 9 Jun 2021 14:42:45 +0800 Subject: [PATCH 1/9] feat(committee): simple EnsureMember for committee origin --- pallets/committee/src/types.rs | 39 ++++++++++++++++++++++++++++++---- runtime/src/lib.rs | 10 +++++++-- 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/pallets/committee/src/types.rs b/pallets/committee/src/types.rs index 2978edbc83..111098b689 100644 --- a/pallets/committee/src/types.rs +++ b/pallets/committee/src/types.rs @@ -1,11 +1,12 @@ // Copyright 2021 ChainSafe Systems // SPDX-License-Identifier: LGPL-3.0-only -use crate::Config; +use crate::{Config, Origin}; use frame_support::{ pallet_prelude::*, sp_std::{self, prelude::Vec}, }; +// use frame_system::ensure_signed; use sp_runtime::traits::Hash; #[cfg(feature = "std")] @@ -76,6 +77,8 @@ impl MemberVote { pub enum CommitteeOrigin { /// Action is executed by the committee. Contains the closer account and the members that voted Aye ApprovedByCommittee(AccountId, VoteAggregate), + /// It has been condoned by a single member of the committee. + CommitteeMember(AccountId), } #[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug, Default)] @@ -188,16 +191,19 @@ pub struct EnsureApprovedByCommittee( ); impl< O: Into, O>> - + From>, + + From> + + Clone, AccountId: PartialEq + Default, BlockNumber: Default, > EnsureOrigin for EnsureApprovedByCommittee { type Success = AccountId; fn try_origin(o: O) -> Result { - o.into().and_then(|o| match o { + let origin = o.clone().into()?; + match origin { CommitteeOrigin::ApprovedByCommittee(i, _) => Ok(i), - }) + _ => Err(o), + } } #[cfg(feature = "runtime-benchmarks")] @@ -208,3 +214,28 @@ impl< )) } } + +/// Ensure the passing origin is committee member +pub struct EnsureMember(sp_std::marker::PhantomData<(AccountId, T)>); + +impl< + O: Into, O>> + From> + Clone, + AccountId: Default, + T: Config + frame_system::Config, + > EnsureOrigin for EnsureMember +{ + type Success = AccountId; + + fn try_origin(o: O) -> Result { + let origin = o.clone().into()?; + match origin { + CommitteeOrigin::CommitteeMember(account) => Ok(account), + _ => Err(o), + } + } + + #[cfg(feature = "runtime-benchmarks")] + fn successful_origin() -> O { + O::from(RawOrigin::Member(Default::default())) + } +} diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 46d7dea0a2..ba88ee8243 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -480,13 +480,19 @@ type EnsureApprovedByCommittee = frame_system::EnsureOneOf< pallet_committee::EnsureApprovedByCommittee, >; +type EnsureMember = frame_system::EnsureOneOf< + AccountId, + frame_system::EnsureRoot, + pallet_committee::EnsureMember, +>; + impl pallet_committee::Config for Runtime { type ProposalSubmissionPeriod = ProposalSubmissionPeriod; type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; // Using signed as the admin origin for now - type ProposalSubmissionOrigin = frame_system::EnsureSigned; - type ProposalExecutionOrigin = frame_system::EnsureSigned; + type ProposalSubmissionOrigin = EnsureMember; + type ProposalExecutionOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin; From f46fa07262ef82534cf054a1ed2658a263133925 Mon Sep 17 00:00:00 2001 From: clearloop Date: Wed, 9 Jun 2021 17:20:01 +0800 Subject: [PATCH 2/9] feat(committee): support both Committee and Signed Origin --- pallets/committee/src/lib.rs | 8 ++++---- pallets/committee/src/types.rs | 30 +++++++++++++++--------------- runtime/src/lib.rs | 10 +++++----- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/pallets/committee/src/lib.rs b/pallets/committee/src/lib.rs index e91d9202a4..3e408a5486 100644 --- a/pallets/committee/src/lib.rs +++ b/pallets/committee/src/lib.rs @@ -322,8 +322,8 @@ pub mod pallet { /// The provided action will be turned into a proposal and added to the list of current active proposals /// to be voted on in the next voting period. pub fn propose(origin: OriginFor, action: Box) -> DispatchResultWithPostInfo { - let proposer = ensure_signed(origin.clone())?; - T::ProposalSubmissionOrigin::ensure_origin(origin)?; + T::ProposalSubmissionOrigin::ensure_origin(origin.clone())?; + let proposer = ensure_signed(origin)?; // Create a new proposal with a unique nonce let nonce = Self::take_and_increment_nonce()?; @@ -390,8 +390,8 @@ pub mod pallet { origin: OriginFor, proposal_hash: HashFor, ) -> DispatchResultWithPostInfo { - let closer = ensure_signed(origin.clone())?; - T::ProposalExecutionOrigin::ensure_origin(origin)?; + T::ProposalExecutionOrigin::ensure_origin(origin.clone())?; + let closer = ensure_signed(origin)?; // ensure proposal has not already been executed ensure!( diff --git a/pallets/committee/src/types.rs b/pallets/committee/src/types.rs index 111098b689..8ced81dcb2 100644 --- a/pallets/committee/src/types.rs +++ b/pallets/committee/src/types.rs @@ -1,12 +1,11 @@ // Copyright 2021 ChainSafe Systems // SPDX-License-Identifier: LGPL-3.0-only -use crate::{Config, Origin}; +use crate::Config; use frame_support::{ pallet_prelude::*, sp_std::{self, prelude::Vec}, }; -// use frame_system::ensure_signed; use sp_runtime::traits::Hash; #[cfg(feature = "std")] @@ -215,27 +214,28 @@ impl< } } -/// Ensure the passing origin is committee member -pub struct EnsureMember(sp_std::marker::PhantomData<(AccountId, T)>); - impl< - O: Into, O>> + From> + Clone, - AccountId: Default, - T: Config + frame_system::Config, - > EnsureOrigin for EnsureMember + O: Into, O>> + + From> + + Clone, + AccountId: PartialEq + Default, + BlockNumber: Default, + > EnsureOrigin for CommitteeOrigin { type Success = AccountId; - fn try_origin(o: O) -> Result { let origin = o.clone().into()?; - match origin { - CommitteeOrigin::CommitteeMember(account) => Ok(account), - _ => Err(o), - } + Ok(match origin { + CommitteeOrigin::ApprovedByCommittee(i, _) => i, + CommitteeOrigin::CommitteeMember(i) => i, + }) } #[cfg(feature = "runtime-benchmarks")] fn successful_origin() -> O { - O::from(RawOrigin::Member(Default::default())) + O::from(CommitteeOrigin::ApprovedByCommittee( + Default::default(), + Default::default(), + )) } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index ba88ee8243..9a5d7d11b6 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -480,10 +480,10 @@ type EnsureApprovedByCommittee = frame_system::EnsureOneOf< pallet_committee::EnsureApprovedByCommittee, >; -type EnsureMember = frame_system::EnsureOneOf< +type EnsureCommittee = frame_system::EnsureOneOf< AccountId, - frame_system::EnsureRoot, - pallet_committee::EnsureMember, + frame_system::EnsureSigned, + pallet_committee::Origin, >; impl pallet_committee::Config for Runtime { @@ -491,8 +491,8 @@ impl pallet_committee::Config for Runtime { type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; // Using signed as the admin origin for now - type ProposalSubmissionOrigin = EnsureMember; - type ProposalExecutionOrigin = EnsureMember; + type ProposalSubmissionOrigin = EnsureCommittee; + type ProposalExecutionOrigin = EnsureCommittee; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin; From 6c9514a163d2486f795bfacc84a9eb223ce0baa3 Mon Sep 17 00:00:00 2001 From: clearloop Date: Wed, 9 Jun 2021 18:41:24 +0800 Subject: [PATCH 3/9] feat(committee): update benchmarking.rs --- node/src/chain_spec.rs | 17 +++++++++++++++++ pallets/committee/src/benchmarking.rs | 5 +++-- pallets/committee/src/lib.rs | 4 ++-- pallets/committee/src/mock.rs | 6 ++++-- runtime/src/lib.rs | 2 +- 5 files changed, 27 insertions(+), 7 deletions(-) diff --git a/node/src/chain_spec.rs b/node/src/chain_spec.rs index b05c32f933..32a4152ef0 100644 --- a/node/src/chain_spec.rs +++ b/node/src/chain_spec.rs @@ -62,6 +62,12 @@ pub fn pint_development_config(id: ParaId) -> ChainSpec { get_account_id_from_seed::("Alice//stash"), get_account_id_from_seed::("Bob//stash"), ], + vec![ + get_account_id_from_seed::("Alice"), + get_account_id_from_seed::("Bob"), + get_account_id_from_seed::("Charlie"), + get_account_id_from_seed::("Dave"), + ], id, ) }, @@ -100,6 +106,12 @@ pub fn pint_local_config(id: ParaId) -> ChainSpec { get_account_id_from_seed::("Eve//stash"), get_account_id_from_seed::("Ferdie//stash"), ], + vec![ + get_account_id_from_seed::("Alice"), + get_account_id_from_seed::("Bob"), + get_account_id_from_seed::("Charlie"), + get_account_id_from_seed::("Dave"), + ], id, ) }, @@ -117,6 +129,7 @@ pub fn pint_local_config(id: ParaId) -> ChainSpec { fn pint_testnet_genesis( root_key: AccountId, endowed_accounts: Vec, + council_members: Vec, id: ParaId, ) -> parachain_runtime::GenesisConfig { parachain_runtime::GenesisConfig { @@ -126,6 +139,10 @@ fn pint_testnet_genesis( .to_vec(), changes_trie_config: Default::default(), }, + pallet_committee: parachain_runtime::CommitteeConfig { + council_members, + ..Default::default() + }, pallet_balances: parachain_runtime::BalancesConfig { balances: endowed_accounts .iter() diff --git a/pallets/committee/src/benchmarking.rs b/pallets/committee/src/benchmarking.rs index 42246bdcf1..54ac952acb 100644 --- a/pallets/committee/src/benchmarking.rs +++ b/pallets/committee/src/benchmarking.rs @@ -18,6 +18,7 @@ fn submit_proposal(caller: T::AccountId) -> pallet::Proposal { benchmarks! { propose { let caller: T::AccountId = whitelisted_caller(); + assert_ok!(>::add_constituent(SystemOrigin::Root.into(), caller.clone())); let proposal = submit_proposal::(caller.clone()); }: _( SystemOrigin::Signed(caller.clone()), @@ -28,8 +29,8 @@ benchmarks! { vote { let caller: T::AccountId = whitelisted_caller(); - let proposal = submit_proposal::(caller.clone()); assert_ok!(>::add_constituent(SystemOrigin::Root.into(), caller.clone())); + let proposal = submit_proposal::(caller.clone()); // run to voting period >::set_block_number( @@ -50,8 +51,8 @@ benchmarks! { close { let caller: T::AccountId = whitelisted_caller(); - let proposal: pallet::Proposal = submit_proposal::(caller.clone()); assert_ok!(>::add_constituent(SystemOrigin::Root.into(), caller.clone())); + let proposal: pallet::Proposal = submit_proposal::(caller.clone()); let voters = ["a", "b", "c", "d", "e"]; // run to voting period diff --git a/pallets/committee/src/lib.rs b/pallets/committee/src/lib.rs index 3e408a5486..d86f28c41a 100644 --- a/pallets/committee/src/lib.rs +++ b/pallets/committee/src/lib.rs @@ -323,7 +323,7 @@ pub mod pallet { /// to be voted on in the next voting period. pub fn propose(origin: OriginFor, action: Box) -> DispatchResultWithPostInfo { T::ProposalSubmissionOrigin::ensure_origin(origin.clone())?; - let proposer = ensure_signed(origin)?; + let proposer = Self::ensure_member(origin)?; // Create a new proposal with a unique nonce let nonce = Self::take_and_increment_nonce()?; @@ -341,7 +341,7 @@ pub mod pallet { Votes::::insert(proposal_hash, VoteAggregate::new_with_end(end)); - Self::deposit_event(Event::Proposed(proposer, nonce, proposal_hash)); + Self::deposit_event(Event::Proposed(proposer.account_id, nonce, proposal_hash)); Ok(().into()) } diff --git a/pallets/committee/src/mock.rs b/pallets/committee/src/mock.rs index 31cf4ec143..83d779444e 100644 --- a/pallets/committee/src/mock.rs +++ b/pallets/committee/src/mock.rs @@ -75,7 +75,7 @@ parameter_types! { pub const VotingPeriod: ::BlockNumber = VOTING_PERIOD; } pub(crate) const PROPOSER_ACCOUNT_ID: AccountId = 88; -pub(crate) const EXECUTER_ACCOUNT_ID: AccountId = 88; +pub(crate) const EXECUTER_ACCOUNT_ID: AccountId = PROPOSER_ACCOUNT_ID; pub(crate) const MIN_COUNCIL_VOTES: usize = 4; ord_parameter_types! { @@ -125,8 +125,10 @@ where .build_storage::() .unwrap(); + let mut council_members = vec![PROPOSER_ACCOUNT_ID]; + council_members.append(&mut members.into_iter().collect()); pallet_committee::GenesisConfig:: { - council_members: members.into_iter().collect(), + council_members, constituent_members: Default::default(), } .assimilate_storage(&mut t) diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 9a5d7d11b6..af267e7c66 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -711,7 +711,7 @@ construct_runtime!( // PINT pallets AssetIndex: pallet_asset_index::{Pallet, Call, Storage, Event}, AssetDepository: pallet_asset_depository::{Pallet, Call, Storage, Event}, - Committee: pallet_committee::{Pallet, Call, Storage, Origin, Event}, + Committee: pallet_committee::{Pallet, Call, Storage, Origin, Event, Config}, LocalTreasury: pallet_local_treasury::{Pallet, Call, Storage, Event}, SaftRegistry: pallet_saft_registry::{Pallet, Call, Storage, Event}, RemoteAssetManager: pallet_remote_asset_manager::{Pallet, Call, Storage, Event}, From 344cc2e88840843149b391c49c8b8092f8dfe838 Mon Sep 17 00:00:00 2001 From: clearloop Date: Wed, 9 Jun 2021 20:33:00 +0800 Subject: [PATCH 4/9] feat(committee): check votes in EnsureApprovedByCommittee --- pallets/committee/src/mock.rs | 2 +- pallets/committee/src/tests.rs | 22 +++---------- pallets/committee/src/types.rs | 58 +++++++++++++++++----------------- runtime/src/lib.rs | 7 ++-- 4 files changed, 37 insertions(+), 52 deletions(-) diff --git a/pallets/committee/src/mock.rs b/pallets/committee/src/mock.rs index 83d779444e..27ae4019ef 100644 --- a/pallets/committee/src/mock.rs +++ b/pallets/committee/src/mock.rs @@ -88,7 +88,7 @@ ord_parameter_types! { type EnsureApprovedByCommittee = frame_system::EnsureOneOf< AccountId, frame_system::EnsureRoot, - crate::EnsureApprovedByCommittee, + crate::EnsureApprovedByCommittee, >; impl pallet_committee::Config for Test { diff --git a/pallets/committee/src/tests.rs b/pallets/committee/src/tests.rs index 72aec6f3df..84c2a1262c 100644 --- a/pallets/committee/src/tests.rs +++ b/pallets/committee/src/tests.rs @@ -3,7 +3,7 @@ use crate as pallet; use crate::mock::*; -use crate::{CommitteeMember, CommitteeOrigin, MemberType, Vote, VoteAggregate}; +use crate::{CommitteeMember, MemberType, Vote, VoteAggregate}; use frame_support::{assert_noop, assert_ok, codec::Encode}; use frame_system as system; use sp_runtime::traits::BadOrigin; @@ -451,22 +451,11 @@ fn cannot_execute_proposal_twice() { // Constituent Committee Council Selection // -/// An `ApprovedByCommittee` origin -fn approved_by_committee() -> CommitteeOrigin { - CommitteeOrigin::ApprovedByCommittee( - PROPOSER_ACCOUNT_ID, - VoteAggregate { - votes: Vec::new(), - end: START_OF_V1, - }, - ) -} - #[test] fn cannot_add_constituent_if_already_is_council() { new_test_ext(PROPOSER_ACCOUNT_ID..PROPOSER_ACCOUNT_ID + 1).execute_with(|| { assert_noop!( - Committee::add_constituent(approved_by_committee().into(), PROPOSER_ACCOUNT_ID), + Committee::add_constituent(Origin::root().into(), PROPOSER_ACCOUNT_ID), >::AlreadyCouncilMember ); }); @@ -475,13 +464,10 @@ fn cannot_add_constituent_if_already_is_council() { #[test] fn cannot_add_constituent_if_already_is_constituent() { new_test_ext(PROPOSER_ACCOUNT_ID..PROPOSER_ACCOUNT_ID + 1).execute_with(|| { - assert_ok!(Committee::add_constituent( - approved_by_committee().into(), - 42 - )); + assert_ok!(Committee::add_constituent(Origin::root().into(), 42)); assert_noop!( - Committee::add_constituent(approved_by_committee().into(), 42), + Committee::add_constituent(Origin::root().into(), 42), >::AlreadyConstituentMember ); }); diff --git a/pallets/committee/src/types.rs b/pallets/committee/src/types.rs index 8ced81dcb2..a7aedc8e6a 100644 --- a/pallets/committee/src/types.rs +++ b/pallets/committee/src/types.rs @@ -1,7 +1,7 @@ // Copyright 2021 ChainSafe Systems // SPDX-License-Identifier: LGPL-3.0-only -use crate::Config; +use crate::{Config, Members, Origin}; use frame_support::{ pallet_prelude::*, sp_std::{self, prelude::Vec}, @@ -185,22 +185,21 @@ pub enum Vote { /// // This is for the extrinsics only can be called after the /// approval of the committee -pub struct EnsureApprovedByCommittee( - sp_std::marker::PhantomData<(AccountId, BlockNumber)>, -); -impl< - O: Into, O>> - + From> - + Clone, - AccountId: PartialEq + Default, - BlockNumber: Default, - > EnsureOrigin for EnsureApprovedByCommittee +pub struct EnsureApprovedByCommittee(sp_std::marker::PhantomData); + +impl, O>> + From> + Clone, T: Config> EnsureOrigin + for EnsureApprovedByCommittee { - type Success = AccountId; + type Success = ::AccountId; fn try_origin(o: O) -> Result { let origin = o.clone().into()?; match origin { - CommitteeOrigin::ApprovedByCommittee(i, _) => Ok(i), + CommitteeOrigin::ApprovedByCommittee(i, votes) => { + votes + .is_accepted(T::MinCouncilVotes::get()) + .map_err(|_| o)?; + Ok(i) + } _ => Err(o), } } @@ -214,28 +213,29 @@ impl< } } -impl< - O: Into, O>> - + From> - + Clone, - AccountId: PartialEq + Default, - BlockNumber: Default, - > EnsureOrigin for CommitteeOrigin +/// Ensure committee origin +pub struct EnsureMember(sp_std::marker::PhantomData); + +impl, O>> + From> + Clone, T: Config> EnsureOrigin + for EnsureMember { - type Success = AccountId; + type Success = ::AccountId; fn try_origin(o: O) -> Result { let origin = o.clone().into()?; - Ok(match origin { - CommitteeOrigin::ApprovedByCommittee(i, _) => i, - CommitteeOrigin::CommitteeMember(i) => i, - }) + match origin { + CommitteeOrigin::CommitteeMember(i) => { + if >::contains_key(i.clone()) { + Ok(i) + } else { + Err(o) + } + } + _ => Err(o), + } } #[cfg(feature = "runtime-benchmarks")] fn successful_origin() -> O { - O::from(CommitteeOrigin::ApprovedByCommittee( - Default::default(), - Default::default(), - )) + O::from(CommitteeOrigin::CommitteeMember(Default::default())) } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index af267e7c66..f0d30af776 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -477,20 +477,19 @@ ord_parameter_types! { type EnsureApprovedByCommittee = frame_system::EnsureOneOf< AccountId, frame_system::EnsureRoot, - pallet_committee::EnsureApprovedByCommittee, + pallet_committee::EnsureApprovedByCommittee, >; type EnsureCommittee = frame_system::EnsureOneOf< AccountId, - frame_system::EnsureSigned, - pallet_committee::Origin, + pallet_committee::EnsureApprovedByCommittee, + pallet_committee::EnsureMember, >; impl pallet_committee::Config for Runtime { type ProposalSubmissionPeriod = ProposalSubmissionPeriod; type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; - // Using signed as the admin origin for now type ProposalSubmissionOrigin = EnsureCommittee; type ProposalExecutionOrigin = EnsureCommittee; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; From b8311a7a7adb1bb201d8604795e53caa2f516e72 Mon Sep 17 00:00:00 2001 From: clearloop Date: Thu, 10 Jun 2021 07:32:39 +0800 Subject: [PATCH 5/9] feat(committee): complete EnsureMember --- pallets/committee/src/benchmarking.rs | 93 ++++++++++++++------------- pallets/committee/src/lib.rs | 43 +++++++------ pallets/committee/src/mock.rs | 7 +- pallets/committee/src/tests.rs | 7 +- pallets/committee/src/types.rs | 14 ++-- runtime/src/lib.rs | 12 ++-- 6 files changed, 93 insertions(+), 83 deletions(-) diff --git a/pallets/committee/src/benchmarking.rs b/pallets/committee/src/benchmarking.rs index 54ac952acb..a9ec7faea2 100644 --- a/pallets/committee/src/benchmarking.rs +++ b/pallets/committee/src/benchmarking.rs @@ -1,36 +1,40 @@ // Copyright 2021 ChainSafe Systems // SPDX-License-Identifier: LGPL-3.0-only use super::*; -use frame_benchmarking::{account, benchmarks, vec, whitelisted_caller, Box}; -use frame_support::{assert_noop, assert_ok, traits::Get}; -use frame_system::{Call as SystemCall, Pallet as System, RawOrigin as SystemOrigin}; +use frame_benchmarking::{account, benchmarks, vec, Box}; +use frame_support::{ + assert_noop, assert_ok, + traits::{EnsureOrigin, Get, UnfilteredDispatchable}, +}; +use frame_system::{ + ensure_signed, Call as SystemCall, Pallet as System, RawOrigin as SystemOrigin, +}; -fn submit_proposal(caller: T::AccountId) -> pallet::Proposal { +fn submit_proposal(origin: ::Origin) -> pallet::Proposal { let action: T::Action = >::remark(vec![0; 0]).into(); let expected_nonce = pallet::ProposalCount::::get(); - assert_ok!(>::propose( - SystemOrigin::Signed(caller).into(), - Box::new(action.clone()) + assert_ok!(>::add_constituent( + SystemOrigin::Root.into(), + ensure_signed(origin.clone()).unwrap(), )); + let call = >::propose(Box::new(action.clone())); + assert_ok!(call.dispatch_bypass_filter(origin)); pallet::Proposal::::new(expected_nonce, action) } benchmarks! { propose { - let caller: T::AccountId = whitelisted_caller(); - assert_ok!(>::add_constituent(SystemOrigin::Root.into(), caller.clone())); - let proposal = submit_proposal::(caller.clone()); - }: _( - SystemOrigin::Signed(caller.clone()), - Box::new(>::remark(vec![0; 0]).into()) - ) verify { + let origin = T::ProposalSubmissionOrigin::successful_origin(); + let proposal = submit_proposal::(origin.clone()); + let call = >::propose(Box::new(>::remark(vec![0; 0]).into())); + }: { + call.dispatch_bypass_filter(origin)? + } verify { assert!(>::get_proposal(&proposal.hash()) == Some(proposal)); } vote { - let caller: T::AccountId = whitelisted_caller(); - assert_ok!(>::add_constituent(SystemOrigin::Root.into(), caller.clone())); - let proposal = submit_proposal::(caller.clone()); + let proposal = submit_proposal::(T::ProposalVoteOrigin::successful_origin()); // run to voting period >::set_block_number( @@ -38,11 +42,12 @@ benchmarks! { + ::VotingPeriod::get() + ::ProposalSubmissionPeriod::get() + 1_u32.into(), ); - }: _( - SystemOrigin::Signed(caller.clone()), - proposal.hash(), - Vote::Abstain - ) verify { + + // construct call + let call = >::vote(proposal.hash(), Vote::Abstain); + }: { + call.dispatch_bypass_filter(T::ProposalVoteOrigin::successful_origin())? + } verify { assert_eq!( >::get_votes_for(&proposal.hash()).unwrap().votes.len(), 1, @@ -50,25 +55,21 @@ benchmarks! { } close { - let caller: T::AccountId = whitelisted_caller(); - assert_ok!(>::add_constituent(SystemOrigin::Root.into(), caller.clone())); - let proposal: pallet::Proposal = submit_proposal::(caller.clone()); - let voters = ["a", "b", "c", "d", "e"]; + let proposal: pallet::Proposal = submit_proposal::(T::ProposalSubmissionOrigin::successful_origin()); - // run to voting period - >::set_block_number(>::block_number() + ::VotingPeriod::get() + ::ProposalSubmissionPeriod::get() + 1_u32.into()); - - // generate members - for i in &voters { - let voter: T::AccountId = account(i, 0, 0); - >::insert(voter.clone(), MemberType::Council); - - // vote aye - assert_ok!(>::vote( - SystemOrigin::Signed(voter).into(), - proposal.hash(), - Vote::Aye, - )); + // vote + for i in 0..5 { + let voter: T::AccountId = account("voter", i, 0); + assert_ok!(Votes::::try_mutate(&proposal.hash(), |votes| { + if let Some(votes) = votes { + votes.cast_vote( + MemberVote::new(CommitteeMember::new(voter, MemberType::Council), Vote::Aye), + ); + Ok(()) + } else { + Err(Error::::NoProposalWithHash) + } + })); } // run out of voting period @@ -78,12 +79,14 @@ benchmarks! { + ::ProposalSubmissionPeriod::get() + 1_u32.into() ); - }: _( - SystemOrigin::Signed(caller.clone()), - proposal.hash() - ) verify { + + // construct call + let call = >::close(proposal.hash()); + }: { + call.dispatch_bypass_filter(T::ProposalExecutionOrigin::successful_origin())? + } verify { assert_noop!( - >::close(SystemOrigin::Signed(caller.clone()).into(), proposal.hash()), + >::close(T::ProposalExecutionOrigin::successful_origin(), proposal.hash()), >::ProposalAlreadyExecuted ); } diff --git a/pallets/committee/src/lib.rs b/pallets/committee/src/lib.rs index d86f28c41a..ca22ce91a3 100644 --- a/pallets/committee/src/lib.rs +++ b/pallets/committee/src/lib.rs @@ -76,10 +76,22 @@ pub mod pallet { type MinCouncilVotes: Get; /// Origin that is permitted to create proposals - type ProposalSubmissionOrigin: EnsureOrigin<::Origin>; + type ProposalSubmissionOrigin: EnsureOrigin< + ::Origin, + Success = ::AccountId, + >; /// Origin that is permitted to execute approved proposals - type ProposalExecutionOrigin: EnsureOrigin<::Origin>; + type ProposalExecutionOrigin: EnsureOrigin< + ::Origin, + Success = ::AccountId, + >; + + /// Origin that is permitted to execute approved proposals + type ProposalVoteOrigin: EnsureOrigin< + ::Origin, + Success = ::AccountId, + >; /// Origin that is permitted to execute `add_constituent` type ApprovedByCommitteeOrigin: EnsureOrigin<::Origin>; @@ -265,19 +277,6 @@ pub mod pallet { Votes::::get(hash) } - /// Used to check if an origin is signed and the signer is a member of - /// the committee - pub fn ensure_member( - origin: OriginFor, - ) -> Result>, DispatchError> { - let who = ensure_signed(origin)?; - if let Some(member_type) = Members::::get(who.clone()) { - Ok(CommitteeMember::new(who, member_type)) - } else { - Err(Error::::NotMember.into()) - } - } - /// Returns the block at the end of the next voting period pub fn get_next_voting_period_end( block_number: &BlockNumberFor, @@ -322,8 +321,7 @@ pub mod pallet { /// The provided action will be turned into a proposal and added to the list of current active proposals /// to be voted on in the next voting period. pub fn propose(origin: OriginFor, action: Box) -> DispatchResultWithPostInfo { - T::ProposalSubmissionOrigin::ensure_origin(origin.clone())?; - let proposer = Self::ensure_member(origin)?; + let proposer = T::ProposalSubmissionOrigin::ensure_origin(origin.clone())?; // Create a new proposal with a unique nonce let nonce = Self::take_and_increment_nonce()?; @@ -341,7 +339,7 @@ pub mod pallet { Votes::::insert(proposal_hash, VoteAggregate::new_with_end(end)); - Self::deposit_event(Event::Proposed(proposer.account_id, nonce, proposal_hash)); + Self::deposit_event(Event::Proposed(proposer, nonce, proposal_hash)); Ok(().into()) } @@ -357,7 +355,11 @@ pub mod pallet { vote: Vote, ) -> DispatchResultWithPostInfo { // Only members can vote - let voter = Self::ensure_member(origin)?; + let caller = T::ProposalVoteOrigin::ensure_origin(origin)?; + let voter = CommitteeMember::new( + caller.clone(), + >::get(caller).ok_or(>::NotMember)?, + ); Votes::::try_mutate(&proposal_hash, |votes| { if let Some(votes) = votes { @@ -390,8 +392,7 @@ pub mod pallet { origin: OriginFor, proposal_hash: HashFor, ) -> DispatchResultWithPostInfo { - T::ProposalExecutionOrigin::ensure_origin(origin.clone())?; - let closer = ensure_signed(origin)?; + let closer = T::ProposalExecutionOrigin::ensure_origin(origin.clone())?; // ensure proposal has not already been executed ensure!( diff --git a/pallets/committee/src/mock.rs b/pallets/committee/src/mock.rs index 27ae4019ef..5097e16c5d 100644 --- a/pallets/committee/src/mock.rs +++ b/pallets/committee/src/mock.rs @@ -4,7 +4,7 @@ // Required as construct_runtime! produces code that violates this lint #![allow(clippy::from_over_into)] -use crate as pallet_committee; +use crate::{self as pallet_committee, EnsureMember}; #[cfg(feature = "std")] use frame_support::traits::GenesisBuild; use frame_support::{ @@ -95,8 +95,9 @@ impl pallet_committee::Config for Test { type ProposalSubmissionPeriod = ProposalSubmissionPeriod; type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; - type ProposalSubmissionOrigin = frame_system::EnsureSignedBy; - type ProposalExecutionOrigin = frame_system::EnsureSignedBy; + type ProposalSubmissionOrigin = EnsureMember; + type ProposalExecutionOrigin = EnsureMember; + type ProposalVoteOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin; diff --git a/pallets/committee/src/tests.rs b/pallets/committee/src/tests.rs index 84c2a1262c..2b4fed4716 100644 --- a/pallets/committee/src/tests.rs +++ b/pallets/committee/src/tests.rs @@ -130,7 +130,7 @@ fn non_member_cannot_vote() { let expected_votes = VoteAggregate::new_with_end(START_OF_V1); assert_noop!( Committee::vote(Origin::signed(ASHLEY), proposal.hash(), Vote::Aye), - pallet::Error::::NotMember + BadOrigin, ); assert_eq!( Committee::get_votes_for(&proposal.hash()), @@ -326,12 +326,15 @@ where #[test] fn non_execution_origin_cannot_close() { new_test_ext(0..4).execute_with(|| { + let non_execution_origin = 5; let proposal = submit_proposal(123); run_to_block(START_OF_S1); vote_with_each(0..4, proposal.hash(), Vote::Aye); + + run_to_block(START_OF_V1 + 1); assert_noop!( - Committee::close(Origin::signed(ASHLEY), proposal.hash()), + Committee::close(Origin::signed(non_execution_origin), proposal.hash()), BadOrigin ); }); diff --git a/pallets/committee/src/types.rs b/pallets/committee/src/types.rs index a7aedc8e6a..36fca60cf7 100644 --- a/pallets/committee/src/types.rs +++ b/pallets/committee/src/types.rs @@ -5,7 +5,9 @@ use crate::{Config, Members, Origin}; use frame_support::{ pallet_prelude::*, sp_std::{self, prelude::Vec}, + traits::EnsureOrigin, }; +use frame_system::RawOrigin; use sp_runtime::traits::Hash; #[cfg(feature = "std")] @@ -216,14 +218,18 @@ impl, O>> + From> + Clone, T: Config> EnsureO /// Ensure committee origin pub struct EnsureMember(sp_std::marker::PhantomData); -impl, O>> + From> + Clone, T: Config> EnsureOrigin - for EnsureMember +impl< + O: Into::AccountId>, O>> + + From::AccountId>> + + Clone, + T: Config, + > EnsureOrigin for EnsureMember { type Success = ::AccountId; fn try_origin(o: O) -> Result { let origin = o.clone().into()?; match origin { - CommitteeOrigin::CommitteeMember(i) => { + RawOrigin::Signed(i) => { if >::contains_key(i.clone()) { Ok(i) } else { @@ -236,6 +242,6 @@ impl, O>> + From> + Clone, T: Config> EnsureO #[cfg(feature = "runtime-benchmarks")] fn successful_origin() -> O { - O::from(CommitteeOrigin::CommitteeMember(Default::default())) + O::from(RawOrigin::Signed(Default::default())) } } diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index f0d30af776..8dee11773d 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -474,24 +474,20 @@ ord_parameter_types! { pub const MinCouncilVotes: usize = 4; } +type EnsureMember = pallet_committee::EnsureMember; type EnsureApprovedByCommittee = frame_system::EnsureOneOf< AccountId, frame_system::EnsureRoot, pallet_committee::EnsureApprovedByCommittee, >; -type EnsureCommittee = frame_system::EnsureOneOf< - AccountId, - pallet_committee::EnsureApprovedByCommittee, - pallet_committee::EnsureMember, ->; - impl pallet_committee::Config for Runtime { type ProposalSubmissionPeriod = ProposalSubmissionPeriod; type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; - type ProposalSubmissionOrigin = EnsureCommittee; - type ProposalExecutionOrigin = EnsureCommittee; + type ProposalSubmissionOrigin = EnsureMember; + type ProposalExecutionOrigin = EnsureMember; + type ProposalVoteOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin; From f7867376c42f248e81d8a216193d963fe60c2c07 Mon Sep 17 00:00:00 2001 From: clearloop Date: Thu, 10 Jun 2021 07:43:18 +0800 Subject: [PATCH 6/9] perf(committee): fix docs --- pallets/committee/src/lib.rs | 2 +- pallets/committee/src/types.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pallets/committee/src/lib.rs b/pallets/committee/src/lib.rs index ca22ce91a3..8ba54dc60e 100644 --- a/pallets/committee/src/lib.rs +++ b/pallets/committee/src/lib.rs @@ -87,7 +87,7 @@ pub mod pallet { Success = ::AccountId, >; - /// Origin that is permitted to execute approved proposals + /// Origin that is permitted to vote type ProposalVoteOrigin: EnsureOrigin< ::Origin, Success = ::AccountId, diff --git a/pallets/committee/src/types.rs b/pallets/committee/src/types.rs index 36fca60cf7..025c12bfdf 100644 --- a/pallets/committee/src/types.rs +++ b/pallets/committee/src/types.rs @@ -215,7 +215,7 @@ impl, O>> + From> + Clone, T: Config> EnsureO } } -/// Ensure committee origin +/// Ensure committee member pub struct EnsureMember(sp_std::marker::PhantomData); impl< From 5aa9d9cf094f25aa29083fcd0ff4b4388ab70b9c Mon Sep 17 00:00:00 2001 From: clearloop Date: Thu, 10 Jun 2021 17:38:06 +0800 Subject: [PATCH 7/9] feat(committee): remove ProposalVoteOrigin --- pallets/committee/src/benchmarking.rs | 5 +++-- pallets/committee/src/lib.rs | 25 ++++++++++++++----------- pallets/committee/src/mock.rs | 1 - runtime/src/lib.rs | 1 - 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/pallets/committee/src/benchmarking.rs b/pallets/committee/src/benchmarking.rs index a9ec7faea2..b70b94a754 100644 --- a/pallets/committee/src/benchmarking.rs +++ b/pallets/committee/src/benchmarking.rs @@ -34,7 +34,8 @@ benchmarks! { } vote { - let proposal = submit_proposal::(T::ProposalVoteOrigin::successful_origin()); + let origin = T::ProposalSubmissionOrigin::successful_origin(); + let proposal = submit_proposal::(origin.clone()); // run to voting period >::set_block_number( @@ -46,7 +47,7 @@ benchmarks! { // construct call let call = >::vote(proposal.hash(), Vote::Abstain); }: { - call.dispatch_bypass_filter(T::ProposalVoteOrigin::successful_origin())? + call.dispatch_bypass_filter(origin)? } verify { assert_eq!( >::get_votes_for(&proposal.hash()).unwrap().votes.len(), diff --git a/pallets/committee/src/lib.rs b/pallets/committee/src/lib.rs index 8ba54dc60e..69b0f6f535 100644 --- a/pallets/committee/src/lib.rs +++ b/pallets/committee/src/lib.rs @@ -87,12 +87,6 @@ pub mod pallet { Success = ::AccountId, >; - /// Origin that is permitted to vote - type ProposalVoteOrigin: EnsureOrigin< - ::Origin, - Success = ::AccountId, - >; - /// Origin that is permitted to execute `add_constituent` type ApprovedByCommitteeOrigin: EnsureOrigin<::Origin>; @@ -312,6 +306,19 @@ pub mod pallet { }) }) } + + /// Used to check if an origin is signed and the signer is a member of + /// the committee + pub fn ensure_member( + origin: OriginFor, + ) -> Result>, DispatchError> { + let who = ensure_signed(origin)?; + if let Some(member_type) = >::get(who.clone()) { + Ok(CommitteeMember::new(who, member_type)) + } else { + Err(>::NotMember.into()) + } + } } #[pallet::call] @@ -355,11 +362,7 @@ pub mod pallet { vote: Vote, ) -> DispatchResultWithPostInfo { // Only members can vote - let caller = T::ProposalVoteOrigin::ensure_origin(origin)?; - let voter = CommitteeMember::new( - caller.clone(), - >::get(caller).ok_or(>::NotMember)?, - ); + let voter = Self::ensure_member(origin)?; Votes::::try_mutate(&proposal_hash, |votes| { if let Some(votes) = votes { diff --git a/pallets/committee/src/mock.rs b/pallets/committee/src/mock.rs index 5097e16c5d..423677aa98 100644 --- a/pallets/committee/src/mock.rs +++ b/pallets/committee/src/mock.rs @@ -97,7 +97,6 @@ impl pallet_committee::Config for Test { type MinCouncilVotes = MinCouncilVotes; type ProposalSubmissionOrigin = EnsureMember; type ProposalExecutionOrigin = EnsureMember; - type ProposalVoteOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin; diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 8dee11773d..5253553607 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -487,7 +487,6 @@ impl pallet_committee::Config for Runtime { type MinCouncilVotes = MinCouncilVotes; type ProposalSubmissionOrigin = EnsureMember; type ProposalExecutionOrigin = EnsureMember; - type ProposalVoteOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin; From a32fdbf00c79a5f3d51b458a62433abb2f559948 Mon Sep 17 00:00:00 2001 From: clearloop Date: Thu, 10 Jun 2021 18:09:33 +0800 Subject: [PATCH 8/9] feat(committee): fix the tests of BadOrigin --- pallets/committee/src/tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pallets/committee/src/tests.rs b/pallets/committee/src/tests.rs index 2b4fed4716..d6651bbf0a 100644 --- a/pallets/committee/src/tests.rs +++ b/pallets/committee/src/tests.rs @@ -130,7 +130,7 @@ fn non_member_cannot_vote() { let expected_votes = VoteAggregate::new_with_end(START_OF_V1); assert_noop!( Committee::vote(Origin::signed(ASHLEY), proposal.hash(), Vote::Aye), - BadOrigin, + >::NotMember, ); assert_eq!( Committee::get_votes_for(&proposal.hash()), From 7457582e01b13d4b173b8c9d2ba309436d2f476f Mon Sep 17 00:00:00 2001 From: clearloop Date: Thu, 10 Jun 2021 20:45:34 +0800 Subject: [PATCH 9/9] feat(committee): EnsureSigned for ProposalSubmissionOrigin --- pallets/committee/src/mock.rs | 4 ++-- runtime/src/lib.rs | 11 +++++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/pallets/committee/src/mock.rs b/pallets/committee/src/mock.rs index 423677aa98..471f35d64a 100644 --- a/pallets/committee/src/mock.rs +++ b/pallets/committee/src/mock.rs @@ -11,7 +11,7 @@ use frame_support::{ ord_parameter_types, parameter_types, traits::{OnFinalize, OnInitialize}, }; -use frame_system as system; +use frame_system::{self as system, EnsureSignedBy}; use sp_core::H256; use sp_runtime::{ @@ -95,7 +95,7 @@ impl pallet_committee::Config for Test { type ProposalSubmissionPeriod = ProposalSubmissionPeriod; type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; - type ProposalSubmissionOrigin = EnsureMember; + type ProposalSubmissionOrigin = EnsureSignedBy; type ProposalExecutionOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; diff --git a/runtime/src/lib.rs b/runtime/src/lib.rs index 5253553607..7fffd47c03 100644 --- a/runtime/src/lib.rs +++ b/runtime/src/lib.rs @@ -25,7 +25,10 @@ use sp_std::prelude::*; use sp_version::NativeVersion; use sp_version::RuntimeVersion; -use frame_system::limits::{BlockLength, BlockWeights}; +use frame_system::{ + limits::{BlockLength, BlockWeights}, + EnsureSigned, +}; // Polkadot imports use cumulus_primitives_core::ParaId; @@ -52,6 +55,7 @@ pub use frame_support::{ }; use pallet_asset_index::{MultiAssetAdapter, MultiAssetRegistry}; pub use pallet_balances::Call as BalancesCall; +use pallet_committee::EnsureMember; pub use pallet_timestamp::Call as TimestampCall; #[cfg(any(feature = "std", test))] pub use sp_runtime::BuildStorage; @@ -474,7 +478,6 @@ ord_parameter_types! { pub const MinCouncilVotes: usize = 4; } -type EnsureMember = pallet_committee::EnsureMember; type EnsureApprovedByCommittee = frame_system::EnsureOneOf< AccountId, frame_system::EnsureRoot, @@ -485,8 +488,8 @@ impl pallet_committee::Config for Runtime { type ProposalSubmissionPeriod = ProposalSubmissionPeriod; type VotingPeriod = VotingPeriod; type MinCouncilVotes = MinCouncilVotes; - type ProposalSubmissionOrigin = EnsureMember; - type ProposalExecutionOrigin = EnsureMember; + type ProposalSubmissionOrigin = EnsureSigned; + type ProposalExecutionOrigin = EnsureMember; type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee; type ProposalNonce = u32; type Origin = Origin;