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

Conversation

PraetorP
Copy link

@PraetorP PraetorP commented Jul 14, 2023

This 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<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>;

To achieve the backward compatibility, the users of the pallet can use the old type via the new morph:

type AddOrigin = MapSuccess<Self::PromoteOrigin, Ignore>;
type RemoveOrigin = Self::DemoteOrigin;

Polkadot companion: paritytech/polkadot#7500
Cumulus companion: paritytech/cumulus#2881

New associated types(`AddOrigin` & `RemoveOrigin`) have been added to `Config`.
@PraetorP PraetorP requested review from a team July 14, 2023 09:57
@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 14, 2023

User @PraetorP, please sign the CLA here.

@@ -538,7 +545,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.

@@ -490,7 +497,7 @@ pub mod pallet {
#[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

primitives/runtime/src/traits.rs Outdated Show resolved Hide resolved
@paritytech-ci paritytech-ci requested a review from a team July 14, 2023 10:27
@ggwpez ggwpez added the T1-runtime This PR/Issue is related to the topic “runtime”. label Jul 14, 2023
/// 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

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.

@paritytech-ci paritytech-ci requested a review from a team July 14, 2023 10:54
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-check-each-crate-macos
Logs: https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/3415827

PraetorP and others added 2 commits August 19, 2023 13:21
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T1-runtime This PR/Issue is related to the topic “runtime”.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

6 participants