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

feat: set proposer-based rewards to 0 #309

Merged

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented Apr 20, 2023

Closes celestiaorg/celestia-app#1589

I briefly considered overriding these params in celestia-app similar to what we do for the gov module here but that's a messier diff and the diff introduced in this PR won't be difficult to resolve b/c upstream has deprecated these pararms in cosmos#12876. Tests in this PR were updated based on cosmos#12876.

Note: no Mainnet milestone exists in this repo but this is needed for Mainnet.

@rootulp rootulp added the C: app Changes related to the celestia-app branches label Apr 20, 2023
@rootulp rootulp self-assigned this Apr 20, 2023
@rootulp
Copy link
Collaborator Author

rootulp commented Apr 20, 2023

It seems a little silly that it's easier to override default params like this in our fork of Cosmos SDK rather than in the app repo but it appears common per this search

@rootulp rootulp marked this pull request as ready for review April 20, 2023 19:54
Copy link
Member

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

cool, thanks for the info. Its only the defaults anyway

@codecov-commenter
Copy link

Codecov Report

Merging #309 (7ea8885) into release/v0.46.x-celestia (472f7a7) will decrease coverage by 0.17%.
The diff coverage is 68.58%.

Impacted file tree graph

@@                     Coverage Diff                      @@
##           release/v0.46.x-celestia     #309      +/-   ##
============================================================
- Coverage                     65.69%   65.52%   -0.17%     
============================================================
  Files                           679      666      -13     
  Lines                         72212    70980    -1232     
============================================================
- Hits                          47439    46512     -927     
+ Misses                        22108    21891     -217     
+ Partials                       2665     2577      -88     
Impacted Files Coverage Δ
baseapp/abci.go 60.53% <0.00%> (-0.44%) ⬇️
baseapp/baseapp.go 78.61% <0.00%> (-0.36%) ⬇️
baseapp/options.go 59.64% <0.00%> (-1.07%) ⬇️
client/flags/flags.go 19.04% <ø> (ø)
crypto/armor.go 82.96% <ø> (ø)
crypto/keyring/keyring.go 63.04% <ø> (ø)
server/config/toml.go 66.66% <ø> (ø)
server/mock/store.go 17.92% <0.00%> (-0.35%) ⬇️
server/rollback.go 0.00% <0.00%> (ø)
store/cachekv/internal/mergeiterator.go 0.00% <0.00%> (ø)
... and 31 more

... and 1 file with indirect coverage changes

Comment on lines +29 to +30
BaseProposerReward: sdk.ZeroDec(), // deprecated
BonusProposerReward: sdk.ZeroDec(), // deprecated
Copy link
Member

Choose a reason for hiding this comment

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

I would have expected that these two can simply be deleted as we do not need to be backwards compatible at all. But I guess for the sake of minimal changes to the upstream SDK, this is the more reasonable approach still.

Q: These would still be changeable via governance though?

Copy link
Member

Choose a reason for hiding this comment

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

These would still be changeable via governance though?

yes, this is only changing the defaults

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can delete the params if that's preferred. I set the params to zero b/c the PR in the description above and the issue @liamsi created:

In case we launch with 0.46, we should set the param to 0

@rootulp
Copy link
Collaborator Author

rootulp commented Apr 21, 2023

Will merge this as-is b/c I don't see any blocking feedback.

@rootulp rootulp merged commit 50f4c49 into celestiaorg:release/v0.46.x-celestia Apr 21, 2023
@rootulp rootulp deleted the rp/proposer-rewards branch April 21, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: app Changes related to the celestia-app branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set proposer-based rewards to 0%
4 participants