Membership module refactoring. #962
Conversation
- move all the code from the members mod to the lib.rs - update all dependencies
Make MemberIdsByRootAccountId and MemberIdsByControllerAccountId private in the membership module.
- remove roles from the membership module - remove roles from the content working group module - move ActorId from membership to the content working group - add MemberId to te content working group Lead - update tests - delete commented code
Change call from ensure_is_controller_account_for_member() to the ensure_member_controller_account_signed().
# Conflicts: # Cargo.lock # runtime-modules/content-working-group/Cargo.toml # runtime-modules/proposals/discussion/src/lib.rs # runtime-modules/roles/src/actors.rs # runtime-modules/roles/src/lib.rs # runtime-modules/roles/src/mock.rs # runtime-modules/storage/src/data_directory.rs # runtime-modules/storage/src/tests/mock.rs # runtime-modules/working-group/Cargo.toml # runtime/src/integration/proposals/council_origin_validator.rs # runtime/src/integration/proposals/membership_origin_validator.rs # runtime/src/lib.rs # runtime/src/tests/proposals_integration/mod.rs
…ges. - restore ActorId in the membership module.
shamil-gadelshin
added
refactoring
tech-debt
mnaamani
left a comment
mnaamani
left a comment
There was a problem hiding this comment.
Nice to see this great refactoring.
Although large, PRs where code is mostly removed seems fundamentally easier to review :)
I think I left one small change request, otherwise looks good to me.
| let new_lead = LeadById::<Test>::get(new_lead_id); | ||
|
|
||
| let expected_new_lead = Lead { | ||
| member_id: 0, |
There was a problem hiding this comment.
| member_id: 0, | |
| member_id: self.member_id, |
| + Ord; | ||
|
|
||
| /// Initial balance of members created at genesis | ||
| type InitialMembersBalance: Get<BalanceOf<Self>>; |
There was a problem hiding this comment.
lets as well drop this now. The way we import members at genesis needs to be improved and one balance for all members isn't very useful.
| #[derive(Encode, Decode, Debug, PartialEq)] | ||
| pub enum EntryMethod<PaidTermId, AccountId> { | ||
| Paid(PaidTermId), | ||
| Screening(AccountId), |
There was a problem hiding this comment.
Another place where we can change to identify the Screening role with a memberid instead of an account id
There was a problem hiding this comment.
I'm not sure how can we change the EntryMethod to Membership to use member_id because we don't have it until we create the membership.
There was a problem hiding this comment.
The variant would still be Screening(MemberId) and the member id is that of the member who is the designated screening authority, set by the council or sudo. So this involves also changing the the storage value ScreeningAuthority to Option<MemberId> and updating the signature of fn add_screened_member() dispatch.
I think we can deal with this at a later time when we add "screeing authority working group"..
| const DEFAULT_MAX_AVATAR_URI_LENGTH: u32 = 1024; | ||
| const DEFAULT_MAX_ABOUT_TEXT_LENGTH: u32 = 2048; | ||
|
|
||
| pub type MembershipOf<T> = Membership< |
There was a problem hiding this comment.
We've used this naming a few times in the library, but it makes the public api look odd with this Of suffix.
Its probably better to keep the public type that will appear in the api to be Membership and then the internal structure used can be named something different, similar to what I did here with DataObject
Also as I'm reviewing this, and thinking about public type names, I know in the latest substrate there is a membership pallet and I'm sure we might get some name clashes with types there. I guess we will cross that bridge when we get to it.
There was a problem hiding this comment.
Renamed to MembershipObject and Membership.
| <Module<T>>::insert_member(&who, &user_info, EntryMethod::Genesis); | ||
|
|
||
| // Give member starting balance | ||
| T::Currency::deposit_creating(&who, T::InitialMembersBalance::get()); |
There was a problem hiding this comment.
I think we should drop this initial balance, and later update the build() method to also take an initial balance for each account.
| fn default() -> Self { | ||
| Self::Curator | ||
| } | ||
| } |
There was a problem hiding this comment.
do we event need the Role type anymore ? I can't find a reference to it anymore
There was a problem hiding this comment.
Merge artifact. Deleted.
| Council: council::{Module, Call, Storage, Event<T>, Config<T>}, | ||
| Memo: memo::{Module, Call, Storage, Event<T>}, | ||
| Members: members::{Module, Call, Storage, Event<T>, Config<T>}, | ||
| Members: membership::{Module, Call, Storage, Event<T>, Config<T>}, |
There was a problem hiding this comment.
headsup @Lezek123 all api.query.members.* and api.query.tx.members.* in polkadot-js apps will need to be updated to api.*.membership.* after this change.
There was a problem hiding this comment.
It seems like @polkadot/api still sees it as members module (ie. I can call api.query.members.nextMemberId(), but cannot api.query.membership.nextMemberId())
3 participants
The new incarnation of this PR. Merged with the Nicaea release branch.