From 80ac2b791734094a428ef42b0f5e96fc8ff0b00c Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Fri, 28 Jan 2022 17:13:22 -0800 Subject: [PATCH 1/2] Factor out checks for staking's & --- frame/staking/src/pallet/impls.rs | 85 ++++++++++++++++++++++++++++++- frame/staking/src/pallet/mod.rs | 66 ++---------------------- 2 files changed, 88 insertions(+), 63 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index ae20550cd40b6..e63147e716292 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -32,7 +32,7 @@ use frame_support::{ use frame_system::pallet_prelude::BlockNumberFor; use pallet_session::historical; use sp_runtime::{ - traits::{Bounded, Convert, SaturatedConversion, Saturating, Zero}, + traits::{Bounded, Convert, SaturatedConversion, Saturating, StaticLookup, Zero}, Perbill, }; use sp_staking::{ @@ -848,6 +848,89 @@ impl Pallet { DispatchClass::Mandatory, ); } + + /// Checks for [`Self::bond`] that can be completed at the beginning of the calls logic. + pub(crate) fn do_bond_checks( + stash: &T::AccountId, + controller: &T::AccountId, + value: BalanceOf, + ) -> Result<(), DispatchError> { + if Bonded::::contains_key(&stash) { + Err(Error::::AlreadyBonded)? + } + + if Ledger::::contains_key(&controller) { + Err(Error::::AlreadyPaired)? + } + + // Reject a bond which is considered to be _dust_. + if value < T::Currency::minimum_balance() { + Err(Error::::InsufficientBond)? + } + + Ok(()) + } + + /// Checks for [`Self::nominate`] that must be called prior to + /// [`Self::do_unchecked_nominate_writes`]. + pub(crate) fn do_nominate_checks( + controller: &T::AccountId, + targets: Vec<::Source>, + ) -> Result<(T::AccountId, BoundedVec), DispatchError> { + let ledger = Self::ledger(controller).ok_or(Error::::NotController)?; + ensure!(ledger.active >= MinNominatorBond::::get(), Error::::InsufficientBond); + + // Only check limits if they are not already a nominator. + if !Nominators::::contains_key(&ledger.stash) { + // If this error is reached, we need to adjust the `MinNominatorBond` and start + // calling `chill_other`. Until then, we explicitly block new nominators to protect + // the runtime. + if let Some(max_nominators) = MaxNominatorsCount::::get() { + ensure!(Nominators::::count() < max_nominators, Error::::TooManyNominators); + } + } + + ensure!(!targets.is_empty(), Error::::EmptyTargets); + ensure!(targets.len() <= T::MaxNominations::get() as usize, Error::::TooManyTargets); + + let old = + Nominators::::get(&ledger.stash).map_or_else(Vec::new, |x| x.targets.into_inner()); + + let targets: BoundedVec<_, _> = targets + .into_iter() + .map(|t| T::Lookup::lookup(t).map_err(DispatchError::from)) + .map(|n| { + n.and_then(|n| { + if old.contains(&n) || !Validators::::get(&n).blocked { + Ok(n) + } else { + Err(Error::::BadTarget.into()) + } + }) + }) + .collect::, _>>()? + .try_into() + .map_err(|_| Error::::TooManyNominators)?; + + Ok((ledger.stash, targets)) + } + + /// Write the information for nominating. The caller must first call + /// [`Self::do_nominate_checks`]. + pub(crate) fn do_unchecked_nominate_writes( + stash: &T::AccountId, + targets: BoundedVec, + ) { + let nominations = Nominations { + targets, + // Initial nominations are considered submitted at era 0. See `Nominations` doc. + submitted_in: Self::current_era().unwrap_or(0), + suppressed: false, + }; + + Self::do_remove_validator(&stash); + Self::do_add_nominator(&stash, nominations); + } } impl ElectionDataProvider for Pallet { diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 2a870fda063d3..955179270a04b 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -29,7 +29,7 @@ use frame_support::{ use frame_system::{ensure_root, ensure_signed, offchain::SendTransactionTypes, pallet_prelude::*}; use sp_runtime::{ traits::{CheckedSub, SaturatedConversion, StaticLookup, Zero}, - DispatchError, Perbill, Percent, + Perbill, Percent, }; use sp_staking::{EraIndex, SessionIndex}; use sp_std::{convert::From, prelude::*}; @@ -737,21 +737,9 @@ pub mod pallet { payee: RewardDestination, ) -> DispatchResult { let stash = ensure_signed(origin)?; - - if >::contains_key(&stash) { - Err(Error::::AlreadyBonded)? - } - let controller = T::Lookup::lookup(controller)?; - if >::contains_key(&controller) { - Err(Error::::AlreadyPaired)? - } - - // Reject a bond which is considered to be _dust_. - if value < T::Currency::minimum_balance() { - Err(Error::::InsufficientBond)? - } + Self::do_bond_checks(&stash, &controller, value)?; frame_system::Pallet::::inc_consumers(&stash).map_err(|_| Error::::BadState)?; @@ -1003,54 +991,8 @@ pub mod pallet { targets: Vec<::Source>, ) -> DispatchResult { let controller = ensure_signed(origin)?; - - let ledger = Self::ledger(&controller).ok_or(Error::::NotController)?; - ensure!(ledger.active >= MinNominatorBond::::get(), Error::::InsufficientBond); - let stash = &ledger.stash; - - // Only check limits if they are not already a nominator. - if !Nominators::::contains_key(stash) { - // If this error is reached, we need to adjust the `MinNominatorBond` and start - // calling `chill_other`. Until then, we explicitly block new nominators to protect - // the runtime. - if let Some(max_nominators) = MaxNominatorsCount::::get() { - ensure!( - Nominators::::count() < max_nominators, - Error::::TooManyNominators - ); - } - } - - ensure!(!targets.is_empty(), Error::::EmptyTargets); - ensure!(targets.len() <= T::MaxNominations::get() as usize, Error::::TooManyTargets); - - let old = Nominators::::get(stash).map_or_else(Vec::new, |x| x.targets.into_inner()); - - let targets: BoundedVec<_, _> = targets - .into_iter() - .map(|t| T::Lookup::lookup(t).map_err(DispatchError::from)) - .map(|n| { - n.and_then(|n| { - if old.contains(&n) || !Validators::::get(&n).blocked { - Ok(n) - } else { - Err(Error::::BadTarget.into()) - } - }) - }) - .collect::, _>>()? - .try_into() - .map_err(|_| Error::::TooManyNominators)?; - - let nominations = Nominations { - targets, - // Initial nominations are considered submitted at era 0. See `Nominations` doc. - submitted_in: Self::current_era().unwrap_or(0), - suppressed: false, - }; - - Self::do_remove_validator(stash); - Self::do_add_nominator(stash, nominations); + let (stash, targets) = Self::do_nominate_checks(&controller, targets)?; + Self::do_unchecked_nominate_writes(&stash, targets); Ok(()) } From f24f53f50b8809efdff6ef9916347ad90aa8ada0 Mon Sep 17 00:00:00 2001 From: emostov <32168567+emostov@users.noreply.github.com> Date: Sat, 29 Jan 2022 11:19:37 -0800 Subject: [PATCH 2/2] Use ensure naming --- frame/staking/src/pallet/impls.rs | 8 ++++---- frame/staking/src/pallet/mod.rs | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/frame/staking/src/pallet/impls.rs b/frame/staking/src/pallet/impls.rs index e63147e716292..4267c6dbaf002 100644 --- a/frame/staking/src/pallet/impls.rs +++ b/frame/staking/src/pallet/impls.rs @@ -850,7 +850,7 @@ impl Pallet { } /// Checks for [`Self::bond`] that can be completed at the beginning of the calls logic. - pub(crate) fn do_bond_checks( + pub(crate) fn ensure_can_bond( stash: &T::AccountId, controller: &T::AccountId, value: BalanceOf, @@ -873,7 +873,7 @@ impl Pallet { /// Checks for [`Self::nominate`] that must be called prior to /// [`Self::do_unchecked_nominate_writes`]. - pub(crate) fn do_nominate_checks( + pub(crate) fn ensure_can_nominate( controller: &T::AccountId, targets: Vec<::Source>, ) -> Result<(T::AccountId, BoundedVec), DispatchError> { @@ -916,8 +916,8 @@ impl Pallet { } /// Write the information for nominating. The caller must first call - /// [`Self::do_nominate_checks`]. - pub(crate) fn do_unchecked_nominate_writes( + /// [`Self::ensure_can_nominate`]. + pub(crate) fn do_unchecked_nominate( stash: &T::AccountId, targets: BoundedVec, ) { diff --git a/frame/staking/src/pallet/mod.rs b/frame/staking/src/pallet/mod.rs index 955179270a04b..d159e9332b0d4 100644 --- a/frame/staking/src/pallet/mod.rs +++ b/frame/staking/src/pallet/mod.rs @@ -739,7 +739,7 @@ pub mod pallet { let stash = ensure_signed(origin)?; let controller = T::Lookup::lookup(controller)?; - Self::do_bond_checks(&stash, &controller, value)?; + Self::ensure_can_bond(&stash, &controller, value)?; frame_system::Pallet::::inc_consumers(&stash).map_err(|_| Error::::BadState)?; @@ -991,8 +991,8 @@ pub mod pallet { targets: Vec<::Source>, ) -> DispatchResult { let controller = ensure_signed(origin)?; - let (stash, targets) = Self::do_nominate_checks(&controller, targets)?; - Self::do_unchecked_nominate_writes(&stash, targets); + let (stash, targets) = Self::ensure_can_nominate(&controller, targets)?; + Self::do_unchecked_nominate(&stash, targets); Ok(()) }