Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fix|NominationPools] Only allow apply slash to be executed if the slash amount is atleast ED #6540

Merged
merged 11 commits into from
Nov 21, 2024
Merged

[Fix|NominationPools] Only allow apply slash to be executed if the slash amount is atleast ED #6540

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
89fc9f1
only allow apply slash if amount is atleast ED
Ank4n Nov 19, 2024
195204d
revert delegated staking changes
Ank4n Nov 19, 2024
eff1f31
add prdoc
Ank4n Nov 19, 2024
43dd30a
keep it backward compatible
Ank4n Nov 19, 2024
086d7cd
remove duplicate test
Ank4n Nov 19, 2024
ce55a7f
minor
Ank4n Nov 19, 2024
5407bf4
small refactor
Ank4n Nov 19, 2024
02a73ae
Merge branch 'master' into ankan/fix/pool-min-slash-check
Ank4n Nov 19, 2024
d076dc1
Merge branch 'master' into ankan/fix/pool-min-slash-check
Ank4n Nov 20, 2024
d8413c2
Merge branch 'master' into ankan/fix/pool-min-slash-check
Ank4n Nov 20, 2024
c3d887a
Update prdoc/pr_6540.prdoc
Ank4n Nov 21, 2024
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: 16 additions & 0 deletions prdoc/pr_6540.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: Only allow apply slash to be executed if the slash amount is atleast ED

doc:
- audience: Runtime User
description: |
This change prevents `pools::apply_slash` from being executed when the pending slash amount of the member is lower
than the ED. With this change, such small slashes will still be applied but only when member funds are withdrawn.

crates:
- name: pallet-nomination-pools-runtime-api
bump: patch
- name: pallet-nomination-pools
bump: major
3 changes: 3 additions & 0 deletions substrate/frame/nomination-pools/runtime-api/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ sp_api::decl_runtime_apis! {
fn pool_pending_slash(pool_id: PoolId) -> Balance;

/// Returns the pending slash for a given pool member.
///
/// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on
/// chain.
fn member_pending_slash(member: AccountId) -> Balance;

/// Returns true if the pool with `pool_id` needs migration.
Expand Down
23 changes: 18 additions & 5 deletions substrate/frame/nomination-pools/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1944,6 +1944,8 @@ pub mod pallet {
NothingToAdjust,
/// No slash pending that can be applied to the member.
NothingToSlash,
/// The slash amount is too low to be applied.
SlashTooLow,
/// The pool or member delegation has already migrated to delegate stake.
AlreadyMigrated,
/// The pool or member delegation has not migrated yet to delegate stake.
Expand Down Expand Up @@ -2300,7 +2302,7 @@ pub mod pallet {

let slash_weight =
// apply slash if any before withdraw.
match Self::do_apply_slash(&member_account, None) {
match Self::do_apply_slash(&member_account, None, false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My previous comment only made sense when taken with another made to this line, which asked: why don't we want to enforce the ED checks here?

Sorry, I had the GH open for a while, it might have been lost when I refreshed the browser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point, a gas fee is charged for slashing. Additionally, we need to clean up all bookkeeping before a member withdraws.

Ok(_) => T::WeightInfo::apply_slash(),
Err(e) => {
let no_pending_slash: DispatchResult = Err(Error::<T>::NothingToSlash.into());
Expand Down Expand Up @@ -2974,8 +2976,10 @@ pub mod pallet {
/// Fails unless [`crate::pallet::Config::StakeAdapter`] is of strategy type:
/// [`adapter::StakeStrategyType::Delegate`].
///
/// This call can be dispatched permissionlessly (i.e. by any account). If the member has
/// slash to be applied, caller may be rewarded with the part of the slash.
/// The pending slash amount of the member must be equal or more than `ExistentialDeposit`.
/// This call can be dispatched permissionlessly (i.e. by any account). If the execution
/// is successful, fee is refunded and caller may be rewarded with a part of the slash
/// based on the [`crate::pallet::Config::StakeAdapter`] configuration.
#[pallet::call_index(23)]
#[pallet::weight(T::WeightInfo::apply_slash())]
pub fn apply_slash(
Expand All @@ -2989,7 +2993,7 @@ pub mod pallet {

let who = ensure_signed(origin)?;
let member_account = T::Lookup::lookup(member_account)?;
Self::do_apply_slash(&member_account, Some(who))?;
Self::do_apply_slash(&member_account, Some(who), true)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we want to to avoid slashing below the ED here (pub fn apply_slash)?


// If successful, refund the fees.
Ok(Pays::No.into())
Expand Down Expand Up @@ -3574,15 +3578,21 @@ impl<T: Config> Pallet<T> {
fn do_apply_slash(
member_account: &T::AccountId,
reporter: Option<T::AccountId>,
enforce_min_slash: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit but in order to prevent spreading do_apply_slash(..., bool) across the code which is not very readable, how about creating an enum that helps with readability? something along the lines of

enum SlashConfig {
 EnforceMinimum,
 DoNotEnforceMinimuim
}

Not sure about the enum and variant names, but in general

// reads better
do_apply_slash(..., SlashConfig::EnforceMinimum);

// than this
do_apply_slash(..., true);

) -> DispatchResult {
let member = PoolMembers::<T>::get(member_account).ok_or(Error::<T>::PoolMemberNotFound)?;

let pending_slash =
Self::member_pending_slash(Member::from(member_account.clone()), member.clone())?;

// if nothing to slash, return error.
// ensure there is something to slash.
ensure!(!pending_slash.is_zero(), Error::<T>::NothingToSlash);

if enforce_min_slash {
// ensure slashed amount is at least the minimum balance.
ensure!(pending_slash >= T::Currency::minimum_balance(), Error::<T>::SlashTooLow);
}

T::StakeAdapter::member_slash(
Member::from(member_account.clone()),
Pool::from(Pallet::<T>::generate_bonded_account(member.pool_id)),
Expand Down Expand Up @@ -3946,6 +3956,9 @@ impl<T: Config> Pallet<T> {
/// Returns the unapplied slash of a member.
///
/// Pending slash is only applicable with [`adapter::DelegateStake`] strategy.
///
/// If pending slash of the member exceeds `ExistentialDeposit`, it can be reported on
/// chain via [`Call::apply_slash`].
pub fn api_member_pending_slash(who: T::AccountId) -> BalanceOf<T> {
PoolMembers::<T>::get(who.clone())
.map(|pool_member| {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/nomination-pools/test-delegate-stake/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ sp-staking = { workspace = true, default-features = true }
sp-core = { workspace = true, default-features = true }

frame-system = { workspace = true, default-features = true }
frame-support = { workspace = true, default-features = true }
frame-support = { features = ["experimental"], workspace = true, default-features = true }
frame-election-provider-support = { workspace = true, default-features = true }

pallet-timestamp = { workspace = true, default-features = true }
Expand Down
37 changes: 33 additions & 4 deletions substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
mod mock;

use frame_support::{
assert_noop, assert_ok,
assert_noop, assert_ok, hypothetically,
traits::{fungible::InspectHold, Currency},
};
use mock::*;
Expand Down Expand Up @@ -537,10 +537,10 @@ fn pool_slash_proportional() {
// a typical example where 3 pool members unbond in era 99, 100, and 101, and a slash that
// happened in era 100 should only affect the latter two.
new_test_ext().execute_with(|| {
ExistentialDeposit::set(1);
ExistentialDeposit::set(2);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(CurrentEra::<T>::get(), None);
assert_eq!(Balances::minimum_balance(), 2);
assert_eq!(Staking::current_era(), None);

// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 40, 10, 10, 10));
Expand Down Expand Up @@ -670,6 +670,34 @@ fn pool_slash_proportional() {

// no pending slash yet.
assert_eq!(Pools::api_pool_pending_slash(1), 0);
// and therefore applying slash fails
assert_noop!(
Pools::apply_slash(RuntimeOrigin::signed(10), 21),
PoolsError::<Runtime>::NothingToSlash
);

hypothetically!({
// a very small amount is slashed
pallet_staking::slashing::do_slash::<Runtime>(
&POOL1_BONDED,
3,
&mut Default::default(),
&mut Default::default(),
100,
);

// ensure correct amount is pending to be slashed
assert_eq!(Pools::api_pool_pending_slash(1), 3);

// 21 has pending slash lower than ED (2)
assert_eq!(Pools::api_member_pending_slash(21), 1);

// slash fails as minimum pending slash amount not met.
assert_noop!(
Pools::apply_slash(RuntimeOrigin::signed(10), 21),
PoolsError::<Runtime>::SlashTooLow
);
});

pallet_staking::slashing::do_slash::<Runtime>(
&POOL1_BONDED,
Expand Down Expand Up @@ -909,6 +937,7 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() {
);
});
}

#[test]
fn pool_migration_e2e() {
new_test_ext().execute_with(|| {
Expand Down
Loading