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

staking: Flexible generation of reward curve and associated tweaks #8327

Merged
26 commits merged into from
Mar 16, 2021
Merged

staking: Flexible generation of reward curve and associated tweaks #8327

26 commits merged into from
Mar 16, 2021

Conversation

@gavofyork
Copy link
Member

@gavofyork gavofyork commented Mar 11, 2021 •
edited
Loading

Breaks the API, sadly, but it's easy to fix your impl. Just change the line:

type RewardCurve = RewardCurve;

into:

type EraPayout = pallet_staking::ConvertCurve<RewardCurve>;

Also tweaks to per_things.rs:

  • from_rational_approximation becomes from_rational
  • from_fraction becomes from_float
  • Improve efficiency of pow
  • Implement several ops traits
  • Ensure that mul is correctly characterised as non-overflowing

Also some minor refactoring to the Gilt pallet to avoid collapsing into u128.

Polkadot companion: paritytech/polkadot#2610

@gavofyork gavofyork requested a review from kianenigma as a code owner March 11, 2021 11:06
@gavofyork gavofyork changed the title Flexible generation of reward curve staking: Flexible generation of reward curve Mar 11, 2021
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 11, 2021
@gavofyork gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. B2-breaksapi C1-low PR touches the given topic and has a low impact on builders. and removed A0-please_review Pull request needs code review. labels Mar 11, 2021
@gavofyork gavofyork requested a review from andresilva as a code owner March 11, 2021 11:15
@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. A7-needspolkadotpr labels Mar 11, 2021
@gavofyork gavofyork requested a review from gui1117 March 11, 2021 14:34
@gavofyork
Copy link
Member Author

gavofyork commented Mar 11, 2021

cc @kianenigma

@gui1117 gui1117 mentioned this pull request Mar 11, 2021
Merged
@gavofyork gavofyork changed the title staking: Flexible generation of reward curve staking: Flexible generation of reward curve and associated tweaks Mar 13, 2021
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
frame/gilt/src/lib.rs
Comment on lines +101 to +105
/// Just the `Currency::Balance` type; we have this item to allow us to constrain it to
/// `From<u64>`.
type CurrencyBalance:
sp_runtime::traits::AtLeast32BitUnsigned + codec::FullCodec + Copy
+ MaybeSerializeDeserialize + sp_std::fmt::Debug + Default + From<u64>;
Copy link
Contributor

@kianenigma kianenigma Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The only alternative that I can think of is to add a where <T::Currency as _>::Balance: From<u64> to most functions of this pallet, and while this is not pretty either, I guess it has less footprint than sprinkling wheres.

kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
primitives/npos-elections/src/helpers.rs
distribution: vec![
(10u32, Perbill::from_fraction(0.5)),
(20, Perbill::from_fraction(0.5)),
(10u32, Perbill::from_float(0.5)),
Copy link
Contributor

@kianenigma kianenigma Mar 15, 2021

Choose a reason for hiding this comment

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

Would be good to ensure new names are consistent with FixedPointNumber. I think the only one needing revision os this ^^, it is still called from_fraction there. Can also be a follow up.

Copy link
Member

@shawntabrizi shawntabrizi Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

#8364

kianenigma
kianenigma approved these changes Mar 15, 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.

Some naming and doc notes worth fixing here and there, but nothing major.

kianenigma
kianenigma reviewed Mar 15, 2021
View reviewed changes
shawntabrizi
shawntabrizi approved these changes Mar 15, 2021
View reviewed changes
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

lgtm

primitives/npos-elections/src/helpers.rs
distribution: vec![
(10u32, Perbill::from_fraction(0.5)),
(20, Perbill::from_fraction(0.5)),
(10u32, Perbill::from_float(0.5)),
Copy link
Member

@shawntabrizi shawntabrizi Mar 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

#8364

shawntabrizi
shawntabrizi reviewed Mar 15, 2021
View reviewed changes
frame/staking/src/lib.rs
}
}

pub struct ConvertCurve<T>(sp_std::marker::PhantomData<T>);
Copy link
Member

@shawntabrizi shawntabrizi Mar 15, 2021

Choose a reason for hiding this comment

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

probably could use a description

@shawntabrizi shawntabrizi mentioned this pull request Mar 15, 2021
Closed
@gavofyork
Copy link
Member Author

gavofyork commented Mar 16, 2021

bot merge

@ghost
Copy link

ghost commented Mar 16, 2021

Trying merge.

@ghost ghost merged commit 39b3131 into master Mar 16, 2021
@ghost ghost deleted the gav-abstract-payout-curve branch March 16, 2021 12:03
hirschenberger pushed a commit to hirschenberger/substrate that referenced this pull request Apr 14, 2021
@gavofyork @shawntabrizi
@kianenigma @hirschenberger
staking: Flexible generation of reward curve and associated tweaks (p…
f55ad87 
…aritytech#8327)

* Initial abstraction

* Alter rest of APIs

* Fixes

* Some extra getters in Gilt pallet.

* Refactor Gilt to avoid u128 conversions

* Simplify and improve pow in per_things

* Add scalar division to per_things

* Renaming from_fraction -> from_float, drop _approximation

* Fixes

* Fixes

* Fixes

* Fixes

* Make stuff build

* Fixes

* Fixes

* Fixes

* Fixes

* Update .gitignore

Co-authored-by: Kian Paimani <[email protected]>

* Update frame/gilt/src/lib.rs

Co-authored-by: Kian Paimani <[email protected]>

* Update frame/gilt/src/mock.rs

Co-authored-by: Kian Paimani <[email protected]>

* Fixes

* Fixes

* Fixes

Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
@gavofyork gavofyork added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Apr 19, 2021
@redzsina redzsina 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 Apr 20, 2021
@kianenigma kianenigma mentioned this pull request Aug 11, 2022
Merged
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@andresilva andresilva Awaiting requested review from andresilva

@athei athei Awaiting requested review from athei athei is a code owner

3 more reviewers

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@kianenigma kianenigma kianenigma approved these changes

@gui1117 gui1117 gui1117 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low 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.

7 participants

@gavofyork @shawntabrizi @kianenigma @gui1117 @paritytech-ci @redzsina