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

Improve Staking Limits #9193

Merged
8 commits merged into from Jun 28, 2021
Merged

Improve Staking Limits #9193

8 commits merged into from Jun 28, 2021

Conversation

shawntabrizi
Copy link
Contributor

@shawntabrizi shawntabrizi commented Jun 24, 2021 •
edited

When introducing the original staking limits: #8920

we were a bit too strict on the behaviors allowed during otherwise okay scenarios.

The following changes are made in this PR:

  • If we are not within our max nominator / validator threshold, we do not allow anyone to call chill_other on a different user.
  • If we are an existing nominator / validator, we can still update our preferences using nominate / validate

polkadot companion: paritytech/polkadot#3376

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Jun 24, 2021
@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 24, 2021
@shawntabrizi shawntabrizi added this to In progress in Runtime via automation Jun 24, 2021
@shawntabrizi shawntabrizi moved this from In progress to Please Review in Runtime Jun 24, 2021
@emostov emostov self-requested a review June 24, 2021 02:47
coriolinus
coriolinus reviewed Jun 24, 2021
View reviewed changes
frame/staking/src/benchmarking.rs Show resolved Hide resolved
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
kianenigma
kianenigma approved these changes Jun 24, 2021
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.

+1 on the 75 being a param, otherwise LGTM

@antonkhvorov
Copy link

If we are an existing nominator / validator, we can still update our preferences using nominate / validate

Kudos 😎

emostov
emostov reviewed Jun 25, 2021
View reviewed changes
frame/staking/src/lib.rs
@@ -2307,19 +2314,29 @@ pub mod pallet {
let ledger = Self::ledger(&controller).ok_or(Error::<T>::NotController)?;
Copy link
Contributor

@emostov emostov Jun 25, 2021 •
edited

Choose a reason for hiding this comment

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

Should the origin param for chill_other be configurable for the pallet? (I believe @shawntabrizi mentioned this idea already somewhere)

The current, permission nature of this call is useful for scenarios where we want a network to be continuously "self adjusting". But I think we can imagine scenarios where this makes more sense as a governance only call (that then can be turned into a permission-less call with a runtime upgrade).

While I don't see how it would improve security properties, I think having a call like this go through governance could benefit the community

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagential, is it possible to make a call origin configurable without a runtime upgrade?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tagential, is it possible to make a call origin configurable without a runtime upgrade?

Nope, that's logic change and needs altering :code.

emostov
emostov reviewed Jun 25, 2021
View reviewed changes
frame/staking/src/lib.rs Outdated Show resolved Hide resolved
emostov
emostov approved these changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@emostov emostov left a comment •
edited

Choose a reason for hiding this comment

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

+1 on the 75 being a param, otherwise LGTM

+1 :)

Outside the goals of this PR, I am curious if it is worth looking into a governance origin for chill_other; see #9193 (comment)

Runtime automation moved this from Please Review to Needs Audit Jun 25, 2021
@kianenigma kianenigma added C3-medium PR touches the given topic and has a medium impact on builders. and removed C1-low PR touches the given topic and has a low impact on builders. labels Jun 27, 2021
@emostov
Copy link
Contributor

emostov commented Jun 27, 2021

Just read through commits f7c1698, 4723d5a, 1778970 and the updates lgtm 👍

@shawntabrizi shawntabrizi mentioned this pull request Jun 28, 2021
Merged
@shawntabrizi
Copy link
Contributor Author

bot merge

@ghost
Copy link

ghost commented Jun 28, 2021

Trying merge.

@ghost ghost merged commit c6240ce into master Jun 28, 2021
@ghost ghost deleted the shawntabrizi-improve-staking-limits branch June 28, 2021 12:54
Runtime automation moved this from Needs Audit to Done Jun 28, 2021
@shawntabrizi shawntabrizi moved this from Done to Archive in Runtime Jul 8, 2021
@github-actions github-actions bot mentioned this pull request Jul 8, 2021
Closed
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jul 12, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@coriolinus coriolinus coriolinus approved these changes

@emostov emostov emostov approved these changes

@sorpaas sorpaas Awaiting requested review from sorpaas

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C3-medium PR touches the given topic and has a medium impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@shawntabrizi @antonkhvorov @emostov @kianenigma @coriolinus @jakoblell