diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index a67f4855ac56b..4d74ef926297c 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -90,7 +90,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion { // and set impl_version to 0. If only runtime // implementation changes and behavior does not, then leave spec_version as // is and increment impl_version. - spec_version: 247, + spec_version: 248, impl_version: 0, apis: RUNTIME_API_VERSIONS, transaction_version: 1, @@ -377,6 +377,7 @@ impl pallet_democracy::Trait for Runtime { parameter_types! { pub const CouncilMotionDuration: BlockNumber = 5 * DAYS; + pub const DesiredMembers: u32 = 13; } type CouncilCollective = pallet_collective::Instance1; @@ -385,13 +386,13 @@ impl pallet_collective::Trait for Runtime { type Proposal = Call; type Event = Event; type MotionDuration = CouncilMotionDuration; + type MaxMembers = DesiredMembers; } parameter_types! { pub const CandidacyBond: Balance = 10 * DOLLARS; pub const VotingBond: Balance = 1 * DOLLARS; pub const TermDuration: BlockNumber = 7 * DAYS; - pub const DesiredMembers: u32 = 13; pub const DesiredRunnersUp: u32 = 7; pub const ElectionsPhragmenModuleId: LockIdentifier = *b"phrelect"; } @@ -417,6 +418,7 @@ impl pallet_elections_phragmen::Trait for Runtime { parameter_types! { pub const TechnicalMotionDuration: BlockNumber = 5 * DAYS; + pub const TechnicalMaxMembers: u32 = 100; } type TechnicalCollective = pallet_collective::Instance2; @@ -425,6 +427,7 @@ impl pallet_collective::Trait for Runtime { type Proposal = Call; type Event = Event; type MotionDuration = TechnicalMotionDuration; + type MaxMembers = TechnicalMaxMembers; } impl pallet_membership::Trait for Runtime { diff --git a/frame/collective/src/benchmarking.rs b/frame/collective/src/benchmarking.rs index 5c25051fd0837..f21ac500b8eb3 100644 --- a/frame/collective/src/benchmarking.rs +++ b/frame/collective/src/benchmarking.rs @@ -28,7 +28,6 @@ use crate::Module as Collective; const SEED: u32 = 0; -const MAX_MEMBERS: u32 = 1000; const MAX_PROPOSALS: u32 = 100; const MAX_BYTES: u32 = 1_024; @@ -44,8 +43,8 @@ benchmarks_instance! { _{ } set_members { - let m in 1 .. MAX_MEMBERS; - let n in 1 .. MAX_MEMBERS; + let m in 1 .. T::MaxMembers::get(); + let n in 1 .. T::MaxMembers::get(); // Set old members. // We compute the difference of old and new members, so it should influence timing. @@ -74,12 +73,12 @@ benchmarks_instance! { } execute { - let m in 1 .. MAX_MEMBERS; + let m in 1 .. T::MaxMembers::get(); let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -100,12 +99,12 @@ benchmarks_instance! { // This tests when execution would happen immediately after proposal propose_execute { - let m in 1 .. MAX_MEMBERS; + let m in 1 .. T::MaxMembers::get(); let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -127,13 +126,13 @@ benchmarks_instance! { // This tests when proposal is created and queued as "proposed" propose_proposed { - let m in 1 .. MAX_MEMBERS; + let m in 1 .. T::MaxMembers::get(); let p in 0 .. MAX_PROPOSALS; let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -164,13 +163,13 @@ benchmarks_instance! { } vote_insert { - let m in 2 .. MAX_MEMBERS; + let m in 5 .. T::MaxMembers::get(); let p in 1 .. MAX_PROPOSALS; let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -180,7 +179,7 @@ benchmarks_instance! { Collective::::set_members(SystemOrigin::Root.into(), members.clone(), None)?; // Threshold is 1 less than the number of members so that one person can vote nay - let threshold = m; + let threshold = m - 1; // Add previous proposals let mut last_hash = T::Hash::default(); @@ -192,7 +191,7 @@ benchmarks_instance! { } // Have everyone vote aye on last proposal, while keeping it from passing - for j in 2 .. m { + for j in 2 .. m - 1 { let voter = &members[j as usize]; let approve = true; Collective::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; @@ -209,18 +208,18 @@ benchmarks_instance! { // All proposals exist and the last proposal has just been updated. assert_eq!(Collective::::proposals().len(), p as usize); let voting = Collective::::voting(&last_hash).ok_or(Error::::ProposalMissing)?; - assert_eq!(voting.ayes.len(), (m - 2) as usize); + assert_eq!(voting.ayes.len(), (m - 3) as usize); assert_eq!(voting.nays.len(), 1); } vote_disapproved { - let m in 2 .. MAX_MEMBERS; + let m in 2 .. T::MaxMembers::get(); let p in 1 .. MAX_PROPOSALS; let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -230,7 +229,7 @@ benchmarks_instance! { Collective::::set_members(SystemOrigin::Root.into(), members.clone(), None)?; // Threshold is total members so that one nay will disapprove the vote - let threshold = m + 1; + let threshold = m; // Add previous proposals let mut last_hash = T::Hash::default(); @@ -242,7 +241,7 @@ benchmarks_instance! { } // Have everyone vote aye on last proposal, while keeping it from passing - for j in 1 .. m { + for j in 1 .. m - 1 { let voter = &members[j as usize]; let approve = true; Collective::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; @@ -262,13 +261,13 @@ benchmarks_instance! { } vote_approved { - let m in 2 .. MAX_MEMBERS; + let m in 4 .. T::MaxMembers::get(); let p in 1 .. MAX_PROPOSALS; let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -293,7 +292,7 @@ benchmarks_instance! { Collective::::vote(SystemOrigin::Signed(caller.clone()).into(), last_hash.clone(), p - 1, false)?; // Have everyone vote nay on last proposal, while keeping it from failing - for j in 2 .. m { + for j in 2 .. m - 1 { let voter = &members[j as usize]; let approve = false; Collective::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; @@ -316,13 +315,13 @@ benchmarks_instance! { } close_disapproved { - let m in 2 .. MAX_MEMBERS; + let m in 4 .. T::MaxMembers::get(); let p in 1 .. MAX_PROPOSALS; let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -332,7 +331,7 @@ benchmarks_instance! { Collective::::set_members(SystemOrigin::Root.into(), members.clone(), Some(caller.clone()))?; // Threshold is one less than total members so that two nays will disapprove the vote - let threshold = m; + let threshold = m - 1; // Add proposals let mut last_hash = T::Hash::default(); @@ -345,7 +344,7 @@ benchmarks_instance! { // Have everyone vote aye on last proposal, while keeping it from passing // A few abstainers will be the nay votes needed to fail the vote - for j in 2 .. m { + for j in 2 .. m - 1 { let voter = &members[j as usize]; let approve = true; Collective::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; @@ -366,13 +365,13 @@ benchmarks_instance! { close_approved { - let m in 2 .. MAX_MEMBERS; + let m in 4 .. T::MaxMembers::get(); let p in 1 .. MAX_PROPOSALS; let b in 1 .. MAX_BYTES; // Construct `members`. let mut members = vec![]; - for i in 0 .. m { + for i in 0 .. m - 1 { let member = account("member", i, SEED); members.push(member); } @@ -395,7 +394,7 @@ benchmarks_instance! { // Have everyone vote nay on last proposal, while keeping it from failing // A few abstainers will be the aye votes needed to pass the vote - for j in 2 .. m { + for j in 2 .. m - 1 { let voter = &members[j as usize]; let approve = false; Collective::::vote(SystemOrigin::Signed(voter.clone()).into(), last_hash.clone(), p - 1, approve)?; diff --git a/frame/collective/src/lib.rs b/frame/collective/src/lib.rs index 662465616ed1b..618da6c090bbf 100644 --- a/frame/collective/src/lib.rs +++ b/frame/collective/src/lib.rs @@ -41,7 +41,8 @@ use sp_core::u32_trait::Value as U32; use sp_runtime::RuntimeDebug; use sp_runtime::traits::Hash; use frame_support::{ - dispatch::{Dispatchable, Parameter}, codec::{Encode, Decode}, + debug, + dispatch::{Dispatchable, DispatchResult, Parameter}, codec::{Encode, Decode}, traits::{Get, ChangeMembers, InitializeMembers, EnsureOrigin}, decl_module, decl_event, decl_storage, decl_error, ensure, weights::DispatchClass, @@ -72,6 +73,9 @@ pub trait Trait: frame_system::Trait { /// The time-out for council motions. type MotionDuration: Get; + + /// Maxmimum number of members allowed as part of the collective. + type MaxMembers: Get; } /// Origin for the collective module. @@ -168,6 +172,8 @@ decl_error! { AlreadyInitialized, /// The close call is made too early, before the end of the voting. TooEarly, + /// There can only be a maximum of `MaxMembers`. + TooManyMembers, } } @@ -190,10 +196,14 @@ decl_module! { #[weight = (100_000_000, DispatchClass::Operational)] fn set_members(origin, new_members: Vec, prime: Option) { ensure_root(origin)?; + // We know that our implementation of `ensure_can_change_members` does not use the old members. + >::ensure_can_change_members(&new_members, &[])?; + + let old = Members::::get(); let mut new_members = new_members; new_members.sort(); - let old = Members::::get(); - >::set_members_sorted(&new_members[..], &old); + // We can drop the members count from `ChangeMembers` because we used the ensure above. + >::set_members_sorted(&new_members, &old); Prime::::set(prime); } @@ -379,11 +389,32 @@ impl, I: Instance> Module { } impl, I: Instance> ChangeMembers for Module { + /// Only allow changing the members when they don't exceed the maximum. + fn ensure_can_change_members(new: &[T::AccountId], _old: &[T::AccountId]) -> DispatchResult { + ensure!(new.len() <= T::MaxMembers::get() as usize, Error::::TooManyMembers); + Ok(()) + } + + /// Update the members of the collective. Votes are updated and the prime is reset. + /// + /// NOTE: Will only allow setting up to `MaxMembers` members. Any excess members will be + /// dropped and not inserted. Use `ensure_can_change_members` to check before + /// calling this to make sure no members are dropped. fn change_members_sorted( _incoming: &[T::AccountId], outgoing: &[T::AccountId], new: &[T::AccountId], - ) { + ) -> Vec { + // limit members to `MaxMembers` + let length = new.len().min(T::MaxMembers::get() as usize); + if length != new.len() { + debug::error!( + "Dropping excess members because new members are longer ({}) than MaxMembers ({})", + new.len(), + T::MaxMembers::get() + ); + } + let new = &new[..length]; // remove accounts from all current voting in motions. let mut outgoing = outgoing.to_vec(); outgoing.sort_unstable(); @@ -402,6 +433,7 @@ impl, I: Instance> ChangeMembers for Module { } Members::::put(new); Prime::::kill(); + new.to_vec() } fn set_prime(prime: Option) { @@ -523,7 +555,7 @@ impl< #[cfg(test)] mod tests { use super::*; - use frame_support::{Hashable, assert_ok, assert_noop, parameter_types, weights::Weight}; + use frame_support::{Hashable, assert_err, assert_ok, assert_noop, parameter_types, weights::Weight}; use frame_system::{self as system, EventRecord, Phase}; use hex_literal::hex; use sp_core::H256; @@ -539,6 +571,7 @@ mod tests { pub const MaximumBlockLength: u32 = 2 * 1024; pub const AvailableBlockRatio: Perbill = Perbill::one(); pub const MotionDuration: u64 = 3; + pub const MaxMembers: MemberCount = 100; } impl frame_system::Trait for Test { type Origin = Origin; @@ -569,12 +602,14 @@ mod tests { type Proposal = Call; type Event = Event; type MotionDuration = MotionDuration; + type MaxMembers = MaxMembers; } impl Trait for Test { type Origin = Origin; type Proposal = Call; type Event = Event; type MotionDuration = MotionDuration; + type MaxMembers = MaxMembers; } pub type Block = sp_runtime::generic::Block; @@ -788,6 +823,19 @@ mod tests { }); } + #[test] + fn limit_members() { + new_test_ext().execute_with(|| { + let new_members: Vec = (0..MaxMembers::get() as u64 + 5).collect(); + assert_err!( + Collective::ensure_can_change_members(&new_members, &Collective::members()), + Error::::TooManyMembers + ); + Collective::change_members_sorted(&Collective::members(), &[], &new_members); + assert_eq!(Collective::members().len(), MaxMembers::get() as usize); + }) + } + #[test] fn motions_ignoring_non_collective_proposals_works() { new_test_ext().execute_with(|| { diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 040ef00ac27cd..73d750df7ef24 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -549,10 +549,12 @@ impl Module { let members = members_with_stake.into_iter().map(|m| m.0).collect::>(); let result = Ok(maybe_replacement.is_some()); let old = [who.clone()]; + // We don't need to check the actual new members length because it will be the same + // as before or one less. match maybe_replacement { Some(new) => T::ChangeMembers::change_members_sorted(&[new], &old, &members), None => T::ChangeMembers::change_members_sorted(&[], &old, &members), - } + }; result } else { Err(Error::::NotMember)? @@ -973,7 +975,7 @@ mod tests { pub struct TestChangeMembers; impl ChangeMembers for TestChangeMembers { - fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) { + fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> Vec { // new, incoming, outgoing must be sorted. let mut new_sorted = new.to_vec(); new_sorted.sort(); @@ -1004,6 +1006,8 @@ mod tests { MEMBERS.with(|m| *m.borrow_mut() = new.to_vec()); PRIME.with(|p| *p.borrow_mut() = None); + + new.to_vec() } fn set_prime(who: Option) { diff --git a/frame/elections/src/lib.rs b/frame/elections/src/lib.rs index a684ce3144d80..d4ed6ea8b02fa 100644 --- a/frame/elections/src/lib.rs +++ b/frame/elections/src/lib.rs @@ -682,6 +682,7 @@ decl_module! { .collect(); >::put(&new_set); let new_set = new_set.into_iter().map(|x| x.0).collect::>(); + // This is save to just call because we are removing members, not adding any. T::ChangeMembers::change_members(&[], &[who], new_set); } diff --git a/frame/elections/src/mock.rs b/frame/elections/src/mock.rs index c779f22a3203b..6e64ebff47b2d 100644 --- a/frame/elections/src/mock.rs +++ b/frame/elections/src/mock.rs @@ -111,7 +111,7 @@ impl Get for DecayRatio { pub struct TestChangeMembers; impl ChangeMembers for TestChangeMembers { - fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) { + fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> Vec { let mut old_plus_incoming = MEMBERS.with(|m| m.borrow().to_vec()); old_plus_incoming.extend_from_slice(incoming); old_plus_incoming.sort(); @@ -121,6 +121,7 @@ impl ChangeMembers for TestChangeMembers { assert_eq!(old_plus_incoming, new_plus_outgoing); MEMBERS.with(|m| *m.borrow_mut() = new.to_vec()); + new.to_vec() } } diff --git a/frame/membership/src/lib.rs b/frame/membership/src/lib.rs index faf2be8e11e5e..62fc737facd5a 100644 --- a/frame/membership/src/lib.rs +++ b/frame/membership/src/lib.rs @@ -25,6 +25,7 @@ use sp_std::prelude::*; use frame_support::{ decl_module, decl_storage, decl_event, decl_error, + dispatch::DispatchResult, traits::{ChangeMembers, InitializeMembers, EnsureOrigin}, }; use frame_system::{self as system, ensure_root, ensure_signed}; @@ -125,7 +126,9 @@ decl_module! { let mut members = >::get(); let location = members.binary_search(&who).err().ok_or(Error::::AlreadyMember)?; + let old = members.clone(); members.insert(location, who.clone()); + T::MembershipChanged::ensure_can_change_members(&members, &old)?; >::put(&members); T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]); @@ -147,6 +150,8 @@ decl_module! { members.remove(location); >::put(&members); + // This is safe without the `ensure_can_change_members` check because we only + // reduce the length. T::MembershipChanged::change_members_sorted(&[], &[who], &members[..]); Self::rejig_prime(&members); @@ -173,6 +178,8 @@ decl_module! { members.sort(); >::put(&members); + // This is safe without the `ensure_can_change_members` check because the length + // stays the same. T::MembershipChanged::change_members_sorted( &[add], &[remove], @@ -195,11 +202,13 @@ decl_module! { let mut members = members; members.sort(); - >::mutate(|m| { - T::MembershipChanged::set_members_sorted(&members[..], m); + >::try_mutate(|m| -> DispatchResult { + T::MembershipChanged::ensure_can_change_members(&members, m)?; + T::MembershipChanged::set_members_sorted(&members, m); Self::rejig_prime(&members); *m = members; - }); + Ok(()) + })?; Self::deposit_event(RawEvent::MembersReset); @@ -222,6 +231,8 @@ decl_module! { members.sort(); >::put(&members); + // This is safe without the `ensure_can_change_members` check because the length + // stays the same. T::MembershipChanged::change_members_sorted( &[new.clone()], &[remove.clone()], @@ -340,7 +351,7 @@ mod tests { pub struct TestChangeMembers; impl ChangeMembers for TestChangeMembers { - fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) { + fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> Vec { let mut old_plus_incoming = MEMBERS.with(|m| m.borrow().to_vec()); old_plus_incoming.extend_from_slice(incoming); old_plus_incoming.sort(); @@ -351,7 +362,9 @@ mod tests { MEMBERS.with(|m| *m.borrow_mut() = new.to_vec()); PRIME.with(|p| *p.borrow_mut() = None); + new.to_vec() } + fn set_prime(who: Option) { PRIME.with(|p| *p.borrow_mut() = who); } diff --git a/frame/scored-pool/src/lib.rs b/frame/scored-pool/src/lib.rs index 1ba999cc7c838..194edbac07799 100644 --- a/frame/scored-pool/src/lib.rs +++ b/frame/scored-pool/src/lib.rs @@ -40,6 +40,9 @@ //! - [`Call`](./enum.Call.html) //! - [`Module`](./struct.Module.html) //! +//! NOTE: This pallet does not provide proper weights for its functions, yet. +//! Benchmark and adjust the weights before using in production. +//! //! ## Interface //! //! ### Public Functions @@ -390,7 +393,7 @@ impl, I: Instance> Module { /// Fetches the `MemberCount` highest scoring members from /// `Pool` and puts them into `Members`. /// - /// The `notify` parameter is used to deduct which associated + /// The `notify` parameter is used to decide which associated /// type function to invoke at the end of the method. fn refresh_members( pool: PoolT, @@ -412,12 +415,15 @@ impl, I: Instance> Module { match notify { ChangeReceiver::MembershipInitialized => T::MembershipInitialized::initialize_members(&new_members), - ChangeReceiver::MembershipChanged => + ChangeReceiver::MembershipChanged => { + // TODO: This is not safe as members might be truncated. T::MembershipChanged::set_members_sorted( &new_members[..], &old_members[..], - ), + ); + }, } + } /// Removes an entity `remove` at `index` from the `Pool`. diff --git a/frame/scored-pool/src/mock.rs b/frame/scored-pool/src/mock.rs index 6d914c60aae2c..ebfca076e793d 100644 --- a/frame/scored-pool/src/mock.rs +++ b/frame/scored-pool/src/mock.rs @@ -92,7 +92,7 @@ thread_local! { pub struct TestChangeMembers; impl ChangeMembers for TestChangeMembers { - fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) { + fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> Vec { let mut old_plus_incoming = MEMBERS.with(|m| m.borrow().to_vec()); old_plus_incoming.extend_from_slice(incoming); old_plus_incoming.sort(); @@ -104,6 +104,7 @@ impl ChangeMembers for TestChangeMembers { assert_eq!(old_plus_incoming, new_plus_outgoing); MEMBERS.with(|m| *m.borrow_mut() = new.to_vec()); + new.to_vec() } } diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 0230937fc49c7..0f9222a5f6767 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -934,40 +934,71 @@ impl WithdrawReasons { } } -/// Trait for type that can handle incremental changes to a set of account IDs. +/// Trait for types that can handle incremental changes to a set of account IDs. +/// +/// Allows member changes according to the "verify first, write last" maxime by +/// providing the `ensure_can_change_members` function for checking as well as the +/// actual writing logic. pub trait ChangeMembers { + + /// Returns whether changing the members will execute without errors. + /// + /// Defaults to a noop. Implement this if any of the other functions could fail. + /// + /// If this returns `Err`, calls to `change_members`, `change_members_sorted` and + /// `set_members_sorted` are not guaranteed to have the desired effect. + fn ensure_can_change_members(_new: &[AccountId], _old: &[AccountId]) -> DispatchResult { + Ok(()) + } + /// A number of members `incoming` just joined the set and replaced some `outgoing` ones. The /// new set is given by `new`, and need not be sorted. /// /// This resets any previous value of prime. - fn change_members(incoming: &[AccountId], outgoing: &[AccountId], mut new: Vec) { + /// + /// Returns the actual members set. This may differ from `new` if there are errors. + /// Use `ensure_can_change_members` to check for those beforehand. + fn change_members( + incoming: &[AccountId], + outgoing: &[AccountId], + mut new: Vec + ) -> Vec { new.sort_unstable(); - Self::change_members_sorted(incoming, outgoing, &new[..]); + Self::change_members_sorted(incoming, outgoing, &new[..]) } /// A number of members `_incoming` just joined the set and replaced some `_outgoing` ones. The /// new set is thus given by `sorted_new` and **must be sorted**. /// - /// NOTE: This is the only function that needs to be implemented in `ChangeMembers`. + /// NOTE: This is the only function that needs to be implemented in `ChangeMembers`, but make + /// sure that `ensure_can_change_members` is implemented correctly. /// /// This resets any previous value of prime. + /// + /// Returns the actual members set. This may differ from `sorted_new` if there are errors. + /// Use `ensure_can_change_members` to check for those beforehand. fn change_members_sorted( incoming: &[AccountId], outgoing: &[AccountId], sorted_new: &[AccountId], - ); + ) -> Vec; /// Set the new members; they **must already be sorted**. This will compute the diff and use it to /// call `change_members_sorted`. /// /// This resets any previous value of prime. - fn set_members_sorted(new_members: &[AccountId], old_members: &[AccountId]) { + /// + /// Returns the actual members set. This may differ from `new_members` if there are errors. + /// Use `ensure_can_change_members` to check for those beforehand. + fn set_members_sorted( + new_members: &[AccountId], + old_members: &[AccountId] + ) -> Vec { let (incoming, outgoing) = Self::compute_members_diff(new_members, old_members); - Self::change_members_sorted(&incoming[..], &outgoing[..], &new_members); + Self::change_members_sorted(&incoming[..], &outgoing[..], &new_members) } - /// Set the new members; they **must already be sorted**. This will compute the diff and use it to - /// call `change_members_sorted`. + /// Compute the changes between `new_members` and `old_members` and return them as `(incoming, outgoing)`. fn compute_members_diff( new_members: &[AccountId], old_members: &[AccountId] @@ -1003,13 +1034,15 @@ pub trait ChangeMembers { } /// Set the prime member. + /// + /// Defaults to a noop. fn set_prime(_prime: Option) {} } impl ChangeMembers for () { - fn change_members(_: &[T], _: &[T], _: Vec) {} - fn change_members_sorted(_: &[T], _: &[T], _: &[T]) {} - fn set_members_sorted(_: &[T], _: &[T]) {} + fn change_members(_: &[T], _: &[T], new: Vec) -> Vec { new } + fn change_members_sorted(_: &[T], _: &[T], new: &[T]) -> Vec { new.to_vec() } + fn set_members_sorted(new: &[T], _: &[T]) -> Vec { new.to_vec() } fn set_prime(_: Option) {} }