-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: add version params to parameters #9432
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9432 +/- ##
==========================================
- Coverage 63.46% 63.43% -0.03%
==========================================
Files 571 571
Lines 37524 37524
==========================================
- Hits 23815 23804 -11
- Misses 11851 11862 +11
Partials 1858 1858
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does baseapp params handle migration. I dont see a migration script in there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm, left a couple of comments re migrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion on how to move forward with this PR
d3fb6ad
to
2d63a91
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @marbar3778!
func ConsensusParamsKeyTable() types.KeyTable { | ||
return types.NewKeyTable( | ||
types.NewParamSetPair( | ||
func ConsensusParamsKeyTable() KeyTable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add an API breaking changelog entry for this guy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, on it
will open another PR. the bot moved too fast |
<!-- < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < < ☺ v ✰ Thanks for creating a PR! ✰ v Before smashing the submit button please review the checkboxes. v If a checkbox is n/a - please still include it but + a little note why ☺ > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > --> ## Description add version paramters to Tendermint consensus params. closes: cosmos#7472 @AmauryM this may need to be incorporated into the upgrade module. I need to read into how upgrades work to figure it out.
Description
add version paramters to Tendermint consensus params.
closes: #7472
@AmauryM this may need to be incorporated into the upgrade module. I need to read into how upgrades work to figure it out.