[Staking] Noop refactor to prep pallet for currency fungible migration #5399

Ank4n · 2024-08-18T19:04:39Z

This is a no-op refactor of staking pallet to move all T::Currency api calls under one module.

A followup PR (#5501) will implement the Currency <> Fungible migration for the pallet.

Introduces the new asset module that centralizes all interaction with T::Currency. This is an attempt to try minimising staking logic changes to minimal parts of the codebase.

Things of note

T::Currency::free_balance in current implementation includes both staked (locked) and liquid tokens (kinda sounds wrong to call it free then). This PR renames it to stakeable_balance (any better name suggestions?). With [Staking] Currency <> Fungible migration #5501, this will become free balance that can be held/staked + already held/staked balance.

gpestana · 2024-09-09T20:17:33Z

substrate/frame/staking/src/asset.rs

+/// Stakeable balance of `who`.
+///
+/// This includes balance free to stake along with any balance that is already staked.
+pub fn stakeable_balance<T: Config>(who: &T::AccountId) -> BalanceOf<T> {


How about we do it from another perspective and expose both:

fn asset::staked and

fn asset::free_to_stake

And the remove fn stakeable_balance, which can be calculated by summing "staked + free_to_stake".

This makes sense in this PR but probably not here.

Clarification: The only way to calculate free_to_stake in currency::locks world is by checking free_balance (aka stakeable_balance) - locked_balance.

gpestana · 2024-09-09T20:19:55Z

substrate/frame/staking/src/asset.rs

+/// Kill the stake of `who`.
+///
+/// All locked amount is unlocked.
+pub fn kill_stake<T: Config>(who: &T::AccountId) {


Maybe unlock_all_stake instead? Kill stake is very similar to kill ledger etc, although in a completely different context.

I actually named it keeping in convention with kill ledger. This is only to be used while killing ledger.
Can rename if you still think it is confusing.

gpestana · 2024-09-09T20:23:36Z

substrate/frame/staking/src/ledger.rs

@@ -269,7 +261,7 @@ impl<T: Config> StakingLedger<T> {
 			// kill virtual staker if it exists.
 			if <VirtualStakers<T>>::take(&stash).is_none() {
 				// if not virtual staker, clear locks.
-				T::Currency::remove_lock(STAKING_ID, &ledger.stash);
+				asset::kill_stake::<T>(&ledger.stash);


Suggested change

asset::kill_stake::<T>(&ledger.stash);

asset::unlock_all_stake::<T>(&ledger.stash);

nit

gpestana · 2024-09-09T20:32:32Z

substrate/frame/staking/src/asset.rs

+/// Mint `value` into an existing account `who`.
+///
+/// This does not increase the total issuance.
+pub fn mint_existing<T: Config>(


It seems to me that one goal of this module is to abstract the asset interface so that we can make the currency->fungible refactor and ensure the changes are concentrated in this module. I think this is a good idea. How about going a step forward and ensure the naming does not need to change in future designs of the inflation/rewards? E.g. in Plaza, we are not expected to "mint_into" but rather transfer or reward into an account. wdyt?

Yeah makes sense. I can call it mint_reward? OTOH, this would be a simple rename in future so we probably don't need to worry about it much.

gpestana · 2024-09-09T20:34:31Z

substrate/frame/staking/src/pallet/mod.rs

@@ -779,7 +779,7 @@ pub mod pallet {
 					status
 				);
 				assert!(
-					T::Currency::free_balance(stash) >= balance,
+					asset::stakeable_balance::<T>(stash) >= balance,


Suggested change

asset::stakeable_balance::<T>(stash) >= balance,

asset::free_to_stake::<T>(stash) >= balance,

Just an idea, as per my comment in the asset module.

gpestana · 2024-09-09T20:35:26Z

substrate/primitives/staking/src/lib.rs

@@ -619,7 +619,7 @@ pub trait DelegationMigrator {
 	///
 	/// Also removed from [`StakingUnchecked`] as a Virtual Staker. Useful for testing.
 	#[cfg(feature = "runtime-benchmarks")]
-	fn migrate_to_direct_staker(agent: Agent<Self::AccountId>);
+	fn force_kill_agent(agent: Agent<Self::AccountId>);


unrelated change?

I backported some changes from #5501 to minimise diff in the other one. Earlier DelegationMigrator::migrate_to_direct_staker would kill agent + migrate to direct staker in staking. But due to the fungible changes this is not possible anymore (freezes can apply even without enough balance but for holds, we need to kill agent, transfer all funds from delegators to agent and then make it a direct staker).

It is also cleaner like this.

Ank4n · 2024-09-17T11:39:52Z

substrate/frame/delegated-staking/src/impls.rs

@@ -124,7 +124,7 @@ impl<T: Config> DelegationMigrator for Pallet<T> {

 	/// Only used for testing.
 	#[cfg(feature = "runtime-benchmarks")]
-	fn migrate_to_direct_staker(agent: Agent<Self::AccountId>) {


renamed this since it does not actually migrate to direct staker anymore but only kills agent.

kianenigma · 2024-09-18T09:05:42Z

substrate/frame/staking/src/asset.rs

+#[cfg(feature = "runtime-benchmarks")]
+pub fn burn<T: Config>(amount: BalanceOf<T>) -> PositiveImbalanceOf<T> {
+	T::Currency::burn(amount)
+}


This file is like a shim that makes the usage of LockableCurrency and Hold overlap, I can kind of see where you are going with this and it is good :)

kianenigma

LGTM as long as it is double checked to be a noop, I only checked asset.rs and not individual usage.

gui1117

PR is indeed no-op except for one benchmark which seems fixed.

gui1117 · 2024-10-07T14:44:29Z

substrate/frame/nomination-pools/benchmarking/src/inner.rs

@@ -131,6 +131,8 @@ fn migrate_to_transfer_stake<T: Config>(pool_id: PoolId) {
 			)
 			.expect("member should have enough balance to transfer");
 		});
+
+	pallet_staking::Pallet::<T>::migrate_to_direct_staker(&pool_acc);


this is not no-op, but it seems to be a fix.

It is actually no-op as well. Previously, T::StakeAdapter::remove_as_agent would do the migrate_to_direct_staker implicitly.

Ank4n · 2024-10-07T15:48:04Z

bot fmt

command-bot · 2024-10-07T15:48:09Z

@Ank4n https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7526196 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 25-8b26308c-7ab6-4b68-91a4-eb8f509c9c3b to cancel this command or bot cancel to cancel all commands in this pull request.

command-bot · 2024-10-07T15:50:01Z

@Ank4n Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7526196 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7526196/artifacts/download.

seadanda

Largely nits, no blockers

seadanda · 2024-10-07T16:00:03Z

substrate/frame/election-provider-multi-phase/test-staking-e2e/src/lib.rs

@@ -322,24 +322,24 @@ fn automatic_unbonding_pools() {
 		assert_eq!(<Runtime as pallet_nomination_pools::Config>::MaxUnbonding::get(), 1);

 		// init state of pool members.
-		let init_free_balance_2 = Balances::free_balance(2);
-		let init_free_balance_3 = Balances::free_balance(3);
+		let init_stakeable_balance_2 = pallet_staking::asset::stakeable_balance::<Runtime>(&2);


Maybe worth a use here to shorten these since they're used a few times. Likewise in other files with the new functions

seadanda · 2024-10-07T16:02:01Z

substrate/frame/fast-unstake/src/tests.rs


 		// Controller then changes mind and deregisters.
 		assert_ok!(FastUnstake::deregister(RuntimeOrigin::signed(1)));
-		assert_eq!(<T as Config>::Currency::reserved_balance(&1), 0);
+		assert_eq!(<T as Config>::Currency::reserved_balance(&1) - pre_reserved, 0);


Suggested change

assert_eq!(<T as Config>::Currency::reserved_balance(&1) - pre_reserved, 0);

assert_eq!(<T as Config>::Currency::reserved_balance(&1), pre_reserved);

seadanda · 2024-10-07T16:02:54Z

substrate/frame/fast-unstake/src/tests.rs

@@ -289,7 +297,7 @@ mod on_idle {
 			);
 			assert_eq!(Queue::<T>::count(), 3);

-			assert_eq!(<T as Config>::Currency::reserved_balance(&1), 0);
+			assert_eq!(<T as Config>::Currency::reserved_balance(&1) - pre_reserved, 0);


Suggested change

assert_eq!(<T as Config>::Currency::reserved_balance(&1) - pre_reserved, 0);

assert_eq!(<T as Config>::Currency::reserved_balance(&1), pre_reserved);

seadanda · 2024-10-07T16:08:32Z

substrate/frame/staking/src/asset.rs

+	T::Currency::total_issuance()
+}
+
+/// Total balance of `who`. Includes both, free and reserved.


Suggested change

/// Total balance of `who`. Includes both, free and reserved.

/// Total balance of `who`. Includes both free and reserved.

seadanda · 2024-10-07T16:32:49Z

substrate/frame/staking/src/asset.rs

+/// Mint `value` into an existing account `who`.
+///
+/// This does not increase the total issuance.
+pub fn mint_existing<T: Config>(


I would keep the "into" part of this here, as it's quite different from mint_creating.
mint_creating = mint into an account that you create
mint_into_existing = mint into an existing account that you don't create

So to me mint_into and mint_creating would make more sense if you want to keep things short, or just keeping the mint_into_existing pattern from before

Ank4n and others added 17 commits August 18, 2024 20:02

prep for abstracting out all currency calls in a separate mod

02d3a70

replace ED calls

fa1064f

replace ti, burn, and set balance calls

babc205

add read balance and update lock fn to asset

2f0cd28

replace all currency call with existing asset fn

4af4aee

add remaining currency fn

1995e06

all currency fn replaced

f3a3081

fix imports

6a03abb

remove unused import

040c630

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

3d3ceac

fix clippy

61eb7b7

repalce Balances api calls in test to asset as well

b8a0e4b

clippy

2926255

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

9e38e8e

rustdoc update

34e1d48

refactor

31c6467

reorder

851c5f3

Ank4n added the T2-pallets This PR/Issue is related to a particular pallet. label Aug 27, 2024

Ank4n changed the title ~~[WIP] Prep staking pallet for currency fungible migration~~ [Staking] Noop refactor to prep pallet for currency fungible migration Aug 27, 2024

Ank4n marked this pull request as ready for review August 27, 2024 12:17

Ank4n added the R0-silent Changes should not be mentioned in any release notes label Aug 27, 2024

Ank4n and others added 4 commits August 27, 2024 14:28

add license

fe893a8

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

087692b

fix rust doc

67bd3fa

refactor bench test since moving from currency to fungible change events

994c6c5

Ank4n requested a review from a team as a code owner August 28, 2024 12:40

Ank4n and others added 4 commits August 28, 2024 14:49

improve test comments

940e5e0

refactor offence benchmark

0c296ab

fix balance of

61bac35

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

12607ec

Ank4n and others added 6 commits August 28, 2024 17:28

refactor epm e2e tests

f62354b

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

a156423

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

a8b78a7

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

17144b0

port some changes from migration pr back to base

c1fc3dc

fmt

679f290

gpestana reviewed Sep 9, 2024

View reviewed changes

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

49c2045

paritytech-review-bot bot requested a review from a team September 11, 2024 15:32

Ank4n added 3 commits September 13, 2024 15:10

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

5af34d5

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

00841b6

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

d6258bc

Ank4n requested a review from kianenigma September 17, 2024 11:32

Ank4n commented Sep 17, 2024

View reviewed changes

Ank4n requested a review from gpestana September 17, 2024 11:51

kianenigma reviewed Sep 18, 2024

View reviewed changes

kianenigma approved these changes Sep 18, 2024

View reviewed changes

Ank4n added 2 commits September 20, 2024 17:02

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

b2d2245

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

748ec96

gui1117 approved these changes Oct 7, 2024

View reviewed changes

Ank4n enabled auto-merge October 7, 2024 15:31

Merge branch 'master' into ankan/staking-migrate-currency-to-fungible

e7c4e6e

seadanda approved these changes Oct 7, 2024

View reviewed changes

Ank4n added this pull request to the merge queue Oct 7, 2024

Merged via the queue into master with commit 9128dca Oct 7, 2024
206 of 208 checks passed

Ank4n deleted the ankan/staking-migrate-currency-to-fungible branch October 7, 2024 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Staking] Noop refactor to prep pallet for currency fungible migration #5399

[Staking] Noop refactor to prep pallet for currency fungible migration #5399

Ank4n commented Aug 18, 2024 •

edited

Loading

gpestana Sep 9, 2024

Ank4n Sep 11, 2024

Ank4n Sep 17, 2024

gpestana Sep 9, 2024

Ank4n Sep 17, 2024

gpestana Sep 9, 2024

gpestana Sep 9, 2024

Ank4n Sep 17, 2024

gpestana Sep 9, 2024

gpestana Sep 9, 2024

Ank4n Sep 17, 2024

Ank4n Sep 17, 2024 •

edited

Loading

kianenigma Sep 18, 2024

kianenigma left a comment

gui1117 left a comment

gui1117 Oct 7, 2024

Ank4n Oct 7, 2024

Ank4n commented Oct 7, 2024

command-bot bot commented Oct 7, 2024 •

edited

Loading

command-bot bot commented Oct 7, 2024

seadanda left a comment

seadanda Oct 7, 2024

seadanda Oct 7, 2024

seadanda Oct 7, 2024

seadanda Oct 7, 2024

seadanda Oct 7, 2024

	asset::kill_stake::<T>(&ledger.stash);
	asset::unlock_all_stake::<T>(&ledger.stash);

	asset::stakeable_balance::<T>(stash) >= balance,
	asset::free_to_stake::<T>(stash) >= balance,

	assert_eq!(<T as Config>::Currency::reserved_balance(&1) - pre_reserved, 0);
	assert_eq!(<T as Config>::Currency::reserved_balance(&1), pre_reserved);

	/// Total balance of `who`. Includes both, free and reserved.
	/// Total balance of `who`. Includes both free and reserved.

[Staking] Noop refactor to prep pallet for currency fungible migration #5399

[Staking] Noop refactor to prep pallet for currency fungible migration #5399

Conversation

Ank4n commented Aug 18, 2024 • edited Loading

Things of note

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

gui1117 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n commented Oct 7, 2024

command-bot bot commented Oct 7, 2024 • edited Loading

command-bot bot commented Oct 7, 2024

seadanda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ank4n commented Aug 18, 2024 •

edited

Loading

Ank4n Sep 17, 2024 •

edited

Loading

command-bot bot commented Oct 7, 2024 •

edited

Loading