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

feat(pallet-ranked-collective): added types #14577

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

feat(pallet-ranked-collective): added types #14577

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
74254e9
feat(pallet-ranked-collective): added types
PraetorP Jul 13, 2023
837cbac
Merge remote-tracking branch 'origin/master' into ranked-collective-c…
PraetorP Jul 14, 2023
4d4711b
doc(pallet-ranked-collective): update fn docs
PraetorP Jul 14, 2023
b2ac5d1
Update primitives/runtime/src/traits.rs
PraetorP Jul 14, 2023
5d1b180
test(pallet-ranked-collective): switch to `Ignore`
PraetorP Jul 14, 2023
e1f8af4
Update primitives/runtime/src/traits.rs
PraetorP Aug 19, 2023
2261fb0
Revert "Update primitives/runtime/src/traits.rs"
PraetorP Aug 19, 2023
9137fb3
Update primitives/runtime/src/traits.rs
PraetorP Aug 22, 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
2 changes: 2 additions & 0 deletions bin/node/runtime/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,6 +924,8 @@ impl pallet_referenda::Config<pallet_referenda::Instance2> for Runtime {
impl pallet_ranked_collective::Config for Runtime {
type WeightInfo = pallet_ranked_collective::weights::SubstrateWeight<Self>;
type RuntimeEvent = RuntimeEvent;
type AddOrigin = EnsureRoot<AccountId>;
type RemoveOrigin = EnsureRootWithSuccess<AccountId, ConstU16<65535>>;
type PromoteOrigin = EnsureRootWithSuccess<AccountId, ConstU16<65535>>;
type DemoteOrigin = EnsureRootWithSuccess<AccountId, ConstU16<65535>>;
type Polls = RankedPolls;
Expand Down
12 changes: 6 additions & 6 deletions frame/ranked-collective/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ fn make_member<T: Config<I>, I: 'static>(rank: Rank) -> T::AccountId {
let who = account::<T::AccountId>("member", MemberCount::<T, I>::get(0), SEED);
let who_lookup = T::Lookup::unlookup(who.clone());
assert_ok!(Pallet::<T, I>::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 {
Expand All @@ -56,7 +56,7 @@ benchmarks_instance_pallet! {
let who = account::<T::AccountId>("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::<T, I>::add_member { who: who_lookup };
}: { call.dispatch_bypass_filter(origin)? }
verify {
Expand All @@ -73,7 +73,7 @@ benchmarks_instance_pallet! {
let last = make_member::<T, I>(rank);
let last_index = (0..=rank).map(|r| IdToIndex::<T, I>::get(r, &last).unwrap()).collect::<Vec<_>>();
let origin =
T::DemoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
T::RemoveOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let call = Call::<T, I>::remove_member { who: who_lookup, min_rank: rank };
}: { call.dispatch_bypass_filter(origin)? }
verify {
Expand Down Expand Up @@ -124,8 +124,8 @@ benchmarks_instance_pallet! {
let caller: T::AccountId = whitelisted_caller();
let caller_lookup = T::Lookup::unlookup(caller.clone());
assert_ok!(Pallet::<T, I>::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"),
caller_lookup.clone(),
));
// Create a poll
Expand Down
26 changes: 16 additions & 10 deletions frame/ranked-collective/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,12 +382,19 @@ pub mod pallet {
type RuntimeEvent: From<Event<Self, I>>
+ IsType<<Self as frame_system::Config>::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<Self::RuntimeOrigin, Success = ()>;

/// The origin required to remove a member. The success value indicates the
/// maximum rank *from which* the removal may be.
type RemoveOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Rank>;

/// The origin required to promote a member. The success value indicates the
/// maximum rank *to which* the promotion may be.
type PromoteOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = Rank>;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it improve flexibility?
PromoteOrigin returns the Rank, if it returns 0, it can only add_member.
Same with DemoteOrigin.

Copy link
Author

Choose a reason for hiding this comment

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

The AddOrigin allows a chain to use a different (from the PromoteOrigin) authority to add members. E.g., entering the ranked collective could be managed separately from promoting inside it. The same with the RemoveOrigin.

Copy link
Contributor

@muharem muharem Jul 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

you can do the same with only PromoteOrigin since it returns Rank.
for your add origin the promote origin type will return rank 0 hence it can only add.

Copy link
Author

Choose a reason for hiding this comment

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

In current impl where only PromoteOrigin exists, one could add a dedicated origin with a 0 rank to the PromoteOrigin to add new members. However, anyone who can promote can also add a new member in this case, not only our dedicated origin. So we can't express the setting where one can promote an existing member but can't add a new one.


/// 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<Self::RuntimeOrigin, Success = Rank>;

/// The polling system used for our voting.
Expand Down Expand Up @@ -482,22 +489,21 @@ pub mod pallet {
impl<T: Config<I>, I: 'static> Pallet<T, I> {
/// 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<T>, who: AccountIdLookupOf<T>) -> DispatchResult {
let _ = T::PromoteOrigin::ensure_origin(origin)?;
T::AddOrigin::ensure_origin(origin)?;
Copy link
Member

Choose a reason for hiding this comment

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

Function docs needs to be updated.

Copy link
Author

Choose a reason for hiding this comment

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

Done. + Refresh docs to some other functions

let who = T::Lookup::lookup(who)?;
Self::do_add_member(who)
}

/// 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)`
Expand All @@ -512,7 +518,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.
Expand All @@ -526,7 +532,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.
///
Expand All @@ -538,7 +544,7 @@ pub mod pallet {
who: AccountIdLookupOf<T>,
min_rank: Rank,
) -> DispatchResultWithPostInfo {
let max_rank = T::DemoteOrigin::ensure_origin(origin)?;
let max_rank = T::RemoveOrigin::ensure_origin(origin)?;
Copy link
Member

Choose a reason for hiding this comment

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

The function doc mentions AdminOrigin.

Copy link
Author

Choose a reason for hiding this comment

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

Done.

let who = T::Lookup::lookup(who)?;
let MemberRecord { rank, .. } = Self::ensure_member(&who)?;
ensure!(min_rank >= rank, Error::<T, I>::InvalidWitness);
Expand Down
4 changes: 3 additions & 1 deletion frame/ranked-collective/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ use frame_support::{
};
use sp_core::{Get, H256};
use sp_runtime::{
traits::{BlakeTwo256, IdentityLookup, ReduceBy},
traits::{BlakeTwo256, IdentityLookup, Ignore, ReduceBy},
BuildStorage,
};

Expand Down Expand Up @@ -176,6 +176,8 @@ parameter_types! {
impl Config for Test {
type WeightInfo = ();
type RuntimeEvent = RuntimeEvent;
type AddOrigin = MapSuccess<Self::PromoteOrigin, Ignore>;
type RemoveOrigin = Self::DemoteOrigin;
type PromoteOrigin = EitherOf<
// Root can promote arbitrarily.
frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
Expand Down
3 changes: 3 additions & 0 deletions primitives/runtime/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,9 @@ morph_types! {
/// Morpher to disregard the source value and replace with another.
pub type Replace<V: TypedGet> = |_| -> V::Type { V::get() };

/// Morpher to disregard the source value.
pub type Ignore = |_| -> () { () };
PraetorP marked this conversation as resolved.
Show resolved Hide resolved

/// Mutator which reduces a scalar by a particular amount.
pub type ReduceBy<N: TypedGet> = |r: N::Type| -> N::Type {
r.checked_sub(&N::get()).unwrap_or(Zero::zero())
Expand Down