From 691953a9587002665650d9a1d6e868bab83b3a41 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Wed, 31 Jan 2024 17:29:48 +0100 Subject: [PATCH] [FRAME] Make `core-fellowship` ans `salary` work for swapped members (#3156) Fixup for https://github.com/paritytech/polkadot-sdk/pull/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 --- substrate/bin/node/runtime/src/lib.rs | 3 + substrate/frame/core-fellowship/Cargo.toml | 5 + .../frame/core-fellowship/src/benchmarking.rs | 4 +- substrate/frame/core-fellowship/src/lib.rs | 39 +++ .../core-fellowship/src/tests/integration.rs | 278 ++++++++++++++++++ .../frame/core-fellowship/src/tests/mod.rs | 23 ++ .../src/{tests.rs => tests/unit.rs} | 32 +- substrate/frame/ranked-collective/Cargo.toml | 1 + .../ranked-collective/src/benchmarking.rs | 9 +- substrate/frame/ranked-collective/src/lib.rs | 29 +- .../frame/ranked-collective/src/tests.rs | 21 ++ substrate/frame/salary/Cargo.toml | 5 + substrate/frame/salary/src/benchmarking.rs | 4 +- substrate/frame/salary/src/lib.rs | 42 ++- .../frame/salary/src/tests/integration.rs | 278 ++++++++++++++++++ substrate/frame/salary/src/tests/mod.rs | 23 ++ .../salary/src/{tests.rs => tests/unit.rs} | 32 +- substrate/frame/support/src/traits.rs | 2 +- .../frame/support/src/traits/dispatch.rs | 2 +- substrate/frame/support/src/traits/members.rs | 7 + 20 files changed, 770 insertions(+), 69 deletions(-) create mode 100644 substrate/frame/core-fellowship/src/tests/integration.rs create mode 100644 substrate/frame/core-fellowship/src/tests/mod.rs rename substrate/frame/core-fellowship/src/{tests.rs => tests/unit.rs} (92%) create mode 100644 substrate/frame/salary/src/tests/integration.rs create mode 100644 substrate/frame/salary/src/tests/mod.rs rename substrate/frame/salary/src/{tests.rs => tests/unit.rs} (95%) diff --git a/substrate/bin/node/runtime/src/lib.rs b/substrate/bin/node/runtime/src/lib.rs index c2472e906de6a..02d084c6c62b5 100644 --- a/substrate/bin/node/runtime/src/lib.rs +++ b/substrate/bin/node/runtime/src/lib.rs @@ -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 { diff --git a/substrate/frame/core-fellowship/Cargo.toml b/substrate/frame/core-fellowship/Cargo.toml index 0a1706ae22d3c..8e59725d31744 100644 --- a/substrate/frame/core-fellowship/Cargo.toml +++ b/substrate/frame/core-fellowship/Cargo.toml @@ -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", @@ -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", ] diff --git a/substrate/frame/core-fellowship/src/benchmarking.rs b/substrate/frame/core-fellowship/src/benchmarking.rs index a3c410fac0a13..ddde70bd7ce16 100644 --- a/substrate/frame/core-fellowship/src/benchmarking.rs +++ b/substrate/frame/core-fellowship/src/benchmarking.rs @@ -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, } } diff --git a/substrate/frame/core-fellowship/src/lib.rs b/substrate/frame/core-fellowship/src/lib.rs index a0a45c7c594da..e3924594321aa 100644 --- a/substrate/frame/core-fellowship/src/lib.rs +++ b/substrate/frame/core-fellowship/src/lib.rs @@ -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, }; @@ -249,6 +251,8 @@ pub mod pallet { }, /// Pre-ranked account has been inducted at their current rank. Imported { who: T::AccountId, rank: RankOf }, + /// A member had its AccountId swapped. + Swapped { who: T::AccountId, new_who: T::AccountId }, } #[pallet::error] @@ -603,3 +607,38 @@ impl_ensure_origin_with_arg_ignoring_arg! { EnsureOriginWithArg for EnsureInducted {} } + +impl, I: 'static> RankedMembersSwapHandler for Pallet { + fn swapped(old: &T::AccountId, new: &T::AccountId, _rank: u16) { + if old == new { + defensive!("Should not try to swap with self"); + return + } + if !Member::::contains_key(old) { + defensive!("Should not try to swap non-member"); + return + } + if Member::::contains_key(new) { + defensive!("Should not try to overwrite existing member"); + return + } + + if let Some(member) = Member::::take(old) { + Member::::insert(new, member); + } + if let Some(we) = MemberEvidence::::take(old) { + MemberEvidence::::insert(new, we); + } + + Self::deposit_event(Event::::Swapped { who: old.clone(), new_who: new.clone() }); + } +} + +#[cfg(feature = "runtime-benchmarks")] +impl, I: 'static> + pallet_ranked_collective::BenchmarkSetup<::AccountId> for Pallet +{ + fn ensure_member(who: &::AccountId) { + Self::import(frame_system::RawOrigin::Signed(who.clone()).into()).unwrap(); + } +} diff --git a/substrate/frame/core-fellowship/src/tests/integration.rs b/substrate/frame/core-fellowship/src/tests/integration.rs new file mode 100644 index 0000000000000..57f9cad3dcb36 --- /dev/null +++ b/substrate/frame/core-fellowship/src/tests/integration.rs @@ -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; + +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 = 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; + type InductOrigin = EnsureInducted; + type ApproveOrigin = TryMapSuccess, u64>, TryMorphInto>; + type PromoteOrigin = TryMapSuccess, u64>, TryMorphInto>; + type EvidenceSize = EvidenceSize; +} + +pub struct TestPolls; +impl Polling> for TestPolls { + type Index = u8; + type Votes = Votes; + type Moment = u64; + type Class = Class; + + fn classes() -> Vec { + unimplemented!() + } + fn as_ongoing(_: u8) -> Option<(TallyOf, Self::Class)> { + unimplemented!() + } + fn access_poll( + _: Self::Index, + _: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, + ) -> R { + unimplemented!() + } + fn try_access_poll( + _: Self::Index, + _: impl FnOnce( + PollStatus<&mut TallyOf, Self::Moment, Self::Class>, + ) -> Result, + ) -> Result { + unimplemented!() + } + + #[cfg(feature = "runtime-benchmarks")] + fn create_ongoing(_: Self::Class) -> Result { + 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(PhantomData); +impl> Convert for MinRankOfClass { + 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>, + // Members can promote up to the rank of 2 below them. + MapSuccess, ReduceBy>>, + >; + type DemoteOrigin = EitherOf< + // Root can demote arbitrarily. + frame_system::EnsureRootWithSuccess>, + // Members can demote up to the rank of 3 below them. + MapSuccess, ReduceBy>>, + >; + type ExchangeOrigin = EitherOf< + // Root can exchange arbitrarily. + frame_system::EnsureRootWithSuccess>, + // Members can exchange up to the rank of 2 below them. + MapSuccess, ReduceBy>>, + >; + type Polls = TestPolls; + type MinRankOfClass = MinRankOfClass; + 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::::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: ::RuntimeEvent) { + let events = frame_system::Pallet::::events(); + let system_event: ::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 { + e.encode() + .into_iter() + .cycle() + .take(1024) + .collect::>() + .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::::AlreadyMember + ); + // Swapping for the same member is a noop: + assert_noop!( + Club::exchange_member(RuntimeOrigin::root(), 0, 0), + pallet_ranked_collective::Error::::SameMember + ); + }); +} diff --git a/substrate/frame/core-fellowship/src/tests/mod.rs b/substrate/frame/core-fellowship/src/tests/mod.rs new file mode 100644 index 0000000000000..9ec619d0fb6b6 --- /dev/null +++ b/substrate/frame/core-fellowship/src/tests/mod.rs @@ -0,0 +1,23 @@ +// 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. + +#![cfg(test)] + +//! Tests for this crate. + +pub(crate) mod integration; +pub(crate) mod unit; diff --git a/substrate/frame/core-fellowship/src/tests.rs b/substrate/frame/core-fellowship/src/tests/unit.rs similarity index 92% rename from substrate/frame/core-fellowship/src/tests.rs rename to substrate/frame/core-fellowship/src/tests/unit.rs index 838bd9bbdc80e..52a31e5e106f2 100644 --- a/substrate/frame/core-fellowship/src/tests.rs +++ b/substrate/frame/core-fellowship/src/tests/unit.rs @@ -23,18 +23,14 @@ use frame_support::{ assert_noop, assert_ok, derive_impl, ord_parameter_types, pallet_prelude::Weight, parameter_types, - traits::{tokens::GetSalary, ConstU32, ConstU64, Everything, IsInVec, TryMapSuccess}, + traits::{tokens::GetSalary, ConstU32, IsInVec, TryMapSuccess}, }; use frame_system::EnsureSignedBy; -use sp_core::H256; -use sp_runtime::{ - traits::{BlakeTwo256, IdentityLookup, TryMorphInto}, - BuildStorage, DispatchError, DispatchResult, -}; +use sp_runtime::{traits::TryMorphInto, BuildStorage, DispatchError, DispatchResult}; use sp_std::cell::RefCell; -use super::*; use crate as pallet_core_fellowship; +use crate::*; type Block = frame_system::mocking::MockBlock; @@ -53,29 +49,7 @@ parameter_types! { #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] impl frame_system::Config for Test { - type BaseCallFilter = Everything; - type BlockWeights = (); - type BlockLength = (); - type DbWeight = (); - type RuntimeOrigin = RuntimeOrigin; - type Nonce = u64; - type RuntimeCall = RuntimeCall; - type Hash = H256; - type Hashing = BlakeTwo256; - type AccountId = u64; - type Lookup = IdentityLookup; type Block = Block; - type RuntimeEvent = RuntimeEvent; - type BlockHashCount = ConstU64<250>; - type Version = (); - type PalletInfo = PalletInfo; - type AccountData = (); - type OnNewAccount = (); - type OnKilledAccount = (); - type SystemWeightInfo = (); - type SS58Prefix = (); - type OnSetCode = (); - type MaxConsumers = ConstU32<16>; } thread_local! { diff --git a/substrate/frame/ranked-collective/Cargo.toml b/substrate/frame/ranked-collective/Cargo.toml index 431783b24b7ad..62e01df372bde 100644 --- a/substrate/frame/ranked-collective/Cargo.toml +++ b/substrate/frame/ranked-collective/Cargo.toml @@ -27,6 +27,7 @@ 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 } +impl-trait-for-tuples = "0.2.2" [features] default = ["std"] diff --git a/substrate/frame/ranked-collective/src/benchmarking.rs b/substrate/frame/ranked-collective/src/benchmarking.rs index 8d3eca98cc761..332d8292e53f1 100644 --- a/substrate/frame/ranked-collective/src/benchmarking.rs +++ b/substrate/frame/ranked-collective/src/benchmarking.rs @@ -33,6 +33,10 @@ fn assert_last_event, I: 'static>(generic_event: >:: frame_system::Pallet::::assert_last_event(generic_event.into()); } +fn assert_has_event, I: 'static>(generic_event: >::RuntimeEvent) { + frame_system::Pallet::::assert_has_event(generic_event.into()); +} + fn make_member, I: 'static>(rank: Rank) -> T::AccountId { let who = account::("member", MemberCount::::get(0), SEED); let who_lookup = T::Lookup::unlookup(who.clone()); @@ -175,17 +179,18 @@ benchmarks_instance_pallet! { exchange_member { let who = make_member::(1); + T::BenchmarkSetup::ensure_member(&who); let who_lookup = T::Lookup::unlookup(who.clone()); let new_who = account::("new-member", 0, SEED); let new_who_lookup = T::Lookup::unlookup(new_who.clone()); let origin = T::ExchangeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; - let call = Call::::exchange_member { who: who_lookup, new_who:new_who_lookup}; + let call = Call::::exchange_member { who: who_lookup, new_who: new_who_lookup }; }: { call.dispatch_bypass_filter(origin)? } verify { assert_eq!(Members::::get(&new_who).unwrap().rank, 1); assert_eq!(Members::::get(&who), None); - assert_last_event::(Event::MemberExchanged { who, new_who }.into()); + assert_has_event::(Event::MemberExchanged { who, new_who }.into()); } impl_benchmark_test_suite!(RankedCollective, crate::tests::new_test_ext(), crate::tests::Test); diff --git a/substrate/frame/ranked-collective/src/lib.rs b/substrate/frame/ranked-collective/src/lib.rs index 88631884c5a78..d0303fee0b367 100644 --- a/substrate/frame/ranked-collective/src/lib.rs +++ b/substrate/frame/ranked-collective/src/lib.rs @@ -54,7 +54,10 @@ use sp_std::{marker::PhantomData, prelude::*}; use frame_support::{ dispatch::{DispatchResultWithPostInfo, PostDispatchInfo}, ensure, impl_ensure_origin_with_arg_ignoring_arg, - traits::{EnsureOrigin, EnsureOriginWithArg, PollStatus, Polling, RankedMembers, VoteTally}, + traits::{ + EnsureOrigin, EnsureOriginWithArg, PollStatus, Polling, RankedMembers, + RankedMembersSwapHandler, VoteTally, + }, CloneNoBound, EqNoBound, PartialEqNoBound, RuntimeDebugNoBound, }; @@ -365,6 +368,13 @@ impl_ensure_origin_with_arg_ignoring_arg! { {} } +/// Helper functions to setup benchmarking. +#[impl_trait_for_tuples::impl_for_tuples(8)] +pub trait BenchmarkSetup { + /// Ensure that this member is registered correctly. + fn ensure_member(acc: &AccountId); +} + #[frame_support::pallet] pub mod pallet { use super::*; @@ -402,11 +412,21 @@ pub mod pallet { /// "a rank of at least the poll class". type MinRankOfClass: Convert, Rank>; + /// An external handler that will be notified when two members are swapped. + type MemberSwappedHandler: RankedMembersSwapHandler< + as RankedMembers>::AccountId, + as RankedMembers>::Rank, + >; + /// Convert a rank_delta into a number of votes the rank gets. /// /// Rank_delta is defined as the number of ranks above the minimum required to take part /// in the poll. type VoteWeight: Convert; + + /// Setup a member for benchmarking. + #[cfg(feature = "runtime-benchmarks")] + type BenchmarkSetup: BenchmarkSetup; } /// The number of members in the collective who have at least the rank according to the index @@ -679,7 +699,12 @@ pub mod pallet { Self::do_remove_member_from_rank(&who, rank)?; Self::do_add_member_to_rank(new_who.clone(), rank, false)?; - Self::deposit_event(Event::MemberExchanged { who, new_who }); + Self::deposit_event(Event::MemberExchanged { + who: who.clone(), + new_who: new_who.clone(), + }); + T::MemberSwappedHandler::swapped(&who, &new_who, rank); + Ok(()) } } diff --git a/substrate/frame/ranked-collective/src/tests.rs b/substrate/frame/ranked-collective/src/tests.rs index f5e83c48acd05..58e21ded35814 100644 --- a/substrate/frame/ranked-collective/src/tests.rs +++ b/substrate/frame/ranked-collective/src/tests.rs @@ -172,7 +172,10 @@ impl Config for Test { >; type Polls = TestPolls; type MinRankOfClass = MinRankOfClass; + type MemberSwappedHandler = (); type VoteWeight = Geometric; + #[cfg(feature = "runtime-benchmarks")] + type BenchmarkSetup = (); } pub fn new_test_ext() -> sp_io::TestExternalities { @@ -602,3 +605,21 @@ fn exchange_member_works() { ); }); } + +#[test] +fn exchange_member_same_noops() { + new_test_ext().execute_with(|| { + assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); + assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); + assert_ok!(Club::add_member(RuntimeOrigin::root(), 2)); + assert_ok!(Club::promote_member(RuntimeOrigin::root(), 2)); + + // Swapping the same accounts is a noop: + assert_noop!(Club::exchange_member(RuntimeOrigin::root(), 1, 1), Error::::SameMember); + // Swapping with a different member is a noop: + assert_noop!( + Club::exchange_member(RuntimeOrigin::root(), 1, 2), + Error::::AlreadyMember + ); + }); +} diff --git a/substrate/frame/salary/Cargo.toml b/substrate/frame/salary/Cargo.toml index 2f697deebd473..90a44da1a1045 100644 --- a/substrate/frame/salary/Cargo.toml +++ b/substrate/frame/salary/Cargo.toml @@ -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", @@ -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", ] diff --git a/substrate/frame/salary/src/benchmarking.rs b/substrate/frame/salary/src/benchmarking.rs index 7528293506aec..aeae8d2d67f88 100644 --- a/substrate/frame/salary/src/benchmarking.rs +++ b/substrate/frame/salary/src/benchmarking.rs @@ -177,7 +177,7 @@ mod benchmarks { impl_benchmark_test_suite! { Salary, - crate::tests::new_test_ext(), - crate::tests::Test, + crate::tests::unit::new_test_ext(), + crate::tests::unit::Test, } } diff --git a/substrate/frame/salary/src/lib.rs b/substrate/frame/salary/src/lib.rs index f2903f527f25a..18e5c624218c5 100644 --- a/substrate/frame/salary/src/lib.rs +++ b/substrate/frame/salary/src/lib.rs @@ -27,11 +27,12 @@ use sp_runtime::{Perbill, RuntimeDebug}; use sp_std::{marker::PhantomData, prelude::*}; use frame_support::{ + defensive, dispatch::DispatchResultWithPostInfo, ensure, traits::{ tokens::{GetSalary, Pay, PaymentStatus}, - RankedMembers, + RankedMembers, RankedMembersSwapHandler, }, }; @@ -173,6 +174,8 @@ pub mod pallet { }, /// The next cycle begins. CycleStarted { index: CycleIndexOf }, + /// A member swapped their account. + Swapped { who: T::AccountId, new_who: T::AccountId }, } #[pallet::error] @@ -447,3 +450,40 @@ pub mod pallet { } } } + +impl, I: 'static> + RankedMembersSwapHandler::Rank> for Pallet +{ + fn swapped( + who: &T::AccountId, + new_who: &T::AccountId, + _rank: ::Rank, + ) { + if who == new_who { + defensive!("Should not try to swap with self"); + return + } + if Claimant::::contains_key(new_who) { + defensive!("Should not try to overwrite existing claimant"); + return + } + + let Some(claimant) = Claimant::::take(who) else { + frame_support::defensive!("Claimant should exist when swapping"); + return; + }; + + Claimant::::insert(new_who, claimant); + Self::deposit_event(Event::::Swapped { who: who.clone(), new_who: new_who.clone() }); + } +} + +#[cfg(feature = "runtime-benchmarks")] +impl, I: 'static> + pallet_ranked_collective::BenchmarkSetup<::AccountId> for Pallet +{ + fn ensure_member(who: &::AccountId) { + Self::init(frame_system::RawOrigin::Signed(who.clone()).into()).unwrap(); + Self::induct(frame_system::RawOrigin::Signed(who.clone()).into()).unwrap(); + } +} diff --git a/substrate/frame/salary/src/tests/integration.rs b/substrate/frame/salary/src/tests/integration.rs new file mode 100644 index 0000000000000..bf1e82b028c6d --- /dev/null +++ b/substrate/frame/salary/src/tests/integration.rs @@ -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. + +//! The crate's tests. + +use frame_support::{ + assert_noop, assert_ok, derive_impl, hypothetically, + pallet_prelude::Weight, + parameter_types, + traits::{ConstU64, EitherOf, MapSuccess, PollStatus, Polling}, +}; +use pallet_ranked_collective::{EnsureRanked, Geometric, TallyOf, Votes}; +use sp_core::{ConstU16, Get}; +use sp_runtime::{ + traits::{Convert, ReduceBy}, + BuildStorage, DispatchError, +}; + +use crate as pallet_salary; +use crate::*; + +type Rank = u16; +type Block = frame_system::mocking::MockBlock; + +frame_support::construct_runtime!( + pub enum Test + { + System: frame_system, + Salary: pallet_salary, + Club: pallet_ranked_collective, + } +); + +parameter_types! { + pub BlockWeights: frame_system::limits::BlockWeights = + frame_system::limits::BlockWeights::simple_max(Weight::from_parts(1_000_000, 0)); +} + +#[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] +impl frame_system::Config for Test { + type Block = Block; +} + +pub struct TestPolls; +impl Polling> for TestPolls { + type Index = u8; + type Votes = Votes; + type Moment = u64; + type Class = Rank; + + fn classes() -> Vec { + unimplemented!() + } + fn as_ongoing(_index: u8) -> Option<(TallyOf, Self::Class)> { + unimplemented!() + } + fn access_poll( + _index: Self::Index, + _f: impl FnOnce(PollStatus<&mut TallyOf, Self::Moment, Self::Class>) -> R, + ) -> R { + unimplemented!() + } + fn try_access_poll( + _index: Self::Index, + _f: impl FnOnce( + PollStatus<&mut TallyOf, Self::Moment, Self::Class>, + ) -> Result, + ) -> Result { + unimplemented!() + } + + #[cfg(feature = "runtime-benchmarks")] + fn create_ongoing(_class: Self::Class) -> Result { + unimplemented!() + } + + #[cfg(feature = "runtime-benchmarks")] + fn end_ongoing(_index: Self::Index, _approved: bool) -> Result<(), ()> { + unimplemented!() + } +} + +pub struct MinRankOfClass(PhantomData); +impl> Convert for MinRankOfClass { + fn convert(a: u16) -> Rank { + a.saturating_sub(Delta::get()) + } +} + +pub struct TestPay; +impl Pay for TestPay { + type Beneficiary = u64; + type Balance = u64; + type Id = u64; + type AssetKind = (); + type Error = (); + + fn pay( + _: &Self::Beneficiary, + _: Self::AssetKind, + _: Self::Balance, + ) -> Result { + unreachable!() + } + fn check_payment(_: Self::Id) -> PaymentStatus { + unreachable!() + } + #[cfg(feature = "runtime-benchmarks")] + fn ensure_successful(_: &Self::Beneficiary, _: Self::AssetKind, _: Self::Balance) {} + #[cfg(feature = "runtime-benchmarks")] + fn ensure_concluded(_: Self::Id) { + unreachable!() + } +} + +parameter_types! { + pub static Budget: u64 = 10; +} + +impl Config for Test { + type WeightInfo = (); + type RuntimeEvent = RuntimeEvent; + type Paymaster = TestPay; + type Members = Club; + type Salary = FixedSalary; + type RegistrationPeriod = ConstU64<2>; + type PayoutPeriod = ConstU64<2>; + type Budget = Budget; +} + +pub struct FixedSalary; +impl GetSalary for FixedSalary { + fn get_salary(_rank: u16, _who: &u64) -> u64 { + 123 + } +} + +parameter_types! { + pub static MinRankOfClassDelta: Rank = 0; +} + +impl pallet_ranked_collective::Config for Test { + type WeightInfo = (); + type RuntimeEvent = RuntimeEvent; + type PromoteOrigin = EitherOf< + // Root can promote arbitrarily. + frame_system::EnsureRootWithSuccess>, + // Members can promote up to the rank of 2 below them. + MapSuccess, ReduceBy>>, + >; + type DemoteOrigin = EitherOf< + // Root can demote arbitrarily. + frame_system::EnsureRootWithSuccess>, + // Members can demote up to the rank of 3 below them. + MapSuccess, ReduceBy>>, + >; + type ExchangeOrigin = EitherOf< + // Root can exchange arbitrarily. + frame_system::EnsureRootWithSuccess>, + // Members can exchange up to the rank of 2 below them. + MapSuccess, ReduceBy>>, + >; + type Polls = TestPolls; + type MinRankOfClass = MinRankOfClass; + type MemberSwappedHandler = Salary; + type VoteWeight = Geometric; + #[cfg(feature = "runtime-benchmarks")] + type BenchmarkSetup = Salary; +} + +pub fn new_test_ext() -> sp_io::TestExternalities { + let t = frame_system::GenesisConfig::::default().build_storage().unwrap(); + let mut ext = sp_io::TestExternalities::new(t); + ext.execute_with(|| System::set_block_number(1)); + ext +} + +fn assert_last_event(generic_event: ::RuntimeEvent) { + let events = frame_system::Pallet::::events(); + let system_event: ::RuntimeEvent = generic_event.into(); + let frame_system::EventRecord { event, .. } = events.last().expect("Event expected"); + assert_eq!(event, &system_event.into()); +} + +fn promote_n_times(acc: u64, r: u16) { + for _ in 0..r { + assert_ok!(Club::promote_member(RuntimeOrigin::root(), acc)); + } +} + +#[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); + let _ = Salary::init(RuntimeOrigin::signed(acc)); + assert_ok!(Salary::induct(RuntimeOrigin::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()); + } + }); +} + +#[test] +fn swap_exhaustive_works() { + new_test_ext().execute_with(|| { + let root_add = hypothetically!({ + assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); + assert_ok!(Club::promote_member(RuntimeOrigin::root(), 1)); + assert_ok!(Salary::init(RuntimeOrigin::signed(1))); + assert_ok!(Salary::induct(RuntimeOrigin::signed(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)); + assert_ok!(Club::promote_member(RuntimeOrigin::root(), 0)); + assert_ok!(Salary::init(RuntimeOrigin::signed(0))); + assert_ok!(Salary::induct(RuntimeOrigin::signed(0))); + + assert_ok!(Club::exchange_member(RuntimeOrigin::root(), 0, 1)); + + // The events mess up the storage root: + 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!(Salary::init(RuntimeOrigin::signed(0))); + assert_ok!(Salary::induct(RuntimeOrigin::signed(0))); + assert_ok!(Club::add_member(RuntimeOrigin::root(), 1)); + promote_n_times(1, 1); + assert_ok!(Salary::induct(RuntimeOrigin::signed(1))); + + // Swapping for another member is a noop: + assert_noop!( + Club::exchange_member(RuntimeOrigin::root(), 0, 1), + pallet_ranked_collective::Error::::AlreadyMember + ); + // Swapping for the same member is a noop: + assert_noop!( + Club::exchange_member(RuntimeOrigin::root(), 0, 0), + pallet_ranked_collective::Error::::SameMember + ); + }); +} diff --git a/substrate/frame/salary/src/tests/mod.rs b/substrate/frame/salary/src/tests/mod.rs new file mode 100644 index 0000000000000..cd29d8fabeed7 --- /dev/null +++ b/substrate/frame/salary/src/tests/mod.rs @@ -0,0 +1,23 @@ +// 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. + +#![cfg(test)] + +//! Unit and integration tests for the salary pallet. + +pub(crate) mod integration; +pub(crate) mod unit; diff --git a/substrate/frame/salary/src/tests.rs b/substrate/frame/salary/src/tests/unit.rs similarity index 95% rename from substrate/frame/salary/src/tests.rs rename to substrate/frame/salary/src/tests/unit.rs index f25acd9345284..07ef5ce63d5b9 100644 --- a/substrate/frame/salary/src/tests.rs +++ b/substrate/frame/salary/src/tests/unit.rs @@ -23,17 +23,13 @@ use frame_support::{ assert_noop, assert_ok, derive_impl, pallet_prelude::Weight, parameter_types, - traits::{tokens::ConvertRank, ConstU32, ConstU64, Everything}, -}; -use sp_core::H256; -use sp_runtime::{ - traits::{BlakeTwo256, Identity, IdentityLookup}, - BuildStorage, DispatchResult, + traits::{tokens::ConvertRank, ConstU64}, }; +use sp_runtime::{traits::Identity, BuildStorage, DispatchResult}; use sp_std::cell::RefCell; -use super::*; use crate as pallet_salary; +use crate::*; type Block = frame_system::mocking::MockBlock; @@ -52,29 +48,7 @@ parameter_types! { #[derive_impl(frame_system::config_preludes::TestDefaultConfig as frame_system::DefaultConfig)] impl frame_system::Config for Test { - type BaseCallFilter = Everything; - type BlockWeights = (); - type BlockLength = (); - type DbWeight = (); - type RuntimeOrigin = RuntimeOrigin; - type Nonce = u64; - type RuntimeCall = RuntimeCall; - type Hash = H256; - type Hashing = BlakeTwo256; - type AccountId = u64; - type Lookup = IdentityLookup; type Block = Block; - type RuntimeEvent = RuntimeEvent; - type BlockHashCount = ConstU64<250>; - type Version = (); - type PalletInfo = PalletInfo; - type AccountData = (); - type OnNewAccount = (); - type OnKilledAccount = (); - type SystemWeightInfo = (); - type SS58Prefix = (); - type OnSetCode = (); - type MaxConsumers = ConstU32<16>; } thread_local! { diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index a18faee680596..2a42fca76b3c5 100644 --- a/substrate/frame/support/src/traits.rs +++ b/substrate/frame/support/src/traits.rs @@ -37,7 +37,7 @@ pub use members::{AllowAll, DenyAll, Filter}; pub use members::{ AsContains, ChangeMembers, Contains, ContainsLengthBound, ContainsPair, Equals, Everything, EverythingBut, FromContainsPair, InitializeMembers, InsideBoth, IsInVec, Nothing, - RankedMembers, SortedMembers, TheseExcept, + RankedMembers, RankedMembersSwapHandler, SortedMembers, TheseExcept, }; mod validation; diff --git a/substrate/frame/support/src/traits/dispatch.rs b/substrate/frame/support/src/traits/dispatch.rs index d0cedb708cf1d..facc35a77ae09 100644 --- a/substrate/frame/support/src/traits/dispatch.rs +++ b/substrate/frame/support/src/traits/dispatch.rs @@ -490,7 +490,7 @@ pub trait OriginTrait: Sized { /// Add a filter to the origin. fn add_filter(&mut self, filter: impl Fn(&Self::Call) -> bool + 'static); - /// Reset origin filters to default one, i.e `frame_system::Config::BaseCallFilter`. + /// Reset origin filters to default one, i.e `frame_system::1fig::BaseCallFilter`. fn reset_filter(&mut self); /// Replace the caller with caller from the other origin diff --git a/substrate/frame/support/src/traits/members.rs b/substrate/frame/support/src/traits/members.rs index d667eaa7e9d8d..3a6e3719593a2 100644 --- a/substrate/frame/support/src/traits/members.rs +++ b/substrate/frame/support/src/traits/members.rs @@ -297,6 +297,13 @@ pub trait RankedMembers { fn demote(who: &Self::AccountId) -> DispatchResult; } +/// Handler that can deal with the swap of two members. +#[impl_trait_for_tuples::impl_for_tuples(16)] +pub trait RankedMembersSwapHandler { + /// Member `old` was swapped with `new` at `rank`. + fn swapped(who: &AccountId, new_who: &AccountId, rank: Rank); +} + /// Trait for type that can handle the initialization of account IDs at genesis. pub trait InitializeMembers { /// Initialize the members to the given `members`.