Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

exchange member with a new account and same rank in the ranked collec… #2587

Merged
merged 41 commits into from
Jan 30, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
07b93db
exchange member with a new account and same rank in the ranked collec…
dharjeezy Dec 3, 2023
66b2f08
Merge branch 'master' into dami/change-account
dharjeezy Dec 11, 2023
fbf91f3
address concerns and benchmarks
dharjeezy Dec 11, 2023
8d68ca5
Merge remote-tracking branch 'origin/dami/change-account' into dami/c…
dharjeezy Dec 11, 2023
cd50907
change comment
dharjeezy Dec 11, 2023
05f6ee6
fmt
dharjeezy Dec 11, 2023
255ad1f
weight
dharjeezy Dec 21, 2023
4df260b
Merge branch 'master' into dami/change-account
dharjeezy Dec 21, 2023
413ce7f
Merge branch 'master' into dami/change-account
dharjeezy Dec 21, 2023
89a8c29
Merge branch 'master' into dami/change-account
dharjeezy Jan 2, 2024
6dc14e9
Merge branch 'paritytech:master' into dami/change-account
dharjeezy Jan 17, 2024
279ed33
Merge branch 'master' of https://github.com/dharjeezy/polkadot-sdk in…
dharjeezy Jan 18, 2024
f7d68ef
boolean flag to toggle emission of events
dharjeezy Jan 18, 2024
065c619
Merge remote-tracking branch 'origin/dami/change-account' into dami/c…
dharjeezy Jan 18, 2024
ec954ad
doc
dharjeezy Jan 18, 2024
205c2d8
Merge branch 'master' into dami/change-account
dharjeezy Jan 18, 2024
3d0c8f7
subtle changes
dharjeezy Jan 21, 2024
b92210e
Merge remote-tracking branch 'origin/dami/change-account' into dami/c…
dharjeezy Jan 21, 2024
3115f39
Merge branch 'master' into dami/change-account
dharjeezy Jan 21, 2024
fd68eb1
Merge branch 'master' into dami/change-account
dharjeezy Jan 22, 2024
59bb68a
Merge branch 'master' into dami/change-account
dharjeezy Jan 24, 2024
a33fe94
Merge branch 'master' into dami/change-account
dharjeezy Jan 24, 2024
b7078b0
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
Jan 24, 2024
7d25b5c
".git/.scripts/commands/bench/bench.sh" --subcommand=runtime --runtim…
Jan 24, 2024
fdbbe76
nit
dharjeezy Jan 24, 2024
2758890
Merge remote-tracking branch 'origin/dami/change-account' into dami/c…
dharjeezy Jan 24, 2024
239831c
Merge branch 'master' into dami/change-account
dharjeezy Jan 25, 2024
99fc1b7
nit
dharjeezy Jan 25, 2024
76963a1
Merge remote-tracking branch 'origin/dami/change-account' into dami/c…
dharjeezy Jan 25, 2024
5c6b2cf
nit
dharjeezy Jan 25, 2024
0cb7494
nit
dharjeezy Jan 25, 2024
f58c20a
nit
dharjeezy Jan 25, 2024
0da55a7
Merge branch 'master' into dami/change-account
dharjeezy Jan 25, 2024
7ed5a3d
nit
dharjeezy Jan 26, 2024
598c2bc
Merge branch 'master' into dami/change-account
dharjeezy Jan 26, 2024
3f03aaf
Merge branch 'master' into dami/change-account
dharjeezy Jan 26, 2024
6e9e14d
fmt
ggwpez Jan 26, 2024
0920307
Add prdoc
ggwpez Jan 26, 2024
43773cb
Merge branch 'master' into dami/change-account
ggwpez Jan 26, 2024
684beb3
helper method
dharjeezy Jan 30, 2024
754184f
Merge branch 'master' into dami/change-account
dharjeezy Jan 30, 2024
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
11 changes: 11 additions & 0 deletions polkadot/runtime/rococo/src/governance/fellowship.rs
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,17 @@ impl pallet_ranked_collective::Config<FellowshipCollectiveInstance> for Runtime
TryMapSuccess<origins::EnsureFellowship, CheckedReduceBy<ConstU16<2>>>,
>,
>;
// Exchange is by any of:
// - Root can exchange arbitrarily.
// - the FellowshipAdmin origin (i.e. token holder referendum);
// - a vote by the rank *above* the new rank.
type ExchangeOrigin = EitherOf<
frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
EitherOf<
MapSuccess<FellowshipAdmin, Replace<ConstU16<9>>>,
TryMapSuccess<origins::EnsureFellowship, CheckedReduceBy<ConstU16<2>>>,
>,
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
>;
type Polls = FellowshipReferenda;
type MinRankOfClass = sp_runtime::traits::Identity;
type VoteWeight = pallet_ranked_collective::Geometric;
Expand Down
1 change: 1 addition & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,7 @@ impl pallet_ranked_collective::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type PromoteOrigin = EnsureRootWithSuccess<AccountId, ConstU16<65535>>;
type DemoteOrigin = EnsureRootWithSuccess<AccountId, ConstU16<65535>>;
type ExchangeOrigin = EnsureRootWithSuccess<AccountId, ConstU16<65535>>;
type Polls = RankedPolls;
type MinRankOfClass = traits::Identity;
type VoteWeight = pallet_ranked_collective::Geometric;
Expand Down
15 changes: 15 additions & 0 deletions substrate/frame/ranked-collective/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -173,5 +173,20 @@ benchmarks_instance_pallet! {
assert_eq!(Voting::<T, I>::iter().count(), 0);
}

exchange_member {
let who = make_member::<T, I>(1);
let who_lookup = T::Lookup::unlookup(who.clone());
let new_who = account::<T::AccountId>("new-member", 0, SEED);
let new_who_lookup = T::Lookup::unlookup(new_who.clone());
let origin =
T::ExchangeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let call = Call::<T, I>::exchange_member { who: who_lookup, new_who:new_who_lookup};
}: { call.dispatch_bypass_filter(origin)? }
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
verify {
assert_eq!(Members::<T, I>::get(&new_who).unwrap().rank, 1);
assert_eq!(Members::<T, I>::get(&who), None);
assert_last_event::<T, I>(Event::MemberExchanged { who, new_who }.into());
}

impl_benchmark_test_suite!(RankedCollective, crate::tests::new_test_ext(), crate::tests::Test);
}
77 changes: 63 additions & 14 deletions substrate/frame/ranked-collective/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,7 @@ impl<T: Config<I>, I: 'static, M: GetMaxVoters<Class = ClassOf<T, I>>>
crate::Pallet::<T, I>::do_add_member_to_rank(
who,
T::MinRankOfClass::convert(class.clone()),
true,
)
.expect("could not add members for benchmarks");
}
Expand Down Expand Up @@ -299,7 +300,7 @@ impl<T: Config<I>, I: 'static> EnsureOriginWithArg<T::RuntimeOrigin, Rank> for E
#[cfg(feature = "runtime-benchmarks")]
fn try_successful_origin(min_rank: &Rank) -> Result<T::RuntimeOrigin, ()> {
let who = frame_benchmarking::account::<T::AccountId>("successful_origin", 0, 0);
crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), *min_rank)
crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), *min_rank, true)
.expect("Could not add members for benchmarks");
Ok(frame_system::RawOrigin::Signed(who).into())
}
Expand Down Expand Up @@ -352,7 +353,7 @@ impl<T: Config<I>, I: 'static, const MIN_RANK: u16> EnsureOrigin<T::RuntimeOrigi
#[cfg(feature = "runtime-benchmarks")]
fn try_successful_origin() -> Result<T::RuntimeOrigin, ()> {
let who = frame_benchmarking::account::<T::AccountId>("successful_origin", 0, 0);
crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), MIN_RANK)
crate::Pallet::<T, I>::do_add_member_to_rank(who.clone(), MIN_RANK, true)
.expect("Could not add members for benchmarks");
Ok(frame_system::RawOrigin::Signed(who).into())
}
Expand Down Expand Up @@ -390,6 +391,11 @@ pub mod pallet {
/// maximum rank *from which* the demotion/removal may be.
type DemoteOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Rank>;

/// The origin required to exchange an account for a member.
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
///
/// The success value indicates the maximum rank *from which* the exchange may be.
type ExchangeOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Rank>;
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved

/// The polling system used for our voting.
type Polls: Polling<TallyOf<Self, I>, Votes = Votes, Moment = BlockNumberFor<Self>>;

Expand Down Expand Up @@ -451,9 +457,10 @@ pub mod pallet {
RankChanged { who: T::AccountId, rank: Rank },
/// The member `who` of given `rank` has been removed from the collective.
MemberRemoved { who: T::AccountId, rank: Rank },
/// The member `who` has voted for the `poll` with the given `vote` leading to an updated
/// `tally`.
/// The member `who` had their `AccountId` changed to `new_who`.
Voted { who: T::AccountId, poll: PollIndexOf<T, I>, vote: VoteRecord, tally: TallyOf<T, I> },
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
/// The member `who` had their `AccountId` changed to `new_who`.
MemberExchanged { who: T::AccountId, new_who: T::AccountId },
}

#[pallet::error]
Expand All @@ -476,6 +483,8 @@ pub mod pallet {
InvalidWitness,
/// The origin is not sufficiently privileged to do the operation.
NoPermission,
/// The new member to exchange is the same as the old member
SameMember,
}

#[pallet::call]
Expand All @@ -492,7 +501,7 @@ pub mod pallet {
pub fn add_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
let _ = T::PromoteOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;
Self::do_add_member(who)
Self::do_add_member(who, true)
}

/// Increment the rank of an existing member by one.
Expand All @@ -506,7 +515,7 @@ pub mod pallet {
pub fn promote_member(origin: OriginFor<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
let max_rank = T::PromoteOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;
Self::do_promote_member(who, Some(max_rank))
Self::do_promote_member(who, Some(max_rank), true)
}

/// Decrement the rank of an existing member by one. If the member is already at rank zero,
Expand Down Expand Up @@ -650,6 +659,37 @@ pub mod pallet {
pays_fee: Pays::No,
})
}

/// Exchanges a member with a new account and the same existing rank.
///
/// - `origin`: Must be the `AdminOrigin`.
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
/// - `who`: Account of existing member of rank greater than zero to be exchanged.
/// - `new_who`: New Account of existing member of rank greater than zero to exchanged to.
#[pallet::call_index(6)]
#[pallet::weight(T::WeightInfo::exchange_member())]
pub fn exchange_member(
origin: OriginFor<T>,
who: AccountIdLookupOf<T>,
new_who: AccountIdLookupOf<T>,
) -> DispatchResult {
// remove who
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
T::ExchangeOrigin::ensure_origin(origin)?;
ensure!(who != new_who, Error::<T, I>::SameMember);
let who = T::Lookup::lookup(who)?;
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
let MemberRecord { rank, .. } = Self::ensure_member(&who)?;

for r in 0..=rank {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make an equivalent of do_remove_member_to_rank that does these two operations, it would be a good counterpart to do_add_member_to_rank, it would make the code a bit nicer, and you can use it in other places as well.

Self::remove_from_rank(&who, r)?;
}
Members::<T, I>::remove(&who);

// add new_who member
let new_who = T::Lookup::lookup(new_who)?;
dharjeezy marked this conversation as resolved.
Show resolved Hide resolved
Self::do_add_member_to_rank(new_who.clone(), rank, false)?;

Self::deposit_event(Event::MemberExchanged { who, new_who });
bkchr marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
Expand Down Expand Up @@ -683,7 +723,7 @@ pub mod pallet {
/// Adds a member into the ranked collective at level 0.
///
/// No origin checks are executed.
pub fn do_add_member(who: T::AccountId) -> DispatchResult {
pub fn do_add_member(who: T::AccountId, emit_event: bool) -> DispatchResult {
ensure!(!Members::<T, I>::contains_key(&who), Error::<T, I>::AlreadyMember);
let index = MemberCount::<T, I>::get(0);
let count = index.checked_add(1).ok_or(Overflow)?;
Expand All @@ -692,7 +732,9 @@ pub mod pallet {
IdToIndex::<T, I>::insert(0, &who, index);
IndexToId::<T, I>::insert(0, index, &who);
MemberCount::<T, I>::insert(0, count);
Self::deposit_event(Event::MemberAdded { who });
if emit_event {
Self::deposit_event(Event::MemberAdded { who });
}
Ok(())
}

Expand All @@ -703,6 +745,7 @@ pub mod pallet {
pub fn do_promote_member(
who: T::AccountId,
maybe_max_rank: Option<Rank>,
emit_event: bool,
) -> DispatchResult {
let record = Self::ensure_member(&who)?;
let rank = record.rank.checked_add(1).ok_or(Overflow)?;
Expand All @@ -714,7 +757,9 @@ pub mod pallet {
IdToIndex::<T, I>::insert(rank, &who, index);
IndexToId::<T, I>::insert(rank, index, &who);
Members::<T, I>::insert(&who, MemberRecord { rank });
Self::deposit_event(Event::RankChanged { who, rank });
if emit_event {
Self::deposit_event(Event::RankChanged { who, rank });
}
Ok(())
}

Expand Down Expand Up @@ -747,10 +792,14 @@ pub mod pallet {

/// Add a member to the rank collective, and continue to promote them until a certain rank
/// is reached.
pub fn do_add_member_to_rank(who: T::AccountId, rank: Rank) -> DispatchResult {
Self::do_add_member(who.clone())?;
pub fn do_add_member_to_rank(
who: T::AccountId,
rank: Rank,
emit_event: bool,
) -> DispatchResult {
Self::do_add_member(who.clone(), emit_event)?;
for _ in 0..rank {
Self::do_promote_member(who.clone(), None)?;
Self::do_promote_member(who.clone(), None, emit_event)?;
}
Ok(())
}
Expand Down Expand Up @@ -778,11 +827,11 @@ pub mod pallet {
}

fn induct(who: &Self::AccountId) -> DispatchResult {
Self::do_add_member(who.clone())
Self::do_add_member(who.clone(), true)
}

fn promote(who: &Self::AccountId) -> DispatchResult {
Self::do_promote_member(who.clone(), None)
Self::do_promote_member(who.clone(), None, true)
}

fn demote(who: &Self::AccountId) -> DispatchResult {
Expand Down
30 changes: 28 additions & 2 deletions substrate/frame/ranked-collective/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,12 @@ impl Config for Test {
// Members can demote up to the rank of 3 below them.
MapSuccess<EnsureRanked<Test, (), 3>, ReduceBy<ConstU16<3>>>,
>;
type ExchangeOrigin = EitherOf<
// Root can exchange arbitrarily.
frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
// Members can exchange up to the rank of 2 below them.
MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
>;
type Polls = TestPolls;
type MinRankOfClass = MinRankOfClass<MinRankOfClassDelta>;
type VoteWeight = Geometric;
Expand Down Expand Up @@ -516,8 +522,8 @@ fn ensure_ranked_works() {
fn do_add_member_to_rank_works() {
new_test_ext().execute_with(|| {
let max_rank = 9u16;
assert_ok!(Club::do_add_member_to_rank(69, max_rank / 2));
assert_ok!(Club::do_add_member_to_rank(1337, max_rank));
assert_ok!(Club::do_add_member_to_rank(69, max_rank / 2, true));
assert_ok!(Club::do_add_member_to_rank(1337, max_rank, true));
for i in 0..=max_rank {
if i <= max_rank / 2 {
assert_eq!(member_count(i), 2);
Expand Down Expand Up @@ -568,3 +574,23 @@ fn tally_support_correct() {
MinRankOfClassDelta::set(0);
});
}

#[test]
fn exchange_member_works() {
Copy link
Contributor

Choose a reason for hiding this comment

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

would be good to see a test for EnsureRanked<Test, (), 2> in action as well.

new_test_ext().execute_with(|| {
assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
assert_eq!(member_count(0), 1);

assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1));

let member_record = MemberRecord { rank: 1 };
assert_eq!(Members::<Test>::get(1), Some(member_record.clone()));
assert_eq!(Members::<Test>::get(2), None);

assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 1, 2));
assert_eq!(member_count(0), 1);

assert_eq!(Members::<Test>::get(1), None);
assert_eq!(Members::<Test>::get(2), Some(member_record));
});
}
39 changes: 39 additions & 0 deletions substrate/frame/ranked-collective/src/weights.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading