Skip to content

Commit

Permalink
[FRAME] Make core-fellowship ans salary work for swapped members (p…
Browse files Browse the repository at this point in the history
…aritytech#3156)

Fixup for paritytech#2587 to make
the `core-fellowship` crate work with swapped members.

Adds a `MemberSwappedHandler` to the `ranked-collective` pallet that are
implemented by `core-fellowship+salary`.
There is are exhaustive tests
[here](https://github.com/paritytech/polkadot-sdk/blob/72aa7ac17a0e5b16faab5d2992aa2db2e01b05d0/substrate/frame/core-fellowship/src/tests/integration.rs#L338)
and
[here](https://github.com/paritytech/polkadot-sdk/blob/ab3cdb05a5ebc1ff841f8dda67edef0ea40bbba5/substrate/frame/salary/src/tests/integration.rs#L224)
to check that adding member `1` is equivalent to adding member `0` and
then swapping.

---------

Signed-off-by: Oliver Tale-Yazdi <[email protected]>
  • Loading branch information
ggwpez authored Jan 31, 2024
1 parent 553b805 commit 691953a
Show file tree
Hide file tree
Showing 20 changed files with 770 additions and 69 deletions.
3 changes: 3 additions & 0 deletions substrate/bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1019,6 +1019,9 @@ impl pallet_ranked_collective::Config for Runtime {
type Polls = RankedPolls;
type MinRankOfClass = traits::Identity;
type VoteWeight = pallet_ranked_collective::Geometric;
type MemberSwappedHandler = (CoreFellowship, Salary);
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkSetup = (CoreFellowship, Salary);
}

impl pallet_remark::Config for Runtime {
Expand Down
5 changes: 5 additions & 0 deletions substrate/frame/core-fellowship/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,18 @@ sp-core = { path = "../../primitives/core", default-features = false }
sp-io = { path = "../../primitives/io", default-features = false }
sp-runtime = { path = "../../primitives/runtime", default-features = false }
sp-std = { path = "../../primitives/std", default-features = false }
pallet-ranked-collective = { path = "../ranked-collective", default-features = false, optional = true }

[features]
default = ["std"]
std = [
"codec/std",
"frame-benchmarking?/std",
"frame-support/experimental",
"frame-support/std",
"frame-system/std",
"log/std",
"pallet-ranked-collective/std",
"scale-info/std",
"sp-arithmetic/std",
"sp-core/std",
Expand All @@ -47,10 +50,12 @@ runtime-benchmarks = [
"frame-benchmarking/runtime-benchmarks",
"frame-support/runtime-benchmarks",
"frame-system/runtime-benchmarks",
"pallet-ranked-collective/runtime-benchmarks",
"sp-runtime/runtime-benchmarks",
]
try-runtime = [
"frame-support/try-runtime",
"frame-system/try-runtime",
"pallet-ranked-collective?/try-runtime",
"sp-runtime/try-runtime",
]
4 changes: 2 additions & 2 deletions substrate/frame/core-fellowship/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ mod benchmarks {

impl_benchmark_test_suite! {
CoreFellowship,
crate::tests::new_test_ext(),
crate::tests::Test,
crate::tests::unit::new_test_ext(),
crate::tests::unit::Test,
}
}
39 changes: 39 additions & 0 deletions substrate/frame/core-fellowship/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,12 @@ use sp_runtime::RuntimeDebug;
use sp_std::{marker::PhantomData, prelude::*};

use frame_support::{
defensive,
dispatch::DispatchResultWithPostInfo,
ensure, impl_ensure_origin_with_arg_ignoring_arg,
traits::{
tokens::Balance as BalanceTrait, EnsureOrigin, EnsureOriginWithArg, Get, RankedMembers,
RankedMembersSwapHandler,
},
BoundedVec,
};
Expand Down Expand Up @@ -249,6 +251,8 @@ pub mod pallet {
},
/// Pre-ranked account has been inducted at their current rank.
Imported { who: T::AccountId, rank: RankOf<T, I> },
/// A member had its AccountId swapped.
Swapped { who: T::AccountId, new_who: T::AccountId },
}

#[pallet::error]
Expand Down Expand Up @@ -603,3 +607,38 @@ impl_ensure_origin_with_arg_ignoring_arg! {
EnsureOriginWithArg<T::RuntimeOrigin, A> for EnsureInducted<T, I, MIN_RANK>
{}
}

impl<T: Config<I>, I: 'static> RankedMembersSwapHandler<T::AccountId, u16> for Pallet<T, I> {
fn swapped(old: &T::AccountId, new: &T::AccountId, _rank: u16) {
if old == new {
defensive!("Should not try to swap with self");
return
}
if !Member::<T, I>::contains_key(old) {
defensive!("Should not try to swap non-member");
return
}
if Member::<T, I>::contains_key(new) {
defensive!("Should not try to overwrite existing member");
return
}

if let Some(member) = Member::<T, I>::take(old) {
Member::<T, I>::insert(new, member);
}
if let Some(we) = MemberEvidence::<T, I>::take(old) {
MemberEvidence::<T, I>::insert(new, we);
}

Self::deposit_event(Event::<T, I>::Swapped { who: old.clone(), new_who: new.clone() });
}
}

#[cfg(feature = "runtime-benchmarks")]
impl<T: Config<I>, I: 'static>
pallet_ranked_collective::BenchmarkSetup<<T as frame_system::Config>::AccountId> for Pallet<T, I>
{
fn ensure_member(who: &<T as frame_system::Config>::AccountId) {
Self::import(frame_system::RawOrigin::Signed(who.clone()).into()).unwrap();
}
}
278 changes: 278 additions & 0 deletions substrate/frame/core-fellowship/src/tests/integration.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,278 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! Integration test together with the ranked-collective pallet.
use frame_support::{
assert_noop, assert_ok, derive_impl, hypothetically, ord_parameter_types,
pallet_prelude::Weight,
parameter_types,
traits::{ConstU16, EitherOf, IsInVec, MapSuccess, PollStatus, Polling, TryMapSuccess},
};
use frame_system::EnsureSignedBy;
use pallet_ranked_collective::{EnsureRanked, Geometric, Rank, TallyOf, Votes};
use sp_core::Get;
use sp_runtime::{
traits::{Convert, ReduceBy, TryMorphInto},
BuildStorage, DispatchError,
};
type Class = Rank;

use crate as pallet_core_fellowship;
use crate::*;

type Block = frame_system::mocking::MockBlock<Test>;

frame_support::construct_runtime!(
pub enum Test
{
System: frame_system,
CoreFellowship: pallet_core_fellowship,
Club: pallet_ranked_collective,
}
);

parameter_types! {
pub BlockWeights: frame_system::limits::BlockWeights =
frame_system::limits::BlockWeights::simple_max(Weight::from_parts(1_000_000, u64::max_value()));
}

#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)]
impl frame_system::Config for Test {
type Block = Block;
}

parameter_types! {
pub static MinRankOfClassDelta: Rank = 0;
}

parameter_types! {
pub ZeroToNine: Vec<u64> = vec![0, 1, 2, 3, 4, 5, 6, 7, 8, 9];
pub EvidenceSize: u32 = 1024;
}
ord_parameter_types! {
pub const One: u64 = 1;
}

impl Config for Test {
type WeightInfo = ();
type RuntimeEvent = RuntimeEvent;
type Members = Club;
type Balance = u64;
type ParamsOrigin = EnsureSignedBy<One, u64>;
type InductOrigin = EnsureInducted<Test, (), 1>;
type ApproveOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type PromoteOrigin = TryMapSuccess<EnsureSignedBy<IsInVec<ZeroToNine>, u64>, TryMorphInto<u16>>;
type EvidenceSize = EvidenceSize;
}

pub struct TestPolls;
impl Polling<TallyOf<Test>> for TestPolls {
type Index = u8;
type Votes = Votes;
type Moment = u64;
type Class = Class;

fn classes() -> Vec<Self::Class> {
unimplemented!()
}
fn as_ongoing(_: u8) -> Option<(TallyOf<Test>, Self::Class)> {
unimplemented!()
}
fn access_poll<R>(
_: Self::Index,
_: impl FnOnce(PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>) -> R,
) -> R {
unimplemented!()
}
fn try_access_poll<R>(
_: Self::Index,
_: impl FnOnce(
PollStatus<&mut TallyOf<Test>, Self::Moment, Self::Class>,
) -> Result<R, DispatchError>,
) -> Result<R, DispatchError> {
unimplemented!()
}

#[cfg(feature = "runtime-benchmarks")]
fn create_ongoing(_: Self::Class) -> Result<Self::Index, ()> {
unimplemented!()
}

#[cfg(feature = "runtime-benchmarks")]
fn end_ongoing(_: Self::Index, _: bool) -> Result<(), ()> {
unimplemented!()
}
}

/// Convert the tally class into the minimum rank required to vote on the poll.
/// MinRank(Class) = Class - Delta
pub struct MinRankOfClass<Delta>(PhantomData<Delta>);
impl<Delta: Get<Rank>> Convert<Class, Rank> for MinRankOfClass<Delta> {
fn convert(a: Class) -> Rank {
a.saturating_sub(Delta::get())
}
}

impl pallet_ranked_collective::Config for Test {
type WeightInfo = ();
type RuntimeEvent = RuntimeEvent;
type PromoteOrigin = EitherOf<
// Root can promote arbitrarily.
frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
// Members can promote up to the rank of 2 below them.
MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
>;
type DemoteOrigin = EitherOf<
// Root can demote arbitrarily.
frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
// Members can demote up to the rank of 3 below them.
MapSuccess<EnsureRanked<Test, (), 3>, ReduceBy<ConstU16<3>>>,
>;
type ExchangeOrigin = EitherOf<
// Root can exchange arbitrarily.
frame_system::EnsureRootWithSuccess<Self::AccountId, ConstU16<65535>>,
// Members can exchange up to the rank of 2 below them.
MapSuccess<EnsureRanked<Test, (), 2>, ReduceBy<ConstU16<2>>>,
>;
type Polls = TestPolls;
type MinRankOfClass = MinRankOfClass<MinRankOfClassDelta>;
type MemberSwappedHandler = CoreFellowship;
type VoteWeight = Geometric;
#[cfg(feature = "runtime-benchmarks")]
type BenchmarkSetup = CoreFellowship;
}

pub fn new_test_ext() -> sp_io::TestExternalities {
let t = frame_system::GenesisConfig::<Test>::default().build_storage().unwrap();
let mut ext = sp_io::TestExternalities::new(t);
ext.execute_with(|| {
let params = ParamsType {
active_salary: [10, 20, 30, 40, 50, 60, 70, 80, 90],
passive_salary: [1, 2, 3, 4, 5, 6, 7, 8, 9],
demotion_period: [2, 4, 6, 8, 10, 12, 14, 16, 18],
min_promotion_period: [3, 6, 9, 12, 15, 18, 21, 24, 27],
offboard_timeout: 1,
};
assert_ok!(CoreFellowship::set_params(signed(1), Box::new(params)));
System::set_block_number(1);
});
ext
}

fn promote_n_times(acc: u64, r: u16) {
for _ in 0..r {
assert_ok!(Club::promote_member(RuntimeOrigin::root(), acc));
}
}

fn signed(who: u64) -> RuntimeOrigin {
RuntimeOrigin::signed(who)
}

fn assert_last_event(generic_event: <Test as Config>::RuntimeEvent) {
let events = frame_system::Pallet::<Test>::events();
let system_event: <Test as frame_system::Config>::RuntimeEvent = generic_event.into();
let frame_system::EventRecord { event, .. } = events.last().expect("Event expected");
assert_eq!(event, &system_event.into());
}

fn evidence(e: u32) -> Evidence<Test, ()> {
e.encode()
.into_iter()
.cycle()
.take(1024)
.collect::<Vec<_>>()
.try_into()
.expect("Static length matches")
}

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

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

// Swapping normally works:
assert_ok!(Club::exchange_member(RuntimeOrigin::root(), acc, acc + 10));
assert_last_event(Event::Swapped { who: acc, new_who: acc + 10 }.into());
}
});
}

/// Exhaustively test that adding member `1` is equivalent to adding member `0` and then swapping.
///
/// The member also submits evidence before the swap.
#[test]
fn swap_exhaustive_works() {
new_test_ext().execute_with(|| {
let root_add = hypothetically!({
assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
promote_n_times(1, 4);
assert_ok!(CoreFellowship::import(signed(1)));
assert_ok!(CoreFellowship::submit_evidence(signed(1), Wish::Retention, evidence(1)));

// The events mess up the storage root:
System::reset_events();
sp_io::storage::root(sp_runtime::StateVersion::V1)
});

let root_swap = hypothetically!({
assert_ok!(Club::add_member(RuntimeOrigin::root(), 0));
promote_n_times(0, 4);
assert_ok!(CoreFellowship::import(signed(0)));
assert_ok!(CoreFellowship::submit_evidence(signed(0), Wish::Retention, evidence(1)));

// Now we swap:
assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 0, 1));

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

assert_eq!(root_add, root_swap);
// Ensure that we dont compare trivial stuff like `()` from a type error above.
assert_eq!(root_add.len(), 32);
});
}

#[test]
fn swap_bad_noops() {
new_test_ext().execute_with(|| {
assert_ok!(Club::add_member(RuntimeOrigin::root(), 0));
promote_n_times(0, 0);
assert_ok!(CoreFellowship::import(signed(0)));
assert_ok!(Club::add_member(RuntimeOrigin::root(), 1));
promote_n_times(1, 1);
assert_ok!(CoreFellowship::import(signed(1)));

// Swapping for another member is a noop:
assert_noop!(
Club::exchange_member(RuntimeOrigin::root(), 0, 1),
pallet_ranked_collective::Error::<Test>::AlreadyMember
);
// Swapping for the same member is a noop:
assert_noop!(
Club::exchange_member(RuntimeOrigin::root(), 0, 0),
pallet_ranked_collective::Error::<Test>::SameMember
);
});
}
Loading

0 comments on commit 691953a

Please sign in to comment.