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

Fellowship-core: add fast promote #4877

Merged
merged 19 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,16 @@ impl<T: frame_system::Config> pallet_core_fellowship::WeightInfo for WeightInfo<
.saturating_add(T::DbWeight::get().reads(5))
.saturating_add(T::DbWeight::get().writes(6))
}
fn promote_fast(r: u32) -> Weight {
// FAIL-CI
// Proof Size summary in bytes:
// Measured: `16931`
// Estimated: `19894`
// Minimum execution time: 55_062_000 picoseconds.
Weight::from_parts(58_422_000, 19894)
.saturating_add(T::DbWeight::get().reads(5_u64))
.saturating_add(T::DbWeight::get().writes(6_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 @@ -170,6 +170,16 @@ impl<T: frame_system::Config> pallet_core_fellowship::WeightInfo for WeightInfo<
.saturating_add(T::DbWeight::get().reads(5))
.saturating_add(T::DbWeight::get().writes(6))
}
fn promote_fast(r: u32) -> Weight {
// FAIL-CI
// Proof Size summary in bytes:
// Measured: `16931`
// Estimated: `19894`
// Minimum execution time: 55_062_000 picoseconds.
Weight::from_parts(58_422_000, 19894)
.saturating_add(T::DbWeight::get().reads(5_u64))
.saturating_add(T::DbWeight::get().writes(6_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
11 changes: 11 additions & 0 deletions prdoc/pr_4877.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
title: "Core-Fellowship: new promote_fast call"

doc:
- audience: Runtime User
description: |
Adds the ability quickly promote someone within a collective by bypassing the promotion
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
cooldown. This can help in special situations and comes with a new origin: `FastPromoteOrigin`.

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

/// Benchmark the `promote_fast` extrinsic to promote someone up to `r`.
#[benchmark]
fn promote_fast(r: Linear<1, { T::MaxRank::get() as u32 }>) -> Result<(), BenchmarkError> {
let r = r.try_into().expect("r is too large");
let member = make_member::<T, I>(0)?;

ensure_evidence::<T, I>(&member)?;

#[extrinsic_call]
_(RawOrigin::Root, member.clone(), r);

assert_eq!(T::Members::rank_of(&member), Some(r));
assert!(!MemberEvidence::<T, I>::contains_key(&member));
Ok(())
}

#[benchmark]
fn offboard() -> Result<(), BenchmarkError> {
let member = make_member::<T, I>(0)?;
Expand Down
42 changes: 42 additions & 0 deletions substrate/frame/core-fellowship/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,10 @@ pub mod pallet {
/// rank to which it can promote.
type PromoteOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = RankOf<Self, I>>;

/// The origin which has permission to "fast" promote a member by ignoring promotion periods
ggwpez marked this conversation as resolved.
Show resolved Hide resolved
/// and skipping ranks. The `Success` value is the maximum rank to which it can promote.
type FastPromoteOrigin: EnsureOrigin<Self::RuntimeOrigin, Success = RankOf<Self, I>>;

/// The maximum size in bytes submitted evidence is allowed to be.
#[pallet::constant]
type EvidenceSize: Get<u32>;
Expand Down Expand Up @@ -498,6 +502,44 @@ pub mod pallet {
Ok(())
}

/// Fast promotions can skip ranks and ignore the `min_promotion_period`.
///
/// This is useful for out-of-band promotions, hence it has its own `FastPromoteOrigin` to
/// be (possibly) more restrictive than `PromoteOrigin`. Note that the member must already
/// be inducted.
#[pallet::weight(T::WeightInfo::promote_fast(*to_rank as u32))]
#[pallet::call_index(10)]
pub fn promote_fast(
origin: OriginFor<T>,
who: T::AccountId,
to_rank: RankOf<T, I>,
) -> DispatchResult {
match T::FastPromoteOrigin::try_origin(origin) {
Ok(allow_rank) => ensure!(allow_rank >= to_rank, Error::<T, I>::NoPermission),
Err(origin) => ensure_root(origin)?,
}
ensure!(to_rank as u32 <= T::MaxRank::get(), Error::<T, I>::InvalidRank);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe refactor those into a helper function so it can be shared with promote

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes could do, but it needs to go into the release today and its already approved 🫠

let curr_rank = T::Members::rank_of(&who).ok_or(Error::<T, I>::Unranked)?;
ensure!(to_rank > curr_rank, Error::<T, I>::UnexpectedRank);

let mut member = Member::<T, I>::get(&who).ok_or(Error::<T, I>::NotTracked)?;
let now = frame_system::Pallet::<T>::block_number();
member.last_promotion = now;
member.last_proof = now;

for rank in (curr_rank + 1)..=to_rank {
T::Members::promote(&who)?;

// NOTE: We could factor this out, but it would destroy our invariants:
Member::<T, I>::insert(&who, &member);

Self::dispose_evidence(who.clone(), rank.saturating_sub(1), Some(rank));
Self::deposit_event(Event::<T, I>::Promoted { who: who.clone(), to_rank: rank });
}

Ok(())
}

/// Stop tracking a prior member who is now not a ranked member of the collective.
///
/// - `origin`: A `Signed` origin of an account.
Expand Down
1 change: 1 addition & 0 deletions substrate/frame/core-fellowship/src/tests/integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ impl Config for Test {
type InductOrigin = EnsureInducted<Test, (), 1>;
type ApproveOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type PromoteOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type FastPromoteOrigin = Self::PromoteOrigin;
type EvidenceSize = EvidenceSize;
type MaxRank = ConstU32<9>;
}
Expand Down
96 changes: 95 additions & 1 deletion substrate/frame/core-fellowship/src/tests/unit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::collections::BTreeMap;

use core::cell::RefCell;
use frame_support::{
assert_noop, assert_ok, derive_impl, ord_parameter_types,
assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types,
pallet_prelude::Weight,
parameter_types,
traits::{tokens::GetSalary, ConstU32, IsInVec, TryMapSuccess},
Expand Down Expand Up @@ -115,6 +115,7 @@ impl Config for Test {
type InductOrigin = EnsureInducted<Test, (), 1>;
type ApproveOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type PromoteOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type FastPromoteOrigin = Self::PromoteOrigin;
type EvidenceSize = ConstU32<1024>;
type MaxRank = ConstU32<9>;
}
Expand Down Expand Up @@ -256,6 +257,99 @@ fn promote_works() {
});
}

#[test]
fn promote_fast_works() {
let alice = 1;

new_test_ext().execute_with(|| {
assert_noop!(
CoreFellowship::promote_fast(signed(alice), alice, 1),
Error::<Test>::Unranked
);
set_rank(alice, 0);
assert_noop!(
CoreFellowship::promote_fast(signed(alice), alice, 1),
Error::<Test>::NotTracked
);
assert_ok!(CoreFellowship::import(signed(alice)));

// Cannot fast promote to the same rank:
assert_noop!(
CoreFellowship::promote_fast(signed(alice), alice, 0),
Error::<Test>::UnexpectedRank
);
assert_ok!(CoreFellowship::promote_fast(signed(alice), alice, 1));
assert_eq!(TestClub::rank_of(&alice), Some(1));

// Cannot promote normally because of the period:
assert_noop!(CoreFellowship::promote(signed(2), alice, 2), Error::<Test>::TooSoon);
// But can fast promote:
assert_ok!(CoreFellowship::promote_fast(signed(2), alice, 2));
assert_eq!(TestClub::rank_of(&alice), Some(2));

// Cannot promote to lower rank:
assert_noop!(
CoreFellowship::promote_fast(signed(alice), alice, 0),
Error::<Test>::UnexpectedRank
);
assert_noop!(
CoreFellowship::promote_fast(signed(alice), alice, 1),
Error::<Test>::UnexpectedRank
);
// Permission is checked:
assert_noop!(
CoreFellowship::promote_fast(signed(alice), alice, 2),
Error::<Test>::NoPermission
);

// Can fast promote up to the maximum:
assert_ok!(CoreFellowship::promote_fast(signed(9), alice, 9));
// But not past the maximum:
assert_noop!(
CoreFellowship::promote_fast(RuntimeOrigin::root(), alice, 10),
Error::<Test>::InvalidRank
);
});
}

/// Compare the storage root hashes of a normal promote and a fast promote.
#[test]
fn promote_fast_identical_to_promote() {
let alice = 1;

new_test_ext().execute_with(|| {
set_rank(alice, 0);
assert_eq!(TestClub::rank_of(&alice), Some(0));
assert_ok!(CoreFellowship::import(signed(alice)));
run_to(3);
assert_eq!(TestClub::rank_of(&alice), Some(0));
assert_ok!(CoreFellowship::submit_evidence(
signed(alice),
Wish::Promotion,
bounded_vec![0; 1024]
));

let root_promote = hypothetically!({
assert_ok!(CoreFellowship::promote(signed(alice), alice, 1));
// Don't clean the events since they should emit the same events:
sp_io::storage::root(sp_runtime::StateVersion::V1)
});

// This is using thread locals instead of storage...
TestClub::demote(&alice).unwrap();

let root_promote_fast = hypothetically!({
assert_ok!(CoreFellowship::promote_fast(signed(alice), alice, 1));

sp_io::storage::root(sp_runtime::StateVersion::V1)
});

assert_eq!(root_promote, root_promote_fast);
// Ensure that we don't compare trivial stuff like `()` from a type error above.
assert_eq!(root_promote.len(), 32);
});
}

#[test]
fn sync_works() {
new_test_ext().execute_with(|| {
Expand Down
22 changes: 22 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