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

Initialize TotalValueUnbonding #2514

Closed
wants to merge 3 commits into from
Closed

Conversation

eagr
Copy link
Contributor

@eagr eagr commented Nov 28, 2023

closes #441

Adding a TotalValueUnbonding to on-chain storage for tracking the sum of unbonding balances across pools, which

  • increases on unbonding/unstaking
  • decreases on withdrawing unbonded funds (not when funds are unbonded)

These are implemented on BondedPool coz that's where the TVL ops go. I gather it'd be better to sorta put the similar logic in one place.

Verifications are done with

ensure!(LVU == sum(total_stake - active_stake))
ensure!(LVU <= sum(member.unbonding_balance()))

LMK if I'm mistaken about anything. :))

TODOs

  • update docs (after confirming the implementation)

Sorry, something went wrong.

@eagr eagr requested review from a team November 28, 2023 04:11
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 28, 2023 04:11
Comment on lines 114 to 117
ensure!(
TotalValueUnbonding::<T>::get() <= members_balance_unbonding,
"TotalValueUnbonding cannot surpass the sum of unbonding funds of members across all pools.",
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why's this <= not ==?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH, I'm not sure. :)) I'm just following the pattern above: TotalValueLocked <= members_balance_total

let total_stake = T::Staking::total_stake(&bonded_account).unwrap_or_default();
let active_stake =
T::Staking::active_stake(&bonded_account).unwrap_or_default();
total_stake - active_stake
Copy link
Contributor

Choose a reason for hiding this comment

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

Should use safe math here I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

active_stake <= total_stake && active_stake >= 0 should always hold right? If that's the contract, I would rather let the migration fail than carry on with the wrong states in this case.

Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't fail, it would silently underflow :P

@liamaharon
Copy link
Contributor

cc @gpestana @Ank4n

@eagr eagr marked this pull request as draft November 28, 2023 05:06
@@ -4333,6 +4378,8 @@ mod withdraw_unbonded {
pool_events_since_last_call(),
vec![Event::Withdrawn { member: 30, pool_id: 1, points: 5, balance: 5 }]
);
// FIXME eagr actual value 5?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this mean a bug in my code or I misunderstand the test steps?

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4526803

@eagr eagr marked this pull request as ready for review November 29, 2023 02:16
@eagr
Copy link
Contributor Author

eagr commented Nov 29, 2023

Some things I'm uncertain about.

Are the added assertions in the tests excessive or even necessary? Providing the added ensure! assertions seem to run on every test case. If those aren't necessary, I'm happy to remove them.

How do we determine whether the migration could be carried out in one block?

@gpestana
Copy link
Contributor

Are the added assertions in the tests excessive or even necessary?

I think that's OK. However, another way to achieve this is by implemeting a try_state check and make sure it runs at the end of each tests (which seems to be the case already). That way, you can probably relax on the amount of asserts in the tests that check the total unlocking and rely on the try state checks (test will if the checks fail). In general, this seems to be a good try-state check to have, although it may be heavy to run on-chain.

Copy link
Contributor

@gpestana gpestana left a comment

Choose a reason for hiding this comment

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

High level, this implementation for exposing the total value unbonding in the context of nomination pools seems fine. However, I think it would be better to implement this at a higher lever in the stack. Nomination pools are just a keyless staker account, module a few cosmetic differences. This said, I think it would be great if we could implement this feature so that both nomination pools, validators and nominations would expose this value.

To achieve this, we probably need to implement this logic at the staking pallet level and perhaps expose this functionality as part of the StakingInterface trait. wdyt?

@eagr
Copy link
Contributor Author

eagr commented Nov 30, 2023

Thanks for the advices! A couple questions.

  • Are TVL and TVU (if one should go upstream so should the other) useful enough to justify upstreaming them? I mean, there's also a risk of bloating the base trait with features that people may not need.
  • How about using a new trait for those features? Off the top of my head, something like trait SwarmStakingInterface, or maybe trait SwarmStakingInterface: StakingInterface.

I'm assuming that we want to keep the pallets like nomination-pools extensible (not coupled with the staking pallet), so I'm not considering the staking pallet. That right?

I don't see the whole picture, so I'd trust your judgements.

bkontur added a commit that referenced this pull request Dec 5, 2023
09215c5 Backport from `polkadot-sdk` + bump (#2725)
6327261 Bump serde from 1.0.192 to 1.0.193
fff9ddd Bump sysinfo from 0.29.10 to 0.29.11
4be99fe Monitoring and alerts for Rococo/Westend (#2710)
67a683a Bump ed25519-dalek from 2.0.0 to 2.1.0
8e0e794 quick and dirty fix for the `wait -p` and older distros (#2712)
3ab6562 Add withdraw reserve assets to zombienet tests (#2711)
c2c409b increase init timeouts in zombienet tests (#2706)
a8c60b4 fix lane id and bridged chain id (#2705)
9ac0f26 removed bp-asset-hub-kusama and bp-asset-hub-polkadot (#2703)
4916475 Some fixes for zombienet tests (polkadot-staging) (#2704)
6f9a147 zombienet from Wococo to Westend (#2699)
3ba7910 Porting changes from polkadot-sdk to polkadot-staging - before update subtree with removed wococo stuff (#2696)
653448f Remove Woococo related stuff (#2692)
03aaab2 Gitspiegel polkadot staging (#2695)
702a4c1 Drop Rialto <> Millau bridges (#2663) (#2694)
6a63b5f Start version guards for the ED loop (#2678)
896b9a9 typo (#2690)
671d27c Bump serde from 1.0.190 to 1.0.192
991b229 Bump clap from 4.4.7 to 4.4.8
ec267ec Bump env_logger from 0.10.0 to 0.10.1
592e407 Bump tokio from 1.33.0 to 1.34.0
c49ce3d Bump serde_json from 1.0.107 to 1.0.108
04b3319 Update subxt-codegen version (#2674)
03f9804 backport #2139 (#2673)
49245dd removed unused PARACHAINS_FINALITY_PALLET_NAME constant (#2670)
658a3f5 BHR/BHWE spec_version according to the `polkadot-sdk` (#2668)
7666b94 Nit from `polkadot-sdk` (#2665)
b5c43bb Adjusted constant because for measuring we used mistakenly rococo constants (#2664)
062449d Add Rococo<>Westend bridge support/relay (#2647)
55eb44e Add basic zombienet test to be used in the future (#2649) (#2660)
93b6b3f Bump clap from 4.4.6 to 4.4.7
4c01ab0 Bump futures from 0.3.28 to 0.3.29
a31a6c0 Bump tempfile from 3.8.0 to 3.8.1
bcdfe83 Bump serde from 1.0.189 to 1.0.190
f7433b0 Port #2648 to polkadot-staging (#2651)
3896738 Bump scale-info from 2.9.0 to 2.10.0
12d62c5 Bump thiserror from 1.0.49 to 1.0.50
1d78aa1 Backport from `polkadot-sdk` with actual master (#2633)
ab4de94 Grandpa justifications: Avoid duplicate vote ancestries (#2634) (#2635)
465562a add missing crate descriptions (#2629)
28d3680 Bump fixed-hash
67528c4 Bump serde from 1.0.188 to 1.0.189
d450c47 Bump time from 0.3.29 to 0.3.30
6a19f83 Bump async-trait from 0.1.73 to 0.1.74
a92d213 Millau, Rialto: accept equivocation reports (#2614) (#2617)
a61f777 Bump tokio from 1.32.0 to 1.33.0
0052f64 Bump subxt from 0.32.0 to 0.32.1
ccc849d Bump num-traits from 0.2.16 to 0.2.17
22f2752 apply late suggestions for #2600 (#2603)
0320172 actualize check_obsolete_call comment (#2601)
5cbbd25 Reject transactions if bridge pallets are halted (#2600)
ca4dfe3 Bump subxt from 0.31.0 to 0.32.0
8bf7b58 Bump clap from 4.4.4 to 4.4.6
88b0b99 Bump thiserror from 1.0.48 to 1.0.49
263833b https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3833103 (#2589)
4f44968 Backport changes from polkadot-sdk (#2588)
7200ed1 fiox overflow when computing priority boost (#2587)
e02cbd3 Bump time from 0.3.28 to 0.3.29
a097dd2 Bump clap from 4.4.3 to 4.4.4
801ce88 Merge bulletin chain changes into polkadot staging (#2574)
a3803ce Add unit tests for the equivocation detection loop (#2571)
26dfc31 Bump clap from 4.4.2 to 4.4.3
66a8beb Bump serde_json from 1.0.106 to 1.0.107
18c50da Bump trie-db from 0.27.1 to 0.28.0
4c4fa92 Equivocation detection loop: Reorganize block checking logic as state machine (#2555) (#2557)
6bd317a Bump serde_json from 1.0.105 to 1.0.106
a7e6bfd Backport for polkadot-sdk#1446 (#2546)
d9f8050 Bump sysinfo from 0.29.9 to 0.29.10
901f44c Bump thiserror from 1.0.47 to 1.0.48
82eeb50 Bump sysinfo from 0.29.8 to 0.29.9
a0c934b Bump strum from 0.24.1 to 0.25.0
1064fbf Bump subxt from 0.28.0 to 0.31.0
e50398d bridges subtree fixes (#2528)
99af075 Markdown linter (#1309) (#2526)
733ff0f `polkadot-staging` branch: Use polkadot-sdk dependencies (#2524)
e8a59f1 Fix benchmark with new XCM::V3 `MAX_INSTRUCTIONS_TO_DECODE` (#2514)
62b185d Backport `polkadot-sdk` changes to `polkadot-staging` (#2518)
d9658f4 Fix equivocation detection containers startup (#2516) (#2517)
d65db28 Backport: building images from locally built binaries (#2513)
5fdbaf4 Start the equivocation detection loop from the complex relayer (#2507) (#2512)
7fbb67d Backport: Implement basic equivocations detection loop (#2375)
cb7efe2 Manually update deps in polkadot staging (#2371)
d17981f #2351 to polkadot-staging (#2359)

git-subtree-dir: bridges
git-subtree-split: 09215c5
bkontur added a commit that referenced this pull request Dec 18, 2023
68d8650 Bump thiserror from 1.0.50 to 1.0.51
009c989 remove no longer valid check from the ensure_weights_are_correct (#2740)
94c44a7 Added Rococo BH <> Rococo Bulletin bridge (#2724)
5fe0f2f Bump tokio from 1.34.0 to 1.35.0
25f8251 Grafana update stuff (#2733)
06fbe8b Improved `ExportXcm::validate` implementation for BridgeHubs - step 1 (#2727)
390e836 Select header that will be fully refunded in on-demand batch finality relay (#2729)
ce701dd separate constants for average and worst case relay headers (#2728)
09215c5 Backport from `polkadot-sdk` + bump (#2725)
6327261 Bump serde from 1.0.192 to 1.0.193
fff9ddd Bump sysinfo from 0.29.10 to 0.29.11
4be99fe Monitoring and alerts for Rococo/Westend (#2710)
67a683a Bump ed25519-dalek from 2.0.0 to 2.1.0
8e0e794 quick and dirty fix for the `wait -p` and older distros (#2712)
3ab6562 Add withdraw reserve assets to zombienet tests (#2711)
c2c409b increase init timeouts in zombienet tests (#2706)
a8c60b4 fix lane id and bridged chain id (#2705)
9ac0f26 removed bp-asset-hub-kusama and bp-asset-hub-polkadot (#2703)
4916475 Some fixes for zombienet tests (polkadot-staging) (#2704)
6f9a147 zombienet from Wococo to Westend (#2699)
3ba7910 Porting changes from polkadot-sdk to polkadot-staging - before update subtree with removed wococo stuff (#2696)
653448f Remove Woococo related stuff (#2692)
03aaab2 Gitspiegel polkadot staging (#2695)
702a4c1 Drop Rialto <> Millau bridges (#2663) (#2694)
6a63b5f Start version guards for the ED loop (#2678)
896b9a9 typo (#2690)
671d27c Bump serde from 1.0.190 to 1.0.192
991b229 Bump clap from 4.4.7 to 4.4.8
ec267ec Bump env_logger from 0.10.0 to 0.10.1
592e407 Bump tokio from 1.33.0 to 1.34.0
c49ce3d Bump serde_json from 1.0.107 to 1.0.108
04b3319 Update subxt-codegen version (#2674)
03f9804 backport #2139 (#2673)
49245dd removed unused PARACHAINS_FINALITY_PALLET_NAME constant (#2670)
658a3f5 BHR/BHWE spec_version according to the `polkadot-sdk` (#2668)
7666b94 Nit from `polkadot-sdk` (#2665)
b5c43bb Adjusted constant because for measuring we used mistakenly rococo constants (#2664)
062449d Add Rococo<>Westend bridge support/relay (#2647)
55eb44e Add basic zombienet test to be used in the future (#2649) (#2660)
93b6b3f Bump clap from 4.4.6 to 4.4.7
4c01ab0 Bump futures from 0.3.28 to 0.3.29
a31a6c0 Bump tempfile from 3.8.0 to 3.8.1
bcdfe83 Bump serde from 1.0.189 to 1.0.190
f7433b0 Port #2648 to polkadot-staging (#2651)
3896738 Bump scale-info from 2.9.0 to 2.10.0
12d62c5 Bump thiserror from 1.0.49 to 1.0.50
1d78aa1 Backport from `polkadot-sdk` with actual master (#2633)
ab4de94 Grandpa justifications: Avoid duplicate vote ancestries (#2634) (#2635)
465562a add missing crate descriptions (#2629)
28d3680 Bump fixed-hash
67528c4 Bump serde from 1.0.188 to 1.0.189
d450c47 Bump time from 0.3.29 to 0.3.30
6a19f83 Bump async-trait from 0.1.73 to 0.1.74
a92d213 Millau, Rialto: accept equivocation reports (#2614) (#2617)
a61f777 Bump tokio from 1.32.0 to 1.33.0
0052f64 Bump subxt from 0.32.0 to 0.32.1
ccc849d Bump num-traits from 0.2.16 to 0.2.17
22f2752 apply late suggestions for #2600 (#2603)
0320172 actualize check_obsolete_call comment (#2601)
5cbbd25 Reject transactions if bridge pallets are halted (#2600)
ca4dfe3 Bump subxt from 0.31.0 to 0.32.0
8bf7b58 Bump clap from 4.4.4 to 4.4.6
88b0b99 Bump thiserror from 1.0.48 to 1.0.49
263833b https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3833103 (#2589)
4f44968 Backport changes from polkadot-sdk (#2588)
7200ed1 fiox overflow when computing priority boost (#2587)
e02cbd3 Bump time from 0.3.28 to 0.3.29
a097dd2 Bump clap from 4.4.3 to 4.4.4
801ce88 Merge bulletin chain changes into polkadot staging (#2574)
a3803ce Add unit tests for the equivocation detection loop (#2571)
26dfc31 Bump clap from 4.4.2 to 4.4.3
66a8beb Bump serde_json from 1.0.106 to 1.0.107
18c50da Bump trie-db from 0.27.1 to 0.28.0
4c4fa92 Equivocation detection loop: Reorganize block checking logic as state machine (#2555) (#2557)
6bd317a Bump serde_json from 1.0.105 to 1.0.106
a7e6bfd Backport for polkadot-sdk#1446 (#2546)
d9f8050 Bump sysinfo from 0.29.9 to 0.29.10
901f44c Bump thiserror from 1.0.47 to 1.0.48
82eeb50 Bump sysinfo from 0.29.8 to 0.29.9
a0c934b Bump strum from 0.24.1 to 0.25.0
1064fbf Bump subxt from 0.28.0 to 0.31.0
e50398d bridges subtree fixes (#2528)
99af075 Markdown linter (#1309) (#2526)
733ff0f `polkadot-staging` branch: Use polkadot-sdk dependencies (#2524)
e8a59f1 Fix benchmark with new XCM::V3 `MAX_INSTRUCTIONS_TO_DECODE` (#2514)
62b185d Backport `polkadot-sdk` changes to `polkadot-staging` (#2518)
d9658f4 Fix equivocation detection containers startup (#2516) (#2517)
d65db28 Backport: building images from locally built binaries (#2513)
5fdbaf4 Start the equivocation detection loop from the complex relayer (#2507) (#2512)
7fbb67d Backport: Implement basic equivocations detection loop (#2375)
cb7efe2 Manually update deps in polkadot staging (#2371)
d17981f #2351 to polkadot-staging (#2359)

git-subtree-dir: bridges
git-subtree-split: 68d8650
@kianenigma
Copy link
Contributor

Just reading the description, this is a misunderstanding from the original issue. What I ment to request is total unbonding at the staking level, not nomination pools.

In other words, in staking, we have a mapping as such: AccountId -> Ledger whereby Ledger { active, total }. What we need is a tracker over the sum(Ledger::active) and sum(Ledger::total). Subtracting the two should give the total unbonding amount.

As for the implementation, once #1654 is done, staking will have fully implement the OnStakingUpdate. This can be implemented as a component that is implementing OnStakingUpdate and using that.

@kianenigma
Copy link
Contributor

I think the work here is valuable and should be replicated at the right abstraction level, but since this PR is certainly not going to be merged as-is, I will close this.

@kianenigma kianenigma closed this Feb 5, 2024
bkchr pushed a commit that referenced this pull request Apr 10, 2024
* Fix benchmark with new XCM::V3 MAX_INSTRUCTIONS_TO_DECODE

* Small refactor

* `ClearOrigin` replaced by ExpectPallet

* Doc

* Spellcheck

* Fixes and pr reviews
@eagr eagr deleted the unstaking-total branch September 19, 2024 10:25
@eagr eagr restored the unstaking-total branch September 19, 2024 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pallet-staking: track total unstaking amount
5 participants