From c552fb5495d28ef29a2da62e54cf7e0dedf776d0 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Tue, 6 Feb 2024 13:45:40 +0100 Subject: [PATCH] Ranked collective `Add`+`Remove` origins (#3212) Superseeds https://github.com/paritytech/polkadot-sdk/pull/1245 This PR is a migration of the https://github.com/paritytech/substrate/pull/14577. The PR added associated types (`AddOrigin` & `RemoveOrigin`) to `Config`. It allows you to decouple types and areas of responsibility, since at the moment the same types are responsible for adding and promoting(removing and demoting). This will improve the flexibility of the pallet configuration. ``` /// The origin required to add a member. type AddOrigin: EnsureOrigin; /// The origin required to remove a member. The success value indicates the /// maximum rank *from which* the removal may be. type RemoveOrigin: EnsureOrigin; ``` To achieve the backward compatibility, the users of the pallet can use the old type via the new morph: ``` type AddOrigin = MapSuccess; type RemoveOrigin = Self::DemoteOrigin; ``` --------- Signed-off-by: Oliver Tale-Yazdi Co-authored-by: PraetorP Co-authored-by: Pavel Orlov <45266194+PraetorP@users.noreply.github.com> --- .../collectives-westend/src/ambassador/mod.rs | 4 ++- .../collectives-westend/src/fellowship/mod.rs | 10 +++++-- .../rococo/src/governance/fellowship.rs | 12 ++++++++- prdoc/pr_3212.prdoc | 9 +++++++ substrate/bin/node/runtime/src/lib.rs | 2 ++ .../core-fellowship/src/tests/integration.rs | 4 ++- .../ranked-collective/src/benchmarking.rs | 26 +++++------------- substrate/frame/ranked-collective/src/lib.rs | 27 ++++++++++++------- .../frame/ranked-collective/src/tests.rs | 7 ++++- .../frame/salary/src/tests/integration.rs | 4 ++- substrate/primitives/runtime/src/traits.rs | 3 +++ 11 files changed, 72 insertions(+), 36 deletions(-) create mode 100644 prdoc/pr_3212.prdoc 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 d9042f2a55..0c9f428c13 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/ambassador/mod.rs @@ -40,7 +40,7 @@ use origins::pallet_origins::{ EnsureAmbassadorsVoice, EnsureAmbassadorsVoiceFrom, EnsureHeadAmbassadorsVoice, Origin, }; use sp_core::ConstU128; -use sp_runtime::traits::{CheckedReduceBy, ConstU16, ConvertToValue, Replace}; +use sp_runtime::traits::{CheckedReduceBy, ConstU16, ConvertToValue, Replace, ReplaceWithDefault}; use xcm::prelude::*; use xcm_builder::{AliasesIntoAccountId32, PayOverXcm}; @@ -108,8 +108,10 @@ pub type ExchangeOrigin = EitherOf for Runtime { type WeightInfo = weights::pallet_ranked_collective_ambassador_collective::WeightInfo; type RuntimeEvent = RuntimeEvent; + type AddOrigin = MapSuccess>; type PromoteOrigin = PromoteOrigin; type DemoteOrigin = DemoteOrigin; + type RemoveOrigin = Self::DemoteOrigin; type ExchangeOrigin = ExchangeOrigin; type Polls = AmbassadorReferenda; type MinRankOfClass = sp_runtime::traits::Identity; 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 6632d511f7..3816d2ed84 100644 --- a/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs +++ b/cumulus/parachains/runtimes/collectives/collectives-westend/src/fellowship/mod.rs @@ -113,12 +113,17 @@ impl pallet_ranked_collective::Config for Runtime type WeightInfo = weights::pallet_ranked_collective_fellowship_collective::WeightInfo; type RuntimeEvent = RuntimeEvent; - #[cfg(not(feature = "runtime-benchmarks"))] // Promotions and the induction of new members are serviced by `FellowshipCore` pallet instance. - type PromoteOrigin = frame_system::EnsureNever; + #[cfg(not(feature = "runtime-benchmarks"))] + type AddOrigin = frame_system::EnsureNever<()>; #[cfg(feature = "runtime-benchmarks")] + type AddOrigin = frame_system::EnsureRoot; + // The maximum value of `u16` set as a success value for the root to ensure the benchmarks will // pass. + #[cfg(not(feature = "runtime-benchmarks"))] + type PromoteOrigin = frame_system::EnsureNever; + #[cfg(feature = "runtime-benchmarks")] type PromoteOrigin = EnsureRootWithSuccess>; // Demotion is by any of: @@ -127,6 +132,7 @@ impl pallet_ranked_collective::Config for Runtime // // The maximum value of `u16` set as a success value for the root to ensure the benchmarks will // pass. + type RemoveOrigin = Self::DemoteOrigin; type DemoteOrigin = EitherOf< EnsureRootWithSuccess>, MapSuccess< diff --git a/polkadot/runtime/rococo/src/governance/fellowship.rs b/polkadot/runtime/rococo/src/governance/fellowship.rs index 9ac47e008a..a589b768af 100644 --- a/polkadot/runtime/rococo/src/governance/fellowship.rs +++ b/polkadot/runtime/rococo/src/governance/fellowship.rs @@ -17,7 +17,7 @@ //! Elements of governance concerning the Rococo Fellowship. use frame_support::traits::{MapSuccess, TryMapSuccess}; -use sp_runtime::traits::{CheckedReduceBy, ConstU16, Replace}; +use sp_runtime::traits::{CheckedReduceBy, ConstU16, Replace, ReplaceWithDefault}; use super::*; use crate::{CENTS, DAYS}; @@ -315,6 +315,11 @@ pub type FellowshipCollectiveInstance = pallet_ranked_collective::Instance1; impl pallet_ranked_collective::Config for Runtime { type WeightInfo = weights::pallet_ranked_collective::WeightInfo; type RuntimeEvent = RuntimeEvent; + // Adding is by any of: + // - Root. + // - the FellowshipAdmin origin. + // - a Fellowship origin. + type AddOrigin = MapSuccess>; // Promotion is by any of: // - Root can demote arbitrarily. // - the FellowshipAdmin origin (i.e. token holder referendum); @@ -326,6 +331,11 @@ impl pallet_ranked_collective::Config for Runtime TryMapSuccess>>, >, >; + // Removing is by any of: + // - Root can remove arbitrarily. + // - the FellowshipAdmin origin (i.e. token holder referendum); + // - a vote by the rank two above the current rank. + type RemoveOrigin = Self::DemoteOrigin; // Demotion is by any of: // - Root can demote arbitrarily. // - the FellowshipAdmin origin (i.e. token holder referendum); diff --git a/prdoc/pr_3212.prdoc b/prdoc/pr_3212.prdoc new file mode 100644 index 0000000000..a9de1e28cd --- /dev/null +++ b/prdoc/pr_3212.prdoc @@ -0,0 +1,9 @@ +title: "Ranked collective introduce `Add` and `Remove` origins" + +doc: + - audience: Runtime Dev + description: | + Add two new origins to the ranked-collective pallet. One to add new members and one to remove members, named `AddOrigin` and `RemoveOrigin` respectively. + +crates: + - name: pallet-ranked-collective diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index 02d084c6c6..515272cd99 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -1013,6 +1013,8 @@ impl pallet_referenda::Config for Runtime { impl pallet_ranked_collective::Config for Runtime { type WeightInfo = pallet_ranked_collective::weights::SubstrateWeight; type RuntimeEvent = RuntimeEvent; + type AddOrigin = EnsureRoot; + type RemoveOrigin = Self::DemoteOrigin; type PromoteOrigin = EnsureRootWithSuccess>; type DemoteOrigin = EnsureRootWithSuccess>; type ExchangeOrigin = EnsureRootWithSuccess>; diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs index 57f9cad3dc..6f177ba66d 100644 --- a/substrate/frame/core-fellowship/src/tests/integration.rs +++ b/substrate/frame/core-fellowship/src/tests/integration.rs @@ -27,7 +27,7 @@ use frame_system::EnsureSignedBy; use pallet_ranked_collective::{EnsureRanked, Geometric, Rank, TallyOf, Votes}; use sp_core::Get; use sp_runtime::{ - traits::{Convert, ReduceBy, TryMorphInto}, + traits::{Convert, ReduceBy, ReplaceWithDefault, TryMorphInto}, BuildStorage, DispatchError, }; type Class = Rank; @@ -137,12 +137,14 @@ impl pallet_ranked_collective::Config for Test { // Members can promote up to the rank of 2 below them. MapSuccess, ReduceBy>>, >; + type AddOrigin = MapSuccess>; type DemoteOrigin = EitherOf< // Root can demote arbitrarily. frame_system::EnsureRootWithSuccess>, // Members can demote up to the rank of 3 below them. MapSuccess, ReduceBy>>, >; + type RemoveOrigin = Self::DemoteOrigin; type ExchangeOrigin = EitherOf< // Root can exchange arbitrarily. frame_system::EnsureRootWithSuccess>, diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 332d8292e5..f093fd93e6 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -41,8 +41,8 @@ fn make_member, I: 'static>(rank: Rank) -> T::AccountId { let who = account::("member", MemberCount::::get(0), SEED); let who_lookup = T::Lookup::unlookup(who.clone()); assert_ok!(Pallet::::add_member( - T::PromoteOrigin::try_successful_origin() - .expect("PromoteOrigin has no successful origin required for the benchmark"), + T::AddOrigin::try_successful_origin() + .expect("AddOrigin has no successful origin required for the benchmark"), who_lookup.clone(), )); for _ in 0..rank { @@ -60,7 +60,7 @@ benchmarks_instance_pallet! { let who = account::("member", 0, SEED); let who_lookup = T::Lookup::unlookup(who.clone()); let origin = - T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + T::AddOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let call = Call::::add_member { who: who_lookup }; }: { call.dispatch_bypass_filter(origin)? } verify { @@ -77,7 +77,7 @@ benchmarks_instance_pallet! { let last = make_member::(rank); let last_index = (0..=rank).map(|r| IdToIndex::::get(r, &last).unwrap()).collect::>(); let origin = - T::DemoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + T::RemoveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; let call = Call::::remove_member { who: who_lookup, min_rank: rank }; }: { call.dispatch_bypass_filter(origin)? } verify { @@ -125,23 +125,11 @@ benchmarks_instance_pallet! { } vote { - let caller: T::AccountId = whitelisted_caller(); - let caller_lookup = T::Lookup::unlookup(caller.clone()); - assert_ok!(Pallet::::add_member( - T::PromoteOrigin::try_successful_origin() - .expect("PromoteOrigin has no successful origin required for the benchmark"), - caller_lookup.clone(), - )); - // Create a poll let class = T::Polls::classes().into_iter().next().unwrap(); let rank = T::MinRankOfClass::convert(class.clone()); - for _ in 0..rank { - assert_ok!(Pallet::::promote_member( - T::PromoteOrigin::try_successful_origin() - .expect("PromoteOrigin has no successful origin required for the benchmark"), - caller_lookup.clone(), - )); - } + + let caller = make_member::(rank); + let caller_lookup = T::Lookup::unlookup(caller.clone()); let poll = T::Polls::create_ongoing(class).expect("Must always be able to create a poll for rank 0"); diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index d0303fee0b..65ea886acc 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -393,12 +393,20 @@ pub mod pallet { type RuntimeEvent: From> + IsType<::RuntimeEvent>; - /// The origin required to add or promote a mmember. The success value indicates the + /// The origin required to add a member. + type AddOrigin: EnsureOrigin; + + /// The origin required to remove a member. + /// + /// The success value indicates the maximum rank *from which* the removal may be. + type RemoveOrigin: EnsureOrigin; + + /// The origin required to promote a member. The success value indicates the /// maximum rank *to which* the promotion may be. type PromoteOrigin: EnsureOrigin; - /// The origin required to demote or remove a member. The success value indicates the - /// maximum rank *from which* the demotion/removal may be. + /// The origin required to demote a member. The success value indicates the + /// maximum rank *from which* the demotion may be. type DemoteOrigin: EnsureOrigin; /// The origin that can swap the account of a member. @@ -510,22 +518,21 @@ pub mod pallet { impl, I: 'static> Pallet { /// Introduce a new member. /// - /// - `origin`: Must be the `AdminOrigin`. + /// - `origin`: Must be the `AddOrigin`. /// - `who`: Account of non-member which will become a member. - /// - `rank`: The rank to give the new member. /// /// Weight: `O(1)` #[pallet::call_index(0)] #[pallet::weight(T::WeightInfo::add_member())] pub fn add_member(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResult { - let _ = T::PromoteOrigin::ensure_origin(origin)?; + T::AddOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; Self::do_add_member(who, true) } /// Increment the rank of an existing member by one. /// - /// - `origin`: Must be the `AdminOrigin`. + /// - `origin`: Must be the `PromoteOrigin`. /// - `who`: Account of existing member. /// /// Weight: `O(1)` @@ -540,7 +547,7 @@ pub mod pallet { /// Decrement the rank of an existing member by one. If the member is already at rank zero, /// then they are removed entirely. /// - /// - `origin`: Must be the `AdminOrigin`. + /// - `origin`: Must be the `DemoteOrigin`. /// - `who`: Account of existing member of rank greater than zero. /// /// Weight: `O(1)`, less if the member's index is highest in its rank. @@ -554,7 +561,7 @@ pub mod pallet { /// Remove the member entirely. /// - /// - `origin`: Must be the `AdminOrigin`. + /// - `origin`: Must be the `RemoveOrigin`. /// - `who`: Account of existing member of rank greater than zero. /// - `min_rank`: The rank of the member or greater. /// @@ -566,7 +573,7 @@ pub mod pallet { who: AccountIdLookupOf, min_rank: Rank, ) -> DispatchResultWithPostInfo { - let max_rank = T::DemoteOrigin::ensure_origin(origin)?; + let max_rank = T::RemoveOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; let MemberRecord { rank, .. } = Self::ensure_member(&who)?; ensure!(min_rank >= rank, Error::::InvalidWitness); diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index 58e21ded35..c5fccd3f77 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -26,7 +26,10 @@ use frame_support::{ traits::{ConstU16, EitherOf, MapSuccess, Polling}, }; use sp_core::Get; -use sp_runtime::{traits::ReduceBy, BuildStorage}; +use sp_runtime::{ + traits::{ReduceBy, ReplaceWithDefault}, + BuildStorage, +}; use super::*; use crate as pallet_ranked_collective; @@ -152,6 +155,8 @@ parameter_types! { impl Config for Test { type WeightInfo = (); type RuntimeEvent = RuntimeEvent; + type AddOrigin = MapSuccess>; + type RemoveOrigin = Self::DemoteOrigin; type PromoteOrigin = EitherOf< // Root can promote arbitrarily. frame_system::EnsureRootWithSuccess>, diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs index bf1e82b028..a49b5637b8 100644 --- a/substrate/frame/salary/src/tests/integration.rs +++ b/substrate/frame/salary/src/tests/integration.rs @@ -26,7 +26,7 @@ use frame_support::{ use pallet_ranked_collective::{EnsureRanked, Geometric, TallyOf, Votes}; use sp_core::{ConstU16, Get}; use sp_runtime::{ - traits::{Convert, ReduceBy}, + traits::{Convert, ReduceBy, ReplaceWithDefault}, BuildStorage, DispatchError, }; @@ -162,12 +162,14 @@ impl pallet_ranked_collective::Config for Test { // Members can promote up to the rank of 2 below them. MapSuccess, ReduceBy>>, >; + type AddOrigin = MapSuccess>; type DemoteOrigin = EitherOf< // Root can demote arbitrarily. frame_system::EnsureRootWithSuccess>, // Members can demote up to the rank of 3 below them. MapSuccess, ReduceBy>>, >; + type RemoveOrigin = Self::DemoteOrigin; type ExchangeOrigin = EitherOf< // Root can exchange arbitrarily. frame_system::EnsureRootWithSuccess>, diff --git a/substrate/primitives/runtime/src/traits.rs b/substrate/primitives/runtime/src/traits.rs index 4213117334..caede5e2b5 100644 --- a/substrate/primitives/runtime/src/traits.rs +++ b/substrate/primitives/runtime/src/traits.rs @@ -540,6 +540,9 @@ morph_types! { /// Morpher to disregard the source value and replace with another. pub type Replace = |_| -> V::Type { V::get() }; + /// Morpher to disregard the source value and replace with the default of `V`. + pub type ReplaceWithDefault = |_| -> V { Default::default() }; + /// Mutator which reduces a scalar by a particular amount. pub type ReduceBy = |r: N::Type| -> N::Type { r.checked_sub(&N::get()).unwrap_or(Zero::zero())