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

go/staking: Enable changing the reward schedule #5352

Merged
merged 1 commit into from
Aug 23, 2023

Conversation

abukosek
Copy link
Contributor

This PR adds the ability to change the reward schedule in the staking consensus parameters through a governance vote. An additional e2e test for the rewards was also added.

@abukosek abukosek force-pushed the andrej/feature/fixed-rewards branch 2 times, most recently from 1cf6265 to ecba4bc Compare August 17, 2023 13:09
@abukosek abukosek marked this pull request as ready for review August 17, 2023 13:15
@abukosek abukosek force-pushed the andrej/feature/fixed-rewards branch 4 times, most recently from a13a11a to 2271f4d Compare August 22, 2023 13:11
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #5352 (2271f4d) into master (5fca705) will increase coverage by 0.64%.
Report is 1 commits behind head on master.
The diff coverage is 100.00%.

❗ Current head 2271f4d differs from pull request most recent head 782345f. Consider uploading reports for the commit 782345f to get more accurate results

@@            Coverage Diff             @@
##           master    #5352      +/-   ##
==========================================
+ Coverage   66.93%   67.58%   +0.64%     
==========================================
  Files         525      525              
  Lines       55763    55772       +9     
==========================================
+ Hits        37323    37691     +368     
+ Misses      13909    13589     -320     
+ Partials     4531     4492      -39     
Files Changed Coverage Δ
go/beacon/api/api.go 75.00% <ø> (ø)
go/staking/api/api.go 68.78% <100.00%> (+7.95%) ⬆️
go/staking/api/sanity_check.go 53.01% <100.00%> (+1.46%) ⬆️

... and 80 files with indirect coverage changes

Copy link
Member

Choose a reason for hiding this comment

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

This should be breaking.

if len(p.RewardSchedule) > 0 {
var prevUntil beacon.EpochTime
for _, step := range p.RewardSchedule {
if step.Until < prevUntil {
Copy link
Member

Choose a reason for hiding this comment

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

Equality should probably also be disallowed?

Copy link
Contributor

@peternose peternose Aug 22, 2023

Choose a reason for hiding this comment

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

Suggested change
if step.Until < prevUntil {
if step.Scale.Cmp(Zero) == -1 || step.Scale.Cmp(RewardAmountDenominator) == 1 {
fmt.Errorf("reward scale needs to be a non-negative integer not greater than %d", RewardAmountDenominator)
}
if step.Until == beacon.EpochInvalid {
return fmt.Errorf("invalid epoch")
}
if step.Until <= prevUntil {

Update tests also.

@@ -42,12 +42,24 @@ func (p *ConsensusParameters) SanityCheck() error {
return fmt.Errorf("minimum commission %v/%v over unity", p.CommissionScheduleRules, CommissionRateDenominator)
}

// Reward schedule steps must be sequential.
if len(p.RewardSchedule) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if len(p.RewardSchedule) > 0 {

if len(p.RewardSchedule) > 0 {
var prevUntil beacon.EpochTime
for _, step := range p.RewardSchedule {
if step.Until < prevUntil {
Copy link
Contributor

@peternose peternose Aug 22, 2023

Choose a reason for hiding this comment

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

Suggested change
if step.Until < prevUntil {
if step.Scale.Cmp(Zero) == -1 || step.Scale.Cmp(RewardAmountDenominator) == 1 {
fmt.Errorf("reward scale needs to be a non-negative integer not greater than %d", RewardAmountDenominator)
}
if step.Until == beacon.EpochInvalid {
return fmt.Errorf("invalid epoch")
}
if step.Until <= prevUntil {

Update tests also.

@abukosek abukosek force-pushed the andrej/feature/fixed-rewards branch from 2271f4d to 782345f Compare August 23, 2023 12:35
{10, *over},
},
}
require.Error(r9.SanityCheck(), "reward schedule step scale should not be greater than the reward amount denominator")
Copy link
Contributor

Choose a reason for hiding this comment

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

I forgot to add that I'm not sure if this must be true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It makes sense to me -- can't give more than what is available for rewards, right? :)

@abukosek abukosek merged commit 04eabea into master Aug 23, 2023
1 check passed
@abukosek abukosek deleted the andrej/feature/fixed-rewards branch August 23, 2023 14:31
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.

3 participants