Alliance pallet: retirement notice call (paritytech#11970)

* Alliance pallet: retirement notice

* add alliance pallet to benchmark list for dev chain

* fix type

* ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

* ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

* link weight generated by bench for retirement_notice method

* migration to clear UpForKicking storage prefix

* rename migration from v1 to v0_to_v1

* Apply suggestions from code review

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* rename `retirement-notice to give-retirement-notice

* Apply suggestions from code review

Co-authored-by: Squirrel <gilescope@gmail.com>

* review fixes: update doc, saturating add, BlockNumber type alias

* add suffix to duratin consts *_IN_BLOCKS

* ".git/.scripts/bench-bot.sh" pallet dev pallet_alliance

* add negative tests (paritytech#11995)

* add negative tests

* remove tests powerless asserts checking against announcment origin

* assert bad origin from announcement origin checks

Co-authored-by: muharem <ismailov.m.h@gmail.com>

Co-authored-by: command-bot <>
Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>
Co-authored-by: Squirrel <gilescope@gmail.com>

bin/node/runtime/src/lib.rs

@@ -1518,8 +1518,10 @@ impl pallet_state_trie_migration::Config for Runtime {
  type WeightInfo = ();
 }
 
+const ALLIANCE_MOTION_DURATION_IN_BLOCKS: BlockNumber = 5 * DAYS;
+
 parameter_types! {
- pub const AllianceMotionDuration: BlockNumber = 5 * DAYS;
+ pub const AllianceMotionDuration: BlockNumber = ALLIANCE_MOTION_DURATION_IN_BLOCKS;
  pub const AllianceMaxProposals: u32 = 100;
  pub const AllianceMaxMembers: u32 = 100;
 }
@@ -1541,6 +1543,7 @@ parameter_types! {
  pub const MaxFellows: u32 = AllianceMaxMembers::get() - MaxFounders::get();
  pub const MaxAllies: u32 = 100;
  pub const AllyDeposit: Balance = 10 * DOLLARS;
+ pub const RetirementPeriod: BlockNumber = ALLIANCE_MOTION_DURATION_IN_BLOCKS + (1 * DAYS);
 }
 
 impl pallet_alliance::Config for Runtime {
@@ -1577,6 +1580,7 @@ impl pallet_alliance::Config for Runtime {
  type MaxMembersCount = AllianceMaxMembers;
  type AllyDeposit = AllyDeposit;
  type WeightInfo = pallet_alliance::weights::SubstrateWeight<Runtime>;
+ type RetirementPeriod = RetirementPeriod;
 }
 
 construct_runtime!(
@@ -1682,9 +1686,16 @@ pub type Executive = frame_executive::Executive<
  frame_system::ChainContext<Runtime>,
  Runtime,
  AllPalletsWithSystem,
- pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>,
+ Migrations,
 >;
 
+// All migrations executed on runtime upgrade as a nested tuple of types implementing
+// `OnRuntimeUpgrade`.
+type Migrations = (
+ pallet_nomination_pools::migration::v2::MigrateToV2<Runtime>,
+ pallet_alliance::migration::Migration<Runtime>,
+);
+
 /// MMR helper types.
 mod mmr {
  use super::Runtime;
@@ -1703,6 +1714,7 @@ extern crate frame_benchmarking;
 mod benches {
  define_benchmarks!(
  [frame_benchmarking, BaselineBench::<Runtime>]
+ [pallet_alliance, Alliance]
  [pallet_assets, Assets]
  [pallet_babe, Babe]
  [pallet_bags_list, BagsList]

frame/alliance/README.md

@@ -43,6 +43,7 @@ to update the Alliance's rule and make announcements.
 
 #### For Members (All)
 
+- `give_retirement_notice` - Give a retirement notice and start a retirement period required to pass in order to retire.
 - `retire` - Retire from the Alliance and release the caller's deposit.
 
 #### For Members (Founders/Fellows)

frame/alliance/src/benchmarking.rs

@@ -691,12 +691,37 @@ benchmarks_instance_pallet! {
  assert_last_event::<T, I>(Event::AllyElevated { ally: ally1 }.into());
  }
 
+ give_retirement_notice {
+ set_members::<T, I>();
+ let fellow2 = fellow::<T, I>(2);
+
+ assert!(Alliance::<T, I>::is_fellow(&fellow2));
+ }: _(SystemOrigin::Signed(fellow2.clone()))
+ verify {
+ assert!(Alliance::<T, I>::is_member_of(&fellow2, MemberRole::Retiring));
+
+ assert_eq!(
+ RetiringMembers::<T, I>::get(&fellow2),
+ Some(System::<T>::block_number() + T::RetirementPeriod::get())
+ );
+ assert_last_event::<T, I>(
+ Event::MemberRetirementPeriodStarted {member: fellow2}.into()
+ );
+ }
+
  retire {
  set_members::<T, I>();
 
  let fellow2 = fellow::<T, I>(2);
  assert!(Alliance::<T, I>::is_fellow(&fellow2));
- assert!(!Alliance::<T, I>::is_up_for_kicking(&fellow2));
+
+ assert_eq!(
+ Alliance::<T, I>::give_retirement_notice(
+ SystemOrigin::Signed(fellow2.clone()).into()
+ ),
+ Ok(())
+ );
+ System::<T>::set_block_number(System::<T>::block_number() + T::RetirementPeriod::get());
 
  assert_eq!(DepositOf::<T, I>::get(&fellow2), Some(T::AllyDeposit::get()));
  }: _(SystemOrigin::Signed(fellow2.clone()))
@@ -713,11 +738,7 @@ benchmarks_instance_pallet! {
  set_members::<T, I>();
 
  let fellow2 = fellow::<T, I>(2);
- UpForKicking::<T, I>::insert(&fellow2, true);
-
  assert!(Alliance::<T, I>::is_member_of(&fellow2, MemberRole::Fellow));
- assert!(Alliance::<T, I>::is_up_for_kicking(&fellow2));
-
  assert_eq!(DepositOf::<T, I>::get(&fellow2), Some(T::AllyDeposit::get()));
 
  let fellow2_lookup = T::Lookup::unlookup(fellow2.clone());

frame/alliance/src/lib.rs

@@ -60,6 +60,8 @@
 //!
 //! #### For Members (All)
 //!
+//! - `give_retirement_notice` - Give a retirement notice and start a retirement period required to
+//! pass in order to retire.
 //! - `retire` - Retire from the Alliance and release the caller's deposit.
 //!
 //! #### For Members (Founders/Fellows)
@@ -93,13 +95,14 @@ mod tests;
 
 #[cfg(feature = "runtime-benchmarks")]
 mod benchmarking;
+pub mod migration;
 mod types;
 pub mod weights;
 
 use frame_support::pallet_prelude::*;
 use frame_system::pallet_prelude::*;
 use sp_runtime::{
- traits::{StaticLookup, Zero},
+ traits::{Saturating, StaticLookup, Zero},
  RuntimeDebug,
 };
 use sp_std::{convert::TryInto, prelude::*};
@@ -124,6 +127,9 @@ pub use pallet::*;
 pub use types::*;
 pub use weights::*;
 
+/// The log target of this pallet.
+pub const LOG_TARGET: &str = "runtime::alliance";
+
 /// Simple index type for proposal counting.
 pub type ProposalIndex = u32;
 
@@ -198,6 +204,7 @@ pub enum MemberRole {
  Founder,
  Fellow,
  Ally,
+ Retiring,
 }
 
 /// The type of item that may be deemed unscrupulous.
@@ -218,6 +225,7 @@ pub mod pallet {
 
  #[pallet::pallet]
  #[pallet::generate_store(pub (super) trait Store)]
+ #[pallet::storage_version(migration::STORAGE_VERSION)]
  pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
 
  #[pallet::config]
@@ -308,6 +316,10 @@ pub mod pallet {
 
  /// Weight information for extrinsics in this pallet.
  type WeightInfo: WeightInfo;
+
+ /// The number of blocks a member must wait between giving a retirement notice and retiring.
+ /// Supposed to be greater than time required to `kick_member`.
+ type RetirementPeriod: Get<Self::BlockNumber>;
  }
 
  #[pallet::error]
@@ -324,8 +336,6 @@ pub mod pallet {
  NotAlly,
  /// Account is not a founder.
  NotFounder,
- /// This member is up for being kicked from the Alliance and cannot perform this operation.
- UpForKicking,
  /// Account does not have voting rights.
  NoVotingRights,
  /// Account is already an elevated (fellow) member.
@@ -357,6 +367,12 @@ pub mod pallet {
  TooManyMembers,
  /// Number of announcements exceeds `MaxAnnouncementsCount`.
  TooManyAnnouncements,
+ /// Account already gave retirement notice
+ AlreadyRetiring,
+ /// Account did not give a retirement notice required to retire.
+ RetirementNoticeNotGiven,
+ /// Retirement period has not passed.
+ RetirementPeriodNotPassed,
  }
 
  #[pallet::event]
@@ -382,6 +398,8 @@ pub mod pallet {
  },
  /// An ally has been elevated to Fellow.
  AllyElevated { ally: T::AccountId },
+ /// A member gave retirement notice and their retirement period started.
+ MemberRetirementPeriodStarted { member: T::AccountId },
  /// A member has retired with its deposit unreserved.
  MemberRetired { member: T::AccountId, unreserved: Option<BalanceOf<T, I>> },
  /// A member has been kicked out with its deposit slashed.
@@ -485,12 +503,12 @@ pub mod pallet {
  ValueQuery,
  >;
 
- /// A set of members that are (potentially) being kicked out. They cannot retire until the
- /// motion is settled.
+ /// A set of members who gave a retirement notice. They can retire after the end of retirement
+ /// period stored as a future block number.
  #[pallet::storage]
- #[pallet::getter(fn up_for_kicking)]
- pub type UpForKicking<T: Config<I>, I: 'static = ()> =
- StorageMap<_, Blake2_128Concat, T::AccountId, bool, ValueQuery>;
+ #[pallet::getter(fn retiring_members)]
+ pub type RetiringMembers<T: Config<I>, I: 'static = ()> =
+ StorageMap<_, Blake2_128Concat, T::AccountId, T::BlockNumber, OptionQuery>;
 
  /// The current list of accounts deemed unscrupulous. These accounts non grata cannot submit
  /// candidacy.
@@ -525,11 +543,6 @@ pub mod pallet {
  let proposor = ensure_signed(origin)?;
  ensure!(Self::has_voting_rights(&proposor), Error::<T, I>::NoVotingRights);
 
- if let Some(Call::kick_member { who }) = proposal.is_sub_type() {
- let strike = T::Lookup::lookup(who.clone())?;
- <UpForKicking<T, I>>::insert(strike, true);
- }
-
  T::ProposalProvider::propose_proposal(proposor, threshold, proposal, length_bound)?;
  Ok(())
  }
@@ -783,15 +796,40 @@ pub mod pallet {
  Ok(())
  }
 
+ /// As a member, give a retirement notice and start a retirement period required to pass in
+ /// order to retire.
+ #[pallet::weight(T::WeightInfo::give_retirement_notice())]
+ pub fn give_retirement_notice(origin: OriginFor<T>) -> DispatchResult {
+ let who = ensure_signed(origin)?;
+ let role = Self::member_role_of(&who).ok_or(Error::<T, I>::NotMember)?;
+ ensure!(role.ne(&MemberRole::Retiring), Error::<T, I>::AlreadyRetiring);
+
+ Self::remove_member(&who, role)?;
+ Self::add_member(&who, MemberRole::Retiring)?;
+ <RetiringMembers<T, I>>::insert(
+ &who,
+ frame_system::Pallet::<T>::block_number()
+ .saturating_add(T::RetirementPeriod::get()),
+ );
+
+ Self::deposit_event(Event::MemberRetirementPeriodStarted { member: who });
+ Ok(())
+ }
+
  /// As a member, retire from the alliance and unreserve the deposit.
+ /// This can only be done once you have `give_retirement_notice` and it has expired.
  #[pallet::weight(T::WeightInfo::retire())]
  pub fn retire(origin: OriginFor<T>) -> DispatchResult {
  let who = ensure_signed(origin)?;
- // A member up for kicking cannot retire.
- ensure!(!Self::is_up_for_kicking(&who), Error::<T, I>::UpForKicking);
+ let retirement_period_end = RetiringMembers::<T, I>::get(&who)
+ .ok_or(Error::<T, I>::RetirementNoticeNotGiven)?;
+ ensure!(
+ frame_system::Pallet::<T>::block_number() >= retirement_period_end,
+ Error::<T, I>::RetirementPeriodNotPassed
+ );
 
- let role = Self::member_role_of(&who).ok_or(Error::<T, I>::NotMember)?;
- Self::remove_member(&who, role)?;
+ Self::remove_member(&who, MemberRole::Retiring)?;
+ <RetiringMembers<T, I>>::remove(&who);
  let deposit = DepositOf::<T, I>::take(&who);
  if let Some(deposit) = deposit {
  let err_amount = T::Currency::unreserve(&who, deposit);
@@ -814,8 +852,6 @@ pub mod pallet {
  T::Slashed::on_unbalanced(T::Currency::slash_reserved(&member, deposit).0);
  }
 
- <UpForKicking<T, I>>::remove(&member);
-
  Self::deposit_event(Event::MemberKicked { member, slashed: deposit });
  Ok(())
  }
@@ -930,11 +966,6 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
  founders.into()
  }
 
- /// Check if an account's forced removal is up for consideration.
- fn is_up_for_kicking(who: &T::AccountId) -> bool {
- <UpForKicking<T, I>>::contains_key(&who)
- }
-
  /// Add a user to the sorted alliance member set.
  fn add_member(who: &T::AccountId, role: MemberRole) -> DispatchResult {
  <Members<T, I>>::try_mutate(role, |members| -> DispatchResult {

frame/alliance/src/migration.rs

@@ -0,0 +1,72 @@
+// This file is part of Substrate.
+
+// Copyright (C) 2022 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.
+
+use crate::{Config, Pallet, Weight, LOG_TARGET};
+use frame_support::{pallet_prelude::*, storage::migration, traits::OnRuntimeUpgrade};
+use log;
+
+/// The current storage version.
+pub const STORAGE_VERSION: StorageVersion = StorageVersion::new(1);
+
+/// Wrapper for all migrations of this pallet.
+pub fn migrate<T: Config<I>, I: 'static>() -> Weight {
+ let onchain_version = Pallet::<T, I>::on_chain_storage_version();
+ let mut weight: Weight = 0;
+
+ if onchain_version < 1 {
+ weight = weight.saturating_add(v0_to_v1::migrate::<T, I>());
+ }
+
+ STORAGE_VERSION.put::<Pallet<T, I>>();
+ weight = weight.saturating_add(T::DbWeight::get().writes(1));
+
+ weight
+}
+
+/// Implements `OnRuntimeUpgrade` trait.
+pub struct Migration<T, I = ()>(PhantomData<(T, I)>);
+
+impl<T: Config<I>, I: 'static> OnRuntimeUpgrade for Migration<T, I> {
+ fn on_runtime_upgrade() -> Weight {
+ migrate::<T, I>()
+ }
+}
+
+/// v0_to_v1: `UpForKicking` is replaced by a retirement period.
+mod v0_to_v1 {
+ use super::*;
+
+ pub fn migrate<T: Config<I>, I: 'static>() -> Weight {
+ if migration::clear_storage_prefix(
+ <Pallet<T, I>>::name().as_bytes(),
+ b"UpForKicking",
+ b"",
+ None,
+ None,
+ )
+ .maybe_cursor
+ .is_some()
+ {
+ log::error!(
+ target: LOG_TARGET,
+ "Storage prefix 'UpForKicking' is not completely cleared."
+ );
+ }
+
+ T::DbWeight::get().writes(1)
+ }
+}