[Staking] Currency <> Fungible migration #5501
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
Ank4n
changed the base branch from
master
to
ankan/staking-migrate-currency-to-fungible
…staking-migrate-currency-to-fungible-2
| // We should never have more than expected providers. | ||
| ensure!(actual_providers <= expected_providers, Error::<T>::BadState); | ||
|
|
||
| // if actual provider is less than expected, it is already migrated. | ||
| ensure!(actual_providers == expected_providers, Error::<T>::AlreadyMigrated); |
There was a problem hiding this comment.
this means it should be just equal. first ensure is redundant
There was a problem hiding this comment.
The errors will be different. If greater -> bad state, if less -> AlreadyMigrated.
There was a problem hiding this comment.
but you never get AlreadyMigrated error
| let expected_providers = | ||
| // provider is expected to be 1 but someone can always transfer some free funds to | ||
| // these accounts, increasing the provider. | ||
| if asset::free_to_stake::<T>(&stash) >= asset::existential_deposit::<T>() { |
There was a problem hiding this comment.
should not be balance >= ed? free_to_stake does not include the ED
There was a problem hiding this comment.
These (keyless) accounts had a explicit provider added to them by delegated staking.
When you have the extra provider, free_to_stake will include ED. Retrying to migrate will always fail as actual provider should be 0, always less than expected providers.
The test hold_migration::virtual_staker_consumer_provider_dec checks these scenarios.
| @@ -262,7 +264,8 @@ impl<T: Config, Staking: StakingInterface<Balance = BalanceOf<T>, AccountId = T: | |||
| pool_account: Pool<Self::AccountId>, | |||
| _: Member<Self::AccountId>, | |||
| ) -> BalanceOf<T> { | |||
| T::Currency::balance(&pool_account.0).saturating_sub(Self::active_stake(pool_account)) | |||
| // free/liquid balance of the pool account. | |||
There was a problem hiding this comment.
balance() contains the frozen part which means can be not liquid
There was a problem hiding this comment.
Technically this works because pool will never have frozen funds (its a pool generated pot account) but is confusing. I will fix it.
@Ank4n can you explain why force withdrawal not just 59 DOT? so locks are not there? |
Yes we had a bug that caused ledgers to be overwritten. It didn't exactly allow anyone to stake more than their balance, but it overwritten their controller account with stash account's stake. Most of them have been fixed here: polkadot-fellows/runtimes#447.
|
| impl DelegationInterface for DelegateMock { | ||
| type Balance = Balance; | ||
| type AccountId = AccountId; | ||
| fn agent_balance(agent: Agent<Self::AccountId>) -> Option<Self::Balance> { |
There was a problem hiding this comment.
this is basically the "staked, and active in staking (minus pending slash)" balance?
| @@ -1331,11 +1344,12 @@ fn disable_pool_operations_on_non_migrated() { | |||
| assert_ok!(Pools::migrate_pool_to_delegate_stake(RuntimeOrigin::signed(10), 1)); | |||
| assert_eq!( | |||
There was a problem hiding this comment.
This file had really good tests, I learned a lot by reading them!
|
PR is in good shape, and thanks to incremental code, the changes are easy to digest, good work!
|
There are probably more accounts that are affected. 59 is the count of accounts that had more than ED withdrawn from them.
There are runtime apis to query pool and member balance. How should we promote it? |
|
All GitHub workflows were cancelled due to failure one of the required jobs. |
Migrate staking currency from
traits::LockableCurrencytotraits::fungible::holds.Resolves part of #226.
Changes
Nomination Pool
TransferStake is now incompatible with fungible migration as old pools were not meant to have additional ED. Since they are anyways deprecated, removed its usage from all test runtimes.
Staking
Currencybecomes of typeFungiblewhileOldCurrencyis theLockableCurrencyused before.migrate_currency()releases the oldlockalong with some housekeeping.Delegated Staking
The pallet does not add provider for agents anymore.
Migration stats
Polkadot
Total accounts that can be migrated: 59564
Accounts failing to migrate: 0
Accounts with stake force withdrawn greater than ED: 59
Total force withdrawal: 29591.26 DOT
Kusama
Total accounts that can be migrated: 26311
Accounts failing to migrate: 0
Accounts with stake force withdrawn greater than ED: 48
Total force withdrawal: 1036.05 KSM
Full logs here.
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.
This PR now introduces requirement for stakers to maintain ED in their free balance. This helps with removing the bug prone incrementing and decrementing of consumers and providers.
TODO
Call::restore_ledgerbe removed? @gpestanamigrate_currency.Followup