diff --git a/substrate/frame/alliance/src/lib.rs b/substrate/frame/alliance/src/lib.rs index f9e85e270a..111ea5dc6e 100644 --- a/substrate/frame/alliance/src/lib.rs +++ b/substrate/frame/alliance/src/lib.rs @@ -311,7 +311,9 @@ pub mod pallet { #[pallet::error] pub enum Error { /// The founders/fellows/allies have already been initialized. - MembersAlreadyInitialized, + AllianceAlreadyInitialized, + /// The Alliance has not been initialized yet, therefore accounts cannot join it. + AllianceNotYetInitialized, /// Account is already a member. AlreadyMember, /// Account is not a member. @@ -434,6 +436,11 @@ pub mod pallet { Members::::insert(MemberRole::Fellow, members); } if !self.allies.is_empty() { + // Only allow Allies if the Alliance is "initialized". + assert!( + Pallet::::is_initialized(), + "Alliance must have Founders or Fellows to have Allies" + ); let members: BoundedVec = self.allies.clone().try_into().expect("Too many genesis allies"); Members::::insert(MemberRole::Ally, members); @@ -612,6 +619,11 @@ pub mod pallet { ) -> DispatchResult { ensure_root(origin)?; + // Cannot be called if the Alliance already has Founders or Fellows. + // TODO: Remove check and allow Root to set members at any time. + // https://github.com/paritytech/substrate/issues/11928 + ensure!(!Self::is_initialized(), Error::::AllianceAlreadyInitialized); + let mut founders: BoundedVec = founders.try_into().map_err(|_| Error::::TooManyMembers)?; let mut fellows: BoundedVec = @@ -619,12 +631,6 @@ pub mod pallet { let mut allies: BoundedVec = allies.try_into().map_err(|_| Error::::TooManyMembers)?; - ensure!( - !Self::has_member(MemberRole::Founder) && - !Self::has_member(MemberRole::Fellow) && - !Self::has_member(MemberRole::Ally), - Error::::MembersAlreadyInitialized - ); for member in founders.iter().chain(fellows.iter()).chain(allies.iter()) { Self::has_identity(member)?; } @@ -705,6 +711,15 @@ pub mod pallet { pub fn join_alliance(origin: OriginFor) -> DispatchResult { let who = ensure_signed(origin)?; + // We don't want anyone to join as an Ally before the Alliance has been initialized via + // Root call. The reasons are two-fold: + // + // 1. There is no `Rule` or admission criteria, so the joiner would be an ally to + // nought, and + // 2. It adds complexity to the initialization, namely deciding to overwrite accounts + // that already joined as an Ally. + ensure!(Self::is_initialized(), Error::::AllianceNotYetInitialized); + // Unscrupulous accounts are non grata. ensure!(!Self::is_unscrupulous_account(&who), Error::::AccountNonGrata); ensure!(!Self::is_member(&who), Error::::AlreadyMember); @@ -867,6 +882,11 @@ pub mod pallet { } impl, I: 'static> Pallet { + /// Check if the Alliance has been initialized. + fn is_initialized() -> bool { + Self::has_member(MemberRole::Founder) || Self::has_member(MemberRole::Fellow) + } + /// Check if a given role has any members. fn has_member(role: MemberRole) -> bool { Members::::decode_len(role).unwrap_or_default() > 0 diff --git a/substrate/frame/alliance/src/mock.rs b/substrate/frame/alliance/src/mock.rs index d6e9a92a10..91986300aa 100644 --- a/substrate/frame/alliance/src/mock.rs +++ b/substrate/frame/alliance/src/mock.rs @@ -26,7 +26,7 @@ pub use sp_runtime::{ use sp_std::convert::{TryFrom, TryInto}; pub use frame_support::{ - assert_ok, ord_parameter_types, parameter_types, + assert_noop, assert_ok, ord_parameter_types, parameter_types, traits::{EitherOfDiverse, GenesisBuild, SortedMembers}, BoundedVec, }; @@ -291,6 +291,12 @@ pub fn new_test_ext() -> sp_io::TestExternalities { assert_ok!(Identity::provide_judgement(Origin::signed(1), 0, 5, Judgement::KnownGood)); assert_ok!(Identity::set_identity(Origin::signed(6), Box::new(info.clone()))); + // Joining before init should fail. + assert_noop!( + Alliance::join_alliance(Origin::signed(1)), + Error::::AllianceNotYetInitialized + ); + assert_ok!(Alliance::init_members(Origin::root(), vec![1, 2], vec![3], vec![])); System::set_block_number(1);