Skip to content
This repository was 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 1 commit
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
6 changes: 5 additions & 1 deletion 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
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)

- `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());
}

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>::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
71 changes: 48 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)
//!
//! - `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 @@ -198,6 +200,7 @@ pub enum MemberRole {
Founder,
Fellow,
Ally,
Retiring,
}

/// The type of item that may be deemed unscrupulous.
Expand Down Expand Up @@ -306,6 +309,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>;
}

#[pallet::error]
Expand All @@ -322,8 +329,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 +360,12 @@ pub mod pallet {
TooManyMembers,
/// Number of announcements exceeds `MaxAnnouncementsCount`.
TooManyAnnouncements,
/// Account is already gave retirement notice
AlreadyRetiring,
/// Account did not give a retirement notice required to retire.
RetirementNoticeNotGiven,
/// Retirement period is not passed.
RetirementPeriodNotPassed,
}

#[pallet::event]
Expand All @@ -380,6 +391,8 @@ pub mod pallet {
},
/// An ally has been elevated to Fellow.
AllyElevated { ally: T::AccountId },
/// A member gave retirement notice and 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 +496,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 +536,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 +795,39 @@ pub mod pallet {
Ok(())
}

/// As a member, give a retirement notice and start a retirement period required to pass in
/// order to retire.
// TODO setup weight T::WeightInfo::retirement_notice()
#[pallet::weight(0)]
pub fn 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);

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.
#[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 +853,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 +967,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
10 changes: 5 additions & 5 deletions frame/alliance/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,10 @@ impl pallet_balances::Config for Test {
type ReserveIdentifier = [u8; 8];
}

const MOTION_DURATION: u64 = 3;

parameter_types! {
pub const MotionDuration: u64 = 3;
pub const MotionDuration: u64 = MOTION_DURATION;
pub const MaxProposals: u32 = 100;
pub const MaxMembers: u32 = 100;
}
Expand Down Expand Up @@ -199,6 +201,7 @@ parameter_types! {
pub const MaxFellows: u32 = MaxMembers::get() - MaxFounders::get();
pub const MaxAllies: u32 = 100;
pub const AllyDeposit: u64 = 25;
pub const RetirementPeriod: u64 = MOTION_DURATION + 1;
}
impl Config for Test {
type Event = Event;
Expand All @@ -225,6 +228,7 @@ impl Config for Test {
type MaxMembersCount = MaxMembers;
type AllyDeposit = AllyDeposit;
type WeightInfo = ();
type RetirementPeriod = RetirementPeriod;
}

type UncheckedExtrinsic = frame_system::mocking::MockUncheckedExtrinsic<Test>;
Expand Down Expand Up @@ -324,7 +328,3 @@ pub fn make_proposal(value: u64) -> Call {
pub fn make_set_rule_proposal(rule: Cid) -> Call {
Call::Alliance(pallet_alliance::Call::set_rule { rule })
}

pub fn make_kick_member_proposal(who: u64) -> Call {
Call::Alliance(pallet_alliance::Call::kick_member { who })
}
70 changes: 49 additions & 21 deletions frame/alliance/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

use sp_runtime::traits::Hash;

use frame_support::{assert_noop, assert_ok, Hashable};
use frame_support::{assert_noop, assert_ok, error::BadOrigin, Hashable};
use frame_system::{EventRecord, Phase};

use super::*;
Expand Down Expand Up @@ -396,42 +396,70 @@ fn elevate_ally_works() {
}

#[test]
fn retire_works() {
fn retirement_notice_work() {
new_test_ext().execute_with(|| {
let proposal = make_kick_member_proposal(2);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
assert_ok!(Alliance::propose(
Origin::signed(1),
3,
Box::new(proposal.clone()),
proposal_len
assert_noop!(Alliance::retirement_notice(Origin::signed(4)), Error::<Test, ()>::NotMember);

assert_eq!(Alliance::members(MemberRole::Fellow), vec![3]);
assert_ok!(Alliance::retirement_notice(Origin::signed(3)));
assert_eq!(Alliance::members(MemberRole::Fellow), Vec::<u64>::new());
assert_eq!(Alliance::members(MemberRole::Retiring), vec![3]);
System::assert_last_event(mock::Event::Alliance(
crate::Event::MemberRetirementPeriodStarted { member: (3) },
));
assert_noop!(Alliance::retire(Origin::signed(2)), Error::<Test, ()>::UpForKicking);

assert_noop!(Alliance::retire(Origin::signed(4)), Error::<Test, ()>::NotMember);
assert_noop!(
Alliance::retirement_notice(Origin::signed(3)),
Error::<Test, ()>::AlreadyRetiring
);
});
}

#[test]
fn retire_works() {
new_test_ext().execute_with(|| {
assert_noop!(
Alliance::retire(Origin::signed(2)),
Error::<Test, ()>::RetirementNoticeNotGiven
);

assert_noop!(
Alliance::retire(Origin::signed(4)),
Error::<Test, ()>::RetirementNoticeNotGiven
);

assert_eq!(Alliance::members(MemberRole::Fellow), vec![3]);
assert_ok!(Alliance::retirement_notice(Origin::signed(3)));
assert_noop!(
Alliance::retire(Origin::signed(3)),
Error::<Test, ()>::RetirementPeriodNotPassed
);
System::set_block_number(System::block_number() + RetirementPeriod::get());
assert_ok!(Alliance::retire(Origin::signed(3)));
assert_eq!(Alliance::members(MemberRole::Fellow), Vec::<u64>::new());
System::assert_last_event(mock::Event::Alliance(crate::Event::MemberRetired {
member: (3),
unreserved: None,
}));
});
}

#[test]
fn kick_member_works() {
new_test_ext().execute_with(|| {
let proposal = make_kick_member_proposal(2);
let proposal_len: u32 = proposal.using_encoded(|p| p.len() as u32);
assert_ok!(Alliance::propose(
Origin::signed(1),
3,
Box::new(proposal.clone()),
proposal_len
));
assert_eq!(Alliance::up_for_kicking(2), true);
assert_eq!(Alliance::members(MemberRole::Founder), vec![1, 2]);
assert_noop!(Alliance::kick_member(Origin::signed(4), 4), BadOrigin);

assert_noop!(Alliance::kick_member(Origin::signed(2), 4), Error::<Test, ()>::NotMember);

<DepositOf<Test, ()>>::insert(2, 25);
assert_eq!(Alliance::members(MemberRole::Founder), vec![1, 2]);
assert_ok!(Alliance::kick_member(Origin::signed(2), 2));
assert_eq!(Alliance::members(MemberRole::Founder), vec![1]);
assert_eq!(<DepositOf<Test, ()>>::get(2), None);
System::assert_last_event(mock::Event::Alliance(crate::Event::MemberKicked {
member: (2),
slashed: Some(25),
}));
});
}

Expand Down