Skip to content

feat(committee): add CommitteeMember origin for pallet committee#104

Merged
clearloop merged 9 commits intomainfrom
cl/initial-council-members
Jun 10, 2021
Merged

feat(committee): add CommitteeMember origin for pallet committee#104
clearloop merged 9 commits intomainfrom
cl/initial-council-members

Conversation

@clearloop
Copy link
Contributor

@clearloop clearloop commented Jun 9, 2021

Changes

  • check votes in EnsureAprovedByCommitte
  • add CommitteeMember origin
  • add initial committee members in the genesis config
  • update tests
  • update benchmarks

Tests

CI

Issues

Closes #96

@dutterbutter dutterbutter added the WIP Work in progress - do not review or merge label Jun 9, 2021
@clearloop clearloop added needs review PR needs reviewing and removed WIP Work in progress - do not review or merge labels Jun 10, 2021
@clearloop clearloop marked this pull request as ready for review June 10, 2021 00:01
@clearloop clearloop requested a review from mattsse June 10, 2021 00:01
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

The Origin types look great.
But I don't think we need it necessarily need it in the Committee pallet as extra config type, since we can check for inclusion in the Members directly. Because we check for inclusion anyway

Comment on lines +358 to +362
let caller = T::ProposalVoteOrigin::ensure_origin(origin)?;
let voter = CommitteeMember::new(
caller.clone(),
<Members<T>>::get(caller).ok_or(<Error<T>>::NotMember)?,
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since only members are allowed to vote, I believe we should only check for inclusion in Members instead adding another origin, which will probably do the same as ensure_member

Copy link
Contributor Author

@clearloop clearloop Jun 10, 2021

Choose a reason for hiding this comment

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

ur right LOL, added an origin for Vote created a lot of trouble for me QAQ

But the logic of EnsureMember contains the logic of ensure_member, so I want to remove ensure_member crazily LOL

BTW, I'm not afraid of Origin anymore now hhh

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, I'm not afraid of Origin anymore now hhh

That's awesome !

@mattsse mattsse added needs changes PR needs changes and removed needs review PR needs reviewing labels Jun 10, 2021
@clearloop
Copy link
Contributor Author

clearloop commented Jun 10, 2021

But I don't think we need it necessarily need it in the Committee pallet as extra config type, since we can check for inclusion in the Members directly. Because we check for inclusion anyway

This is also a confusing thing while I working on this PR, the difference between our pallet-committee and pallet-collective of substrate is: we don't need to call propose, vote, close outside pallet-committee, so the ProposalSubmissionOrigin and ProposalExecutionOrigin might also not necessary

But for pallets like asset-index, add or remove asset, we might need the EnsureMember provided by this pallet.

How do you like removing ProposalSubmissionOrigin and ProposalExecutionOrigin @mattsse ?

@clearloop clearloop added needs review PR needs reviewing and removed needs changes PR needs changes labels Jun 10, 2021
@clearloop clearloop requested a review from mattsse June 10, 2021 10:25
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

It's intended that anyone can submit a proposal, so this should just be the standard EnsureSigned

You're probably right, however this may change, so my feeling is we should keep the origins in

type MinCouncilVotes = MinCouncilVotes;
type ProposalSubmissionOrigin = frame_system::EnsureSignedBy<AdminAccountId, AccountId>;
type ProposalExecutionOrigin = frame_system::EnsureSignedBy<ExecuterAccountId, AccountId>;
type ProposalSubmissionOrigin = EnsureMember<Self>;
Copy link
Contributor

@mattsse mattsse Jun 10, 2021

Choose a reason for hiding this comment

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

This should be EnsureSigned only since, anyone is allowed to submit proposals

Suggested change
type ProposalSubmissionOrigin = EnsureMember<Self>;
type ProposalSubmissionOrigin = EnsureSigned;

@mattsse mattsse added approved PR approved to merge and removed needs review PR needs reviewing labels Jun 10, 2021
@clearloop clearloop merged commit e36f810 into main Jun 10, 2021
@clearloop clearloop deleted the cl/initial-council-members branch June 10, 2021 13:01
@clearloop clearloop mentioned this pull request Aug 18, 2021
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR approved to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set initial council members in runtime

3 participants