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

fix: simapp and upgrade configuration for e2e v7 upgrade #3136

Merged
merged 19 commits into from
Feb 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/e2e-upgrade.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ jobs:
chain-binary: simd
chain-a-tag: v6.1.0
chain-b-tag: v6.1.0
chain-upgrade-tag: v7.0.0-rc0
chain-upgrade-tag: v7.0.0-rc0 # TODO: needs v7.0.0-rc1 when cut
steps:
- uses: actions/checkout@v3
- uses: actions/setup-go@v3
Expand Down
2 changes: 1 addition & 1 deletion e2e/scripts/run-e2e.sh
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ export CHAIN_BINARY="${CHAIN_BINARY:-simd}"
# context for building the image is one directory up.
if [ "${CI:-}" != "true" ]
then
(cd ..; docker build . -t "${CHAIN_IMAGE}:${CHAIN_A_TAG}")
(cd ..; docker build . -t "${CHAIN_IMAGE}:${CHAIN_A_TAG}" --build-arg="IBC_GO_VERSION=${CHAIN_A_TAG}")
fi

go test -v ./tests/... --run ${ENTRY_POINT} -testify.m ^${TEST}$
54 changes: 46 additions & 8 deletions testing/simapp/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,22 @@ package simapp

import (
"encoding/json"
"fmt"
"io"
"net/http"
"os"
"path/filepath"

autocliv1 "cosmossdk.io/api/cosmos/autocli/v1"
reflectionv1 "cosmossdk.io/api/cosmos/reflection/v1"
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/client"
_ "github.com/cosmos/cosmos-sdk/client/docs/statik" // this is used for serving docs
nodeservice "github.com/cosmos/cosmos-sdk/client/grpc/node"
"github.com/cosmos/cosmos-sdk/client/grpc/tmservice"
"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/types"
runtimeservices "github.com/cosmos/cosmos-sdk/runtime/services"
"github.com/cosmos/cosmos-sdk/server/api"
"github.com/cosmos/cosmos-sdk/server/config"
servertypes "github.com/cosmos/cosmos-sdk/server/types"
Expand Down Expand Up @@ -305,7 +309,7 @@ func NewSimApp(
app.ParamsKeeper = initParamsKeeper(appCodec, legacyAmino, keys[paramstypes.StoreKey], tkeys[paramstypes.TStoreKey])

// set the BaseApp's parameter store
app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, keys[upgradetypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String())
app.ConsensusParamsKeeper = consensusparamkeeper.NewKeeper(appCodec, keys[consensusparamtypes.StoreKey], authtypes.NewModuleAddress(govtypes.ModuleName).String())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a misconfiguration in #2672

bApp.SetParamStore(&app.ConsensusParamsKeeper)

// add capability keeper and ScopeToModule for ibc module
Expand Down Expand Up @@ -602,6 +606,14 @@ func NewSimApp(
app.configurator = module.NewConfigurator(app.appCodec, app.MsgServiceRouter(), app.GRPCQueryRouter())
app.mm.RegisterServices(app.configurator)

autocliv1.RegisterQueryServer(app.GRPCQueryRouter(), runtimeservices.NewAutoCLIQueryService(app.mm.Modules))

reflectionSvc, err := runtimeservices.NewReflectionService()
if err != nil {
panic(err)
}
reflectionv1.RegisterReflectionServiceServer(app.GRPCQueryRouter(), reflectionSvc)

// add test gRPC service for testing gRPC queries in isolation
testdata.RegisterQueryServer(app.GRPCQueryRouter(), testdata.QueryImpl{})

Expand Down Expand Up @@ -658,6 +670,7 @@ func NewSimApp(
app.SetEndBlocker(app.EndBlocker)

app.setupUpgradeHandlers()
app.setupUpgradeStoreLoaders()

if loadLatest {
if err := app.LoadLatestVersion(); err != nil {
Expand Down Expand Up @@ -823,6 +836,9 @@ func (app *SimApp) RegisterAPIRoutes(apiSvr *api.Server, apiConfig config.APICon
// Register legacy and grpc-gateway routes for all modules.
ModuleBasics.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter)

// Register nodeservice grpc-gateway routes.
nodeservice.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter)

// register swagger API from root so that other applications can override easily
if apiConfig.Swagger {
RegisterSwaggerAPI(clientCtx, apiSvr.Router)
Expand Down Expand Up @@ -892,14 +908,14 @@ func BlockedAddresses() map[string]bool {
func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino, key, tkey storetypes.StoreKey) paramskeeper.Keeper {
paramsKeeper := paramskeeper.NewKeeper(appCodec, legacyAmino, key, tkey)

paramsKeeper.Subspace(authtypes.ModuleName)
paramsKeeper.Subspace(banktypes.ModuleName)
paramsKeeper.Subspace(stakingtypes.ModuleName)
paramsKeeper.Subspace(minttypes.ModuleName)
paramsKeeper.Subspace(distrtypes.ModuleName)
paramsKeeper.Subspace(slashingtypes.ModuleName)
paramsKeeper.Subspace(authtypes.ModuleName).WithKeyTable(authtypes.ParamKeyTable())
paramsKeeper.Subspace(banktypes.ModuleName).WithKeyTable(banktypes.ParamKeyTable())
paramsKeeper.Subspace(stakingtypes.ModuleName).WithKeyTable(stakingtypes.ParamKeyTable())
paramsKeeper.Subspace(minttypes.ModuleName).WithKeyTable(minttypes.ParamKeyTable())
paramsKeeper.Subspace(distrtypes.ModuleName).WithKeyTable(distrtypes.ParamKeyTable())
paramsKeeper.Subspace(slashingtypes.ModuleName).WithKeyTable(slashingtypes.ParamKeyTable())
paramsKeeper.Subspace(govtypes.ModuleName).WithKeyTable(govv1.ParamKeyTable())
paramsKeeper.Subspace(crisistypes.ModuleName)
paramsKeeper.Subspace(crisistypes.ModuleName).WithKeyTable(crisistypes.ParamKeyTable())
Comment on lines +911 to +918
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding WithKeyTable(<params_key_table>) for each module here in order to register param key-value types in x/param for migration.

This registration was previously done in each NewKeeper function of the corresponding module. E.g.

if !paramSpace.HasKeyTable() {
	paramSpace = paramSpace.WithKeyTable(types.ParamKeyTable())
}

The param set pair types need to be registered in order to perform the migrations for modules maintaining their own params, otherwise we fail here. Essentially, the key-value attributes are maintained in memory in this map.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find. Will this be removable in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we can remove this post v7. We will have to do something similar when we migrate params for ibc-go modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we can remove this post v7.

Should we have an issue for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #3141

paramsKeeper.Subspace(ibctransfertypes.ModuleName)
paramsKeeper.Subspace(ibcexported.ModuleName)
paramsKeeper.Subspace(icacontrollertypes.SubModuleName)
Expand Down Expand Up @@ -938,6 +954,28 @@ func (app *SimApp) setupUpgradeHandlers() {
app.configurator,
app.appCodec,
app.IBCKeeper.ClientKeeper,
app.ConsensusParamsKeeper,
app.ParamsKeeper,
),
)
}

// setupUpgradeStoreLoaders sets all necessary store loaders required by upgrades.
func (app *SimApp) setupUpgradeStoreLoaders() {
upgradeInfo, err := app.UpgradeKeeper.ReadUpgradeInfoFromDisk()
if err != nil {
tmos.Exit(fmt.Sprintf("failed to read upgrade info from disk %s", err))
}

if upgradeInfo.Name == v7.UpgradeName && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
storeUpgrades := storetypes.StoreUpgrades{
Added: []string{
consensusparamtypes.StoreKey,
crisistypes.StoreKey,
Comment on lines +973 to +974
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adds store keys for x/consensus and x/crisis.

The new x/consensus module maintains consensus parameters previously maintained by the base app subspace.
The x/crisis module previously existed but without a store key. With the deprecation of x/params, the crisis module now uses its own dedicated store to maintain params.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this logic is repeated for every new module. If we need to do it again for a new module, it'd probably make sense to turn into a helper function. Looks great to me as is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I agree. We could possible adopt the osmosis upgrade structure, but may require some refactoring of app.go.

},
}

// configure store loader that checks if version == upgradeHeight and applies store upgrades
app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
}
}
9 changes: 9 additions & 0 deletions testing/simapp/upgrades/v7/upgrades.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
package v7

import (
"github.com/cosmos/cosmos-sdk/baseapp"
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
consensusparamskeeper "github.com/cosmos/cosmos-sdk/x/consensus/keeper"
paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper"
paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"

clientkeeper "github.com/cosmos/ibc-go/v7/modules/core/02-client/keeper"
Expand All @@ -21,13 +25,18 @@ func CreateUpgradeHandler(
configurator module.Configurator,
cdc codec.BinaryCodec,
clientKeeper clientkeeper.Keeper,
consensusParamsKeeper consensusparamskeeper.Keeper,
paramsKeeper paramskeeper.Keeper,
) upgradetypes.UpgradeHandler {
return func(ctx sdk.Context, _ upgradetypes.Plan, vm module.VersionMap) (module.VersionMap, error) {
// OPTIONAL: prune expired tendermint consensus states to save storage space
if _, err := ibctmmigrations.PruneExpiredConsensusStates(ctx, cdc, clientKeeper); err != nil {
return nil, err
}

legacyBaseAppSubspace := paramsKeeper.Subspace(baseapp.Paramspace).WithKeyTable(paramstypes.ConsensusParamsKeyTable())
baseapp.MigrateParams(ctx, legacyBaseAppSubspace, &consensusParamsKeeper)

return mm.RunMigrations(ctx, configurator, vm)
}
}