From 07b93db2112e77654987fad04d72430728472357 Mon Sep 17 00:00:00 2001 From: Damilare Date: Sun, 3 Dec 2023 07:56:23 +0100 Subject: [PATCH 01/18] exchange member with a new account and same rank in the ranked collective --- substrate/frame/ranked-collective/src/lib.rs | 43 +++++++++++++++++++ .../frame/ranked-collective/src/tests.rs | 20 +++++++++ 2 files changed, 63 insertions(+) diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index deb1ccf23578..a3549791a3f0 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -454,6 +454,8 @@ pub mod pallet { /// The member `who` has voted for the `poll` with the given `vote` leading to an updated /// `tally`. Voted { who: T::AccountId, poll: PollIndexOf, vote: VoteRecord, tally: TallyOf }, + /// The member `who` of given `rank` has been exchanged from the collective. + MemberExchanged { who: T::AccountId, new_who: T::AccountId, rank: Rank }, } #[pallet::error] @@ -650,6 +652,47 @@ pub mod pallet { pays_fee: Pays::No, }) } + + /// Exchanges a member with a new account and the same existing rank. + /// + /// - `origin`: Must be the `AdminOrigin`. + /// - `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. + /// - `min_rank`: The rank of the member or greater. + /// + /// Weight: `O(min_rank)`. + #[pallet::call_index(6)] + #[pallet::weight(0)] + pub fn exchange_member( + origin: OriginFor, + who: AccountIdLookupOf, + new_who: AccountIdLookupOf, + min_rank: Rank, + ) -> DispatchResult { + // remove who + let max_rank = T::DemoteOrigin::ensure_origin(origin)?; + let who = T::Lookup::lookup(who)?; + let MemberRecord { rank, .. } = Self::ensure_member(&who)?; + ensure!(min_rank >= rank, Error::::InvalidWitness); + ensure!(max_rank >= rank, Error::::NoPermission); + + for r in 0..=rank { + Self::remove_from_rank(&who, r)?; + } + Members::::remove(&who); + + // add new member + let new_who = T::Lookup::lookup(new_who)?; + Self::do_add_member(new_who.clone())?; + + // promotes new_who to the rank of who + for _i in 1..=rank { + let _ = Self::do_promote_member(new_who.clone(), Some(max_rank.clone()))?; + } + + Self::deposit_event(Event::MemberExchanged { who, new_who: new_who.clone(), rank }); + Ok(()) + } } impl, I: 'static> Pallet { diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index 3a5570651609..60f8527d68f6 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -567,3 +567,23 @@ fn tally_support_correct() { MinRankOfClassDelta::set(0); }); } + +#[test] +fn exchange_member_works() { + 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::::get(1), Some(member_record.clone())); + assert_eq!(Members::::get(2), None); + + assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 1, 2, 2)); + assert_eq!(member_count(0), 1); + + assert_eq!(Members::::get(1), None); + assert_eq!(Members::::get(2), Some(member_record)); + }); +} From fbf91f3a152e496399ae9706d98ec48e50fdcf67 Mon Sep 17 00:00:00 2001 From: Damilare Date: Mon, 11 Dec 2023 17:42:57 +0100 Subject: [PATCH 02/18] address concerns and benchmarks --- .../runtime/rococo/src/governance/fellowship.rs | 11 +++++++++++ substrate/bin/node/runtime/src/lib.rs | 1 + .../frame/ranked-collective/src/benchmarking.rs | 15 +++++++++++++++ substrate/frame/ranked-collective/src/lib.rs | 15 ++++++++------- substrate/frame/ranked-collective/src/tests.rs | 8 +++++++- 5 files changed, 42 insertions(+), 8 deletions(-) diff --git a/polkadot/runtime/rococo/src/governance/fellowship.rs b/polkadot/runtime/rococo/src/governance/fellowship.rs index b5df6cf2df34..2844ac4ebad2 100644 --- a/polkadot/runtime/rococo/src/governance/fellowship.rs +++ b/polkadot/runtime/rococo/src/governance/fellowship.rs @@ -337,6 +337,17 @@ impl pallet_ranked_collective::Config for Runtime TryMapSuccess>>, >, >; + // 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>, + EitherOf< + MapSuccess>>, + TryMapSuccess>>, + >, + >; type Polls = FellowshipReferenda; type MinRankOfClass = sp_runtime::traits::Identity; type VoteWeight = pallet_ranked_collective::Geometric; diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index e2746b1c35bc..fb52247dcee9 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1006,6 +1006,7 @@ impl pallet_ranked_collective::Config for Runtime { type RuntimeEvent = RuntimeEvent; type PromoteOrigin = EnsureRootWithSuccess>; type DemoteOrigin = EnsureRootWithSuccess>; + type ExchangeOrigin = EnsureRootWithSuccess>; type Polls = RankedPolls; type MinRankOfClass = traits::Identity; type VoteWeight = pallet_ranked_collective::Geometric; diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 518428880e4d..8d3eca98cc76 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -173,5 +173,20 @@ benchmarks_instance_pallet! { assert_eq!(Voting::::iter().count(), 0); } + exchange_member { + let who = make_member::(1); + let who_lookup = T::Lookup::unlookup(who.clone()); + let new_who = account::("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::::exchange_member { who: who_lookup, new_who:new_who_lookup}; + }: { call.dispatch_bypass_filter(origin)? } + verify { + assert_eq!(Members::::get(&new_who).unwrap().rank, 1); + assert_eq!(Members::::get(&who), None); + assert_last_event::(Event::MemberExchanged { who, new_who }.into()); + } + impl_benchmark_test_suite!(RankedCollective, crate::tests::new_test_ext(), crate::tests::Test); } diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index a3549791a3f0..609dd0480fc7 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -390,6 +390,10 @@ pub mod pallet { /// maximum rank *from which* the demotion/removal may be. type DemoteOrigin: EnsureOrigin; + /// The origin required to exchange an account for a member. The success value indicates the + /// maximum rank *from which* the exhange may be. + type ExchangeOrigin: EnsureOrigin; + /// The polling system used for our voting. type Polls: Polling, Votes = Votes, Moment = BlockNumberFor>; @@ -455,7 +459,7 @@ pub mod pallet { /// `tally`. Voted { who: T::AccountId, poll: PollIndexOf, vote: VoteRecord, tally: TallyOf }, /// The member `who` of given `rank` has been exchanged from the collective. - MemberExchanged { who: T::AccountId, new_who: T::AccountId, rank: Rank }, + MemberExchanged { who: T::AccountId, new_who: T::AccountId }, } #[pallet::error] @@ -667,14 +671,11 @@ pub mod pallet { origin: OriginFor, who: AccountIdLookupOf, new_who: AccountIdLookupOf, - min_rank: Rank, ) -> DispatchResult { // remove who - let max_rank = T::DemoteOrigin::ensure_origin(origin)?; + let max_rank = T::ExchangeOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; let MemberRecord { rank, .. } = Self::ensure_member(&who)?; - ensure!(min_rank >= rank, Error::::InvalidWitness); - ensure!(max_rank >= rank, Error::::NoPermission); for r in 0..=rank { Self::remove_from_rank(&who, r)?; @@ -687,10 +688,10 @@ pub mod pallet { // promotes new_who to the rank of who for _i in 1..=rank { - let _ = Self::do_promote_member(new_who.clone(), Some(max_rank.clone()))?; + let _ = Self::do_promote_member(new_who.clone(), Some(max_rank))?; } - Self::deposit_event(Event::MemberExchanged { who, new_who: new_who.clone(), rank }); + Self::deposit_event(Event::MemberExchanged { who, new_who }); Ok(()) } } diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index 60f8527d68f6..08040663eb61 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -189,6 +189,12 @@ impl Config for Test { // Members can demote up to the rank of 3 below them. MapSuccess, ReduceBy>>, >; + type ExchangeOrigin = EitherOf< + // Root can exchange arbitrarily. + frame_system::EnsureRootWithSuccess>, + // Members can exchange up to the rank of 2 below them. + MapSuccess, ReduceBy>>, + >; type Polls = TestPolls; type MinRankOfClass = MinRankOfClass; type VoteWeight = Geometric; @@ -580,7 +586,7 @@ fn exchange_member_works() { assert_eq!(Members::::get(1), Some(member_record.clone())); assert_eq!(Members::::get(2), None); - assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 1, 2, 2)); + assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 1, 2)); assert_eq!(member_count(0), 1); assert_eq!(Members::::get(1), None); From cd50907ee95bf92248be45b025c2f3df31e97189 Mon Sep 17 00:00:00 2001 From: Damilare Date: Mon, 11 Dec 2023 17:50:52 +0100 Subject: [PATCH 03/18] change comment --- substrate/frame/ranked-collective/src/lib.rs | 2 +- substrate/frame/ranked-collective/src/tests.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index d780ef6517e3..16dd72ab6973 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -458,7 +458,7 @@ pub mod pallet { /// The member `who` has voted for the `poll` with the given `vote` leading to an updated /// `tally`. Voted { who: T::AccountId, poll: PollIndexOf, vote: VoteRecord, tally: TallyOf }, - /// The member `who` of given `rank` has been exchanged from the collective. + /// The member `who`, of given `rank`, has had their `AccountId` changed to `new_who`. MemberExchanged { who: T::AccountId, new_who: T::AccountId }, } diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index 779ab85a81b9..a6d56261790a 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -167,9 +167,9 @@ impl Config for Test { type ExchangeOrigin = EitherOf< // Root can exchange arbitrarily. frame_system::EnsureRootWithSuccess>, - // Members can exchange up to the rank of 2 below them. - MapSuccess, ReduceBy>>, - >; + // Members can exchange up to the rank of 2 below them. + MapSuccess, ReduceBy>>, + >; type Polls = TestPolls; type MinRankOfClass = MinRankOfClass; type VoteWeight = Geometric; From 05f6ee6f74a7fc61325d67e118f959d21c61a843 Mon Sep 17 00:00:00 2001 From: Damilare Date: Mon, 11 Dec 2023 17:55:08 +0100 Subject: [PATCH 04/18] fmt --- polkadot/runtime/rococo/src/governance/fellowship.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/polkadot/runtime/rococo/src/governance/fellowship.rs b/polkadot/runtime/rococo/src/governance/fellowship.rs index 2844ac4ebad2..d4365d5c3766 100644 --- a/polkadot/runtime/rococo/src/governance/fellowship.rs +++ b/polkadot/runtime/rococo/src/governance/fellowship.rs @@ -343,11 +343,11 @@ impl pallet_ranked_collective::Config for Runtime // - a vote by the rank *above* the new rank. type ExchangeOrigin = EitherOf< frame_system::EnsureRootWithSuccess>, - EitherOf< - MapSuccess>>, - TryMapSuccess>>, - >, - >; + EitherOf< + MapSuccess>>, + TryMapSuccess>>, + >, + >; type Polls = FellowshipReferenda; type MinRankOfClass = sp_runtime::traits::Identity; type VoteWeight = pallet_ranked_collective::Geometric; From 255ad1f41980ba6f1ebcd013846e9c7185e7d412 Mon Sep 17 00:00:00 2001 From: Damilare Date: Thu, 21 Dec 2023 16:44:44 +0100 Subject: [PATCH 05/18] weight --- substrate/frame/ranked-collective/src/lib.rs | 2 +- .../frame/ranked-collective/src/weights.rs | 39 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index 16dd72ab6973..62b76f51a36e 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -666,7 +666,7 @@ pub mod pallet { /// /// Weight: `O(min_rank)`. #[pallet::call_index(6)] - #[pallet::weight(0)] + #[pallet::weight(T::WeightInfo::exchange_member())] pub fn exchange_member( origin: OriginFor, who: AccountIdLookupOf, diff --git a/substrate/frame/ranked-collective/src/weights.rs b/substrate/frame/ranked-collective/src/weights.rs index 9f1a0a818044..4ff0c3337d50 100644 --- a/substrate/frame/ranked-collective/src/weights.rs +++ b/substrate/frame/ranked-collective/src/weights.rs @@ -58,6 +58,7 @@ pub trait WeightInfo { fn demote_member(r: u32, ) -> Weight; fn vote() -> Weight; fn cleanup_poll(n: u32, ) -> Weight; + fn exchange_member() -> Weight; } /// Weights for pallet_ranked_collective using the Substrate node and recommended hardware. @@ -180,6 +181,25 @@ impl WeightInfo for SubstrateWeight { .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into()))) .saturating_add(Weight::from_parts(0, 2540).saturating_mul(n.into())) } + + /// Storage: `RankedCollective::Members` (r:2 w:2) + /// Proof: `RankedCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`) + /// Storage: `RankedCollective::MemberCount` (r:2 w:2) + /// Proof: `RankedCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`) + /// Storage: `RankedCollective::IdToIndex` (r:2 w:2) + /// Proof: `RankedCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) + /// Storage: `RankedCollective::IndexToId` (r:0 w:2) + /// Proof: `RankedCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) + fn exchange_member() -> Weight { + // Proof Size summary in bytes: + // Measured: `437` + // Estimated: `6048` + // Minimum execution time: 138_000_000 picoseconds. + Weight::from_parts(141_000_000, 0) + .saturating_add(Weight::from_parts(0, 6048)) + .saturating_add(T::DbWeight::get().reads(6)) + .saturating_add(T::DbWeight::get().writes(8)) + } } // For backwards compatibility and tests @@ -301,4 +321,23 @@ impl WeightInfo for () { .saturating_add(RocksDbWeight::get().writes((1_u64).saturating_mul(n.into()))) .saturating_add(Weight::from_parts(0, 2540).saturating_mul(n.into())) } + + /// Storage: `RankedCollective::Members` (r:2 w:2) + /// Proof: `RankedCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`) + /// Storage: `RankedCollective::MemberCount` (r:2 w:2) + /// Proof: `RankedCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`) + /// Storage: `RankedCollective::IdToIndex` (r:2 w:2) + /// Proof: `RankedCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) + /// Storage: `RankedCollective::IndexToId` (r:0 w:2) + /// Proof: `RankedCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) + fn exchange_member() -> Weight { + // Proof Size summary in bytes: + // Measured: `437` + // Estimated: `6048` + // Minimum execution time: 138_000_000 picoseconds. + Weight::from_parts(141_000_000, 0) + .saturating_add(Weight::from_parts(0, 6048)) + .saturating_add(RocksDbWeight::get().reads(6)) + .saturating_add(RocksDbWeight::get().writes(8)) + } } From f7d68ef3190d1c506af1e6a2de81670672cbe418 Mon Sep 17 00:00:00 2001 From: Damilare Date: Thu, 18 Jan 2024 23:24:33 +0100 Subject: [PATCH 06/18] boolean flag to toggle emission of events --- .../rococo/src/governance/fellowship.rs | 2 +- substrate/frame/ranked-collective/src/lib.rs | 59 ++++++++++--------- .../frame/ranked-collective/src/tests.rs | 4 +- 3 files changed, 35 insertions(+), 30 deletions(-) diff --git a/polkadot/runtime/rococo/src/governance/fellowship.rs b/polkadot/runtime/rococo/src/governance/fellowship.rs index d4365d5c3766..2251446387bf 100644 --- a/polkadot/runtime/rococo/src/governance/fellowship.rs +++ b/polkadot/runtime/rococo/src/governance/fellowship.rs @@ -345,7 +345,7 @@ impl pallet_ranked_collective::Config for Runtime frame_system::EnsureRootWithSuccess>, EitherOf< MapSuccess>>, - TryMapSuccess>>, + TryMapSuccess>>, >, >; type Polls = FellowshipReferenda; diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index 62b76f51a36e..40e6337c55c1 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -161,6 +161,7 @@ impl, I: 'static, M: GetMaxVoters>> crate::Pallet::::do_add_member_to_rank( who, T::MinRankOfClass::convert(class.clone()), + true, ) .expect("could not add members for benchmarks"); } @@ -299,7 +300,7 @@ impl, I: 'static> EnsureOriginWithArg for E #[cfg(feature = "runtime-benchmarks")] fn try_successful_origin(min_rank: &Rank) -> Result { let who = frame_benchmarking::account::("successful_origin", 0, 0); - crate::Pallet::::do_add_member_to_rank(who.clone(), *min_rank) + crate::Pallet::::do_add_member_to_rank(who.clone(), *min_rank, true) .expect("Could not add members for benchmarks"); Ok(frame_system::RawOrigin::Signed(who).into()) } @@ -352,7 +353,7 @@ impl, I: 'static, const MIN_RANK: u16> EnsureOrigin Result { let who = frame_benchmarking::account::("successful_origin", 0, 0); - crate::Pallet::::do_add_member_to_rank(who.clone(), MIN_RANK) + crate::Pallet::::do_add_member_to_rank(who.clone(), MIN_RANK, true) .expect("Could not add members for benchmarks"); Ok(frame_system::RawOrigin::Signed(who).into()) } @@ -390,8 +391,9 @@ pub mod pallet { /// maximum rank *from which* the demotion/removal may be. type DemoteOrigin: EnsureOrigin; - /// The origin required to exchange an account for a member. The success value indicates the - /// maximum rank *from which* the exhange may be. + /// The origin required to exchange an account for a member. + /// + /// The success value indicates the maximum rank *from which* the exchange may be. type ExchangeOrigin: EnsureOrigin; /// The polling system used for our voting. @@ -455,8 +457,7 @@ 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, vote: VoteRecord, tally: TallyOf }, /// The member `who`, of given `rank`, has had their `AccountId` changed to `new_who`. MemberExchanged { who: T::AccountId, new_who: T::AccountId }, @@ -482,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] @@ -498,7 +501,7 @@ pub mod pallet { pub fn add_member(origin: OriginFor, who: AccountIdLookupOf) -> 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. @@ -512,7 +515,7 @@ pub mod pallet { pub fn promote_member(origin: OriginFor, who: AccountIdLookupOf) -> 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, @@ -662,9 +665,6 @@ pub mod pallet { /// - `origin`: Must be the `AdminOrigin`. /// - `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. - /// - `min_rank`: The rank of the member or greater. - /// - /// Weight: `O(min_rank)`. #[pallet::call_index(6)] #[pallet::weight(T::WeightInfo::exchange_member())] pub fn exchange_member( @@ -673,7 +673,8 @@ pub mod pallet { new_who: AccountIdLookupOf, ) -> DispatchResult { // remove who - let max_rank = T::ExchangeOrigin::ensure_origin(origin)?; + T::ExchangeOrigin::ensure_origin(origin)?; + ensure!(who != new_who, Error::::SameMember); let who = T::Lookup::lookup(who)?; let MemberRecord { rank, .. } = Self::ensure_member(&who)?; @@ -682,14 +683,9 @@ pub mod pallet { } Members::::remove(&who); - // add new member + // add new_who member let new_who = T::Lookup::lookup(new_who)?; - Self::do_add_member(new_who.clone())?; - - // promotes new_who to the rank of who - for _i in 1..=rank { - let _ = Self::do_promote_member(new_who.clone(), Some(max_rank))?; - } + Self::do_add_member_to_rank(new_who.clone(), rank, false)?; Self::deposit_event(Event::MemberExchanged { who, new_who }); Ok(()) @@ -727,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::::contains_key(&who), Error::::AlreadyMember); let index = MemberCount::::get(0); let count = index.checked_add(1).ok_or(Overflow)?; @@ -736,7 +732,9 @@ pub mod pallet { IdToIndex::::insert(0, &who, index); IndexToId::::insert(0, index, &who); MemberCount::::insert(0, count); - Self::deposit_event(Event::MemberAdded { who }); + if emit_event { + Self::deposit_event(Event::MemberAdded { who }); + } Ok(()) } @@ -747,6 +745,7 @@ pub mod pallet { pub fn do_promote_member( who: T::AccountId, maybe_max_rank: Option, + emit_event: bool, ) -> DispatchResult { let record = Self::ensure_member(&who)?; let rank = record.rank.checked_add(1).ok_or(Overflow)?; @@ -758,7 +757,9 @@ pub mod pallet { IdToIndex::::insert(rank, &who, index); IndexToId::::insert(rank, index, &who); Members::::insert(&who, MemberRecord { rank }); - Self::deposit_event(Event::RankChanged { who, rank }); + if emit_event { + Self::deposit_event(Event::RankChanged { who, rank }); + } Ok(()) } @@ -791,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(()) } @@ -822,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 { diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index a6d56261790a..4b696bd3a71b 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -522,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); From ec954ad831b232f57d07ff0368016d2c24210353 Mon Sep 17 00:00:00 2001 From: Damilare Date: Thu, 18 Jan 2024 23:27:53 +0100 Subject: [PATCH 07/18] doc --- substrate/frame/ranked-collective/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index 40e6337c55c1..574df780f513 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -459,7 +459,7 @@ pub mod pallet { MemberRemoved { who: T::AccountId, rank: Rank }, /// The member `who` had their `AccountId` changed to `new_who`. Voted { who: T::AccountId, poll: PollIndexOf, vote: VoteRecord, tally: TallyOf }, - /// The member `who`, of given `rank`, has had their `AccountId` changed to `new_who`. + /// The member `who` had their `AccountId` changed to `new_who`. MemberExchanged { who: T::AccountId, new_who: T::AccountId }, } From 3d0c8f706cd6c9124c37c7ed094f8047f5828684 Mon Sep 17 00:00:00 2001 From: Damilare Date: Sun, 21 Jan 2024 04:56:11 +0100 Subject: [PATCH 08/18] subtle changes --- polkadot/runtime/rococo/src/governance/fellowship.rs | 12 +++--------- .../rococo/src/weights/pallet_ranked_collective.rs | 4 ++++ substrate/frame/ranked-collective/src/lib.rs | 12 +++++------- 3 files changed, 12 insertions(+), 16 deletions(-) diff --git a/polkadot/runtime/rococo/src/governance/fellowship.rs b/polkadot/runtime/rococo/src/governance/fellowship.rs index 2251446387bf..70477dd87ef5 100644 --- a/polkadot/runtime/rococo/src/governance/fellowship.rs +++ b/polkadot/runtime/rococo/src/governance/fellowship.rs @@ -339,15 +339,9 @@ impl pallet_ranked_collective::Config for Runtime >; // 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>, - EitherOf< - MapSuccess>>, - TryMapSuccess>>, - >, - >; + // - the Fellows origin; + type ExchangeOrigin = + EitherOf>, Fellows>; type Polls = FellowshipReferenda; type MinRankOfClass = sp_runtime::traits::Identity; type VoteWeight = pallet_ranked_collective::Geometric; diff --git a/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs b/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs index 8a556c3a248e..8cef4e99b07d 100644 --- a/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs +++ b/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs @@ -172,4 +172,8 @@ impl pallet_ranked_collective::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into()))) .saturating_add(Weight::from_parts(0, 2540).saturating_mul(n.into())) } + + fn exchange_member() -> Weight { + todo!() + } } diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index 574df780f513..daee16fe9d50 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -392,9 +392,7 @@ pub mod pallet { type DemoteOrigin: EnsureOrigin; /// The origin required to exchange an account for a member. - /// - /// The success value indicates the maximum rank *from which* the exchange may be. - type ExchangeOrigin: EnsureOrigin; + type ExchangeOrigin: EnsureOrigin; /// The polling system used for our voting. type Polls: Polling, Votes = Votes, Moment = BlockNumberFor>; @@ -672,10 +670,12 @@ pub mod pallet { who: AccountIdLookupOf, new_who: AccountIdLookupOf, ) -> DispatchResult { - // remove who T::ExchangeOrigin::ensure_origin(origin)?; - ensure!(who != new_who, Error::::SameMember); let who = T::Lookup::lookup(who)?; + let new_who = T::Lookup::lookup(new_who)?; + + ensure!(who != new_who, Error::::SameMember); + let MemberRecord { rank, .. } = Self::ensure_member(&who)?; for r in 0..=rank { @@ -683,8 +683,6 @@ pub mod pallet { } Members::::remove(&who); - // add new_who member - let new_who = T::Lookup::lookup(new_who)?; Self::do_add_member_to_rank(new_who.clone(), rank, false)?; Self::deposit_event(Event::MemberExchanged { who, new_who }); From 7d25b5c8ce7dd07253ceb2316e5beaee149b6a75 Mon Sep 17 00:00:00 2001 From: command-bot <> Date: Wed, 24 Jan 2024 17:56:14 +0000 Subject: [PATCH 09/18] ".git/.scripts/commands/bench/bench.sh" --subcommand=runtime --runtime=rococo --target_dir=polkadot --pallet=pallet_ranked_collective --- .../src/weights/pallet_ranked_collective.rs | 91 +++++++++++-------- 1 file changed, 52 insertions(+), 39 deletions(-) diff --git a/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs b/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs index 8cef4e99b07d..ce9d5fcc0c71 100644 --- a/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs +++ b/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs @@ -17,10 +17,10 @@ //! Autogenerated weights for `pallet_ranked_collective` //! //! THIS FILE WAS AUTO-GENERATED USING THE SUBSTRATE BENCHMARK CLI VERSION 4.0.0-dev -//! DATE: 2023-07-11, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` +//! DATE: 2024-01-24, STEPS: `50`, REPEAT: `20`, LOW RANGE: `[]`, HIGH RANGE: `[]` //! WORST CASE MAP SIZE: `1000000` -//! HOSTNAME: `runner-xerhrdyb-project-163-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz` -//! EXECUTION: `Some(Wasm)`, WASM-EXECUTION: `Compiled`, CHAIN: `Some("rococo-dev")`, DB CACHE: 1024 +//! HOSTNAME: `runner-grjcggob-project-674-concurrent-0`, CPU: `Intel(R) Xeon(R) CPU @ 2.60GHz` +//! WASM-EXECUTION: `Compiled`, CHAIN: `Some("rococo-dev")`, DB CACHE: 1024 // Executed Command: // target/production/polkadot @@ -29,14 +29,13 @@ // --steps=50 // --repeat=20 // --extrinsic=* -// --execution=wasm // --wasm-execution=compiled // --heap-pages=4096 -// --json-file=/builds/parity/mirrors/polkadot/.git/.artifacts/bench.json +// --json-file=/builds/parity/mirrors/polkadot-sdk/.git/.artifacts/bench.json // --pallet=pallet_ranked_collective // --chain=rococo-dev -// --header=./file_header.txt -// --output=./runtime/rococo/src/weights/ +// --header=./polkadot/file_header.txt +// --output=./polkadot/runtime/rococo/src/weights/ #![cfg_attr(rustfmt, rustfmt_skip)] #![allow(unused_parens)] @@ -61,8 +60,8 @@ impl pallet_ranked_collective::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `42` // Estimated: `3507` - // Minimum execution time: 17_632_000 picoseconds. - Weight::from_parts(18_252_000, 0) + // Minimum execution time: 13_480_000 picoseconds. + Weight::from_parts(13_786_000, 0) .saturating_add(Weight::from_parts(0, 3507)) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(4)) @@ -71,24 +70,24 @@ impl pallet_ranked_collective::WeightInfo for WeightInf /// Proof: `FellowshipCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`) /// Storage: `FellowshipCollective::MemberCount` (r:11 w:11) /// Proof: `FellowshipCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`) - /// Storage: `FellowshipCollective::IdToIndex` (r:11 w:11) + /// Storage: `FellowshipCollective::IdToIndex` (r:11 w:22) /// Proof: `FellowshipCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) - /// Storage: `FellowshipCollective::IndexToId` (r:11 w:11) + /// Storage: `FellowshipCollective::IndexToId` (r:11 w:22) /// Proof: `FellowshipCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) /// The range of component `r` is `[0, 10]`. fn remove_member(r: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `517 + r * (281 ±0)` + // Measured: `516 + r * (281 ±0)` // Estimated: `3519 + r * (2529 ±0)` - // Minimum execution time: 27_960_000 picoseconds. - Weight::from_parts(30_632_408, 0) + // Minimum execution time: 28_771_000 picoseconds. + Weight::from_parts(29_256_825, 0) .saturating_add(Weight::from_parts(0, 3519)) - // Standard Error: 22_806 - .saturating_add(Weight::from_parts(13_000_901, 0).saturating_mul(r.into())) + // Standard Error: 21_594 + .saturating_add(Weight::from_parts(14_649_527, 0).saturating_mul(r.into())) .saturating_add(T::DbWeight::get().reads(4)) .saturating_add(T::DbWeight::get().reads((3_u64).saturating_mul(r.into()))) - .saturating_add(T::DbWeight::get().writes(4)) - .saturating_add(T::DbWeight::get().writes((3_u64).saturating_mul(r.into()))) + .saturating_add(T::DbWeight::get().writes(6)) + .saturating_add(T::DbWeight::get().writes((5_u64).saturating_mul(r.into()))) .saturating_add(Weight::from_parts(0, 2529).saturating_mul(r.into())) } /// Storage: `FellowshipCollective::Members` (r:1 w:1) @@ -104,11 +103,11 @@ impl pallet_ranked_collective::WeightInfo for WeightInf // Proof Size summary in bytes: // Measured: `214 + r * (17 ±0)` // Estimated: `3507` - // Minimum execution time: 19_900_000 picoseconds. - Weight::from_parts(20_908_316, 0) + // Minimum execution time: 16_117_000 picoseconds. + Weight::from_parts(16_978_453, 0) .saturating_add(Weight::from_parts(0, 3507)) - // Standard Error: 4_878 - .saturating_add(Weight::from_parts(330_385, 0).saturating_mul(r.into())) + // Standard Error: 4_511 + .saturating_add(Weight::from_parts(324_261, 0).saturating_mul(r.into())) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().writes(4)) } @@ -116,22 +115,22 @@ impl pallet_ranked_collective::WeightInfo for WeightInf /// Proof: `FellowshipCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`) /// Storage: `FellowshipCollective::MemberCount` (r:1 w:1) /// Proof: `FellowshipCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`) - /// Storage: `FellowshipCollective::IdToIndex` (r:1 w:1) + /// Storage: `FellowshipCollective::IdToIndex` (r:1 w:2) /// Proof: `FellowshipCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) - /// Storage: `FellowshipCollective::IndexToId` (r:1 w:1) + /// Storage: `FellowshipCollective::IndexToId` (r:1 w:2) /// Proof: `FellowshipCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) /// The range of component `r` is `[0, 10]`. fn demote_member(r: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `532 + r * (72 ±0)` // Estimated: `3519` - // Minimum execution time: 27_697_000 picoseconds. - Weight::from_parts(30_341_815, 0) + // Minimum execution time: 28_995_000 picoseconds. + Weight::from_parts(31_343_215, 0) .saturating_add(Weight::from_parts(0, 3519)) - // Standard Error: 17_010 - .saturating_add(Weight::from_parts(642_213, 0).saturating_mul(r.into())) + // Standard Error: 16_438 + .saturating_add(Weight::from_parts(637_462, 0).saturating_mul(r.into())) .saturating_add(T::DbWeight::get().reads(4)) - .saturating_add(T::DbWeight::get().writes(4)) + .saturating_add(T::DbWeight::get().writes(6)) } /// Storage: `FellowshipCollective::Members` (r:1 w:0) /// Proof: `FellowshipCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`) @@ -143,10 +142,10 @@ impl pallet_ranked_collective::WeightInfo for WeightInf /// Proof: `Scheduler::Agenda` (`max_values`: None, `max_size`: Some(38963), added: 41438, mode: `MaxEncodedLen`) fn vote() -> Weight { // Proof Size summary in bytes: - // Measured: `638` + // Measured: `603` // Estimated: `83866` - // Minimum execution time: 48_275_000 picoseconds. - Weight::from_parts(49_326_000, 0) + // Minimum execution time: 38_820_000 picoseconds. + Weight::from_parts(40_240_000, 0) .saturating_add(Weight::from_parts(0, 83866)) .saturating_add(T::DbWeight::get().reads(5)) .saturating_add(T::DbWeight::get().writes(4)) @@ -160,20 +159,34 @@ impl pallet_ranked_collective::WeightInfo for WeightInf /// The range of component `n` is `[0, 100]`. fn cleanup_poll(n: u32, ) -> Weight { // Proof Size summary in bytes: - // Measured: `434 + n * (50 ±0)` + // Measured: `400 + n * (50 ±0)` // Estimated: `4365 + n * (2540 ±0)` - // Minimum execution time: 15_506_000 picoseconds. - Weight::from_parts(17_634_029, 0) + // Minimum execution time: 12_972_000 picoseconds. + Weight::from_parts(15_829_333, 0) .saturating_add(Weight::from_parts(0, 4365)) - // Standard Error: 2_117 - .saturating_add(Weight::from_parts(1_126_879, 0).saturating_mul(n.into())) + // Standard Error: 1_754 + .saturating_add(Weight::from_parts(1_116_520, 0).saturating_mul(n.into())) .saturating_add(T::DbWeight::get().reads(2)) .saturating_add(T::DbWeight::get().reads((1_u64).saturating_mul(n.into()))) .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into()))) .saturating_add(Weight::from_parts(0, 2540).saturating_mul(n.into())) } - + /// Storage: `FellowshipCollective::Members` (r:2 w:2) + /// Proof: `FellowshipCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`) + /// Storage: `FellowshipCollective::MemberCount` (r:2 w:2) + /// Proof: `FellowshipCollective::MemberCount` (`max_values`: None, `max_size`: Some(14), added: 2489, mode: `MaxEncodedLen`) + /// Storage: `FellowshipCollective::IdToIndex` (r:2 w:4) + /// Proof: `FellowshipCollective::IdToIndex` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) + /// Storage: `FellowshipCollective::IndexToId` (r:0 w:2) + /// Proof: `FellowshipCollective::IndexToId` (`max_values`: None, `max_size`: Some(54), added: 2529, mode: `MaxEncodedLen`) fn exchange_member() -> Weight { - todo!() + // Proof Size summary in bytes: + // Measured: `337` + // Estimated: `6048` + // Minimum execution time: 44_601_000 picoseconds. + Weight::from_parts(45_714_000, 0) + .saturating_add(Weight::from_parts(0, 6048)) + .saturating_add(T::DbWeight::get().reads(6)) + .saturating_add(T::DbWeight::get().writes(10)) } } From fdbbe76af7abed3d59deb2a8c05a2bfd353829a2 Mon Sep 17 00:00:00 2001 From: Damilare Date: Wed, 24 Jan 2024 23:29:03 +0100 Subject: [PATCH 10/18] nit --- .../runtime/rococo/src/weights/pallet_ranked_collective.rs | 2 +- substrate/frame/ranked-collective/src/lib.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs b/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs index 8cef4e99b07d..63c7a1430a80 100644 --- a/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs +++ b/polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs @@ -174,6 +174,6 @@ impl pallet_ranked_collective::WeightInfo for WeightInf } fn exchange_member() -> Weight { - todo!() + todo!() // FAIL-CI } } diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index daee16fe9d50..c24cf742b0a4 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -391,7 +391,7 @@ pub mod pallet { /// maximum rank *from which* the demotion/removal may be. type DemoteOrigin: EnsureOrigin; - /// The origin required to exchange an account for a member. + /// The origin that can swap the account of a member. type ExchangeOrigin: EnsureOrigin; /// The polling system used for our voting. @@ -455,7 +455,8 @@ 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` had their `AccountId` changed to `new_who`. + /// The member `who` has voted for the `poll` with the given `vote` leading to an updated + /// `tally`. Voted { who: T::AccountId, poll: PollIndexOf, vote: VoteRecord, tally: TallyOf }, /// The member `who` had their `AccountId` changed to `new_who`. MemberExchanged { who: T::AccountId, new_who: T::AccountId }, @@ -660,7 +661,7 @@ pub mod pallet { /// Exchanges a member with a new account and the same existing rank. /// - /// - `origin`: Must be the `AdminOrigin`. + /// - `origin`: Must be the `ExchangeOrigin`. /// - `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)] From 99fc1b7385041cd2bec206268b13fa5e15ac7a54 Mon Sep 17 00:00:00 2001 From: Damilare Date: Fri, 26 Jan 2024 00:00:40 +0100 Subject: [PATCH 11/18] nit --- .../collectives/collectives-westend/src/ambassador/mod.rs | 6 ++++++ .../pallet_ranked_collective_ambassador_collective.rs | 3 +++ .../pallet_ranked_collective_fellowship_collective.rs | 3 +++ 3 files changed, 12 insertions(+) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs index 05b3427ef431..30072a759961 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs @@ -99,6 +99,12 @@ pub type PromoteOrigin = EitherOf< >, >; +/// Exchange is by any of: +/// - Root can exchange arbitrarily. +/// - the Fellows origin +pub type ExchangeOrigin = + EitherOf>, Fellows>; + impl pallet_ranked_collective::Config for Runtime { type WeightInfo = weights::pallet_ranked_collective_ambassador_collective::WeightInfo; type RuntimeEvent = RuntimeEvent; diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_ambassador_collective.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_ambassador_collective.rs index a6372c4b89dc..8f6aa583a41c 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_ambassador_collective.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_ambassador_collective.rs @@ -174,4 +174,7 @@ impl pallet_ranked_collective::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into()))) .saturating_add(Weight::from_parts(0, 2550).saturating_mul(n.into())) } + fn exchange_member() -> Weight { + todo!() + } } diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_fellowship_collective.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_fellowship_collective.rs index 9c773c56ac39..6dfe9b88ff63 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_fellowship_collective.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/weights/pallet_ranked_collective_fellowship_collective.rs @@ -173,4 +173,7 @@ impl pallet_ranked_collective::WeightInfo for WeightInf .saturating_add(T::DbWeight::get().writes((1_u64).saturating_mul(n.into()))) .saturating_add(Weight::from_parts(0, 2550).saturating_mul(n.into())) } + fn exchange_member() -> Weight { + todo!() + } } From 5c6b2cf55e7ac52706c7e84ab8a5e6d0b00c5a6f Mon Sep 17 00:00:00 2001 From: Damilare Date: Fri, 26 Jan 2024 00:21:20 +0100 Subject: [PATCH 12/18] nit --- .../collectives/collectives-westend/src/ambassador/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs index 30072a759961..3ba36b4a1c9f 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs @@ -34,6 +34,7 @@ mod tracks; use super::*; use crate::xcm_config::{FellowshipAdminBodyId, LocationToAccountId, WndAssetHub}; use frame_support::traits::{EitherOf, MapSuccess, TryMapSuccess}; +use frame_system::EnsureRootWithSuccess; pub use origins::pallet_origins as pallet_ambassador_origins; use origins::pallet_origins::{ EnsureAmbassadorsVoice, EnsureAmbassadorsVoiceFrom, EnsureHeadAmbassadorsVoice, Origin, @@ -110,6 +111,7 @@ impl pallet_ranked_collective::Config for Runtime type RuntimeEvent = RuntimeEvent; type PromoteOrigin = PromoteOrigin; type DemoteOrigin = DemoteOrigin; + type ExchangeOrigin = ExchangeOrigin; type Polls = AmbassadorReferenda; type MinRankOfClass = sp_runtime::traits::Identity; type VoteWeight = pallet_ranked_collective::Linear; From 0cb7494fa1777e8d02cbca7c02a0508c62de36ff Mon Sep 17 00:00:00 2001 From: Damilare Date: Fri, 26 Jan 2024 00:33:55 +0100 Subject: [PATCH 13/18] nit --- .../collectives-westend/src/fellowship/mod.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs index 273fa6a34150..c25ee4c83743 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -136,6 +136,11 @@ impl pallet_ranked_collective::Config for Runtime Replace>, >, >; + // Exchange is by any of: + // - Root can exchange arbitrarily. + // - the Fellows origin + type ExchangeOrigin = + EitherOf>, Fellows>; type Polls = FellowshipReferenda; type MinRankOfClass = tracks::MinRankOfClass; type VoteWeight = pallet_ranked_collective::Geometric; @@ -194,6 +199,11 @@ impl pallet_core_fellowship::Config for Runtime { >, EnsureCanPromoteTo, >; + // Exchange is by any of: + // - Root can exchange arbitrarily. + // - the Fellows origin + type ExchangeOrigin = + EitherOf>, Fellows>; type EvidenceSize = ConstU32<65536>; } From f58c20a64408980ccd3e2330b4b584ed5b70627b Mon Sep 17 00:00:00 2001 From: Damilare Date: Fri, 26 Jan 2024 00:39:14 +0100 Subject: [PATCH 14/18] nit --- .../collectives/collectives-westend/src/fellowship/mod.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs index c25ee4c83743..0bdb0586d475 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -199,11 +199,6 @@ impl pallet_core_fellowship::Config for Runtime { >, EnsureCanPromoteTo, >; - // Exchange is by any of: - // - Root can exchange arbitrarily. - // - the Fellows origin - type ExchangeOrigin = - EitherOf>, Fellows>; type EvidenceSize = ConstU32<65536>; } From 7ed5a3dafa807c3af5cca2cbbf7bd30127f73fc9 Mon Sep 17 00:00:00 2001 From: Damilare Date: Fri, 26 Jan 2024 01:04:37 +0100 Subject: [PATCH 15/18] nit --- .../collectives/collectives-westend/src/ambassador/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs index 3ba36b4a1c9f..4b46cc9ca2b9 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs @@ -104,7 +104,7 @@ pub type PromoteOrigin = EitherOf< /// - Root can exchange arbitrarily. /// - the Fellows origin pub type ExchangeOrigin = - EitherOf>, Fellows>; + EitherOf>, Fellows>; impl pallet_ranked_collective::Config for Runtime { type WeightInfo = weights::pallet_ranked_collective_ambassador_collective::WeightInfo; From 6e9e14d57b1507eaca2d4b4754139e284e4bcb87 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 26 Jan 2024 15:56:58 +0100 Subject: [PATCH 16/18] fmt Signed-off-by: Oliver Tale-Yazdi --- .../collectives/collectives-westend/src/ambassador/mod.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs index 4b46cc9ca2b9..a73d8a895d87 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs @@ -103,8 +103,7 @@ pub type PromoteOrigin = EitherOf< /// Exchange is by any of: /// - Root can exchange arbitrarily. /// - the Fellows origin -pub type ExchangeOrigin = - EitherOf>, Fellows>; +pub type ExchangeOrigin = EitherOf>, Fellows>; impl pallet_ranked_collective::Config for Runtime { type WeightInfo = weights::pallet_ranked_collective_ambassador_collective::WeightInfo; From 0920307eafa9da0b6e15f20234acad44570ce3e8 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Fri, 26 Jan 2024 15:59:01 +0100 Subject: [PATCH 17/18] Add prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_2587.prdoc | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 prdoc/pr_2587.prdoc diff --git a/prdoc/pr_2587.prdoc b/prdoc/pr_2587.prdoc new file mode 100644 index 000000000000..1ea23d5cca7b --- /dev/null +++ b/prdoc/pr_2587.prdoc @@ -0,0 +1,9 @@ +title: Allow fellowship members to swap their AccountIds. + +doc: + - audience: Runtime User + description: | + Add a `exchange_member` extrinsic to the `ranked-collective` pallet that allows to vote on + swapping the AccountId of a member. This can be used to recover lost access to a collective. + +crates: [ ] From 684beb3e9a107a4b33d8fede1c56b4fe480cad7f Mon Sep 17 00:00:00 2001 From: Damilare Date: Tue, 30 Jan 2024 15:19:17 +0100 Subject: [PATCH 18/18] helper method extra test for exchange origin --- substrate/frame/ranked-collective/src/lib.rs | 20 ++++++++++--------- .../frame/ranked-collective/src/tests.rs | 8 ++++++++ 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index c24cf742b0a4..88631884c5a7 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -552,10 +552,7 @@ pub mod pallet { ensure!(min_rank >= rank, Error::::InvalidWitness); ensure!(max_rank >= rank, Error::::NoPermission); - for r in 0..=rank { - Self::remove_from_rank(&who, r)?; - } - Members::::remove(&who); + Self::do_remove_member_from_rank(&who, rank)?; Self::deposit_event(Event::MemberRemoved { who, rank }); Ok(PostDispatchInfo { actual_weight: Some(T::WeightInfo::remove_member(rank as u32)), @@ -679,11 +676,7 @@ pub mod pallet { let MemberRecord { rank, .. } = Self::ensure_member(&who)?; - for r in 0..=rank { - Self::remove_from_rank(&who, r)?; - } - Members::::remove(&who); - + Self::do_remove_member_from_rank(&who, rank)?; Self::do_add_member_to_rank(new_who.clone(), rank, false)?; Self::deposit_event(Event::MemberExchanged { who, new_who }); @@ -811,6 +804,15 @@ pub mod pallet { use frame_support::traits::CallerTrait; o.as_signed().and_then(Self::rank_of) } + + /// Removes a member from the rank collective + pub fn do_remove_member_from_rank(who: &T::AccountId, rank: Rank) -> DispatchResult { + for r in 0..=rank { + Self::remove_from_rank(&who, r)?; + } + Members::::remove(&who); + Ok(()) + } } impl, I: 'static> RankedMembers for Pallet { diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index 9af23da69128..f5e83c48acd0 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -592,5 +592,13 @@ fn exchange_member_works() { assert_eq!(Members::::get(1), None); assert_eq!(Members::::get(2), Some(member_record)); + + assert_ok!(Club::add_member(RuntimeOrigin::root(), 3)); + assert_ok!(Club::promote_member(RuntimeOrigin::root(), 3)); + + assert_noop!( + Club::exchange_member(RuntimeOrigin::signed(3), 2, 1), + DispatchError::BadOrigin + ); }); }