Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Refactor staking ledger #14582

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
0c41dd3
Refactors StakingLedger to own module
gpestana Jul 12, 2023
cc12800
Refactors set_lock and remove_lock calls into StakingLedger methods; …
gpestana Jul 13, 2023
9ab1ba2
re-exposes Pallet::<T>::ledger which calls into StakingLedger methods
gpestana Jul 14, 2023
78f4ff6
Fixes benchmarks
gpestana Jul 14, 2023
8c4d2b0
Improves docs
gpestana Jul 17, 2023
2f34a49
Fixes rust docs
gpestana Jul 17, 2023
7382216
Improves expect message for controller
gpestana Jul 17, 2023
acb71b3
Encapsulates Bond mutations and reads into StakingLedger
gpestana Jul 18, 2023
f4b1062
Refactors leftover calls to Bonded and Ledger under StakingLedger
gpestana Jul 18, 2023
f1942b6
Further refactors
gpestana Jul 19, 2023
f01d214
Refactors staking ledger API; Adds documentation
gpestana Jul 20, 2023
b8a785d
Pallet::ledger now accepts a StakingAccount which can be a controller…
gpestana Jul 20, 2023
be0c582
nits
gpestana Jul 20, 2023
cfc4f2b
Simplifies logic and removes the StakingLedgerStatus; Adds ledger tests
gpestana Jul 26, 2023
eb3e731
Removes ledger.bond; fixes benchmarks
gpestana Jul 28, 2023
df90343
Adds ledger.bond again as syntatic sugar for ledger.update
gpestana Jul 29, 2023
f091fac
Adds tests for ledger.bond
gpestana Jul 29, 2023
7fefcef
Update frame/staking/src/pallet/impls.rs
gpestana Jul 29, 2023
9cb56cc
Update frame/staking/src/ledger.rs
gpestana Jul 29, 2023
2acb835
Update frame/staking/src/lib.rs
gpestana Jul 30, 2023
1198297
Refactors staking ledger get to result; other nits
gpestana Jul 31, 2023
283ab0b
nit
gpestana Jul 31, 2023
9e59fcc
Update frame/staking/src/ledger.rs
gpestana Jul 31, 2023
c56fa7e
Update frame/staking/src/pallet/impls.rs
gpestana Jul 31, 2023
2e0f050
Simplifies code and adds some comment nits
gpestana Jul 31, 2023
7bfe9cd
".git/.scripts/commands/fmt/fmt.sh"
Jul 31, 2023
2cd73bc
restart CI jobs
gpestana Jul 31, 2023
61928bc
Merge remote-tracking branch 'origin/master' into gpestana/staking-le…
Jul 31, 2023
d5c7ee3
Fixes EPM e2e tests
gpestana Jul 31, 2023
10fa2b3
Fixes rust docs warning
gpestana Jul 31, 2023
fe27f2c
Update frame/staking/src/ledger.rs
gpestana Aug 1, 2023
1929868
Update frame/staking/src/ledger.rs
gpestana Aug 1, 2023
fceeaa9
Update frame/staking/src/ledger.rs
gpestana Aug 1, 2023
55a1598
Update frame/staking/src/ledger.rs
gpestana Aug 1, 2023
47ff62e
Update frame/staking/src/pallet/impls.rs
gpestana Aug 1, 2023
c9850b1
Update frame/staking/src/pallet/impls.rs
gpestana Aug 1, 2023
778bc00
Adds license and mod rust docs to ledger
gpestana Aug 1, 2023
f788884
Update frame/staking/src/ledger.rs
gpestana Aug 1, 2023
5309a86
Update frame/staking/src/ledger.rs
gpestana Aug 1, 2023
90c2187
".git/.scripts/commands/fmt/fmt.sh"
Aug 1, 2023
ed99af2
addresses PR comments pt1
gpestana Aug 7, 2023
ad9b5ac
Update frame/staking/src/ledger.rs
gpestana Aug 7, 2023
e1093e5
addresses PR comments pt2
gpestana Aug 7, 2023
17abac8
improvements pt3. ledger consume itself on update
gpestana Aug 7, 2023
3060341
Merge branch 'master' into gpestana/staking-ledger-ref
gpestana Aug 10, 2023
c26b8e5
Moves back StakingLedger main impl to src/lib.rs to improve diff
gpestana Aug 10, 2023
c07a79f
adds tracking issue for moving StakingLedger impl
gpestana Aug 10, 2023
aee88cc
rollback to previous impl to decrease diff
gpestana Aug 10, 2023
073ec37
nit
gpestana Aug 10, 2023
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
10 changes: 5 additions & 5 deletions frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ fn ledger_consistency_active_balance_below_ed() {
ExtBuilder::default().staking(StakingExtBuilder::default()).build_offchainify();

ext.execute_with(|| {
assert_eq!(Staking::ledger(&11).unwrap().active, 1000);
assert_eq!(Staking::ledger(11.into()).unwrap().active, 1000);

// unbonding total of active stake fails because the active ledger balance would fall
// below the `MinNominatorBond`.
Expand All @@ -356,13 +356,13 @@ fn ledger_consistency_active_balance_below_ed() {

// the active balance of the ledger entry is 0, while total balance is 1000 until
// `withdraw_unbonded` is called.
assert_eq!(Staking::ledger(&11).unwrap().active, 0);
assert_eq!(Staking::ledger(&11).unwrap().total, 1000);
assert_eq!(Staking::ledger(11.into()).unwrap().active, 0);
assert_eq!(Staking::ledger(11.into()).unwrap().total, 1000);

// trying to withdraw the unbonded balance won't work yet because not enough bonding
// eras have passed.
assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(11), 0));
assert_eq!(Staking::ledger(&11).unwrap().total, 1000);
assert_eq!(Staking::ledger(11.into()).unwrap().total, 1000);

// tries to reap stash after chilling, which fails since the stash total balance is
// above ED.
Expand All @@ -384,6 +384,6 @@ fn ledger_consistency_active_balance_below_ed() {
pool_state,
);
assert_ok!(Staking::withdraw_unbonded(RuntimeOrigin::signed(11), 0));
assert_eq!(Staking::ledger(&11), None);
assert!(Staking::ledger(11.into()).is_err());
});
}
2 changes: 1 addition & 1 deletion frame/session/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ fn check_membership_proof_setup<T: Config>(
use rand::{RngCore, SeedableRng};

let validator = T::Lookup::lookup(who).unwrap();
let controller = pallet_staking::Pallet::<T>::bonded(validator).unwrap();
let controller = pallet_staking::Pallet::<T>::bonded(&validator).unwrap();

let keys = {
let mut keys = [0u8; 128];
Expand Down
16 changes: 8 additions & 8 deletions frame/staking/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ use frame_support::{
};
use sp_runtime::{
traits::{Bounded, One, StaticLookup, TrailingZeroInput, Zero},
Perbill, Percent,
Perbill, Percent, Saturating,
};
use sp_staking::{currency_to_vote::CurrencyToVote, SessionIndex};
use sp_std::prelude::*;
Expand Down Expand Up @@ -685,13 +685,13 @@ benchmarks! {
let stash = scenario.origin_stash1;

add_slashing_spans::<T>(&stash, s);
let l = StakingLedger {
stash: stash.clone(),
active: T::Currency::minimum_balance() - One::one(),
total: T::Currency::minimum_balance() - One::one(),
unlocking: Default::default(),
claimed_rewards: Default::default(),
};
let l = StakingLedger::<T>::new(
stash.clone(),
T::Currency::minimum_balance() - One::one(),
T::Currency::minimum_balance() - One::one(),
Default::default(),
Default::default(),
);
Ledger::<T>::insert(&controller, l);

assert!(Bonded::<T>::contains_key(&stash));
Expand Down
258 changes: 258 additions & 0 deletions frame/staking/src/ledger.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,258 @@
// This file is part of Substrate.

// Copyright (C) Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: Apache-2.0

// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

//! A Ledger implementation for stakers.
//!
//! A [`StakingLedger`] encapsulates all the state and logic related to the stake of bonded
//! stakers, namely, it handles the following storage items:
//! * [`Bonded`]: mutates and reads the state of the controller <> stash bond map (to be deprecated
//! soon);
//! * [`Ledger`]: mutates and reads the state of all the stakers. The [`Ledger`] storage item stores
//! instances of [`StakingLedger`] keyed by the staker's controller account and should be mutated
//! and read through the [`StakingLedger`] API;
//! * [`Payee`]: mutates and reads the reward destination preferences for a bonded stash.
//! * Staking locks: mutates the locks for staking.
//!
//! NOTE: All the storage operations related to the staking ledger (both reads and writes) *MUST* be
//! performed through the methods exposed by the [`StakingLedger`] implementation in order to ensure
//! state consistency.

use frame_support::{
defensive,
traits::{LockableCurrency, WithdrawReasons},
BoundedVec,
};
use sp_staking::{EraIndex, StakingAccount};
use sp_std::prelude::*;

use crate::{
BalanceOf, Bonded, Config, Error, Ledger, Payee, RewardDestination, StakingLedger, UnlockChunk,
STAKING_ID,
};

#[cfg(any(feature = "runtime-benchmarks", test))]
use {
codec::{Decode, Encode, MaxEncodedLen},
scale_info::TypeInfo,
sp_runtime::traits::Zero,
};

impl<T: Config> StakingLedger<T> {
#[cfg(any(feature = "runtime-benchmarks", test))]
pub fn default_from(stash: T::AccountId) -> Self {
Self {
stash,
total: Zero::zero(),
active: Zero::zero(),
unlocking: Default::default(),
claimed_rewards: Default::default(),
controller: Default::default(),
}
}

/// Returns a new instance of a staking ledger.
///
/// The [`Ledger`] storage is not mutated. In order to store, `StakingLedger::update` must be
/// called on the returned staking ledger.
///
/// Note: as the controller accounts are being deprecated, the stash account is the same as the
/// controller account.
pub fn new(
stash: T::AccountId,
active_stake: BalanceOf<T>,
total_stake: BalanceOf<T>,
unlocking: BoundedVec<UnlockChunk<BalanceOf<T>>, T::MaxUnlockingChunks>,
claimed_rewards: BoundedVec<EraIndex, T::HistoryDepth>,
) -> Self {
Self {
stash: stash.clone(),
active: active_stake,
total: total_stake,
unlocking,
claimed_rewards,
// controllers are deprecated and mapped 1-1 to stashes.
controller: Some(stash),
}
}

/// Returns the paired account, if any.
///
/// A "pair" refers to the tuple (stash, controller). If the input is a
/// [`StakingAccount::Stash`] variant, its pair account will be of type
/// [`StakingAccount::Controller`] and vice-versa.
///
/// This method is meant to abstract from the runtime development the difference between stash
/// and controller. This will be deprecated once the controller is fully deprecated as well.
pub(crate) fn paired_account(account: StakingAccount<T::AccountId>) -> Option<T::AccountId> {
match account {
StakingAccount::Stash(stash) => <Bonded<T>>::get(stash),
StakingAccount::Controller(controller) =>
<Ledger<T>>::get(&controller).map(|ledger| ledger.stash),
}
}

/// Returns whether a given account is bonded.
pub(crate) fn is_bonded(account: StakingAccount<T::AccountId>) -> bool {
match account {
StakingAccount::Stash(stash) => <Bonded<T>>::contains_key(stash),
StakingAccount::Controller(controller) => <Ledger<T>>::contains_key(controller),
}
}

/// Returns a staking ledger, if it is bonded and it exists in storage.
///
/// This getter can be called with either a controller or stash account, provided that the
/// account is properly wrapped in the respective [`StakingAccount`] variant. This is meant to
/// abstract the concept of controller/stash accounts from the caller.
pub(crate) fn get(account: StakingAccount<T::AccountId>) -> Result<StakingLedger<T>, Error<T>> {
let controller = match account {
StakingAccount::Stash(stash) => <Bonded<T>>::get(stash).ok_or(Error::<T>::NotStash),
StakingAccount::Controller(controller) => Ok(controller),
}?;

<Ledger<T>>::get(&controller)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not suggesting you do this as it is too much in the fringes, but hear me out:

// NewType for stash
pub struct Stash<T>(T::AccountId)
// NewType for controller 
pub struct Controller<T>(T::AccountId)

enum StakingAccount {
	Stash(Stash),
	Controller(Controller)
}

Now you can actually make the key of Ledger be Controller, not an opaque AccountId.

If deemed worthy, could be a nice follow-up, but I am worried it would have too much clutter, and once Controller is gone, it is all in vein.

But I hope the idea is clear. We are pury complicating things so that we can use Rust's type system for more safety.

.map(|mut ledger| {
ledger.controller = Some(controller.clone());
ledger
})
.ok_or_else(|| Error::<T>::NotController)
}

/// Returns the reward destination of a staking ledger, stored in [`Payee`].
///
/// Note: if the stash is not bonded and/or does not have an entry in [`Payee`], it returns the
/// default reward destination.
pub(crate) fn reward_destination(
account: StakingAccount<T::AccountId>,
) -> RewardDestination<T::AccountId> {
let stash = match account {
StakingAccount::Stash(stash) => Some(stash),
StakingAccount::Controller(controller) =>
Self::paired_account(StakingAccount::Controller(controller)),
};

if let Some(stash) = stash {
Copy link
Contributor

Choose a reason for hiding this comment

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

mostly equal to <Payee<T>>::get(stash).defensive_unwrap_or_default();, but I guess you might want the custom error log.

<Payee<T>>::get(stash)
} else {
defensive!("fetched reward destination from unbonded stash {}", stash);
RewardDestination::default()
}
}

/// Returns the controller account of a staking ledger.
///
/// Note: it will fallback into querying the [`Bonded`] storage with the ledger stash if the
/// controller is not set in `self`, which most likely means that self was fetched directly from
/// [`Ledger`] instead of through the methods exposed in [`StakingLedger`]. If the ledger does
/// not exist in storage, it returns `None`.
pub(crate) fn controller(&self) -> Option<T::AccountId> {
self.controller
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

defensive_or_else?

.or_else(|| Self::paired_account(StakingAccount::Stash(self.stash.clone())))
}

/// Inserts/updates a staking ledger account.
///
/// Bonds the ledger if it is not bonded yet, signalling that this is a new ledger. The staking
/// locks of the stash account are updated accordingly.
///
/// Note: To ensure lock consistency, all the [`Ledger`] storage updates should be made through
/// this helper function.
pub(crate) fn update(self) -> Result<(), Error<T>> {
if !<Bonded<T>>::contains_key(&self.stash) {
return Err(Error::<T>::NotStash)
}

T::Currency::set_lock(STAKING_ID, &self.stash, self.total, WithdrawReasons::all());
Ledger::<T>::insert(
&self.controller().ok_or_else(|| {
defensive!("update called on a ledger that is not bonded.");
Error::<T>::NotController
})?,
&self,
);

Ok(())
}

/// Bonds a ledger.
///
/// It sets the reward preferences for the bonded stash.
pub(crate) fn bond(self, payee: RewardDestination<T::AccountId>) -> Result<(), Error<T>> {
if <Bonded<T>>::contains_key(&self.stash) {
Err(Error::<T>::AlreadyBonded)
} else {
<Payee<T>>::insert(&self.stash, payee);
<Bonded<T>>::insert(&self.stash, &self.stash);
self.update()
}
}

/// Sets the ledger Payee.
pub(crate) fn set_payee(self, payee: RewardDestination<T::AccountId>) -> Result<(), Error<T>> {
if !<Bonded<T>>::contains_key(&self.stash) {
Err(Error::<T>::NotStash)
} else {
<Payee<T>>::insert(&self.stash, payee);
Ok(())
}
}

/// Clears all data related to a staking ledger and its bond in both [`Ledger`] and [`Bonded`]
/// storage items and updates the stash staking lock.
pub(crate) fn kill(stash: &T::AccountId) -> Result<(), Error<T>> {
let controller = <Bonded<T>>::get(stash).ok_or(Error::<T>::NotStash)?;

<Ledger<T>>::get(&controller).ok_or(Error::<T>::NotController).map(|ledger| {
T::Currency::remove_lock(STAKING_ID, &ledger.stash);
Ledger::<T>::remove(controller);

<Bonded<T>>::remove(&stash);
<Payee<T>>::remove(&stash);

Ok(())
})?
}
}

// This structs makes it easy to write tests to compare staking ledgers fetched from storage. This
// is required because the controller field is not stored in storage and it is private.
#[cfg(test)]
#[derive(frame_support::DebugNoBound, Clone, Encode, Decode, TypeInfo, MaxEncodedLen)]
pub struct StakingLedgerInspect<T: Config> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Some note on why we need this? Could PartialEq not be implemented for StakingLedger?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This structs makes it easy to write tests to compare staking ledgers fetched from storage. It is helpful because the controller field of StakingLedger is not stored in storage and it is private. (added comments)

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 a cool trick, just note the comment in #14582 (comment)

pub stash: T::AccountId,
#[codec(compact)]
pub total: BalanceOf<T>,
#[codec(compact)]
pub active: BalanceOf<T>,
pub unlocking: BoundedVec<UnlockChunk<BalanceOf<T>>, T::MaxUnlockingChunks>,
pub claimed_rewards: BoundedVec<EraIndex, T::HistoryDepth>,
}

#[cfg(test)]
impl<T: Config> PartialEq<StakingLedgerInspect<T>> for StakingLedger<T> {
fn eq(&self, other: &StakingLedgerInspect<T>) -> bool {
self.stash == other.stash &&
self.total == other.total &&
self.active == other.active &&
self.unlocking == other.unlocking &&
self.claimed_rewards == other.claimed_rewards
}
}

#[cfg(test)]
impl<T: Config> codec::EncodeLike<StakingLedger<T>> for StakingLedgerInspect<T> {}
Loading