Skip to content

Commit e36f810

Browse files
clearloopclearloop
authored
feat(committee): add EnsureMember origin for pallet committee (#104)
* feat(committee): simple EnsureMember for committee origin * feat(committee): support both Committee and Signed Origin * feat(committee): update benchmarking.rs * feat(committee): check votes in EnsureApprovedByCommittee * feat(committee): complete EnsureMember * perf(committee): fix docs * feat(committee): remove ProposalVoteOrigin * feat(committee): fix the tests of BadOrigin * feat(committee): EnsureSigned for ProposalSubmissionOrigin
1 parent ffaac9f commit e36f810

File tree

7 files changed

+167
-110
lines changed

7 files changed

+167
-110
lines changed

‎node/src/chain_spec.rs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,12 @@ pub fn pint_development_config(id: ParaId) -> ChainSpec {
6262
get_account_id_from_seed::<sr25519::Public>("Alice//stash"),
6363
get_account_id_from_seed::<sr25519::Public>("Bob//stash"),
6464
],
65+
vec![
66+
get_account_id_from_seed::<sr25519::Public>("Alice"),
67+
get_account_id_from_seed::<sr25519::Public>("Bob"),
68+
get_account_id_from_seed::<sr25519::Public>("Charlie"),
69+
get_account_id_from_seed::<sr25519::Public>("Dave"),
70+
],
6571
id,
6672
)
6773
},
@@ -100,6 +106,12 @@ pub fn pint_local_config(id: ParaId) -> ChainSpec {
100106
get_account_id_from_seed::<sr25519::Public>("Eve//stash"),
101107
get_account_id_from_seed::<sr25519::Public>("Ferdie//stash"),
102108
],
109+
vec![
110+
get_account_id_from_seed::<sr25519::Public>("Alice"),
111+
get_account_id_from_seed::<sr25519::Public>("Bob"),
112+
get_account_id_from_seed::<sr25519::Public>("Charlie"),
113+
get_account_id_from_seed::<sr25519::Public>("Dave"),
114+
],
103115
id,
104116
)
105117
},
@@ -117,6 +129,7 @@ pub fn pint_local_config(id: ParaId) -> ChainSpec {
117129
fn pint_testnet_genesis(
118130
root_key: AccountId,
119131
endowed_accounts: Vec<AccountId>,
132+
council_members: Vec<AccountId>,
120133
id: ParaId,
121134
) -> parachain_runtime::GenesisConfig {
122135
parachain_runtime::GenesisConfig {
@@ -126,6 +139,10 @@ fn pint_testnet_genesis(
126139
.to_vec(),
127140
changes_trie_config: Default::default(),
128141
},
142+
pallet_committee: parachain_runtime::CommitteeConfig {
143+
council_members,
144+
..Default::default()
145+
},
129146
pallet_balances: parachain_runtime::BalancesConfig {
130147
balances: endowed_accounts
131148
.iter()

‎pallets/committee/src/benchmarking.rs‎

Lines changed: 49 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1,73 +1,76 @@
11
// Copyright 2021 ChainSafe Systems
22
// SPDX-License-Identifier: LGPL-3.0-only
33
use super::*;
4-
use frame_benchmarking::{account, benchmarks, vec, whitelisted_caller, Box};
5-
use frame_support::{assert_noop, assert_ok, traits::Get};
6-
use frame_system::{Call as SystemCall, Pallet as System, RawOrigin as SystemOrigin};
4+
use frame_benchmarking::{account, benchmarks, vec, Box};
5+
use frame_support::{
6+
assert_noop, assert_ok,
7+
traits::{EnsureOrigin, Get, UnfilteredDispatchable},
8+
};
9+
use frame_system::{
10+
ensure_signed, Call as SystemCall, Pallet as System, RawOrigin as SystemOrigin,
11+
};
712

8-
fn submit_proposal<T: Config>(caller: T::AccountId) -> pallet::Proposal<T> {
13+
fn submit_proposal<T: Config>(origin: <T as frame_system::Config>::Origin) -> pallet::Proposal<T> {
914
let action: T::Action = <SystemCall<T>>::remark(vec![0; 0]).into();
1015
let expected_nonce = pallet::ProposalCount::<T>::get();
11-
assert_ok!(<Pallet<T>>::propose(
12-
SystemOrigin::Signed(caller).into(),
13-
Box::new(action.clone())
16+
assert_ok!(<Pallet<T>>::add_constituent(
17+
SystemOrigin::Root.into(),
18+
ensure_signed(origin.clone()).unwrap(),
1419
));
20+
let call = <Call<T>>::propose(Box::new(action.clone()));
21+
assert_ok!(call.dispatch_bypass_filter(origin));
1522
pallet::Proposal::<T>::new(expected_nonce, action)
1623
}
1724

1825
benchmarks! {
1926
propose {
20-
let caller: T::AccountId = whitelisted_caller();
21-
let proposal = submit_proposal::<T>(caller.clone());
22-
}: _(
23-
SystemOrigin::Signed(caller.clone()),
24-
Box::new(<SystemCall<T>>::remark(vec![0; 0]).into())
25-
) verify {
27+
let origin = T::ProposalSubmissionOrigin::successful_origin();
28+
let proposal = submit_proposal::<T>(origin.clone());
29+
let call = <Call<T>>::propose(Box::new(<SystemCall<T>>::remark(vec![0; 0]).into()));
30+
}: {
31+
call.dispatch_bypass_filter(origin)?
32+
} verify {
2633
assert!(<Pallet<T>>::get_proposal(&proposal.hash()) == Some(proposal));
2734
}
2835

2936
vote {
30-
let caller: T::AccountId = whitelisted_caller();
31-
let proposal = submit_proposal::<T>(caller.clone());
32-
assert_ok!(<Pallet<T>>::add_constituent(SystemOrigin::Root.into(), caller.clone()));
37+
let origin = T::ProposalSubmissionOrigin::successful_origin();
38+
let proposal = submit_proposal::<T>(origin.clone());
3339

3440
// run to voting period
3541
<System<T>>::set_block_number(
3642
<System<T>>::block_number()
3743
+ <T as Config>::VotingPeriod::get()
3844
+ <T as Config>::ProposalSubmissionPeriod::get() + 1_u32.into(),
3945
);
40-
}: _(
41-
SystemOrigin::Signed(caller.clone()),
42-
proposal.hash(),
43-
Vote::Abstain
44-
) verify {
46+
47+
// construct call
48+
let call = <Call<T>>::vote(proposal.hash(), Vote::Abstain);
49+
}: {
50+
call.dispatch_bypass_filter(origin)?
51+
} verify {
4552
assert_eq!(
4653
<Pallet<T>>::get_votes_for(&proposal.hash()).unwrap().votes.len(),
4754
1,
4855
);
4956
}
5057

5158
close {
52-
let caller: T::AccountId = whitelisted_caller();
53-
let proposal: pallet::Proposal<T> = submit_proposal::<T>(caller.clone());
54-
assert_ok!(<Pallet<T>>::add_constituent(SystemOrigin::Root.into(), caller.clone()));
55-
let voters = ["a", "b", "c", "d", "e"];
59+
let proposal: pallet::Proposal<T> = submit_proposal::<T>(T::ProposalSubmissionOrigin::successful_origin());
5660

57-
// run to voting period
58-
<System<T>>::set_block_number(<System<T>>::block_number() + <T as Config>::VotingPeriod::get() + <T as Config>::ProposalSubmissionPeriod::get() + 1_u32.into());
59-
60-
// generate members
61-
for i in &voters {
62-
let voter: T::AccountId = account(i, 0, 0);
63-
<Members<T>>::insert(voter.clone(), MemberType::Council);
64-
65-
// vote aye
66-
assert_ok!(<Pallet<T>>::vote(
67-
SystemOrigin::Signed(voter).into(),
68-
proposal.hash(),
69-
Vote::Aye,
70-
));
61+
// vote
62+
for i in 0..5 {
63+
let voter: T::AccountId = account("voter", i, 0);
64+
assert_ok!(Votes::<T>::try_mutate(&proposal.hash(), |votes| {
65+
if let Some(votes) = votes {
66+
votes.cast_vote(
67+
MemberVote::new(CommitteeMember::new(voter, MemberType::Council), Vote::Aye),
68+
);
69+
Ok(())
70+
} else {
71+
Err(Error::<T>::NoProposalWithHash)
72+
}
73+
}));
7174
}
7275

7376
// run out of voting period
@@ -77,12 +80,14 @@ benchmarks! {
7780
+ <T as Config>::ProposalSubmissionPeriod::get()
7881
+ 1_u32.into()
7982
);
80-
}: _(
81-
SystemOrigin::Signed(caller.clone()),
82-
proposal.hash()
83-
) verify {
83+
84+
// construct call
85+
let call = <Call<T>>::close(proposal.hash());
86+
}: {
87+
call.dispatch_bypass_filter(T::ProposalExecutionOrigin::successful_origin())?
88+
} verify {
8489
assert_noop!(
85-
<Pallet<T>>::close(SystemOrigin::Signed(caller.clone()).into(), proposal.hash()),
90+
<Pallet<T>>::close(T::ProposalExecutionOrigin::successful_origin(), proposal.hash()),
8691
<Error<T>>::ProposalAlreadyExecuted
8792
);
8893
}

‎pallets/committee/src/lib.rs‎

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -76,10 +76,16 @@ pub mod pallet {
7676
type MinCouncilVotes: Get<usize>;
7777

7878
/// Origin that is permitted to create proposals
79-
type ProposalSubmissionOrigin: EnsureOrigin<<Self as frame_system::Config>::Origin>;
79+
type ProposalSubmissionOrigin: EnsureOrigin<
80+
<Self as frame_system::Config>::Origin,
81+
Success = <Self as frame_system::Config>::AccountId,
82+
>;
8083

8184
/// Origin that is permitted to execute approved proposals
82-
type ProposalExecutionOrigin: EnsureOrigin<<Self as frame_system::Config>::Origin>;
85+
type ProposalExecutionOrigin: EnsureOrigin<
86+
<Self as frame_system::Config>::Origin,
87+
Success = <Self as frame_system::Config>::AccountId,
88+
>;
8389

8490
/// Origin that is permitted to execute `add_constituent`
8591
type ApprovedByCommitteeOrigin: EnsureOrigin<<Self as frame_system::Config>::Origin>;
@@ -265,19 +271,6 @@ pub mod pallet {
265271
Votes::<T>::get(hash)
266272
}
267273

268-
/// Used to check if an origin is signed and the signer is a member of
269-
/// the committee
270-
pub fn ensure_member(
271-
origin: OriginFor<T>,
272-
) -> Result<CommitteeMember<AccountIdFor<T>>, DispatchError> {
273-
let who = ensure_signed(origin)?;
274-
if let Some(member_type) = Members::<T>::get(who.clone()) {
275-
Ok(CommitteeMember::new(who, member_type))
276-
} else {
277-
Err(Error::<T>::NotMember.into())
278-
}
279-
}
280-
281274
/// Returns the block at the end of the next voting period
282275
pub fn get_next_voting_period_end(
283276
block_number: &BlockNumberFor<T>,
@@ -313,6 +306,19 @@ pub mod pallet {
313306
})
314307
})
315308
}
309+
310+
/// Used to check if an origin is signed and the signer is a member of
311+
/// the committee
312+
pub fn ensure_member(
313+
origin: OriginFor<T>,
314+
) -> Result<CommitteeMember<AccountIdFor<T>>, DispatchError> {
315+
let who = ensure_signed(origin)?;
316+
if let Some(member_type) = <Members<T>>::get(who.clone()) {
317+
Ok(CommitteeMember::new(who, member_type))
318+
} else {
319+
Err(<Error<T>>::NotMember.into())
320+
}
321+
}
316322
}
317323

318324
#[pallet::call]
@@ -322,8 +328,7 @@ pub mod pallet {
322328
/// The provided action will be turned into a proposal and added to the list of current active proposals
323329
/// to be voted on in the next voting period.
324330
pub fn propose(origin: OriginFor<T>, action: Box<T::Action>) -> DispatchResultWithPostInfo {
325-
let proposer = ensure_signed(origin.clone())?;
326-
T::ProposalSubmissionOrigin::ensure_origin(origin)?;
331+
let proposer = T::ProposalSubmissionOrigin::ensure_origin(origin.clone())?;
327332

328333
// Create a new proposal with a unique nonce
329334
let nonce = Self::take_and_increment_nonce()?;
@@ -390,8 +395,7 @@ pub mod pallet {
390395
origin: OriginFor<T>,
391396
proposal_hash: HashFor<T>,
392397
) -> DispatchResultWithPostInfo {
393-
let closer = ensure_signed(origin.clone())?;
394-
T::ProposalExecutionOrigin::ensure_origin(origin)?;
398+
let closer = T::ProposalExecutionOrigin::ensure_origin(origin.clone())?;
395399

396400
// ensure proposal has not already been executed
397401
ensure!(

‎pallets/committee/src/mock.rs‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,14 @@
44
// Required as construct_runtime! produces code that violates this lint
55
#![allow(clippy::from_over_into)]
66

7-
use crate as pallet_committee;
7+
use crate::{self as pallet_committee, EnsureMember};
88
#[cfg(feature = "std")]
99
use frame_support::traits::GenesisBuild;
1010
use frame_support::{
1111
ord_parameter_types, parameter_types,
1212
traits::{OnFinalize, OnInitialize},
1313
};
14-
use frame_system as system;
14+
use frame_system::{self as system, EnsureSignedBy};
1515

1616
use sp_core::H256;
1717
use sp_runtime::{
@@ -75,7 +75,7 @@ parameter_types! {
7575
pub const VotingPeriod: <Test as system::Config>::BlockNumber = VOTING_PERIOD;
7676
}
7777
pub(crate) const PROPOSER_ACCOUNT_ID: AccountId = 88;
78-
pub(crate) const EXECUTER_ACCOUNT_ID: AccountId = 88;
78+
pub(crate) const EXECUTER_ACCOUNT_ID: AccountId = PROPOSER_ACCOUNT_ID;
7979
pub(crate) const MIN_COUNCIL_VOTES: usize = 4;
8080

8181
ord_parameter_types! {
@@ -88,15 +88,15 @@ ord_parameter_types! {
8888
type EnsureApprovedByCommittee = frame_system::EnsureOneOf<
8989
AccountId,
9090
frame_system::EnsureRoot<AccountId>,
91-
crate::EnsureApprovedByCommittee<AccountId, u64>,
91+
crate::EnsureApprovedByCommittee<Test>,
9292
>;
9393

9494
impl pallet_committee::Config for Test {
9595
type ProposalSubmissionPeriod = ProposalSubmissionPeriod;
9696
type VotingPeriod = VotingPeriod;
9797
type MinCouncilVotes = MinCouncilVotes;
98-
type ProposalSubmissionOrigin = frame_system::EnsureSignedBy<AdminAccountId, AccountId>;
99-
type ProposalExecutionOrigin = frame_system::EnsureSignedBy<ExecuterAccountId, AccountId>;
98+
type ProposalSubmissionOrigin = EnsureSignedBy<AdminAccountId, AccountId>;
99+
type ProposalExecutionOrigin = EnsureMember<Self>;
100100
type ApprovedByCommitteeOrigin = EnsureApprovedByCommittee;
101101
type ProposalNonce = u32;
102102
type Origin = Origin;
@@ -125,8 +125,10 @@ where
125125
.build_storage::<Test>()
126126
.unwrap();
127127

128+
let mut council_members = vec![PROPOSER_ACCOUNT_ID];
129+
council_members.append(&mut members.into_iter().collect());
128130
pallet_committee::GenesisConfig::<Test> {
129-
council_members: members.into_iter().collect(),
131+
council_members,
130132
constituent_members: Default::default(),
131133
}
132134
.assimilate_storage(&mut t)

‎pallets/committee/src/tests.rs‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33

44
use crate as pallet;
55
use crate::mock::*;
6-
use crate::{CommitteeMember, CommitteeOrigin, MemberType, Vote, VoteAggregate};
6+
use crate::{CommitteeMember, MemberType, Vote, VoteAggregate};
77
use frame_support::{assert_noop, assert_ok, codec::Encode};
88
use frame_system as system;
99
use sp_runtime::traits::BadOrigin;
@@ -130,7 +130,7 @@ fn non_member_cannot_vote() {
130130
let expected_votes = VoteAggregate::new_with_end(START_OF_V1);
131131
assert_noop!(
132132
Committee::vote(Origin::signed(ASHLEY), proposal.hash(), Vote::Aye),
133-
pallet::Error::<Test>::NotMember
133+
<pallet::Error<Test>>::NotMember,
134134
);
135135
assert_eq!(
136136
Committee::get_votes_for(&proposal.hash()),
@@ -326,12 +326,15 @@ where
326326
#[test]
327327
fn non_execution_origin_cannot_close() {
328328
new_test_ext(0..4).execute_with(|| {
329+
let non_execution_origin = 5;
329330
let proposal = submit_proposal(123);
330331
run_to_block(START_OF_S1);
331332

332333
vote_with_each(0..4, proposal.hash(), Vote::Aye);
334+
335+
run_to_block(START_OF_V1 + 1);
333336
assert_noop!(
334-
Committee::close(Origin::signed(ASHLEY), proposal.hash()),
337+
Committee::close(Origin::signed(non_execution_origin), proposal.hash()),
335338
BadOrigin
336339
);
337340
});
@@ -451,22 +454,11 @@ fn cannot_execute_proposal_twice() {
451454
// Constituent Committee Council Selection
452455
//
453456

454-
/// An `ApprovedByCommittee` origin
455-
fn approved_by_committee() -> CommitteeOrigin<AccountId, u64> {
456-
CommitteeOrigin::ApprovedByCommittee(
457-
PROPOSER_ACCOUNT_ID,
458-
VoteAggregate {
459-
votes: Vec::new(),
460-
end: START_OF_V1,
461-
},
462-
)
463-
}
464-
465457
#[test]
466458
fn cannot_add_constituent_if_already_is_council() {
467459
new_test_ext(PROPOSER_ACCOUNT_ID..PROPOSER_ACCOUNT_ID + 1).execute_with(|| {
468460
assert_noop!(
469-
Committee::add_constituent(approved_by_committee().into(), PROPOSER_ACCOUNT_ID),
461+
Committee::add_constituent(Origin::root().into(), PROPOSER_ACCOUNT_ID),
470462
<pallet::Error<Test>>::AlreadyCouncilMember
471463
);
472464
});
@@ -475,13 +467,10 @@ fn cannot_add_constituent_if_already_is_council() {
475467
#[test]
476468
fn cannot_add_constituent_if_already_is_constituent() {
477469
new_test_ext(PROPOSER_ACCOUNT_ID..PROPOSER_ACCOUNT_ID + 1).execute_with(|| {
478-
assert_ok!(Committee::add_constituent(
479-
approved_by_committee().into(),
480-
42
481-
));
470+
assert_ok!(Committee::add_constituent(Origin::root().into(), 42));
482471

483472
assert_noop!(
484-
Committee::add_constituent(approved_by_committee().into(), 42),
473+
Committee::add_constituent(Origin::root().into(), 42),
485474
<pallet::Error<Test>>::AlreadyConstituentMember
486475
);
487476
});

0 commit comments

Comments
 (0)