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

Add MinPointsToBalance to nomination pools#11377

Merged
shawntabrizi merged 14 commits intomasterfrom
rb-points-to-balance-ratio
May 17, 2022
Merged

Add MinPointsToBalance to nomination pools#11377
shawntabrizi merged 14 commits intomasterfrom
rb-points-to-balance-ratio

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

rossbulat commented May 9, 2022

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
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

@ggwpez ggwpez May 9, 2022

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?

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
rossbulat
rossbulat commented May 9, 2022
View reviewed changes
kianenigma
kianenigma reviewed May 9, 2022
View reviewed changes
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

rossbulat commented May 12, 2022

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

shawntabrizi commented May 13, 2022

needs companions

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

kianenigma commented May 17, 2022

companions need to be happy here.

@rossbulat
Copy link
Author

rossbulat commented May 17, 2022

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

paritytech-processbot bot commented May 17, 2022

Rebasing

@ggwpez
Copy link
Member

ggwpez commented May 17, 2022

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented May 17, 2022

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

@rossbulat
Copy link
Author

rossbulat commented May 17, 2022

bot merge

@paritytech-processbot
Copy link

paritytech-processbot bot commented May 17, 2022

Error: Statuses failed for f4730be

@rossbulat
Copy link
Author

rossbulat commented May 17, 2022

bot rebase

@paritytech-processbot
Copy link

paritytech-processbot bot commented May 17, 2022

Rebasing

@shawntabrizi
Copy link
Member

shawntabrizi commented May 17, 2022

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

3 more reviewers

@ggwpez ggwpez ggwpez left review comments

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@kianenigma kianenigma kianenigma approved these changes

Reviewers whose approvals may not affect merge requirements

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