-
Notifications
You must be signed in to change notification settings - Fork 668
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Staking] Currency <> Fungible migration #5501
base: master
Are you sure you want to change the base?
Conversation
balance hold checks both frozen and reserved wip: around 25 tests failing check Holds instead of locks 20 tests failing fmt 11 fails 4 fails 2 failing 1 fail all tests pass but pending a hygiene check of code fix compile minor refactor remove T::Currency calls from asset mod
…staking-migrate-currency-to-fungible-2
…staking-migrate-currency-to-fungible-2
…staking-migrate-currency-to-fungible-2
@@ -41,42 +50,78 @@ pub fn total_balance<T: Config>(who: &T::AccountId) -> BalanceOf<T> { | |||
/// | |||
/// 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> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the fact that steakable_balance
naming does not map directly to free + staked, I would probably remove this method and replace it with free_to_stake::<T>(who).saturating_add(staked::<T>(who))
where it's needed.
} | ||
|
||
/// Deposit newly issued or slashed `value` into `who`. | ||
pub fn deposit_slashed<T: Config>(who: &T::AccountId, value: NegativeImbalanceOf<T>) { | ||
T::Currency::resolve_creating(who, value) | ||
let _ = T::Currency::resolve(who, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the docs, resolve
will fail if the total value
cannot be countered, which I would expect this never to happen in our context and probably not needed to propagate that error. However, should we at least fire a debug log if resolve fails?
…staking-migrate-currency-to-fungible-2
#5399) 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 #5501, this will become `free balance that can be held/staked` + `already held/staked balance`.
bot fmt |
@Ank4n https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7538682 was started for your command Comment |
@Ank4n Command |
Migrate staking currency from
traits::LockableCurrency
totraits::fungible::holds
.Summary of changes
Currency
becomes of typeFungible
whileOldCurrency
is theLockableCurrency
used before.migrate_currency()
releases the oldlock
along with some housekeeping.Migration stats
Polkadot
Total accounts that can be migrated: 61752
Accounts failing to migrate: 1
Accounts with some stake force withdrawn: 35
Total force withdrawal: 16287.7 DOT
Kusama
Total accounts that can be migrated: 26835
Accounts failing to migrate: 0
Accounts with some stake force withdrawn: 59
Total force withdrawal: 14.5 KSM
Full logs here. Error in polkadot is caused due to the ledger corruption, will be fixed by this runtime update. It is possible some of the force withdraws are happening because of ledger corruption and the migration stats might change after those ledgers are fixed.
Note about locks (freeze) vs holds
With locks or freezes, staking could use total balance of an account. But with holds, the account needs to be left with at least Existential Deposit in free balance. This would also affect nomination pools which till now has been able to stake all funds contributed to it. An alternate version of this PR is #5658 where staking pallet does not add any provider, but means pools and delegated-staking pallet has to provide for these accounts and makes the end to end logic (of provider and consumer ref) lot less intuitive and prone to bug.
One way to handle this issue is add a provider to staking stash. This then replicates old behaviour of staking where accounts could stake their whole balance. The side effect of this is that, for stakers that have been slashed to zero, their stash may not be reaped until all consumers to this account is removed. This is because,
dec_provider
would fail untilconsumer == 0
.Note about providers and consumers
Before
pallet-staking added consumers for stash account. This meant the agent/pool account must need a provider (by holding ED or system inc provider). We did an explicit inc provider for agents in pallet-delegated staking to allow consumer inc.
Now
This is more simplified.
pallet-staking
adds provider for stash accounts, but no consumer.pallet-delegated-staking
or pools does not need to add provider anymore (or consumer), and as stated before, helps retain the old behaviour of being able to stake all balance via both direct staking or pool.TODO
migrate_currency
.Followup