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.

Sign up for GitHub

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

Jump to bottom

fix: allow application to set and query app version #395

Merged
merged 2 commits into from
Apr 29, 2024
Merged

fix: allow application to set and query app version #395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd2735b
allow application to query and set app version
cmwaters Apr 26, 2024
bf48f81
Apply suggestions from code review
cmwaters Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 9 additions & 13 deletions baseapp/abci.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,16 +128,8 @@ func (app *BaseApp) SetOption(req abci.RequestSetOption) (res abci.ResponseSetOp

// Info implements the ABCI interface.
func (app *BaseApp) Info(req abci.RequestInfo) abci.ResponseInfo {
// get the latest height and app hash
lastCommitID := app.cms.LastCommitID()
// load the app version for a non zero height and zero app hash
if lastCommitID.Version > 0 && app.appVersion == 0 {
ctx, err := app.createQueryContext(lastCommitID.Version, false)
if err != nil {
panic(err)
}
// initialise the app version by checking if it is already in state
app.InitAppVersion(ctx)
}
return abci.ResponseInfo{
Data: app.name,
Version: app.version,
Expand Down Expand Up @@ -621,7 +613,7 @@ func (app *BaseApp) ApplySnapshotChunk(req abci.RequestApplySnapshotChunk) abci.
}

func (app *BaseApp) handleQueryGRPC(handler GRPCQueryHandler, req abci.RequestQuery) abci.ResponseQuery {
ctx, err := app.createQueryContext(req.Height, req.Prove)
ctx, err := app.CreateQueryContext(req.Height, req.Prove)
if err != nil {
return sdkerrors.QueryResult(err, app.trace)
}
Expand Down Expand Up @@ -667,9 +659,9 @@ func checkNegativeHeight(height int64) error {
return nil
}

// createQueryContext creates a new sdk.Context for a query, taking as args
// CreateQueryContext creates a new sdk.Context for a query, taking as args
// the block height and whether the query needs a proof or not.
func (app *BaseApp) createQueryContext(height int64, prove bool) (sdk.Context, error) {
func (app *BaseApp) CreateQueryContext(height int64, prove bool) (sdk.Context, error) {
if err := checkNegativeHeight(height); err != nil {
return sdk.Context{}, err
}
Expand Down Expand Up @@ -711,6 +703,10 @@ func (app *BaseApp) createQueryContext(height int64, prove bool) (sdk.Context, e
)
}

if app.checkState == nil {
app.setCheckState(tmproto.Header{Height: height})
}

// branch the commit-multistore for safety
ctx := sdk.NewContext(
cacheMS, app.checkState.ctx.BlockHeader(), true, app.logger,
Expand Down Expand Up @@ -892,7 +888,7 @@ func handleQueryCustom(app *BaseApp, path []string, req abci.RequestQuery) abci.
return sdkerrors.QueryResult(sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "no custom querier found for route %s", path[1]), app.trace)
}

ctx, err := app.createQueryContext(req.Height, req.Prove)
ctx, err := app.CreateQueryContext(req.Height, req.Prove)
if err != nil {
return sdkerrors.QueryResult(err, app.trace)
}
Expand Down
2 changes: 1 addition & 1 deletion baseapp/abci_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func TestBaseAppCreateQueryContext(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
_, err := app.createQueryContext(tc.height, tc.prove)
_, err := app.CreateQueryContext(tc.height, tc.prove)
if tc.expErr {
require.Error(t, err)
} else {
Expand Down
25 changes: 13 additions & 12 deletions baseapp/baseapp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,12 +223,21 @@ func (app *BaseApp) Name() string {
return app.name
}

func (app *BaseApp) InitAppVersion(ctx sdk.Context) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

[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.

Copy link
Author

Choose a reason for hiding this comment

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

I could keep this function around if we were worried about breaking changes. I had intended to replace the usage of InitAppVersion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

It's not used in v1.x so it should be easy to backport

if app.appVersion == 0 && app.paramStore.Has(ctx, ParamStoreKeyVersionParams) {
// GetAppVersionFromParamStore returns the app version from the param store.
func (app *BaseApp) GetAppVersionFromParamStore(ctx sdk.Context) uint64 {
if app.paramStore.Has(ctx, ParamStoreKeyVersionParams) {
var vp tmproto.VersionParams
app.paramStore.Get(ctx, ParamStoreKeyVersionParams, &vp)
// set the app version
app.appVersion = vp.AppVersion
return vp.AppVersion
}
return 0
Copy link
Member

Choose a reason for hiding this comment

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

What is app version 0? Just a non-empty value to return?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it just means that it's not current set

}

// SetInitialAppVersionInConsensusParams sets the initial app version
// in the consensus params if it has not yet been set.
func (app *BaseApp) SetInitialAppVersionInConsensusParams(ctx sdk.Context, version uint64) {
if !app.paramStore.Has(ctx, ParamStoreKeyVersionParams) {
app.paramStore.Set(ctx, ParamStoreKeyVersionParams, &tmproto.VersionParams{AppVersion: version})
}
}

Expand Down Expand Up @@ -546,14 +555,6 @@ func (app *BaseApp) StoreConsensusParams(ctx sdk.Context, cp *abci.ConsensusPara
}
}

// SetInitialAppVersionInConsensusParams sets the initial app version
// in the consensus params if it has not yet been set.
func (app *BaseApp) SetInitialAppVersionInConsensusParams(ctx sdk.Context, version uint64) {
if !app.paramStore.Has(ctx, ParamStoreKeyVersionParams) {
app.paramStore.Set(ctx, ParamStoreKeyVersionParams, &tmproto.VersionParams{AppVersion: version})
}
}

// getMaximumBlockGas gets the maximum gas from the consensus params. It panics
// if maximum block gas is less than negative one and returns zero if negative
// one.
Expand Down
2 changes: 1 addition & 1 deletion baseapp/grpcserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ func (app *BaseApp) RegisterGRPCServer(server gogogrpc.Server) {

// Create the sdk.Context. Passing false as 2nd arg, as we can't
// actually support proofs with gRPC right now.
sdkCtx, err := app.createQueryContext(height, false)
sdkCtx, err := app.CreateQueryContext(height, false)
if err != nil {
return nil, err
}
Expand Down
7 changes: 0 additions & 7 deletions baseapp/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,6 @@ func (app *BaseApp) GetName() string {
return app.name
}

// CreateQueryContext calls app's createQueryContext.
//
// This method is only accessible in baseapp tests.
func (app *BaseApp) CreateQueryContext(height int64, prove bool) (types.Context, error) {
return app.createQueryContext(height, prove)
}

// MinGasPrices returns minGasPrices.
//
// This method is only accessible in baseapp tests.
Expand Down
Loading