-
Notifications
You must be signed in to change notification settings - Fork 34
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 app version to param store #360
Conversation
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.
to double check my understanding, this is a breaking change ofc but it will not be breaking until the app version if > 2, correct?
This is the intention. I will be able to test it properly on the celestia-app side |
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.
re-approving even tho we don't have to in this repo to indicate that I have reviewed the fix
// get and set the app version | ||
_ = app.AppVersion(ctx) |
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.
[question] where or how does this "set" the app version? The invoked function returns the app version but I don't know what "set" is referring to here.
Lines 208 to 211 in 4c897c6
// AppVersion returns the application's protocol version. | |
func (app *BaseApp) AppVersion() uint64 { | |
return app.appVersion | |
} |
// application's version string | ||
version string | ||
|
||
// application's protocol version that increments on every upgrade | ||
// if BaseApp is passed to the upgrade keeper's NewKeeper method. | ||
appVersion uint64 |
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.
[not blocking] can these two include an example to help further differentiate them?
If version = 2.0.0
then is appVersion = 2
? Is appVersion always expected to be the major version of the version string?
On re-review, I think it would be helpful if we updated the README w/ a line item for the rationale for this modification |
Description
Ported over from #347
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change