Skip to content

Commit

Permalink
remove pallet::getter from pallet-staking (#6184)
Browse files Browse the repository at this point in the history
# Description

Part of #3326
Removes all pallet::getter occurrences from pallet-staking and replaces
them with explicit implementations.
Adds tests to verify that retrieval of affected entities works as
expected so via storage::getter.

## Review Notes

1. Traits added to the `derive` attribute are used in tests (either
directly or indirectly).
2. The getters had to be placed in a separate impl block since the other
one is annotated with `#[pallet::call]` and that requires
`#[pallet::call_index(0)]` annotation on each function in that block. So
I thought it's better to separate them.

---------

Co-authored-by: Dónal Murray <donal.murray@parity.io>
Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>
  • Loading branch information
3 people authored Nov 13, 2024
1 parent 8e3d929 commit 95d98e6
Show file tree
Hide file tree
Showing 21 changed files with 657 additions and 165 deletions.
2 changes: 1 addition & 1 deletion polkadot/runtime/westend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,7 @@ impl Get<u32> for MaybeSignedPhase {
fn get() -> u32 {
// 1 day = 4 eras -> 1 week = 28 eras. We want to disable signed phase once a week to test
// the fallback unsigned phase is able to compute elections on Westend.
if Staking::current_era().unwrap_or(1) % 28 == 0 {
if pallet_staking::CurrentEra::<Runtime>::get().unwrap_or(1) % 28 == 0 {
0
} else {
SignedPhase::get()
Expand Down
24 changes: 24 additions & 0 deletions prdoc/pr_6184.prdoc
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
title: Remove pallet::getter from pallet-staking
doc:
- audience: Runtime Dev
description: |
This PR removes all pallet::getter occurrences from pallet-staking and replaces them with explicit implementations.
It also adds tests to verify that retrieval of affected entities works as expected so via storage::getter.

crates:
- name: pallet-babe
bump: patch
- name: pallet-beefy
bump: patch
- name: pallet-election-provider-multi-phase
bump: patch
- name: pallet-grandpa
bump: patch
- name: pallet-nomination-pools
bump: patch
- name: pallet-root-offences
bump: patch
- name: westend-runtime
bump: patch
- name: pallet-staking
bump: patch
2 changes: 1 addition & 1 deletion substrate/frame/babe/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ pub fn start_session(session_index: SessionIndex) {
/// Progress to the first block at the given era
pub fn start_era(era_index: EraIndex) {
start_session((era_index * 3).into());
assert_eq!(Staking::current_era(), Some(era_index));
assert_eq!(pallet_staking::CurrentEra::<Test>::get(), Some(era_index));
}

pub fn make_primary_pre_digest(
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/babe/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ fn disabled_validators_cannot_author_blocks() {
// so we should still be able to author blocks
start_era(2);

assert_eq!(Staking::current_era().unwrap(), 2);
assert_eq!(pallet_staking::CurrentEra::<Test>::get().unwrap(), 2);

// let's disable the validator at index 0
Session::disable_index(0);
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/beefy/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,5 +366,5 @@ pub fn start_session(session_index: SessionIndex) {

pub fn start_era(era_index: EraIndex) {
start_session((era_index * 3).into());
assert_eq!(Staking::current_era(), Some(era_index));
assert_eq!(pallet_staking::CurrentEra::<Test>::get(), Some(era_index));
}
4 changes: 2 additions & 2 deletions substrate/frame/beefy/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ fn report_equivocation_current_set_works(mut f: impl ReportEquivocationFn) {
let authorities = test_authorities();

ExtBuilder::default().add_authorities(authorities).build_and_execute(|| {
assert_eq!(Staking::current_era(), Some(0));
assert_eq!(pallet_staking::CurrentEra::<Test>::get(), Some(0));
assert_eq!(Session::current_index(), 0);

start_era(1);
Expand Down Expand Up @@ -906,7 +906,7 @@ fn report_fork_voting_invalid_context() {

let mut era = 1;
let block_num = ext.execute_with(|| {
assert_eq!(Staking::current_era(), Some(0));
assert_eq!(pallet_staking::CurrentEra::<Test>::get(), Some(0));
assert_eq!(Session::current_index(), 0);
start_era(era);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ fn log_current_time() {
"block: {:?}, session: {:?}, era: {:?}, EPM phase: {:?} ts: {:?}",
System::block_number(),
Session::current_index(),
Staking::current_era(),
pallet_staking::CurrentEra::<Runtime>::get(),
CurrentPhase::<Runtime>::get(),
Now::<Runtime>::get()
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ use pallet_election_provider_multi_phase::{
unsigned::MinerConfig, Call, CurrentPhase, ElectionCompute, GeometricDepositBase,
QueuedSolution, SolutionAccuracyOf,
};
use pallet_staking::StakerStatus;
use pallet_staking::{ActiveEra, CurrentEra, ErasStartSessionIndex, StakerStatus};
use parking_lot::RwLock;
use std::sync::Arc;

Expand Down Expand Up @@ -806,11 +806,11 @@ pub(crate) fn start_active_era(
}

pub(crate) fn active_era() -> EraIndex {
Staking::active_era().unwrap().index
ActiveEra::<Runtime>::get().unwrap().index
}

pub(crate) fn current_era() -> EraIndex {
Staking::current_era().unwrap()
CurrentEra::<Runtime>::get().unwrap()
}

// Fast forward until EPM signed phase.
Expand Down Expand Up @@ -862,11 +862,11 @@ pub(crate) fn on_offence_now(
>],
slash_fraction: &[Perbill],
) {
let now = Staking::active_era().unwrap().index;
let now = ActiveEra::<Runtime>::get().unwrap().index;
let _ = Staking::on_offence(
offenders,
slash_fraction,
Staking::eras_start_session_index(now).unwrap(),
ErasStartSessionIndex::<Runtime>::get(now).unwrap(),
);
}

Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/grandpa/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ pub fn start_session(session_index: SessionIndex) {

pub fn start_era(era_index: EraIndex) {
start_session((era_index * 3).into());
assert_eq!(Staking::current_era(), Some(era_index));
assert_eq!(pallet_staking::CurrentEra::<Test>::get(), Some(era_index));
}

pub fn initialize_block(number: u64, parent_hash: H256) {
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/grandpa/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ fn report_equivocation_current_set_works() {
let authorities = test_authorities();

new_test_ext_raw_authorities(authorities).execute_with(|| {
assert_eq!(Staking::current_era(), Some(0));
assert_eq!(pallet_staking::CurrentEra::<Test>::get(), Some(0));
assert_eq!(Session::current_index(), 0);

start_era(1);
Expand Down
18 changes: 9 additions & 9 deletions substrate/frame/nomination-pools/test-delegate-stake/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use sp_staking::Agent;
fn pool_lifecycle_e2e() {
new_test_ext().execute_with(|| {
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
Expand Down Expand Up @@ -204,7 +204,7 @@ fn pool_lifecycle_e2e() {
fn pool_chill_e2e() {
new_test_ext().execute_with(|| {
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
Expand Down Expand Up @@ -330,7 +330,7 @@ fn pool_slash_e2e() {
new_test_ext().execute_with(|| {
ExistentialDeposit::set(1);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -540,7 +540,7 @@ fn pool_slash_proportional() {
ExistentialDeposit::set(1);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -758,7 +758,7 @@ fn pool_slash_non_proportional_only_bonded_pool() {
ExistentialDeposit::set(1);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -837,7 +837,7 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() {
ExistentialDeposit::set(1);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -914,7 +914,7 @@ fn pool_migration_e2e() {
new_test_ext().execute_with(|| {
LegacyAdapter::set(true);
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool with TransferStake strategy.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
Expand Down Expand Up @@ -1192,7 +1192,7 @@ fn disable_pool_operations_on_non_migrated() {
new_test_ext().execute_with(|| {
LegacyAdapter::set(true);
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool with TransferStake strategy.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
Expand Down Expand Up @@ -1369,7 +1369,7 @@ fn pool_no_dangling_delegation() {
new_test_ext().execute_with(|| {
ExistentialDeposit::set(1);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);
// pool creator
let alice = 10;
let bob = 20;
Expand Down
12 changes: 6 additions & 6 deletions substrate/frame/nomination-pools/test-transfer-stake/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ use sp_runtime::{bounded_btree_map, traits::Zero};
fn pool_lifecycle_e2e() {
new_test_ext().execute_with(|| {
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
Expand Down Expand Up @@ -286,7 +286,7 @@ fn destroy_pool_with_erroneous_consumer() {
fn pool_chill_e2e() {
new_test_ext().execute_with(|| {
assert_eq!(Balances::minimum_balance(), 5);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 50, 10, 10, 10));
Expand Down Expand Up @@ -412,7 +412,7 @@ fn pool_slash_e2e() {
new_test_ext().execute_with(|| {
ExistentialDeposit::set(1);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -622,7 +622,7 @@ fn pool_slash_proportional() {
ExistentialDeposit::set(1);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -759,7 +759,7 @@ fn pool_slash_non_proportional_only_bonded_pool() {
ExistentialDeposit::set(1);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), 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 @@ -838,7 +838,7 @@ fn pool_slash_non_proportional_bonded_pool_and_chunks() {
ExistentialDeposit::set(1);
BondingDuration::set(28);
assert_eq!(Balances::minimum_balance(), 1);
assert_eq!(Staking::current_era(), None);
assert_eq!(CurrentEra::<T>::get(), None);

// create the pool, we know this has id 1.
assert_ok!(Pools::create(RuntimeOrigin::signed(10), 40, 10, 10, 10));
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/root-offences/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ pub mod pallet {
fn get_offence_details(
offenders: Vec<(T::AccountId, Perbill)>,
) -> Result<Vec<OffenceDetails<T>>, DispatchError> {
let now = Staking::<T>::active_era()
let now = pallet_staking::ActiveEra::<T>::get()
.map(|e| e.index)
.ok_or(Error::<T>::FailedToGetActiveEra)?;

Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/root-offences/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -296,5 +296,5 @@ pub(crate) fn run_to_block(n: BlockNumber) {
}

pub(crate) fn active_era() -> EraIndex {
Staking::active_era().unwrap().index
pallet_staking::ActiveEra::<Test>::get().unwrap().index
}
2 changes: 1 addition & 1 deletion substrate/frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ mod benchmarks {
<ErasValidatorPrefs<T>>::insert(
current_era,
validator.clone(),
<Staking<T>>::validators(&validator),
Validators::<T>::get(&validator),
);

let caller = whitelisted_caller();
Expand Down
4 changes: 2 additions & 2 deletions substrate/frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -996,7 +996,7 @@ impl<T: Config> Convert<T::AccountId, Option<Exposure<T::AccountId, BalanceOf<T>
for ExposureOf<T>
{
fn convert(validator: T::AccountId) -> Option<Exposure<T::AccountId, BalanceOf<T>>> {
<Pallet<T>>::active_era()
ActiveEra::<T>::get()
.map(|active_era| <Pallet<T>>::eras_stakers(active_era.index, &validator))
}
}
Expand Down Expand Up @@ -1326,7 +1326,7 @@ impl<T: Config, const DISABLING_LIMIT_FACTOR: usize> DisablingStrategy<T>
log!(
debug,
"Won't disable: current_era {:?} > slash_era {:?}",
Pallet::<T>::current_era().unwrap_or_default(),
CurrentEra::<T>::get().unwrap_or_default(),
slash_era
);
return None
Expand Down
18 changes: 9 additions & 9 deletions substrate/frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -568,11 +568,11 @@ impl ExtBuilder {
}

pub(crate) fn active_era() -> EraIndex {
Staking::active_era().unwrap().index
pallet_staking::ActiveEra::<Test>::get().unwrap().index
}

pub(crate) fn current_era() -> EraIndex {
Staking::current_era().unwrap()
pallet_staking::CurrentEra::<Test>::get().unwrap()
}

pub(crate) fn bond(who: AccountId, val: Balance) {
Expand Down Expand Up @@ -663,7 +663,7 @@ pub(crate) fn start_active_era(era_index: EraIndex) {

pub(crate) fn current_total_payout_for_duration(duration: u64) -> Balance {
let (payout, _rest) = <Test as Config>::EraPayout::era_payout(
Staking::eras_total_stake(active_era()),
pallet_staking::ErasTotalStake::<Test>::get(active_era()),
pallet_balances::TotalIssuance::<Test>::get(),
duration,
);
Expand All @@ -673,7 +673,7 @@ pub(crate) fn current_total_payout_for_duration(duration: u64) -> Balance {

pub(crate) fn maximum_payout_for_duration(duration: u64) -> Balance {
let (payout, rest) = <Test as Config>::EraPayout::era_payout(
Staking::eras_total_stake(active_era()),
pallet_staking::ErasTotalStake::<Test>::get(active_era()),
pallet_balances::TotalIssuance::<Test>::get(),
duration,
);
Expand Down Expand Up @@ -732,11 +732,11 @@ pub(crate) fn on_offence_in_era(
}
}

if Staking::active_era().unwrap().index == era {
if pallet_staking::ActiveEra::<Test>::get().unwrap().index == era {
let _ = Staking::on_offence(
offenders,
slash_fraction,
Staking::eras_start_session_index(era).unwrap(),
pallet_staking::ErasStartSessionIndex::<Test>::get(era).unwrap(),
);
} else {
panic!("cannot slash in era {}", era);
Expand All @@ -750,7 +750,7 @@ pub(crate) fn on_offence_now(
>],
slash_fraction: &[Perbill],
) {
let now = Staking::active_era().unwrap().index;
let now = pallet_staking::ActiveEra::<Test>::get().unwrap().index;
on_offence_in_era(offenders, slash_fraction, now)
}

Expand Down Expand Up @@ -889,10 +889,10 @@ macro_rules! assert_session_era {
$session,
);
assert_eq!(
Staking::current_era().unwrap(),
CurrentEra::<T>::get().unwrap(),
$era,
"wrong current era {} != {}",
Staking::current_era().unwrap(),
CurrentEra::<T>::get().unwrap(),
$era,
);
};
Expand Down
Loading

0 comments on commit 95d98e6

Please sign in to comment.