Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Alliance pallet: retirement notice call #11970

Merged
merged 17 commits into from
Aug 29, 2022
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1514,8 +1514,10 @@ impl pallet_state_trie_migration::Config for Runtime {
type WeightInfo = ();
}

const ALLIANCE_MOTION_DURATION: BlockNumber = 5 * DAYS;

parameter_types! {
pub const AllianceMotionDuration: BlockNumber = 5 * DAYS;
pub const AllianceMotionDuration: BlockNumber = ALLIANCE_MOTION_DURATION;
pub const AllianceMaxProposals: u32 = 100;
pub const AllianceMaxMembers: u32 = 100;
}
Expand All @@ -1537,6 +1539,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 + (1 * DAYS);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine in the node template but when we introduce a Root call to force dissolve the Alliance, we should make this longer than the root voting period.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding a bit to this, I think it's OK for the retirement period to be really long (e.g. >3 months). We expect Alliance members to be quite committed to the network and they shouldn't retire just because they want to unreserve their DOT the way that someone might unbond their DOT.

}

impl pallet_alliance::Config for Runtime {
Expand Down Expand Up @@ -1573,6 +1576,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!(
Expand Down Expand Up @@ -1678,9 +1682,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;
Expand All @@ -1699,6 +1710,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]
Expand Down
1 change: 1 addition & 0 deletions frame/alliance/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
31 changes: 26 additions & 5 deletions frame/alliance/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()))
Expand All @@ -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 as StaticLookup>::Source = T::Lookup::unlookup(fellow2.clone());
Expand Down
76 changes: 53 additions & 23 deletions frame/alliance/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -93,6 +95,7 @@ mod tests;

#[cfg(feature = "runtime-benchmarks")]
mod benchmarking;
pub mod migration;
mod types;
pub mod weights;

Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -198,6 +204,7 @@ pub enum MemberRole {
Founder,
Fellow,
Ally,
Retiring,
}

/// The type of item that may be deemed unscrupulous.
Expand All @@ -216,6 +223,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]
Expand Down Expand Up @@ -306,6 +314,10 @@ pub mod pallet {

/// Weight information for extrinsics in this pallet.
type WeightInfo: WeightInfo;

/// The period required to pass from retirement notice for a member to retire.
/// Supposed to be greater than time required to `kick_member`.
type RetirementPeriod: Get<Self::BlockNumber>;
bkontur marked this conversation as resolved.
Show resolved Hide resolved
}

#[pallet::error]
Expand All @@ -322,8 +334,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.
Expand Down Expand Up @@ -355,6 +365,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]
Expand All @@ -380,6 +396,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.
Expand Down Expand Up @@ -483,12 +501,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>;
Comment on lines +509 to +511
Copy link
Contributor

@joepetrowski joepetrowski Aug 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly just curious, why not store this in the MemberRole variant itself (i.e. Retiring(T::BlockNumber))? I'm not sure if we have FRAME guidance on which is more idiomatic or when one method is better than another for a given use case (@kianenigma or @shawntabrizi ?).

A storage item makes it easier for UIs to query all retiring members, but they could also periodically just check all the members and build that info from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now there is one storage map to store all members, where the key is a member role and value is map of AccountIds (Members).
And this additional map to store the BlockNumber for retirement period.


/// The current list of accounts deemed unscrupulous. These accounts non grata cannot submit
/// candidacy.
Expand Down Expand Up @@ -523,11 +541,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(())
}
Expand Down Expand Up @@ -787,15 +800,39 @@ 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.eq(&MemberRole::Retiring), Error::<T, I>::AlreadyRetiring);
bkontur marked this conversation as resolved.
Show resolved Hide resolved

Self::remove_member(&who, role)?;
Self::add_member(&who, MemberRole::Retiring)?;
<RetiringMembers<T, I>>::insert(
&who,
frame_system::Pallet::<T>::block_number() + T::RetirementPeriod::get(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

saturating_add might be a good idea here just to be safe as @bkontur mentioned above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

);

Self::deposit_event(Event::MemberRetirementPeriodStarted { member: who });
Ok(())
}

/// As a member, retire from the alliance and unreserve the deposit.
muharem marked this conversation as resolved.
Show resolved Hide resolved
/// 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);
Expand All @@ -821,8 +858,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(())
}
Expand Down Expand Up @@ -937,11 +972,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 {
Expand Down
72 changes: 72 additions & 0 deletions frame/alliance/src/migration.rs
Original file line number Diff line number Diff line change
@@ -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)
}
}
Loading