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

[Enhancement] Convert fast-unstake to use StakingInterface, decouplin… #12424

Merged
merged 48 commits into from
Oct 29, 2022
Merged

[Enhancement] Convert fast-unstake to use StakingInterface, decouplin… #12424

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
3a11e89
[Enhancement] Convert fast-unstake to use StakingInterface, decouplin…
ruseinov Oct 5, 2022
4f39589
Update primitives/staking/src/lib.rs
ruseinov Oct 6, 2022
9b8e123
Update primitives/staking/src/lib.rs
ruseinov Oct 6, 2022
25520d6
fix validator comment
ruseinov Oct 6, 2022
ad90903
remove todo
ruseinov Oct 10, 2022
89b32b5
ideas from Kian (#12474)
ruseinov Oct 11, 2022
f51c073
Merge remote-tracking branch 'origin/master' into ru/feature/fast-uns…
Oct 11, 2022
41d04ad
Rename StakingInterface -> Staking for nomination-pools
ruseinov Oct 12, 2022
7fc52f8
Staking fixes
ruseinov Oct 12, 2022
d3958ba
StakingInterface changes
ruseinov Oct 12, 2022
2ff689c
fix fast-unstake
ruseinov Oct 12, 2022
1fc6510
fix nomination-pools
ruseinov Oct 12, 2022
28d15d7
Merge remote-tracking branch 'origin/master' into ru/feature/fast-uns…
Oct 18, 2022
0705029
Fix fast-unstake tests
ruseinov Oct 18, 2022
39cf580
Fix benches for fast-unstake
ruseinov Oct 18, 2022
8093567
fix is_unbonding
ruseinov Oct 18, 2022
30a5882
fix nomination pools
ruseinov Oct 18, 2022
37107ef
fix node code
ruseinov Oct 19, 2022
097a9d1
add mock comments
ruseinov Oct 19, 2022
613a701
fix imports
ruseinov Oct 19, 2022
e785dd5
remove todo
ruseinov Oct 19, 2022
03586d7
more fixes
ruseinov Oct 19, 2022
20a4f29
more fixes
ruseinov Oct 19, 2022
9807cf2
bench fixes
ruseinov Oct 19, 2022
3639af8
more fixes
ruseinov Oct 19, 2022
b7827d5
more fixes
ruseinov Oct 20, 2022
5232b99
import fix
ruseinov Oct 20, 2022
f881456
more fixes
ruseinov Oct 20, 2022
c3cee71
more bench fix
ruseinov Oct 20, 2022
068c565
refix
ruseinov Oct 20, 2022
ff36f90
refix
ruseinov Oct 20, 2022
6f862e4
Update primitives/staking/src/lib.rs
ruseinov Oct 25, 2022
bff58a4
is_unbonding returns a result
ruseinov Oct 20, 2022
bdb525e
fix
ruseinov Oct 25, 2022
4bd57b1
review fixes
ruseinov Oct 25, 2022
f6040ec
more review fixes
ruseinov Oct 25, 2022
8c5604d
more fixes
ruseinov Oct 25, 2022
731fab2
more fixes
ruseinov Oct 25, 2022
b90082a
Update frame/fast-unstake/src/benchmarking.rs
ruseinov Oct 26, 2022
1eee8a5
remove redundant CurrencyBalance from nom-pools
ruseinov Oct 26, 2022
1f4fad2
remove CB
ruseinov Oct 26, 2022
27530dc
Merge remote-tracking branch 'origin/master' into ru/feature/fast-uns…
Oct 26, 2022
90cec9f
rephrase
ruseinov Oct 26, 2022
b1560aa
Apply suggestions from code review
kianenigma Oct 27, 2022
75a36bb
Update frame/nomination-pools/src/tests.rs
kianenigma Oct 27, 2022
b3e843e
finish damn renamed
kianenigma Oct 27, 2022
963837b
clippy fix
ruseinov Oct 27, 2022
cbed2f9
fix
ruseinov Oct 27, 2022
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
5 changes: 4 additions & 1 deletion frame/fast-unstake/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ pub mod pallet {
pub struct Pallet<T>(_);

#[pallet::config]
pub trait Config: frame_system::Config + pallet_staking::Config {
pub trait Config: frame_system::Config {
/// The overarching event type.
type RuntimeEvent: From<Event<Self>>
+ IsType<<Self as frame_system::Config>::RuntimeEvent>
Expand All @@ -125,6 +125,9 @@ pub mod pallet {
/// The origin that can control this pallet.
type ControlOrigin: frame_support::traits::EnsureOrigin<Self::RuntimeOrigin>;

/// The access to staking functionality.
type Staking: StakingInterface<Balance = BalanceOf<Self>, AccountId = Self::AccountId>;
kianenigma marked this conversation as resolved.
Show resolved Hide resolved

/// The weight information of this pallet.
type WeightInfo: WeightInfo;
}
Expand Down
2 changes: 1 addition & 1 deletion frame/staking/src/pallet/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ pub mod pallet {
type WeightInfo: WeightInfo;
}

/// The ideal number of staking participants.
/// The ideal number of active validators.
#[pallet::storage]
#[pallet::getter(fn validator_count)]
pub type ValidatorCount<T> = StorageValue<_, u32, ValueQuery>;
Expand Down
17 changes: 17 additions & 0 deletions primitives/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,23 @@ pub trait StakingInterface {
num_slashing_spans: u32,
) -> Result<bool, DispatchError>;

/// Get `StakingLedger` by controller account.
fn get_ledger(ctrl: &Self::AccountId) -> StakingLedger<Self>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kianenigma We might want to get rid of those, seems redundant now.:

	fn active_stake(controller: &Self::AccountId) -> Option<Self::Balance> {
		Self::ledger(controller).map(|l| l.active)
	}

	fn total_stake(controller: &Self::AccountId) -> Option<Self::Balance> {
		Self::ledger(controller).map(|l| l.total)
	}

Copy link
Contributor

Choose a reason for hiding this comment

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

Now I see why the old code was not returning StakingLedger. We cannot use StakingLedger in pallet-nomination-pools or pallet-fast-unstake because it is defined in pallet-stakin.

Putting it in a shared crate is also too much of a stretch in my opinion, because these types are arguably an implementation detail.

I think we should keep these, not have get_ledger and get_exposure and instead create other API function that express a condition, a state or a behavior rather than the explicit type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why don't we introduce a special type that will take values out of StakingLedger? Like type Ledger. It could even be an alias for now and then change if needed.

This makes very little sense to keep as is, because:

			let ledger =
				pallet_staking::Ledger::<T>::get(&ctrl).ok_or(Error::<T>::NotController)?;
			ensure!(!Queue::<T>::contains_key(&ledger.stash), Error::<T>::AlreadyQueued);
			ensure!(
				Head::<T>::get().map_or(true, |UnstakeRequest { stash, .. }| stash != ledger.stash),
				Error::<T>::AlreadyHead
			);
			// second part of the && is defensive.
			ensure!(
				ledger.active == ledger.total && ledger.unlocking.is_empty(),
				Error::<T>::NotFullyBonded
			);

We'd need to call three different methods that operate on exactly the same storage item. Actually no, 4!
At the very least that's not very dry and also a bit of overhead.

Copy link
Contributor

Choose a reason for hiding this comment

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

my answer to this and many other aspects can be found in ru/feature/fast-unstake-istaking...kiz-ru/feature/fast-unstake-istaking


/// The ideal number of active validators.
kianenigma marked this conversation as resolved.
Show resolved Hide resolved
fn validator_count() -> u32;

/// Whether or not there is an ongoing election.
fn ongoing() -> bool;

/// Force a current staker to become completely unstaked, immediately.
fn force_unstake(stash: T::AccountId) -> DispatchResult;

/// Checks whether an account `staker` has been exposed in an era.
/// TODO: shall we have this helper here or just expose ErasStakers
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe something to keep in mind: https://github.com/paritytech/ci_cd/issues/626

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I try to keep it readable, but yeah, I guess those comments could be longer. Need to find a good tool for intellij

/// storage instead for the caller to do as they please?
fn is_exposed_in_era(staker: &T::AccountId, era: &EraIndex) -> bool;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for now this API is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Although -- I take it back. We have get_ledger. a similar get_exposure would fit nicer next to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite a bit different, because we have multiple exposures.
So do we just return all the exposures that match staker, era? Otherwise it's inefficient.

I think it's best to keep this api tbh, because we are iterating by prefix. Or otherwise get_exposures with era.

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 actually fine -- see my comment about get_ledger.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine, we don't need eras anywhere else for now, so keeping this signature specific to the use-case sounds good and concise.


/// Get the nominations of a stash, if they are a nominator, `None` otherwise.
#[cfg(feature = "runtime-benchmarks")]
fn nominations(who: Self::AccountId) -> Option<sp_std::prelude::Vec<Self::AccountId>>;
Expand Down