fix: allow application to set and query app version #395
Conversation
cmwaters
requested review from
ramin and
rach-id
and removed request for
a team
|
nice find |
| @@ -223,12 +223,22 @@ func (app *BaseApp) Name() string { | |||
| return app.name | |||
| } | |||
|
|
|||
| func (app *BaseApp) InitAppVersion(ctx sdk.Context) { | |||
There was a problem hiding this comment.
[question] is the removal of this public method technically breaking? I don't think we need to cut a major release of Cosmos SDK as long as know that celestia-app has to replace usage of InitAppVersion when it bumps to a new release of Cosmos SDK with this PR.
Side note: Is this change back-portable to v1.x? If it isn't, then we may need to create a new branch in this repo if there's a bug we need to address on celestia-app v1.x after this PR merges. Perhaps not worth worrying about for now.
There was a problem hiding this comment.
I could keep this function around if we were worried about breaking changes. I had intended to replace the usage of InitAppVersion.
There was a problem hiding this comment.
I'm more concerned with the ability to backport than the breaking change. IMO if it's possible to backport to celestia-app v1.x easily then I'm fine with removing InitAppVersion. If it isn't possible to backport then we may have to maintain multiple Cosmos SDK branches (one per celestia-app major version) which seems like it could be a maintenance burden.
There was a problem hiding this comment.
It's not used in v1.x so it should be easy to backport
Co-authored-by: Rootul P <[email protected]>
| app.appVersion = vp.AppVersion | ||
| return vp.AppVersion | ||
| } | ||
| return 0 |
There was a problem hiding this comment.
What is app version 0? Just a non-empty value to return?
There was a problem hiding this comment.
Yeah, it just means that it's not current set
Description
This add the necessary API to fix the bug with restarting the application that was caused by not being able to correctly set the app version