Skip to content
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

[Pools] Fix issues with member migration to DelegateStake #4822

Merged
merged 54 commits into from
Aug 14, 2024

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Jun 18, 2024

Context

Pool members using the old TransferStake strategy were able to transfer all their funds to the pool. With DelegateStake changes, we want to ensure similar behaviour by allowing members to delegate all their stake to the pool.

Changes

  • Ensure all the balance including ED of an account can be delegated (and used in the pool) by adding a provider for delegators.
  • Gates calls that mutates the pool or pool member if they are in unmigrated state.
  • Adds remote test to migrate all pools and members to DelegateStake which can be used with Kusama and Polkadot runtime state. closes [Pools] Remote Externalities test for DelegateStake strategy #4629.
  • Add new runtime apis to read pool and member balance.

Addressing possible migration errors

Pool members migrating can run into two types of errors:

  • Already Staking: If the pool member is already staking, we cannot migrate them to DelegateStake since this may mean they are able to use the same staked funds in the pool. Users would need to withdraw all their funds from staking, in order to migrate their pool funds.
  • Pool contribution below ED: For these cases transfer from pool account to member account would fail. The affected users can top up their accounts and redo migration.

Another error that was earlier possible was when member's free balance is below ED. This PR adds a provider to delegator allowing all user balance including ED can be contributed towards the pool. This helps 1095 accounts in Polkadot and 41 accounts in Kusama to migrate now which would have earlier failed.

Results from RemoteExternalities Tests.

Kusama

Migration stats: success: 3017, direct_stakers: 361, unexpected_errors: 0

Polkadot

Migration stats: success: 42859, direct_stakers: 643, unexpected_errors: 0

TODO

  • Add runtime api for member total balance.
  • New issue to reap pool members with contribution below ED.
  • Add provider for delegators so whole balance including ED can be held while contributing to pools.
  • Gate all pool extrinsics if pool/member is in non-migrated state.

@Ank4n Ank4n force-pushed the ankan/delegate-stake-remote-tests branch from 396dded to 0c776b8 Compare June 29, 2024 00:24
substrate/frame/delegated-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/delegated-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/delegated-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/delegated-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/delegated-staking/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/nomination-pools/src/lib.rs Show resolved Hide resolved
substrate/frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7000318

@@ -99,3 +99,140 @@ fn check_treasury_pallet_id() {
westend_runtime_constants::TREASURY_PALLET_ID
);
}

#[cfg(all(test, feature = "try-runtime"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

cool 👍

substrate/frame/delegated-staking/src/lib.rs Show resolved Hide resolved
/// Remove self from storage.
pub(crate) fn remove(key: &T::AccountId) {
debug_assert!(<Agents<T>>::contains_key(key), "Agent should exist in storage");
<Agents<T>>::remove(key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<Agents<T>>::remove(key);
Agents::<T>::remove(key);

(same suggestion for fn update)

@Ank4n Ank4n added this pull request to the merge queue Aug 14, 2024
Merged via the queue into master with commit feacf2f Aug 14, 2024
174 of 176 checks passed
@Ank4n Ank4n deleted the ankan/delegate-stake-remote-tests branch August 14, 2024 20:18
ordian added a commit that referenced this pull request Aug 15, 2024
* master: (167 commits)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
  Fix OurViewChange small race (#5356)
  Make ticket non-optional and add ensure_successful method to Consideration trait (#5359)
  [tests] dedup test code, add more tests, improve naming and docs (#5338)
  Stop running the wishlist workflow on forks (#5297)
  Migrate foreign assets v3::Location to v4::Location (#4129)
  Minor clean up (#5284)
  [Pools] Ensure members can always exit the pool gracefully (#4998)
  StorageWeightReclaim: set to node pov size if higher (#5281)
  [Bot] Add prdoc generation (#5331)
  Small nits found accidentally along the way (#5341)
  Create subsystem-benchmarks.yml (#5325)
  Bump libp2p-identity from 0.2.8 to 0.2.9 (#5232)
  Bump authoring duration for async backing to 2s. (#5195)
  Fix spelling issues (#5206)
  Bump the known_good_semver group across 1 directory with 3 updates (#5315)
  ...
ordian added a commit that referenced this pull request Aug 16, 2024
* master:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
ordian added a commit that referenced this pull request Aug 16, 2024
…ct-candidate-weight

* ao-fix-parainclusion-weight-overestimation:
  Remove redundant minimal template workspace (#5330)
  approval-distribution: Fix handling of conclude (#5375)
  More logs in `is_potential_spam` from `dispute-coordinator` (#5252)
  Fix zombienet bridges test (#5373)
  Update Readme of the `polkadot` crate (#5326)
  allow for `u8` to be used as hold/freeze reason (#5348)
  Moving cargo check for runtimes to GHA (#5340)
  Update links in the documentation (#5175)
  fix visibility for `pallet_nfts` types used as call arguments (#3634)
  Correct some typos in crates' descriptions (#5262)
  Aura: Ensure we are building on each relay chain fork (#5352)
  Update Identity pallet README.md (#5183)
  Bump trie-db from 0.29.0 to 0.29.1 (#5231)
  [Coretime] Always include UnpaidExecution, not just when revenue is > 0 (#5369)
  [Pools] fix derivation of pool account (#4999)
  Upgrade accidentally downgraded deps (#5365)
  [Pools] Fix issues with member migration to `DelegateStake` (#4822)
  Unify `no_genesis` check (#5360)
  [CI] Fix prdoc command (#5358)
  Beefy: add benchmarks for `report_fork_voting()` (#5188)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T2-pallets This PR/Issue is related to a particular pallet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Pools] Remote Externalities test for DelegateStake strategy
4 participants