ranked-collective: pub fns to demote and remove members#13377
ranked-collective: pub fns to demote and remove members#13377
Conversation
muharem
added
A0-please_review
frame/ranked-collective/src/lib.rs
Outdated
| /// A `maybe_max_rank` may be provided to check that the member does not get removed from | ||
| /// a certain rank. if `None` is provided, then the rank will be decremented without checks. |
There was a problem hiding this comment.
This is not really true, in the remove_member the max_rank describes the max_rank of the member the origin can remove. Meaning that if the member to remove has a higher rank, it will be rejected.
There was a problem hiding this comment.
please check now.
note, its not necessary a member removing/demoting/promoting, its mostly a referenda on a certain track.
| /// A `maybe_max_rank` may be provided to check that the member does not get removed from | ||
| /// a certain rank. if `None` is provided, then the rank will be decremented without checks. | ||
| pub fn do_remove_member(who: T::AccountId, maybe_max_rank: Option<Rank>) -> DispatchResult { | ||
| let MemberRecord { rank, .. } = Self::ensure_member(&who)?; |
There was a problem hiding this comment.
remove_member already calls this. We should come up with a way to not duplicate this call.
There was a problem hiding this comment.
As I know this will result only one read from the storage, because of the overlay layer.
The logic was to not include the WitnessData check into do_* function, clients might not need it.
Other option could be to pass the actual rank as an argument, but since this function is public, I believe it must be sure (query by itself) about the actual rank, otherwise it might lead to the inconsistency.
@bkchr what you think?
There was a problem hiding this comment.
i think it is probably fine
frame/ranked-collective/src/lib.rs
Outdated
| /// A `maybe_max_rank` may be provided to check that the member does not get demoted beyond | ||
| /// a certain rank. Is `None` is provided, then the rank will be decremented without checks. |
There was a problem hiding this comment.
I find this comment also confusing. Without reading the code, I wouldn't really know what this is for.
|
No trait added? I think that would be very useful - I expect I'll need such a trait in the near future for promotion and demotion logic potentially specific to the Core Technical Fellowship. |
I will add the trait. |
muharem
added
the
A3-in_progress
|
#13378 includes the same refactoring... |
|
solved in this PR #13378 |
4 participants
Fixes #12983
In the PR we adding new abstractions (public functions) to demote and promote members.
No new behaviour is introduced. The abstractions are identical to the existing ones for adding and promoting members.
For the reasoning please check the issue.
// TOOD add trait