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 18 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
6 changes: 3 additions & 3 deletions .github/workflows/e2e-upgrade.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ jobs:
- test: TestV6ToV7ChainUpgrade
chain-image: ghcr.io/cosmos/ibc-go-simd
chain-binary: simd
chain-a-tag: v6.0.0
chain-b-tag: v6.0.0
chain-upgrade-tag: v7.0.0
chain-a-tag: v6.1.0
chain-b-tag: v6.1.0
chain-upgrade-tag: pr-3136
Copy link
Member Author

Choose a reason for hiding this comment

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

This will essentially need to be v7.0.0-rc1 when this PR is merged, backported and tagged in rc1.

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}$
2 changes: 1 addition & 1 deletion e2e/tests/upgrades/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ func (s *UpgradeTestSuite) TestV5ToV6ChainUpgrade() {

t.Run("interchain account executes a bank transfer on behalf of the corresponding owner account", func(t *testing.T) {
t.Run("fund interchain account wallet", func(t *testing.T) {
// fund the host account account so it has some $$ to send
// fund the host account, so it has some $$ to send
err := chainB.SendFunds(ctx, interchaintest.FaucetAccountKeyName, ibc.WalletAmount{
Address: hostAccount,
Amount: testvalues.StartingTokenAmount,
Expand Down
26 changes: 22 additions & 4 deletions e2e/testsuite/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,18 @@ import (
govv1beta1 "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"
grouptypes "github.com/cosmos/cosmos-sdk/x/group"
proposaltypes "github.com/cosmos/cosmos-sdk/x/params/types/proposal"
upgradetypes "github.com/cosmos/cosmos-sdk/x/upgrade/types"
intertxtypes "github.com/cosmos/interchain-accounts/x/inter-tx/types"

icacontrollertypes "github.com/cosmos/ibc-go/v7/modules/apps/27-interchain-accounts/controller/types"
feetypes "github.com/cosmos/ibc-go/v7/modules/apps/29-fee/types"
transfertypes "github.com/cosmos/ibc-go/v7/modules/apps/transfer/types"
v7migrations "github.com/cosmos/ibc-go/v7/modules/core/02-client/migrations/v7"
clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types"
connectiontypes "github.com/cosmos/ibc-go/v7/modules/core/03-connection/types"
channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types"
solomachine "github.com/cosmos/ibc-go/v7/modules/light-clients/06-solomachine"
ibctmtypes "github.com/cosmos/ibc-go/v7/modules/light-clients/07-tendermint"
simappparams "github.com/cosmos/ibc-go/v7/testing/simapp/params"
)

Expand All @@ -30,18 +37,29 @@ func EncodingConfig() simappparams.EncodingConfig {

func codecAndEncodingConfig() (*codec.ProtoCodec, simappparams.EncodingConfig) {
cfg := simappparams.MakeTestEncodingConfig()

// ibc types
icacontrollertypes.RegisterInterfaces(cfg.InterfaceRegistry)
feetypes.RegisterInterfaces(cfg.InterfaceRegistry)
intertxtypes.RegisterInterfaces(cfg.InterfaceRegistry)
solomachine.RegisterInterfaces(cfg.InterfaceRegistry)
v7migrations.RegisterInterfaces(cfg.InterfaceRegistry)
transfertypes.RegisterInterfaces(cfg.InterfaceRegistry)
clienttypes.RegisterInterfaces(cfg.InterfaceRegistry)
channeltypes.RegisterInterfaces(cfg.InterfaceRegistry)
connectiontypes.RegisterInterfaces(cfg.InterfaceRegistry)
ibctmtypes.RegisterInterfaces(cfg.InterfaceRegistry)

// all other types
upgradetypes.RegisterInterfaces(cfg.InterfaceRegistry)
banktypes.RegisterInterfaces(cfg.InterfaceRegistry)
govv1beta1.RegisterInterfaces(cfg.InterfaceRegistry)
govv1.RegisterInterfaces(cfg.InterfaceRegistry)
authtypes.RegisterInterfaces(cfg.InterfaceRegistry)
feetypes.RegisterInterfaces(cfg.InterfaceRegistry)
icacontrollertypes.RegisterInterfaces(cfg.InterfaceRegistry)
sdkcodec.RegisterInterfaces(cfg.InterfaceRegistry)
grouptypes.RegisterInterfaces(cfg.InterfaceRegistry)
proposaltypes.RegisterInterfaces(cfg.InterfaceRegistry)
authz.RegisterInterfaces(cfg.InterfaceRegistry)
transfertypes.RegisterInterfaces(cfg.InterfaceRegistry)
clienttypes.RegisterInterfaces(cfg.InterfaceRegistry)

cdc := codec.NewProtoCodec(cfg.InterfaceRegistry)
return cdc, cfg
Expand Down
18 changes: 6 additions & 12 deletions e2e/testsuite/testsuite.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,6 @@ const (
ChainBRelayerName = "rlyB"
// DefaultGasValue is the default gas value used to configure tx.Factory
DefaultGasValue = 500000
// emptyLogs is the string value returned from `BroadcastMessages`. There are some situations in which
// the result is empty, when this happens we include the raw logs instead to get as much information
// amount the failure as possible.
emptyLogs = "[]"
)

// E2ETestSuite has methods and functionality which can be shared among all test suites.
Expand Down Expand Up @@ -403,14 +399,12 @@ func (s *E2ETestSuite) InitGRPCClients(chain *cosmos.CosmosChain) {
// AssertValidTxResponse verifies that an sdk.TxResponse
// has non-empty values.
func (s *E2ETestSuite) AssertValidTxResponse(resp sdk.TxResponse) {
respLogsMsg := resp.Logs.String()
if respLogsMsg == emptyLogs {
respLogsMsg = resp.RawLog
}
s.Require().NotEqual(int64(0), resp.GasUsed, respLogsMsg)
s.Require().NotEqual(int64(0), resp.GasWanted, respLogsMsg)
s.Require().NotEmpty(resp.Events, respLogsMsg)
s.Require().NotEmpty(resp.Data, respLogsMsg)
errorMsg := fmt.Sprintf("%+v", resp)
s.Require().NotEmpty(resp.TxHash, errorMsg)
s.Require().NotEqual(int64(0), resp.GasUsed, errorMsg)
s.Require().NotEqual(int64(0), resp.GasWanted, errorMsg)
s.Require().NotEmpty(resp.Events, errorMsg)
s.Require().NotEmpty(resp.Data, errorMsg)
}

// AssertPacketRelayed asserts that the packet commitment does not exist on the sending chain.
Expand Down
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
Member 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
Member 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
Member 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
Member 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
Member 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
Member 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)
}
}