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.

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

Add BondingRestriction config trait for staking pallet #8

Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,7 @@ impl pallet_staking::Config for Runtime {
type OnStakerSlash = NominationPools;
type WeightInfo = pallet_staking::weights::SubstrateWeight<Runtime>;
type BenchmarkingConfig = StakingBenchmarkingConfig;
type BondingRestriction = ();
}

parameter_types! {
Expand Down
1 change: 1 addition & 0 deletions frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ impl pallet_staking::Config for Test {
type OnStakerSlash = ();
type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
type WeightInfo = ();
type BondingRestriction = ();
}

impl pallet_offences::Config for Test {
Expand Down
1 change: 1 addition & 0 deletions frame/grandpa/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ impl pallet_staking::Config for Test {
type OnStakerSlash = ();
type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
type WeightInfo = ();
type BondingRestriction = ();
}

impl pallet_offences::Config for Test {
Expand Down
1 change: 1 addition & 0 deletions frame/nomination-pools/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ impl pallet_staking::Config for Runtime {
type OnStakerSlash = Pools;
type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
type WeightInfo = ();
type BondingRestriction = ();
}

parameter_types! {
Expand Down
1 change: 1 addition & 0 deletions frame/offences/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,6 +182,7 @@ impl pallet_staking::Config for Test {
type OnStakerSlash = ();
type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
type WeightInfo = ();
type BondingRestriction = ();
}

impl pallet_im_online::Config for Test {
Expand Down
1 change: 1 addition & 0 deletions frame/session/benchmarking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ impl pallet_staking::Config for Test {
type OnStakerSlash = ();
type BenchmarkingConfig = pallet_staking::TestBenchmarkingConfig;
type WeightInfo = ();
type BondingRestriction = ();
}

impl crate::Config for Test {}
Expand Down
16 changes: 16 additions & 0 deletions frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -917,3 +917,19 @@ impl BenchmarkingConfig for TestBenchmarkingConfig {
type MaxValidators = frame_support::traits::ConstU32<100>;
type MaxNominators = frame_support::traits::ConstU32<100>;
}

/// Means for checking if there is any external restriction on bonding with a specific account
///
/// Allows for parts of the runtime that might implement other forms of fund locking to prevent
/// incompatible locking on a stash account which could lead to unsafe state.
pub trait BondingRestriction<AccountId> {
/// Determine if bonding is allowed with stash and controller combination

Choose a reason for hiding this comment

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

I'm wondering whether it's a good idea to include controller here, since the controller account can also be changed via set_controller, in which case some controller-specific restrictions may be bypassed.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are of course mainly concerned with the stash account, I added controller to give ability to prevent bond in-case the controller had some other lock on it which would prevent user from unbonding or doing other actions with their controller account. Obviously I didn't see the set_controller. (And coming to think of it, even if we detected it earlier, nothing would prevent user from using their controller account to stake so it would not have really prevented that anyway) I will drop controller as suggested.

fn can_bond(stash: &AccountId) -> bool;
}

// No restrictions
impl<AccountId> BondingRestriction<AccountId> for () {
fn can_bond(_stash: &AccountId) -> bool {
true
}
}
11 changes: 11 additions & 0 deletions frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ use std::cell::RefCell;

pub const INIT_TIMESTAMP: u64 = 30_000;
pub const BLOCK_TIME: u64 = 1000;
pub const RESTRICTED_ACCOUNT: AccountId = 55500;

/// The AccountId alias in this test module.
pub(crate) type AccountId = u64;
Expand Down Expand Up @@ -302,6 +303,7 @@ impl crate::pallet::pallet::Config for Test {
type OnStakerSlash = OnStakerSlashMock<Test>;
type BenchmarkingConfig = TestBenchmarkingConfig;
type WeightInfo = ();
type BondingRestriction = AccountRestricted555;
}

impl<LocalCall> frame_system::offchain::SendTransactionTypes<LocalCall> for Test
Expand Down Expand Up @@ -887,3 +889,12 @@ pub(crate) fn staking_events() -> Vec<crate::Event<Test>> {
pub(crate) fn balances(who: &AccountId) -> (Balance, Balance) {
(Balances::free_balance(who), Balances::reserved_balance(who))
}

pub struct AccountRestricted555;

// Restrict stash account
impl BondingRestriction<AccountId> for AccountRestricted555 {
fn can_bond(stash: &AccountId) -> bool {
*stash != RESTRICTED_ACCOUNT
}
}
17 changes: 13 additions & 4 deletions frame/staking/src/pallet/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ mod impls;
pub use impls::*;

use crate::{
slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, EraPayout, EraRewardPoints, Exposure,
Forcing, MaxUnlockingChunks, NegativeImbalanceOf, Nominations, PositiveImbalanceOf, Releases,
RewardDestination, SessionInterface, StakingLedger, UnappliedSlash, UnlockChunk,
ValidatorPrefs,
slashing, weights::WeightInfo, ActiveEraInfo, BalanceOf, BondingRestriction, EraPayout,
EraRewardPoints, Exposure, Forcing, MaxUnlockingChunks, NegativeImbalanceOf, Nominations,
PositiveImbalanceOf, Releases, RewardDestination, SessionInterface, StakingLedger,
UnappliedSlash, UnlockChunk, ValidatorPrefs,
};

const STAKING_ID: LockIdentifier = *b"staking ";
Expand Down Expand Up @@ -200,6 +200,9 @@ pub mod pallet {

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

/// Interface for doing bonding restriction check
type BondingRestriction: BondingRestriction<Self::AccountId>;
}

#[pallet::type_value]
Expand Down Expand Up @@ -705,6 +708,8 @@ pub mod pallet {
TooManyValidators,
/// Commission is too low. Must be at least `MinCommission`.
CommissionTooLow,
/// External restriction prevents bonding with given account
BondingRestricted,
}

#[pallet::hooks]
Expand Down Expand Up @@ -788,6 +793,10 @@ pub mod pallet {
return Err(Error::<T>::AlreadyPaired.into())
}

if !T::BondingRestriction::can_bond(&stash) {
return Err(Error::<T>::BondingRestricted.into())
}

// Reject a bond which is considered to be _dust_.
if value < T::Currency::minimum_balance() {
return Err(Error::<T>::InsufficientBond.into())
Expand Down
29 changes: 28 additions & 1 deletion frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
use super::{ConfigOp, Event, MaxUnlockingChunks, *};
use frame_election_provider_support::{ElectionProvider, SortedListProvider, Support};
use frame_support::{
assert_noop, assert_ok, assert_storage_noop, bounded_vec,
assert_err, assert_noop, assert_ok, assert_storage_noop, bounded_vec,
dispatch::WithPostDispatchInfo,
pallet_prelude::*,
traits::{Currency, Get, ReservableCurrency},
Expand Down Expand Up @@ -5141,3 +5141,30 @@ fn proportional_ledger_slash_works() {
BTreeMap::from([(4, 0), (5, value_slashed), (6, 0), (7, 0)])
);
}

#[test]
fn bond_restriction_works() {
ExtBuilder::default().build_and_execute(|| {
start_session(2);
assert_err!(
Staking::bond(
Origin::signed(RESTRICTED_ACCOUNT),
1,
1000,
RewardDestination::Controller
),
Error::<Test>::BondingRestricted
);

// put some money in account that we'll use.
let _ = Balances::make_free_balance_be(&(RESTRICTED_ACCOUNT + 1), 2000);

// Using unrestricted account should not fail
assert_ok!(Staking::bond(
Origin::signed(RESTRICTED_ACCOUNT + 1),
1,
1000,
RewardDestination::Controller
));
});
}