Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

ranked-collective: pub fns to demote and remove members #13377

Closed
muharem wants to merge 4 commits into from
Closed

ranked-collective: pub fns to demote and remove members #13377

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c788175
pub funcions to demote and remove members
muharem Feb 13, 2023
51827f5
tests
muharem Feb 13, 2023
77e624c
docs
muharem Feb 13, 2023
491db10
update the docs
muharem Feb 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 52 additions & 26 deletions frame/ranked-collective/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,24 +465,7 @@ pub mod pallet {
pub fn demote_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
let max_rank = T::DemoteOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;
let mut record = Self::ensure_member(&who)?;
let rank = record.rank;
ensure!(max_rank >= rank, Error::<T, I>::NoPermission);

Self::remove_from_rank(&who, rank)?;
let maybe_rank = rank.checked_sub(1);
match maybe_rank {
None => {
Members::<T, I>::remove(&who);
Self::deposit_event(Event::MemberRemoved { who, rank: 0 });
},
Some(rank) => {
record.rank = rank;
Members::<T, I>::insert(&who, &record);
Self::deposit_event(Event::RankChanged { who, rank });
},
}
Ok(())
Self::do_demote_member(who, Some(max_rank))
}

/// Remove the member entirely.
Expand All @@ -503,13 +486,9 @@ pub mod pallet {
let who = T::Lookup::lookup(who)?;
let MemberRecord { rank, .. } = Self::ensure_member(&who)?;
ensure!(min_rank >= rank, Error::<T, I>::InvalidWitness);
ensure!(max_rank >= rank, Error::<T, I>::NoPermission);

for r in 0..=rank {
Self::remove_from_rank(&who, r)?;
}
Members::<T, I>::remove(&who);
Self::deposit_event(Event::MemberRemoved { who, rank });
Self::do_remove_member(who, Some(max_rank))?;

Ok(PostDispatchInfo {
actual_weight: Some(T::WeightInfo::remove_member(rank as u32)),
pays_fee: Pays::Yes,
Expand Down Expand Up @@ -654,8 +633,9 @@ pub mod pallet {

/// Promotes a member in the ranked collective into the next role.
///
/// A `maybe_max_rank` may be provided to check that the member does not get promoted beyond
/// a certain rank. Is `None` is provided, then the rank will be incremented without checks.
/// A `maybe_max_rank` may be provided to check that the rank of `who` is not higher than
/// a given `maybe_max_rank`, otherwise fails with `NoPermission` error. if `None` is
/// provided, then the rank of `who` will be incremented without the check.
pub fn do_promote_member(
who: T::AccountId,
maybe_max_rank: Option<Rank>,
Expand Down Expand Up @@ -683,5 +663,51 @@ pub mod pallet {
}
Ok(())
}

/// Decrement the rank of an existing member by one. If the member is already at rank zero,
/// then it is removed entirely.
///
/// A `maybe_max_rank` may be provided to check that the rank of `who` is not higher than
/// a given `maybe_max_rank`, otherwise fails with `NoPermission` error. if `None` is
/// provided, then the rank of `who` will be decremented without the check.
pub fn do_demote_member(who: T::AccountId, maybe_max_rank: Option<Rank>) -> DispatchResult {
let mut record = Self::ensure_member(&who)?;
let rank = record.rank;
if let Some(max_rank) = maybe_max_rank {
ensure!(max_rank >= rank, Error::<T, I>::NoPermission);
}
Self::remove_from_rank(&who, rank)?;
let maybe_rank = rank.checked_sub(1);
match maybe_rank {
None => {
Members::<T, I>::remove(&who);
Self::deposit_event(Event::MemberRemoved { who, rank: 0 });
},
Some(rank) => {
record.rank = rank;
Members::<T, I>::insert(&who, &record);
Self::deposit_event(Event::RankChanged { who, rank });
},
}
Ok(())
}

/// Remove the member entirely.
///
/// A `maybe_max_rank` may be provided to check that the rank of `who` is not higher than
/// a given `maybe_max_rank`, otherwise fails with `NoPermission` error. if `None` is
/// provided, then `who` will be removed from any rank without the check.
pub fn do_remove_member(who: T::AccountId, maybe_max_rank: Option<Rank>) -> DispatchResult {
let MemberRecord { rank, .. } = Self::ensure_member(&who)?;
Copy link
Member

@bkchr bkchr Feb 14, 2023

Choose a reason for hiding this comment

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

remove_member already calls this. We should come up with a way to not duplicate this call.

Copy link
Contributor Author

@muharem muharem Feb 16, 2023

Choose a reason for hiding this comment

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

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?

Copy link
Member

@shawntabrizi shawntabrizi Feb 20, 2023

Choose a reason for hiding this comment

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

i think it is probably fine

if let Some(max_rank) = maybe_max_rank {
ensure!(max_rank >= rank, Error::<T, I>::NoPermission);
}
for r in 0..=rank {
Self::remove_from_rank(&who, r)?;
}
Members::<T, I>::remove(&who);
Self::deposit_event(Event::MemberRemoved { who, rank });
Ok(())
}
}
}
22 changes: 19 additions & 3 deletions frame/ranked-collective/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,19 +355,35 @@ fn promote_demote_by_rank_works() {
assert_ok!(Club::promote_member(RuntimeOrigin::signed(2), 6));
assert_ok!(Club::add_member(RuntimeOrigin::signed(2), 7));
assert_ok!(Club::promote_member(RuntimeOrigin::signed(2), 7));
assert_ok!(Club::promote_member(RuntimeOrigin::signed(2), 7));

// #3 as rank 3 can demote/remove #4 & #5 but not #6 & #7
assert_ok!(Club::demote_member(RuntimeOrigin::signed(3), 4));
assert_ok!(Club::remove_member(RuntimeOrigin::signed(3), 5, 0));
assert_noop!(Club::demote_member(RuntimeOrigin::signed(3), 6), Error::<Test>::NoPermission);
assert_noop!(
Club::remove_member(RuntimeOrigin::signed(3), 7, 1),
Club::remove_member(RuntimeOrigin::signed(3), 7, 2),
Error::<Test>::NoPermission
);
assert_noop!(
Club::remove_member(RuntimeOrigin::signed(3), 7, 1),
Error::<Test>::InvalidWitness
);

// #2 as rank 5 can demote/remove #6 & #7
// #2 as rank 5 can demote #6 until removed
type Rank0 = EnsureRanked<Test, (), 0>;
assert_eq!(Rank0::try_origin(RuntimeOrigin::signed(6)).unwrap(), 1);
assert_ok!(Club::demote_member(RuntimeOrigin::signed(2), 6));
assert_eq!(Rank0::try_origin(RuntimeOrigin::signed(6)).unwrap(), 0);
assert_ok!(Club::demote_member(RuntimeOrigin::signed(2), 6));
assert_ok!(Club::remove_member(RuntimeOrigin::signed(2), 7, 1));
assert!(Rank0::try_origin(RuntimeOrigin::signed(6)).is_err());
assert_noop!(Club::demote_member(RuntimeOrigin::signed(2), 6), Error::<Test>::NotMember);

// #2 as rank 5 can remove #7
assert_eq!(Rank0::try_origin(RuntimeOrigin::signed(7)).unwrap(), 2);
assert_ok!(Club::remove_member(RuntimeOrigin::signed(2), 7, 2));
assert!(Rank0::try_origin(RuntimeOrigin::signed(7)).is_err());
assert_noop!(Club::demote_member(RuntimeOrigin::signed(2), 7), Error::<Test>::NotMember);
});
}

Expand Down