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.

Sign up for GitHub

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

Jump to bottom

Expose add_member_to_rank extrinsic #4778

Closed
wants to merge 1 commit into from
Closed

Expose add_member_to_rank extrinsic #4778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions ...ectives/collectives-westend/src/weights/pallet_ranked_collective_ambassador_collective.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,4 +177,7 @@ impl<T: frame_system::Config> pallet_ranked_collective::WeightInfo for WeightInf
fn exchange_member() -> Weight {
todo!()
}
fn add_member_to_rank(_r: u32, ) -> Weight {
todo!()
}
}
3 changes: 3 additions & 0 deletions ...ectives/collectives-westend/src/weights/pallet_ranked_collective_fellowship_collective.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,4 +176,7 @@ impl<T: frame_system::Config> pallet_ranked_collective::WeightInfo for WeightInf
fn exchange_member() -> Weight {
todo!()
}
fn add_member_to_rank(_r: u32, ) -> Weight {
todo!()
}
}
6 changes: 6 additions & 0 deletions polkadot/runtime/rococo/src/weights/pallet_ranked_collective.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,4 +189,10 @@ impl<T: frame_system::Config> pallet_ranked_collective::WeightInfo for WeightInf
.saturating_add(T::DbWeight::get().reads(6))
.saturating_add(T::DbWeight::get().writes(10))
}
fn add_member_to_rank(r: u32, ) -> Weight {
Weight::from_parts(15_000_000, 3507)
// Standard Error: 1_000
.saturating_add((Weight::from_parts(251_000, 0)).saturating_mul(r.into()))
.saturating_add(T::DbWeight::get().reads(1_u64))
}
}
13 changes: 13 additions & 0 deletions prdoc/pr_4778.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Expose `add_memmber_to_rank`

doc:
- audience: Runtime Dev
description: |
Fixes https://github.com/paritytech/polkadot-sdk/issues/262

crates:
- name: pallet-ranked-collective
bump: patch
18 changes: 16 additions & 2 deletions substrate/frame/ranked-collective/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
// limitations under the License.

//! Staking pallet benchmarking.

use super::*;
#[allow(unused_imports)]
use crate::Pallet as RankedCollective;
Expand All @@ -25,8 +24,8 @@ use frame_benchmarking::v1::{
account, benchmarks_instance_pallet, whitelisted_caller, BenchmarkError,
};
use frame_support::{assert_ok, traits::UnfilteredDispatchable};
use frame_system::RawOrigin as SystemOrigin;

use frame_system::RawOrigin as SystemOrigin;
const SEED: u32 = 0;

fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::RuntimeEvent) {
Expand Down Expand Up @@ -181,5 +180,20 @@ benchmarks_instance_pallet! {
assert_has_event::<T, I>(Event::MemberExchanged { who, new_who }.into());
}

add_member_to_rank {
let r in 0 .. 10;
let rank = r as u16;
let who = account::<T::AccountId>("member-without-rank", 0, SEED);
let who_lookup = T::Lookup::unlookup(who.clone());
let origin =
T::AddOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let call = Call::<T, I>::add_member_to_rank { who: who_lookup, rank };
}: { call.dispatch_bypass_filter(origin)? }
verify {
assert_eq!(Members::<T, I>::get(&who).unwrap().rank, rank);
assert_eq!(MemberCount::<T, I>::get(0),1);
assert_eq!(MemberCount::<T, I>::get(rank),1);
}

impl_benchmark_test_suite!(RankedCollective, crate::tests::ExtBuilder::default().build(), crate::tests::Test);
}
12 changes: 12 additions & 0 deletions substrate/frame/ranked-collective/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -713,6 +713,18 @@ pub mod pallet {

Ok(())
}

#[pallet::call_index(7)]
#[pallet::weight(T::WeightInfo::add_member_to_rank(*rank as u32))]
pub fn add_member_to_rank(
origin: OriginFor<T>,
who: AccountIdLookupOf<T>,
rank: Rank,
) -> DispatchResult {
Comment on lines +719 to +723
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't we need a similar call in core-fellowship? Or will this somehow bypass that configuration?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know. @ggwpez could you please take a look at PR and maybe help to answer this question. Thank you)

Copy link
Member

Choose a reason for hiding this comment

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

I think we should have only one in the core-fellowship pallet. Similar to the induct function there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggwpez few questions:

  1. Does it make sense to make another call similar to promote?
  2. Maybe it's better to close Fellowship: Expose add_member_to_rank #262 if the issue task is no longer valid?

Copy link
Contributor

Choose a reason for hiding this comment

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

The task is definitely valid, it just does not prescribe a solution.

A promote_to function could also be useful for people already inducted but where it makes sense for some reason to promote them over several ranks.

Copy link
Contributor Author

@mateo-moon mateo-moon Jun 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The task is definitely valid, it just does not prescribe a solution.

A promote_to function could also be useful for people already inducted but where it makes sense for some reason to promote them over several ranks.

But this violate the rule of mainefest 4.2.3: "...associated rank is incremented by one". I suppose that it is not possible to increment rank more than 1 at a time. Or am i missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Two things:

One, the manifesto is from the Tech Fellowship. It is not the only collective (see, e.g., the Ambassador collective, which actually requires promoting members by 3 steps). If the Fellowship never wants someone promoted by more than one rank at a time they will need to configure things as such.

Two, we use origins to control access to privileged functions, and typically origins that correspond to public tracks will always outweigh any internal management origins.

What I mean is that rules like that are mainly arbitrary and can always be superseded by governance. You could also do a runtime upgrade or referendum to set_storage to get the same effect. The goal of this task is to provide a transparent, non-footgun way of accomplishing this effect.

Things like manifestos and white papers are guiding principles but they shouldn't trump pragmatism. The JAM prize, which the Fellowship also agreed to steward, has conditions like automatic promotion to X rank for certain accomplishments. Sure, the manifesto has that clause and it can be upheld by the members and enforced by origin configuration, but there are obvious pragmatic shortfalls of the code in some circumstances that led to the creation of this feature request.

T::AddOrigin::ensure_origin(origin)?;
let who = T::Lookup::lookup(who)?;
Self::do_add_member_to_rank(who, rank, true)
}
}

#[pallet::hooks]
Expand Down
17 changes: 17 additions & 0 deletions substrate/frame/ranked-collective/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,3 +645,20 @@ fn exchange_member_same_noops() {
);
});
}

#[test]
pub fn add_member_to_rank_work() {
ExtBuilder::default().build_and_execute(|| {
let max_rank = 4u16;
assert_ok!(Club::add_member_to_rank(RuntimeOrigin::root(), 2, max_rank));
for i in 0..=max_rank {
assert_eq!(member_count(i), 1);
}

//-- Should fail ----------------------------------------
assert_noop!(
Club::add_member_to_rank(RuntimeOrigin::signed(1), 2, 1),
DispatchError::BadOrigin
);
})
}
50 changes: 50 additions & 0 deletions substrate/frame/ranked-collective/src/weights.rs
View file
Open in desktop

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

Loading