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.

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

[core-fellowship] Add permissionless import_member #7030

Merged
merged 16 commits into from
Jan 6, 2025
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,16 @@ impl<T: frame_system::Config> pallet_core_fellowship::WeightInfo for WeightInfo<
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}
// FAIL-CI todo
fn import_member() -> Weight {
// Proof Size summary in bytes:
// Measured: `313`
// Estimated: `3514`
// Minimum execution time: 16_534_000 picoseconds.
Weight::from_parts(17_046_000, 3514)
.saturating_add(T::DbWeight::get().reads(2_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
/// Storage: `AmbassadorCollective::Members` (r:1 w:0)
/// Proof: `AmbassadorCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`)
/// Storage: `AmbassadorCore::Member` (r:1 w:1)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,16 @@ impl<T: frame_system::Config> pallet_core_fellowship::WeightInfo for WeightInfo<
.saturating_add(T::DbWeight::get().reads(2))
.saturating_add(T::DbWeight::get().writes(1))
}
// FAIL-CI todo
fn import_member() -> Weight {
// Proof Size summary in bytes:
// Measured: `313`
// Estimated: `3514`
// Minimum execution time: 16_534_000 picoseconds.
Weight::from_parts(17_046_000, 3514)
.saturating_add(T::DbWeight::get().reads(2_u64))
.saturating_add(T::DbWeight::get().writes(1_u64))
}
/// Storage: `FellowshipCollective::Members` (r:1 w:0)
/// Proof: `FellowshipCollective::Members` (`max_values`: None, `max_size`: Some(42), added: 2517, mode: `MaxEncodedLen`)
/// Storage: `FellowshipCore::Member` (r:1 w:1)
Expand Down
23 changes: 23 additions & 0 deletions prdoc/pr_7030.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
# 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: "[core-fellowship] Add permissionless import_member"

doc:
- audience: [Runtime Dev, Runtime User]
description: |
Changes:
- Add call `import_member` to the core-fellowship pallet.
- Move common logic between `import` and `import_member` into `do_import`.

This is a minor change as to not impact UI and downstream integration.

## `import_member`

Can be used to induct an arbitrary collective member and is callable by any signed origin. Pays no fees upon success.
This is useful in the case that members did not induct themselves and are idling on their rank.


crates:
- name: pallet-core-fellowship
bump: major
17 changes: 17 additions & 0 deletions substrate/frame/core-fellowship/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,23 @@ mod benchmarks {
Ok(())
}

#[benchmark]
fn import_member() -> Result<(), BenchmarkError> {
let member = account("member", 0, SEED);
let sender = account("sender", 0, SEED);

T::Members::induct(&member)?;
T::Members::promote(&member)?;

assert!(!Member::<T, I>::contains_key(&member));

#[extrinsic_call]
_(RawOrigin::Signed(sender), member.clone());

assert!(Member::<T, I>::contains_key(&member));
Ok(())
}

#[benchmark]
fn approve() -> Result<(), BenchmarkError> {
let member = make_member::<T, I>(1)?;
Expand Down
59 changes: 46 additions & 13 deletions substrate/frame/core-fellowship/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -585,28 +585,43 @@
Ok(if replaced { Pays::Yes } else { Pays::No }.into())
}

/// Introduce an already-ranked individual of the collective into this pallet. The rank may
/// still be zero.
/// Introduce an already-ranked individual of the collective into this pallet.
///
/// This resets `last_proof` to the current block and `last_promotion` will be set to zero,
/// thereby delaying any automatic demotion but allowing immediate promotion.
/// The rank may still be zero. This resets `last_proof` to the current block and
/// `last_promotion` will be set to zero, thereby delaying any automatic demotion but
/// allowing immediate promotion.
///
/// - `origin`: A signed origin of a ranked, but not tracked, account.
#[pallet::weight(T::WeightInfo::import())]
#[pallet::call_index(8)]
#[deprecated = "Use `import_member` instead"]
pub fn import(origin: OriginFor<T>) -> DispatchResultWithPostInfo {

Check failure on line 598 in substrate/frame/core-fellowship/src/lib.rs

View workflow job for this annotation

GitHub Actions / cargo-check-all-crate-macos

use of deprecated associated function `pallet::Pallet::<T, I>::import`: Use `import_member` instead
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
let who = ensure_signed(origin)?;
ensure!(!Member::<T, I>::contains_key(&who), Error::<T, I>::AlreadyInducted);
let rank = T::Members::rank_of(&who).ok_or(Error::<T, I>::Unranked)?;
Self::do_import(who)?;

let now = frame_system::Pallet::<T>::block_number();
Member::<T, I>::insert(
&who,
MemberStatus { is_active: true, last_promotion: 0u32.into(), last_proof: now },
);
Self::deposit_event(Event::<T, I>::Imported { who, rank });
Ok(Pays::No.into()) // Successful imports are free
}

Ok(Pays::No.into())
/// Introduce an already-ranked individual of the collective into this pallet.
///
/// The rank may still be zero. Can be called by anyone on any collective member - including
/// the sender.
///
/// This resets `last_proof` to the current block and `last_promotion` will be set to zero,
/// thereby delaying any automatic demotion but allowing immediate promotion.
///
/// - `origin`: A signed origin of a ranked, but not tracked, account.
/// - `who`: The account ID of the collective member to be inducted.
#[pallet::weight(T::WeightInfo::set_partial_params())]
#[pallet::call_index(11)]
pub fn import_member(
origin: OriginFor<T>,
who: T::AccountId,
) -> DispatchResultWithPostInfo {
ensure_signed(origin)?;
Self::do_import(who)?;

Ok(Pays::No.into()) // Successful imports are free
}

/// Set the parameters partially.
Expand Down Expand Up @@ -661,6 +676,24 @@
}
}
}

/// Import `who` into the core-fellowship pallet.
///
/// `who` must be a member of the collective but *not* already imported.
pub(crate) fn do_import(who: T::AccountId) -> DispatchResult {
ensure!(!Member::<T, I>::contains_key(&who), Error::<T, I>::AlreadyInducted);
let rank = T::Members::rank_of(&who).ok_or(Error::<T, I>::Unranked)?;

let now = frame_system::Pallet::<T>::block_number();
Member::<T, I>::insert(
&who,
MemberStatus { is_active: true, last_promotion: 0u32.into(), last_proof: now },
);
Self::deposit_event(Event::<T, I>::Imported { who, rank });

Ok(())
}

/// Convert a rank into a `0..RANK_COUNT` index suitable for the arrays in Params.
///
/// Rank 1 becomes index 0, rank `RANK_COUNT` becomes index `RANK_COUNT - 1`. Any rank not
Expand Down
36 changes: 34 additions & 2 deletions substrate/frame/core-fellowship/src/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
//! Integration test together with the ranked-collective pallet.

use frame_support::{
assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types,
assert_noop, assert_ok, derive_impl, hypothetically, hypothetically_ok, ord_parameter_types,
pallet_prelude::Weight,
parameter_types,
traits::{ConstU16, EitherOf, IsInVec, MapSuccess, NoOpPoll, TryMapSuccess},
Expand Down Expand Up @@ -170,6 +170,37 @@ fn evidence(e: u32) -> Evidence<Test, ()> {
.expect("Static length matches")
}

#[test]
fn import_simple_works() {
new_test_ext().execute_with(|| {
for i in 0u16..9 {
let acc = i as u64;

// Does not work yet
assert_noop!(CoreFellowship::import(signed(acc)), Error::<Test>::Unranked);
assert_noop!(
CoreFellowship::import_member(signed(acc + 1), acc),
Error::<Test>::Unranked
);

assert_ok!(Club::add_member(RuntimeOrigin::root(), acc));
promote_n_times(acc, i);

hypothetically_ok!(CoreFellowship::import(signed(acc)));
hypothetically_ok!(CoreFellowship::import_member(signed(acc), acc));
// Works from other accounts
assert_ok!(CoreFellowship::import_member(signed(acc + 1), acc));

// Does not work again
assert_noop!(CoreFellowship::import(signed(acc)), Error::<Test>::AlreadyInducted);
assert_noop!(
CoreFellowship::import_member(signed(acc + 1), acc),
Error::<Test>::AlreadyInducted
);
}
});
}

#[test]
fn swap_simple_works() {
new_test_ext().execute_with(|| {
Expand All @@ -178,7 +209,8 @@ fn swap_simple_works() {

assert_ok!(Club::add_member(RuntimeOrigin::root(), acc));
promote_n_times(acc, i);
assert_ok!(CoreFellowship::import(signed(acc)));
hypothetically_ok!(CoreFellowship::import(signed(acc)));
assert_ok!(CoreFellowship::import_member(signed(acc), acc));

// Swapping normally works:
assert_ok!(Club::exchange_member(RuntimeOrigin::root(), acc, acc + 10));
Expand Down
60 changes: 60 additions & 0 deletions substrate/frame/core-fellowship/src/tests/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,66 @@ fn set_partial_params_works() {
});
}

#[test]
fn import_member_works() {
new_test_ext().execute_with(|| {
assert_noop!(CoreFellowship::import_member(signed(0), 0), Error::<Test>::Unranked);
assert_noop!(CoreFellowship::import(signed(0)), Error::<Test>::Unranked);

// Make induction work:
set_rank(0, 1);
assert!(!Member::<Test>::contains_key(0), "not yet imported");

// `import_member` can be used to induct ourselves:
hypothetically!({
assert_ok!(CoreFellowship::import_member(signed(0), 0));
assert!(Member::<Test>::contains_key(0), "got imported");

// Twice does not work:
assert_noop!(
CoreFellowship::import_member(signed(0), 0),
Error::<Test>::AlreadyInducted
);
assert_noop!(CoreFellowship::import(signed(0)), Error::<Test>::AlreadyInducted);
});

// But we could have also used `import`:
hypothetically!({
assert_ok!(CoreFellowship::import(signed(0)));
assert!(Member::<Test>::contains_key(0), "got imported");

// Twice does not work:
assert_noop!(
CoreFellowship::import_member(signed(0), 0),
Error::<Test>::AlreadyInducted
);
assert_noop!(CoreFellowship::import(signed(0)), Error::<Test>::AlreadyInducted);
});
});
}

#[test]
fn import_member_same_as_import() {
new_test_ext().execute_with(|| {
for rank in 0..=9 {
set_rank(0, rank);

let import_root = hypothetically!({
assert_ok!(CoreFellowship::import(signed(0)));
sp_io::storage::root(sp_runtime::StateVersion::V1)
});

let import_member_root = hypothetically!({
assert_ok!(CoreFellowship::import_member(signed(1), 0));
sp_io::storage::root(sp_runtime::StateVersion::V1)
});

// `import` and `import_member` do exactly the same thing.
assert_eq!(import_root, import_member_root);
}
});
}

#[test]
fn induct_works() {
new_test_ext().execute_with(|| {
Expand Down
21 changes: 21 additions & 0 deletions substrate/frame/core-fellowship/src/weights.rs

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

Loading