Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Add MinPointsToBalance to nomination pools #11377

Merged
merged 14 commits into from
May 17, 2022
Merged

Add MinPointsToBalance to nomination pools #11377

merged 14 commits into from
May 17, 2022

Conversation

rossbulat
Copy link

@rossbulat rossbulat commented May 9, 2022 •
edited by kianenigma
Loading

This PR adds a new runtime constant item for the minimum points to balance ratio to join a nomination pool. Attempts to address #10792.

The name MinPointsToBalance has been used as an attempt to keep it short.

fixes #11377

The previously hardcoded value of 10, where OverFlowRisk has been tested, has been replaced with the storage item.

Tests have been amended to take the new ratio into consideration.

polkadot companion: paritytech/polkadot#5520

@rossbulat rossbulat added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 9, 2022
@rossbulat
Copy link
Author

Is it worth making min_points_to_balance optional and defaulting a value of 10?

ggwpez
ggwpez reviewed May 9, 2022
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Looks good, benchmarks still need to be re-run 👍

frame/nomination-pools/src/lib.rs Outdated
// while restricting the balance to 1/10th of max total issuance,
let next_bonded_balance = bonded_balance.saturating_add(new_funds);
ensure!(
next_bonded_balance < BalanceOf::<T>::max_value().div(10u32.into()),
next_bonded_balance <
BalanceOf::<T>::max_value().div(MinPointsToBalance::<T>::get().into()),
Copy link
Member

Choose a reason for hiding this comment

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

Can we prevent someone puts zero in here? Otherwise it panics.
Maybe use a checked_div?

frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
frame/nomination-pools/src/lib.rs Outdated Show resolved Hide resolved
Ross Bulat and others added 3 commits May 9, 2022 14:06
@ggwpez
comments
c33d16a 
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
check min_points_to_balance.is_zero
4f27763
Merge branch 'rb-points-to-balance-ratio' of https://github.com/parit…
a705419 
…ytech/substrate into rb-points-to-balance-ratio
kianenigma
kianenigma approved these changes May 12, 2022
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Thanks!

@shawntabrizi
Copy link
Member

shawntabrizi commented May 12, 2022 •
edited
Loading

This PR adds a new storage item for the minimum points to balance ratio

This PR does not add a new storage item.

Can you update the description so I can review intention vs code?

@rossbulat
Copy link
Author

This PR adds a new storage item for the minimum points to balance ratio

This PR does not add a new storage item.

Can you update the description so I can review intention vs code?

yes. The description has been amended.

@shawntabrizi
Copy link
Member

needs companions

@rossbulat rossbulat mentioned this pull request May 13, 2022
Merged
@kianenigma
Copy link
Contributor

companions need to be happy here.

@rossbulat
Copy link
Author

companions need to be happy here.

paritytech/polkadot#5520 Needs another review. Requested @ggwpez

@kianenigma kianenigma linked an issue May 17, 2022 that may be closed by this pull request
nomination-pools: Make max points to balance ratio configurable #10792
Closed
@ggwpez
Copy link
Member

ggwpez commented May 17, 2022

bot rebase

@paritytech-processbot
Copy link

Rebasing

@ggwpez
Copy link
Member

ggwpez commented May 17, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5520

@rossbulat
Copy link
Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for f4730be

@rossbulat
Copy link
Author

bot rebase

@paritytech-processbot
Copy link

Rebasing

@shawntabrizi
Copy link
Member

cumulus CI seems to be breaking here

  error[E0046]: not all trait items implemented, missing: `MinPointsToBalance`
      --> /builds/parity/mirrors/substrate/extra_dependencies/polkadot/runtime/kusama/src/lib.rs:1442:1
       |
  1442 | impl pallet_nomination_pools::Config for Runtime {
       | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `MinPointsToBalance` in implementation
       |
       = help: implement the missing item: `type MinPointsToBalance = Type;`

Dunno where mirrors is coming from, but it does not seem up to date with the companion

@shawntabrizi shawntabrizi merged commit 6a7af64 into master May 17, 2022
@shawntabrizi shawntabrizi deleted the rb-points-to-balance-ratio branch May 17, 2022 18:16
@joao-paulo-parity joao-paulo-parity mentioned this pull request May 18, 2022
Closed
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
@ggwpez @godcodehunter
Add MinPointsToBalance to nomination pools (paritytech#11377)
1f80efd 
* add MinPointsToBalance to pools

* add min_points_to_balance to benchmark mock

* fmt

* comments

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* check min_points_to_balance.is_zero

* comment

* comment

* storage to constant

* fix

* comment

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@ggwpez @DaviRain-Su
Add MinPointsToBalance to nomination pools (paritytech#11377)
073c081 
* add MinPointsToBalance to pools

* add min_points_to_balance to benchmark mock

* fmt

* comments

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* check min_points_to_balance.is_zero

* comment

* comment

* storage to constant

* fix

* comment

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@ggwpez @ark0f
Add MinPointsToBalance to nomination pools (paritytech#11377)
338ba40 
* add MinPointsToBalance to pools

* add min_points_to_balance to benchmark mock

* fmt

* comments

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* check min_points_to_balance.is_zero

* comment

* comment

* storage to constant

* fix

* comment

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ggwpez ggwpez ggwpez left review comments

@kianenigma kianenigma kianenigma approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
(Nominated) Proof of Stake
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

nomination-pools: Make max points to balance ratio configurable
4 participants
@rossbulat @shawntabrizi @kianenigma @ggwpez