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

[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 #867

Merged
merged 20 commits into from
Jan 4, 2024
Merged

[CORE-538] Upgrade to Cosmos 0.50.1 and CometBFT 0.38 #867

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
ee5487e
[CORE-538] Update to Comsos 0.50.1
lcwik Jan 2, 2024
cd7a783
Update cosmovisor version.
lcwik Dec 18, 2023
cbcd03c
Fix simulation since NewContext doesn't pass through the chainId whil…
lcwik Dec 18, 2023
f23cd65
Swap to AppGenesisFromFile as per Cosmos upgrade guide https://docs.c…
lcwik Dec 18, 2023
76ec8d1
Remove TODO since blocktime makes sense to do in BeginBlocker since i…
lcwik Dec 18, 2023
b4dd0dd
Re-order imports
lcwik Dec 18, 2023
68d242c
Fix typo
lcwik Dec 18, 2023
fff691f
Address Taehoon's PR comments.
lcwik Dec 19, 2023
8c1a3f3
Resolve/update minor TODOs.
lcwik Dec 19, 2023
9656edc
Address Roy's comments.
lcwik Dec 19, 2023
bdd0719
Update message types, improve testapp logging/debugging for app hash …
lcwik Dec 21, 2023
4078041
Rebase updates that required changes including ICA message and contro…
lcwik Jan 2, 2024
208f41b
Address Taehoon's comments wrt to message types.
lcwik Jan 2, 2024
0328e71
Fix lint
lcwik Jan 2, 2024
81d7da8
Skip upgrade test till upgrade handler has been updated for 4.0.0
lcwik Jan 3, 2024
069ee8f
Fix data race in test code
lcwik Jan 3, 2024
61fbda5
Increase timeout since some suites of tests are taking longer then th…
lcwik Jan 3, 2024
47ecfd6
Remove the ICA controller and drop ICA from simulation
lcwik Jan 3, 2024
857a768
Update protocol/app/module/interface_registry.go
lcwik Jan 4, 2024
131a772
Address Taehoon's comments.
lcwik Jan 4, 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
2 changes: 1 addition & 1 deletion protocol/app/ante/msg_type_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import (
errorsmod "cosmossdk.io/errors"
"golang.org/x/exp/slices"

"github.com/cosmos/cosmos-sdk/types/tx/signing"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/testutil/testdata"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/cosmos-sdk/types/tx/signing"

customante "github.com/dydxprotocol/v4-chain/protocol/app/ante"
appmsgs "github.com/dydxprotocol/v4-chain/protocol/app/msgs"
Expand Down
22 changes: 21 additions & 1 deletion protocol/app/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,9 @@ import (

// IBC
ica "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts"
icacontroller "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller"
icacontrollerkeeper "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/keeper"
icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
icahost "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host"
icahostkeeper "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/keeper"
icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
Expand Down Expand Up @@ -245,6 +248,7 @@ type App struct {
ParamsKeeper paramskeeper.Keeper
// IBC Keeper must be a pointer in the app, so we can SetRouter on it correctly
IBCKeeper *ibckeeper.Keeper
ICAControllerKeeper icacontrollerkeeper.Keeper
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding ICAControllerKeeper, for my understanding? For supporting interchain accounts we specifically wanted to avoid the controllerkeeper (context).

Copy link
Contributor Author

@lcwik lcwik Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The ICAControllerKeeper is still disabled since Enabled is false in the upgrade (note the Enabled field is taking on the default value).

So instead of it being hard-coded disabled it is being disabled via parameters.

The simulation in IBC v8 expects that the controller module to exist otherwise the update params calls it generates fail with them being unroutable causing the sim tests to fail.

Copy link
Contributor

@teddyding teddyding Jan 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of going with this approach, is it possible to simply exclude ICA module from simulation testing? Empirically, simulation testing is not very high signal for the dYdX chain, let alone a third-party module that is tested elsewhere.

We specifically preferred to explicitly disable (in code, instead of through gov-updatable params) the controller sub-module altogether to reduce surface of risk. This also eliminates the potential bugs in upgrade handler.

Simulation testing alone does not justify altering the original approach, IMO. Lmk if the above is possible and happy to huddle!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I swapped to exclude the interchainaccounts module from simulation as suggested.

ICAHostKeeper icahostkeeper.Keeper
EvidenceKeeper evidencekeeper.Keeper
TransferKeeper ibctransferkeeper.Keeper
Expand Down Expand Up @@ -352,6 +356,7 @@ func New(
govtypes.StoreKey, paramstypes.StoreKey, consensusparamtypes.StoreKey, upgradetypes.StoreKey, feegrant.StoreKey,
ibcexported.StoreKey, ibctransfertypes.StoreKey,
ratelimitmoduletypes.StoreKey,
icacontrollertypes.StoreKey,
icahosttypes.StoreKey,
evidencetypes.StoreKey,
capabilitytypes.StoreKey,
Expand Down Expand Up @@ -551,6 +556,7 @@ func New(

scopedIBCKeeper := app.CapabilityKeeper.ScopeToModule(ibcexported.ModuleName)
scopedIBCTransferKeeper := app.CapabilityKeeper.ScopeToModule(ibctransfertypes.ModuleName)
scopedICAControllerKeeper := app.CapabilityKeeper.ScopeToModule(icacontrollertypes.SubModuleName)
scopedICAHostKeeper := app.CapabilityKeeper.ScopeToModule(icahosttypes.SubModuleName)

app.CapabilityKeeper.Seal()
Expand Down Expand Up @@ -580,6 +586,18 @@ func New(
lib.GovModuleAddress.String(), // authority
)

app.ICAControllerKeeper = icacontrollerkeeper.NewKeeper(
appCodec,
keys[icacontrollertypes.StoreKey], // key
app.getSubspace(icacontrollertypes.SubModuleName), // paramSpace
app.IBCKeeper.ChannelKeeper, // ics4Wrapper, may be replaced with middleware such as ics29 fee
app.IBCKeeper.ChannelKeeper, // channelKeeper
app.IBCKeeper.PortKeeper, // portKeeper
scopedICAControllerKeeper, // scopedKeeper
app.MsgServiceRouter(), // msgRouter
lib.GovModuleAddress.String(), // authority
)

// Create Transfer Keepers
app.TransferKeeper = ibctransferkeeper.NewKeeper(
appCodec,
Expand Down Expand Up @@ -609,11 +627,13 @@ func New(
// TODO(CORE-834): Add ratelimitKeeper to the IBC transfer stack.

icaHostIBCModule := icahost.NewIBCModule(app.ICAHostKeeper)
icaControlerIBCModule := icacontroller.NewIBCMiddleware(icaHostIBCModule, app.ICAControllerKeeper)
// Create static IBC router, add transfer route, then set and seal it
ibcRouter := ibcporttypes.NewRouter()
// Ordering of `AddRoute` does not matter.
ibcRouter.AddRoute(ibctransfertypes.ModuleName, transferIBCModule)
ibcRouter.AddRoute(icahosttypes.SubModuleName, icaHostIBCModule)
ibcRouter.AddRoute(icacontrollertypes.SubModuleName, icaControlerIBCModule)

app.IBCKeeper.SetRouter(ibcRouter)

Expand Down Expand Up @@ -1024,7 +1044,7 @@ func New(
upgrade.NewAppModule(app.UpgradeKeeper, addresscodec.NewBech32Codec(sdk.Bech32PrefixAccAddr)),
evidence.NewAppModule(app.EvidenceKeeper),
ibc.NewAppModule(app.IBCKeeper),
ica.NewAppModule(nil, &app.ICAHostKeeper),
ica.NewAppModule(&app.ICAControllerKeeper, &app.ICAHostKeeper),
params.NewAppModule(app.ParamsKeeper),
consensus.NewAppModule(appCodec, app.ConsensusParamsKeeper),
transferModule,
Expand Down
12 changes: 9 additions & 3 deletions protocol/app/msgs/all_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,9 +308,15 @@ var (
// since ICA Controller Keeper is initialized as nil.
// However, since the ica.AppModuleBasic{} needs to be passed to basic_mananger as a whole, these messages
// registered in the interface registry.
"/ibc.applications.interchain_accounts.v1.InterchainAccount": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTx": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccount": {},
"/ibc.applications.interchain_accounts.v1.InterchainAccount": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTx": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTxResponse": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccount": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccountResponse": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParams": {},
"/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParamsResponse": {},
"/ibc.applications.interchain_accounts.host.v1.MsgUpdateParams": {},
"/ibc.applications.interchain_accounts.host.v1.MsgUpdateParamsResponse": {},
}

// DisallowMsgs are messages that cannot be externally submitted.
Expand Down
19 changes: 13 additions & 6 deletions protocol/app/msgs/internal_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
slashing "github.com/cosmos/cosmos-sdk/x/slashing/types"
staking "github.com/cosmos/cosmos-sdk/x/staking/types"
icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
ibctransfer "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
ibcconn "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
Expand Down Expand Up @@ -152,11 +154,16 @@ var (
"/dydxprotocol.vest.MsgDeleteVestEntryResponse": nil,

// ibc
"/ibc.applications.transfer.v1.MsgUpdateParams": &ibctransfer.MsgUpdateParams{},
"/ibc.applications.transfer.v1.MsgUpdateParamsResponse": nil,
"/ibc.core.client.v1.MsgUpdateParams": &ibcclient.MsgUpdateParams{},
"/ibc.core.client.v1.MsgUpdateParamsResponse": nil,
"/ibc.core.connection.v1.MsgUpdateParams": &ibcconn.MsgUpdateParams{},
"/ibc.core.connection.v1.MsgUpdateParamsResponse": nil,
"/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParams": &icacontrollertypes.
MsgUpdateParams{},
"/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParamsResponse": nil,
"/ibc.applications.interchain_accounts.host.v1.MsgUpdateParams": &icahosttypes.MsgUpdateParams{},
"/ibc.applications.interchain_accounts.host.v1.MsgUpdateParamsResponse": nil,
"/ibc.applications.transfer.v1.MsgUpdateParams": &ibctransfer.MsgUpdateParams{},
"/ibc.applications.transfer.v1.MsgUpdateParamsResponse": nil,
"/ibc.core.client.v1.MsgUpdateParams": &ibcclient.MsgUpdateParams{},
"/ibc.core.client.v1.MsgUpdateParamsResponse": nil,
"/ibc.core.connection.v1.MsgUpdateParams": &ibcconn.MsgUpdateParams{},
"/ibc.core.connection.v1.MsgUpdateParamsResponse": nil,
}
)
4 changes: 4 additions & 0 deletions protocol/app/msgs/internal_msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,10 @@ func TestInternalMsgSamples_Gov_Key(t *testing.T) {
"/dydxprotocol.vest.MsgSetVestEntryResponse",

// ibc
"/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParams",
"/ibc.applications.interchain_accounts.controller.v1.MsgUpdateParamsResponse",
"/ibc.applications.interchain_accounts.host.v1.MsgUpdateParams",
"/ibc.applications.interchain_accounts.host.v1.MsgUpdateParamsResponse",
"/ibc.applications.transfer.v1.MsgUpdateParams",
"/ibc.applications.transfer.v1.MsgUpdateParamsResponse",
"/ibc.core.client.v1.MsgUpdateParams",
Expand Down
5 changes: 3 additions & 2 deletions protocol/app/msgs/unsupported_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package msgs
import (
sdk "github.com/cosmos/cosmos-sdk/types"
govbeta "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"

icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
)

Expand All @@ -15,8 +14,10 @@ var (
"/cosmos.gov.v1beta1.MsgSubmitProposalResponse": nil,

// ICA Controller messages - these are not used since ICA Controller is disabled.
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTx": &icacontrollertypes.MsgSendTx{},
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTx": &icacontrollertypes.MsgSendTx{},
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTxResponse": nil,
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccount": &icacontrollertypes.
MsgRegisterInterchainAccount{},
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccountResponse": nil,
}
)
2 changes: 2 additions & 0 deletions protocol/app/msgs/unsupported_msgs_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ func TestUnsupportedMsgSamples_Key(t *testing.T) {

// ICA Controller messages
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccount",
"/ibc.applications.interchain_accounts.controller.v1.MsgRegisterInterchainAccountResponse",
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTx",
"/ibc.applications.interchain_accounts.controller.v1.MsgSendTxResponse",
}

require.Equal(t, expectedMsgs, lib.GetSortedKeys[sort.StringSlice](msgs.UnsupportedMsgSamples))
Expand Down
1 change: 0 additions & 1 deletion protocol/app/testdata/default_genesis_state.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,6 @@
"port": "icahost"
}
},
"params": null,
"perpetuals": {
"perpetuals": [],
"liquidity_tiers": [],
Expand Down
2 changes: 1 addition & 1 deletion protocol/daemons/liquidation/client/sub_task_runner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package client_test

import (
"context"
"cosmossdk.io/log"
"testing"

"github.com/cometbft/cometbft/libs/log"
"github.com/dydxprotocol/v4-chain/protocol/daemons/flags"
"github.com/dydxprotocol/v4-chain/protocol/daemons/liquidation/api"
"github.com/dydxprotocol/v4-chain/protocol/daemons/liquidation/client"
Expand Down
6 changes: 5 additions & 1 deletion protocol/lib/ante/internal_msg.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ import (
gov "github.com/cosmos/cosmos-sdk/x/gov/types/v1"
slashing "github.com/cosmos/cosmos-sdk/x/slashing/types"
staking "github.com/cosmos/cosmos-sdk/x/staking/types"
icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
icahosttypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/host/types"
ibctransfer "github.com/cosmos/ibc-go/v8/modules/apps/transfer/types"
ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
ibcclient "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" //nolint:staticcheck
ibcconn "github.com/cosmos/ibc-go/v8/modules/core/03-connection/types"
blocktime "github.com/dydxprotocol/v4-chain/protocol/x/blocktime/types"
bridge "github.com/dydxprotocol/v4-chain/protocol/x/bridge/types"
Expand Down Expand Up @@ -111,6 +113,8 @@ func IsInternalMsg(msg sdk.Msg) bool {
*vest.MsgSetVestEntry,

// ibc
*icahosttypes.MsgUpdateParams,
*icacontrollertypes.MsgUpdateParams,
*ibctransfer.MsgUpdateParams,
*ibcclient.MsgUpdateParams,
*ibcconn.MsgUpdateParams:
Expand Down
1 change: 0 additions & 1 deletion protocol/lib/ante/unsupported_msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package ante
import (
sdk "github.com/cosmos/cosmos-sdk/types"
govbeta "github.com/cosmos/cosmos-sdk/x/gov/types/v1beta1"

icacontrollertypes "github.com/cosmos/ibc-go/v8/modules/apps/27-interchain-accounts/controller/types"
)

Expand Down
2 changes: 1 addition & 1 deletion protocol/x/clob/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ import (
storetypes "cosmossdk.io/store/types"
"github.com/cosmos/cosmos-sdk/codec"
sdk "github.com/cosmos/cosmos-sdk/types"
liquidationtypes "github.com/dydxprotocol/v4-chain/protocol/daemons/server/types/liquidations"
"github.com/dydxprotocol/v4-chain/protocol/indexer/indexer_manager"
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/lib/metrics"
flags "github.com/dydxprotocol/v4-chain/protocol/x/clob/flags"
"github.com/dydxprotocol/v4-chain/protocol/x/clob/rate_limit"
"github.com/dydxprotocol/v4-chain/protocol/x/clob/types"
liquidationtypes "github.com/dydxprotocol/v4-chain/protocol/daemons/server/types/liquidations"
)

type (
Expand Down