Skip to content

Latest commit

 

History

History
863 lines (728 loc) · 38.7 KB

Dup1337-Q.md

File metadata and controls

863 lines (728 loc) · 38.7 KB
Raw
Issues
[L-01] Usage of deprecated WrapSDKContext()
[L-02] RandomizedParams still used by the protocol
[L-03] Using deprecated simapp/params.MakeTestEncodingConfig
[L-04] Unnecessary event emission
[L-05] Cometbft logger leftovers
[L-06] Double order set of modules
[L-07] Using deprecated RandomizedParams
[L-08] Using deprecated sdk.Msg interface as input
[L-09] Importing deprecated simapp package
[L-10] Usage of deprecated MakeTestEncodingConfig
[L-11] Incorrect import of protobuf
[L-12] The protobuf strings are hardcoded
[L-13] Usage of deprecated params module
[L-14] Usega of deprecated AppModuleBasic
[L-15] Usage of deprecated ServerContext
[L-16] SimulateFromSeed args
[L-17] CometBFT home directory setting
[L-18] Broken link is used for the proto files
[NC-01] Usage of legacy amino is no longer needed
[NC-02] Old way of passing StoreKey
[NC-03] Missing checks for implementing all interfaces
[NC-04] Not upgrading to []byte from cometbft types

[L-01] Usage of deprecated WrapSDKContext()

WrapSDKContext() has depreciation notice on it:

// Deprecated: there is no need to wrap anymore as the Cosmos SDK context implements context.Context.
func WrapSDKContext(ctx Context) context.Context {
	return ctx
}

Canto still uses this function in multiple places.

List of occurences ``` x/inflation/keeper/grpc_query_test.go|49 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/inflation/keeper/grpc_query_test.go|108 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/inflation/keeper/grpc_query_test.go|158 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/inflation/keeper/grpc_query_test.go|174 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/inflation/keeper/grpc_query_test.go|191 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/inflation/keeper/grpc_query_test.go|206 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/coinswap/keeper/grpc_query_test.go|10 col 40| resp, err := s.queryClient.Params(sdk.WrapSDKContext(s.ctx), &types.QueryParamsRequest{}) x/coinswap/keeper/grpc_query_test.go|20 col 47| resp, err := s.queryClient.LiquidityPool(sdk.WrapSDKContext(s.ctx), &types.QueryLiquidityPoolRequest{LptDenom: pool.LptDenom}) x/coinswap/keeper/grpc_query_test.go|39 col 48| resp, err := s.queryClient.LiquidityPools(sdk.WrapSDKContext(s.ctx), &types.QueryLiquidityPoolsRequest{}) x/coinswap/keeper/grpc_query_test.go|44 col 47| resp, err = s.queryClient.LiquidityPools(sdk.WrapSDKContext(s.ctx), &types.QueryLiquidityPoolsRequest{}) x/onboarding/keeper/grpc_query_test.go|10 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/onboarding/keeper/ibc_callbacks.go|137 col 44| if _, err = k.erc20Keeper.ConvertCoin(sdk.WrapSDKContext(ctx), convertMsg); err != nil { x/epochs/keeper/grpc_query_test.go|185 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/evm.go|187 col 46| gasRes, err := k.evmKeeper.EstimateGas(sdk.WrapSDKContext(ctx), &evmtypes.EthCallRequest{ x/erc20/keeper/keeper_test.go|211 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/keeper_test.go|266 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/keeper_test.go|309 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/keeper_test.go|405 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|343 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|521 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|531 col 14| ctx = sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|1079 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|1138 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|1143 col 14| ctx = sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|1293 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|1471 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/msg_server_test.go|1481 col 14| ctx = sdk.WrapSDKContext(suite.ctx) x/govshuttle/keeper/msg_server_test.go|15 col 44| //return keeper.NewMsgServerImpl(*k), sdk.WrapSDKContext(ctx) x/erc20/keeper/evm_hooks_test.go|213 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/govshuttle/keeper/keeper_test.go|63 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/govshuttle/keeper/grpc_query_params_test.go|12 col 17| // wctx := sdk.WrapSDKContext(ctx) x/erc20/keeper/grpc_query_test.go|69 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/grpc_query_test.go|149 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/erc20/keeper/grpc_query_test.go|164 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|13 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|117 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|135 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|193 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|210 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|287 col 15| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|303 col 13| ctx := sdk.WrapSDKContext(suite.ctx) x/csr/keeper/grpc_query_test.go|315 col 13| ctx := sdk.WrapSDKContext(suite.ctx) ```

[L-02] RandomizedParams still used by the protocol

According to the docs

Remove RandomizedParams from AppModuleSimulation interface. Previously, it used to generate random parameter changes during simulations, however, it does so through ParamChangeProposal which is now legacy. Since all modules were migrated, we can now safely remove this from AppModuleSimulation interface.

It's still used in the codebase though: canto-main/x/epochs/module.go

168 // RandomizedParams creates randomizedepochs param changes for the simulator.
169 func (AppModule) RandomizedParams(r *rand.Rand) []simtypes.LegacyParamChange {
170    return []simtypes.LegacyParamChange{}
171 }

[L-03] Using deprecated simapp/params.MakeTestEncodingConfig

According to docs

simapp.MakeTestEncodingConfig() was deprecated and has been removed. Instead you can use the TestEncodingConfig from the types/module/testutil package

However it's still being used in the code: canto-main/x/coinswap/simulation/operations.go

  3 import (
//[...]
 11    simappparams "cosmossdk.io/simapp/params"
//[...]

177       txGen := simappparams.MakeTestEncodingConfig().TxConfig
178  
179       tx, err := simtestutil.GenSignedMockTx(
180          r,
181          txGen,
182          []sdk.Msg{msg},
183          fees,
184          simtestutil.DefaultGenTxGas,
185          chainID,
186          []uint64{account.GetAccountNumber()},
187          []uint64{account.GetSequence()},
188          simAccount.PrivKey,
189       )

[L-04] Unnecessary event emission

According to docs

EventTypeMessage events, with sdk.AttributeKeyModule and sdk.AttributeKeySender are now emitted directly at message execution (in baseapp). This means that the following boilerplate should be removed from all your custom modules:

ctx.EventManager().EmitEvent( sdk.NewEvent( sdk.EventTypeMessage, sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory), sdk.NewAttribute(sdk.AttributeKeySender, signer/sender), ), )

However Canto still uses it:

canto-main/x/coinswap/keeper/msg_server.go
 59    ctx.EventManager().EmitEvent(
 60       sdk.NewEvent(
 61          sdk.EventTypeMessage,
 62          sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
 63          sdk.NewAttribute(sdk.AttributeKeySender, msg.Sender),
 64       ),

[L-05] Cometbft logger leftovers

According to the docs

Replace all your CometBFT logger imports by cosmossdk.io/log.

Canto still uses comet logger:

server/log/cmt_logger.go

import (
    cmtlog "github.com/cometbft/cometbft/libs/log"

    "cosmossdk.io/log"
)

var _ cmtlog.Logger = (*CometLoggerWrapper)(nil)

// CometLoggerWrapper provides a wrapper around a cosmossdk.io/log instance.
// It implements CometBFT's Logger interface.
type CometLoggerWrapper struct {
    log.Logger
}

Which is then used in server/api/server.go

    go func(enableUnsafeCORS bool) {
        s.logger.Info("starting API server...", "address", cfg.API.Address)

        if enableUnsafeCORS {
            allowAllCORS := handlers.CORS(handlers.AllowedHeaders([]string{"Content-Type"}))
            errCh <- tmrpcserver.Serve(s.listener, allowAllCORS(s.Router), servercmtlog.CometLoggerWrapper{Logger: s.logger}, cmtCfg)
        } else {
            errCh <- tmrpcserver.Serve(s.listener, s.Router, servercmtlog.CometLoggerWrapper{Logger: s.logger}, cmtCfg)
        }
    }(cfg.API.EnableUnsafeCORS)

[L-06] Double order set of modules

According to docs, upgrade module should be initialized only in PreBlocker, however canto initialized it both in pre blocker and begin blocker:

ethermint-main/app/app.go

 639    // NOTE: upgrade module is required to be prioritized
 640    app.ModuleManager.SetOrderPreBlockers(
 641 @>    upgradetypes.ModuleName,
 642    )
 643    // During begin block slashing happens after distr.BeginBlocker so that
 644    // there is nothing left over in the validator fee pool, so as to keep the
 645    // CanWithdrawInvariant invariant.
 646    // NOTE: upgrade module must go first to handle software upgrades.
 647    // NOTE: staking module is required if HistoricalEntries param > 0
 648    // NOTE: capability module's beginblocker must come before any modules using capabilities (e.g. IBC)
 649    app.ModuleManager.SetOrderBeginBlockers(
//[...]
 668 @>    upgradetypes.ModuleName,

The same in canto-main/app/app.go

ModuleManager.SetOrder... checks if it implements correct function, however it leads to confusion, and the behavious may change in the future.

func (m *Manager) BeginBlock(ctx sdk.Context) (sdk.BeginBlock, error) {
    ctx = ctx.WithEventManager(sdk.NewEventManager())
    for _, moduleName := range m.OrderBeginBlockers {
        if module, ok := m.Modules[moduleName].(appmodule.HasBeginBlocker); ok {
            if err := module.BeginBlock(ctx); err != nil {
                return sdk.BeginBlock{}, err
            }
        }
    }

    return sdk.BeginBlock{
        Events: ctx.EventManager().ABCIEvents(),
    }, nil
}

[L-07] Using deprecated RandomizedParams

As per the docs:

Remove RandomizedParams from AppModuleSimulation interface. Previously, it used to generate random parameter changes during simulations, however, it does so through ParamChangeProposal which is now legacy. Since all modules were migrated, we can now safely remove this from AppModuleSimulation interface.

And the existing code has this interface:

Contract: module_simulation.go

47: // RandomizedParams creates randomized  param changes for the simulator
48: func (am AppModule) RandomizedParams(_ *rand.Rand) []simtypes.LegacyParamChange {
49: 
50:     return []simtypes.LegacyParamChange{}
51: }
List of occurences ``` canto-main/x/govshuttle/module_simulation.go|47 col 4| // RandomizedParams creates randomized param changes for the simulator canto-main/x/govshuttle/module_simulation.go|48 col 21| func (am AppModule) RandomizedParams(_ *rand.Rand) []simtypes.LegacyParamChange { canto-main/x/erc20/module.go|151 col 21| func (am AppModule) RandomizedParams(r *rand.Rand) []simtypes.LegacyParamChange { canto-main/x/coinswap/module.go|155 col 4| // RandomizedParams creates randomized coinswap param changes for the simulator. canto-main/x/coinswap/module.go|156 col 18| func (AppModule) RandomizedParams(r *rand.Rand) []simtypes.LegacyParamChange { canto-main/x/inflation/module.go|175 col 4| // RandomizedParams creates randomized inflation param changes for the simulator. canto-main/x/inflation/module.go|176 col 21| func (am AppModule) RandomizedParams(r *rand.Rand) []simtypes.LegacyParamChange { canto-main/x/onboarding/module.go|143 col 18| func (AppModule) RandomizedParams(_ *rand.Rand) []simtypes.LegacyParamChange { canto-main/x/epochs/module.go|168 col 4| // RandomizedParams creates randomizedepochs param changes for the simulator. canto-main/x/epochs/module.go|169 col 18| func (AppModule) RandomizedParams(r *rand.Rand) []simtypes.LegacyParamChange { ```

[L-08] Using deprecated sdk.Msg interface as input

As per the docs:

RFC 001 has defined a simplification of the message validation process for modules. The sdk.Msg interface has been updated to not require the implementation of the ValidateBasic method. It is now recommended to validate message directly in the message server. When the validation is performed in the message server, the ValidateBasic method on a message is no longer required and can be removed. Messages no longer need to implement the LegacyMsg interface and implementations of GetSignBytes can be deleted. Because of this change, global legacy Amino codec definitions and their registration in init() can safely be removed as well.

But it´s passed as an input below:

Contract: utils_test.go

474: // StdSignBytes returns the bytes to sign for a transaction.
475: func StdSignBytes(cdc *codec.LegacyAmino, chainID string, accnum uint64, sequence uint64, timeout uint64, fee legacytx.StdFee, msgs []sdk.Msg, memo string, tip *txtypes.Tip) []byte {
476:     msgsBytes := make([]json.RawMessage, 0, len(msgs))
477:     for _, msg := range msgs {
478:         legacyMsg, ok := msg.(legacytx.LegacyMsg)
479:         if !ok {
480:             panic(fmt.Errorf("expected %T when using amino JSON", (*legacytx.LegacyMsg)(nil)))
481:         }
482: 
483:         msgsBytes = append(msgsBytes, json.RawMessage(legacyMsg.GetSignBytes()))
484:     }

[L-09] Importing deprecated simapp package

As per the docs;

The simapp package should not be imported in your own app. Instead, you should import the runtime.AppI interface, that defines an App, and use the simtestutil package for application testing.

But it´s imported and used in canto-main/app

Contract: app.go

37:     "cosmossdk.io/simapp"
Contract: app.go

338:     // create and set dummy vote extension handler
339:     voteExtOp := func(bApp *baseapp.BaseApp) {
340:         voteExtHandler := simapp.NewVoteExtensionHandler()
Contract: app.go

1067: // InitChainer updates at chain initialization
1068: func (app *Canto) InitChainer(ctx sdk.Context, req *abci.RequestInitChain) (*abci.ResponseInitChain, error) {
1069:     var genesisState simapp.GenesisState

[L-10] Usage of deprecated MakeTestEncodingConfig

As per the docs:

simapp.MakeTestEncodingConfig() was deprecated and has been removed. Instead you can use the TestEncodingConfig from the types/module/testutil package. This means you can replace your usage of simapp.MakeTestEncodingConfig in tests to moduletestutil.MakeTestEncodingConfig, which takes a series of relevant AppModuleBasic as input (the module being tested and any potential dependencies).

But it´s used in the codebase ( coinswap/simulation/operations.go )and in the readme of canto-main/ibc/testing/readme.md;

Contract: operations.go

177:         txGen := simappparams.MakeTestEncodingConfig().TxConfig
Contract: operations.go

332:         txGen := simappparams.MakeTestEncodingConfig().TxConfig
Contract: operations.go

440:         txGen := simappparams.MakeTestEncodingConfig().TxConfig
Contract: README.md

115: ### Initialize TestingApp
116: 
117: The testing package requires that you provide a function to initialize your TestingApp. This is how ibc-go implements the initialize function with its `SimApp`:
118: 
119: 
120: func SetupTestingApp() (TestingApp, map[string]json.RawMessage) {
121:     db := dbm.NewMemDB()
122:     encCdc := simapp.MakeTestEncodingConfig()
123:     app := simapp.NewSimApp(log.NewNopLogger(), db, nil, true, map[int64]bool{}, simapp.DefaultNodeHome, 5, encCdc, simapp.EmptyAppOptions{})
124:     return app, simapp.NewDefaultGenesisState(encCdc.Marshaler)
125: }
126:

[L-11] Incorrect import of protobuf

As per the docs;

The SDK has migrated from gogo/protobuf (which is currently unmaintained), to our own maintained fork, cosmos/gogoproto.

This means you should replace all imports of github.com/gogo/protobuf to github.com/cosmos/gogoproto. This allows you to remove the replace directive replace github.com/gogo/protobuf => github.com/regen-network/protobuf v1.3.3-alpha.regen.1 from your go.mod file.

While go.mod files for canto-main and ethermint-main imports protobuf as:

Contract: go.mod

32:     github.com/gogo/protobuf v1.3.2

The proto-tools-installer.sh script installs the latest one which might cause dependency issues:

Contract: proto-tools-installer.sh

86: f_install_protoc_gen_gocosmos() {
87:     f_print_installing_with_padding protoc-gen-gocosmos
88:     
89:     if ! grep "github.com/gogo/protobuf => github.com/regen-network/protobuf" go.mod &>/dev/null ; then
90:         echo -e "\tPlease run this command from somewhere inside the canto folder."
91:         return 1
92:     fi
93:     
94:     go get github.com/regen-network/cosmos-proto/protoc-gen-gocosmos 2>/dev/null
95:     f_print_done
96: }

Especially given that this: https://github.com/regen-network/cosmos-proto was deprcated in favor of cosmos/gogoproto and v1.5.0 of https://github.com/cosmos/gogoproto/releases/tag/v1.5.0 is current one

[L-12] The protobuf strings are hardcoded

As per the docs:

The SDK is normalizing the strings inside the Protobuf accepts_interface and implements_interface annotations. We require them to be fully-scoped names. They will soon be used by code generators like Pulsar and Telescope to match which messages can or cannot be packed inside Anys.

Here are the following replacements that you need to perform on your proto files:

- "Content"
+ "cosmos.gov.v1beta1.Content"
- "Authorization"
+ "cosmos.authz.v1beta1.Authorization"
- "sdk.Msg"
+ "cosmos.base.v1beta1.Msg"
- "AccountI"
+ "cosmos.auth.v1beta1.AccountI"
- "ModuleAccountI"
+ "cosmos.auth.v1beta1.ModuleAccountI"
- "FeeAllowanceI"
+ "cosmos.feegrant.v1beta1.FeeAllowanceI"

the codebase does not fulfil this requirement: In ethertmint-main/proto/ethermint/evm/v1/tx.proto

Contract: tx.proto

52:   option (cosmos_proto.implements_interface) = "TxData";
Contract: tx.proto

78:   option (cosmos_proto.implements_interface) = "TxData";
Contract: tx.proto

113:   option (cosmos_proto.implements_interface) = "TxData";

[L-13] Usage of deprecated params module

As per the docs:

The params module was deprecated since v0.46. The Cosmos SDK has migrated away from x/params for its own modules. Cosmos SDK modules now store their parameters directly in its repective modules. The params module will be removed in v0.48, as mentioned in v0.46 release. It is strongly encouraged to migrate away from x/params before v0.48.

When performing a chain migration, the params table must be initizalied manually. This was done in the modules keepers in previous versions. Have a look at simapp.RegisterUpgradeHandlers() for an example.

The codebase still uses the module: In canto-main/app/app.go:

Contract: app.go

 99:     "github.com/cosmos/cosmos-sdk/x/params"
100:     paramsclient "github.com/cosmos/cosmos-sdk/x/params/client"
101:     paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper"
102:     paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
103:     paramproposal "github.com/cosmos/cosmos-sdk/x/params/types/proposal"

In canto-main/app/upgrades/v8/upgrades.go

Contract: upgrades.go

13:     paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"

In canto-main/ibc/testing/simapp/app.go:

Contract: app.go

92:     "github.com/cosmos/cosmos-sdk/x/params"
93:     paramsclient "github.com/cosmos/cosmos-sdk/x/params/client"
94:     paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper"
95:     paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
96:     paramproposal "github.com/cosmos/cosmos-sdk/x/params/types/proposal"

And many more for canto-main.

In ethermint-main/app/app.go:

Contract: app.go

105:     "github.com/cosmos/cosmos-sdk/x/params"
106:     paramsclient "github.com/cosmos/cosmos-sdk/x/params/client"
107:     paramskeeper "github.com/cosmos/cosmos-sdk/x/params/keeper"
108:     paramstypes "github.com/cosmos/cosmos-sdk/x/params/types"
109:     paramproposal "github.com/cosmos/cosmos-sdk/x/params/types/proposal"

We're not sure if this is as designed, but https://github.com/cosmos/cosmos-sdk/blob/v0.46.1/UPGRADING.md#xparams mentions pull request that performed migration differently: https://github.com/cosmos/cosmos-sdk/pull/12363/files , specifically via:

x/mint/keeper/migrator.go

// Migrate1to2 migrates the x/mint module state from the consensus version 1 to
// version 2. Specifically, it takes the parameters that are currently stored
// and managed by the x/params modules and stores them directly into the x/mint
// module state.
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
    return v2.Migrate(ctx, ctx.KVStore(m.keeper.storeKey), m.legacySubspace, m.keeper.cdc)
}

[L-14] Using deprecated AppModuleBasic

As per the update:

The notion of basic manager does not exist anymore (and all related helpers).

  • The module manager now can do everything that the basic manager was doing.
  • AppModuleBasic has been deprecated for extension interfaces.
  • Modules can now implement appmodule.HasRegisterInterfaces, module.HasGRPCGateway and module.HasAminoCodec when relevant.
  • SDK modules now directly implement those extension interfaces on AppModule instead of AppModuleBasic.

However, all there apps canto-main/app, canto-main/ibc/testing/simapp, ethermint-main/app uses the removed manager in which the initilizations of the module managers will fail;

Contract: app.go

779:     app.BasicModuleManager = module.NewBasicManagerFromManager(
780:         app.ModuleManager,
781:         map[string]module.AppModuleBasic{
782:             genutiltypes.ModuleName: genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
783:             govtypes.ModuleName: gov.NewAppModuleBasic(
784:                 []govclient.ProposalHandler{
785:                     paramsclient.ProposalHandler,
786:                 },
787:             ),
788:         })
Contract: app.go

607:     app.BasicModuleManager = module.NewBasicManagerFromManager(
608:         app.ModuleManager,
609:         map[string]module.AppModuleBasic{
610:             genutiltypes.ModuleName: genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
611:             govtypes.ModuleName: gov.NewAppModuleBasic(
612:                 []govclient.ProposalHandler{
613:                     paramsclient.ProposalHandler,
614:                 },
615:             ),
616:         })
Contract: app.go

626:     app.BasicModuleManager = module.NewBasicManagerFromManager(
627:         app.ModuleManager,
628:         map[string]module.AppModuleBasic{
629:             genutiltypes.ModuleName: genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator),
630:             govtypes.ModuleName: gov.NewAppModuleBasic(
631:                 []govclient.ProposalHandler{
632:                     paramsclient.ProposalHandler,
633:                 },
634:             ),
635:         })

[L-15] Usage of deprecated ServerContext

As per the update here;

Deprecated ServerContext. To get cmtcfg.Config from cmd, use client.GetCometConfigFromCmd(cmd) instead of server.GetServerContextFromCmd(cmd).Config The codebase uses the depreceated context in:

Contract: genaccounts.go

42:             serverCtx := server.GetServerContextFromCmd(cmd)
43:             config := serverCtx.Config
Contract: init.go

77:             serverCtx := server.GetServerContextFromCmd(cmd)
78:             config := serverCtx.Config

In canto-main/cmd/cantod

Contract: testnet.go

139:             serverCtx := sdkserver.GetServerContextFromCmd(cmd)
140: 
141:             args := initArgs{}
142:             args.outputDir, _ = cmd.Flags().GetString(flagOutputDir)
143:             args.keyringBackend, _ = cmd.Flags().GetString(flags.FlagKeyringBackend)
144:             args.chainID, _ = cmd.Flags().GetString(flags.FlagChainID)
145:             args.minGasPrices, _ = cmd.Flags().GetString(sdkserver.FlagMinGasPrices)
146:             args.nodeDirPrefix, _ = cmd.Flags().GetString(flagNodeDirPrefix)
147:             args.nodeDaemonHome, _ = cmd.Flags().GetString(flagNodeDaemonHome)
148:             args.startingIPAddress, _ = cmd.Flags().GetString(flagStartingIPAddress)
149:             args.numValidators, _ = cmd.Flags().GetInt(flagNumValidators)
150:             args.algo, _ = cmd.Flags().GetString(flags.FlagKeyAlgorithm)
151: 
152:             return initTestnetFiles(clientCtx, cmd, serverCtx.Config, mbm, genBalIterator, clientCtx.TxConfig.SigningContext().ValidatorAddressCodec(), args)
153:         },

In ethermint-main/client

Contract: testnet.go

154:             serverCtx := sdkserver.GetServerContextFromCmd(cmd)
155: 
156:             args := initArgs{}
157:             args.outputDir, _ = cmd.Flags().GetString(flagOutputDir)
158:             args.keyringBackend, _ = cmd.Flags().GetString(flags.FlagKeyringBackend)
159:             args.chainID, _ = cmd.Flags().GetString(flags.FlagChainID)
160:             args.minGasPrices, _ = cmd.Flags().GetString(sdkserver.FlagMinGasPrices)
161:             args.nodeDirPrefix, _ = cmd.Flags().GetString(flagNodeDirPrefix)
162:             args.nodeDaemonHome, _ = cmd.Flags().GetString(flagNodeDaemonHome)
163:             args.startingIPAddress, _ = cmd.Flags().GetString(flagStartingIPAddress)
164:             args.numValidators, _ = cmd.Flags().GetInt(flagNumValidators)
165:             args.algo, _ = cmd.Flags().GetString(flags.FlagKeyAlgorithm)
166: 
167:             return initTestnetFiles(clientCtx, cmd, serverCtx.Config, mbm, genBalIterator, clientCtx.TxConfig.SigningContext().ValidatorAddressCodec(), args)
168:         },
Contract: genaccounts.go

65:             serverCtx := server.GetServerContextFromCmd(cmd)
66:             config := serverCtx.Config
Contract: indexer_cmd.go

44:             serverCtx := server.GetServerContextFromCmd(cmd)
45:             clientCtx, err := client.GetClientQueryContext(cmd)
46:             if err != nil {
47:                 return err
48:             }
49: 
50:             direction := args[0]
51:             if direction != "backward" && direction != "forward" {
52:                 return fmt.Errorf("unknown index direction, expect: backward|forward, got: %s", direction)
53:             }
54: 
55:             cfg := serverCtx.Config
Contract: start.go

131:             serverCtx := server.GetServerContextFromCmd(cmd)
144:             serverCtx := server.GetServerContextFromCmd(cmd)

[L-16] SimulateFromSeed args

As per the PR

SimulateFromSeed now takes an address codec as argument.

But existing codebase does not reflect it:

Contract: sim_test.go

 97:     _, simParams, simErr := simulation.SimulateFromSeed(
 98:         t,
 99:         os.Stdout,
100:         cantoApp.BaseApp,
101:         AppStateFn(cantoApp.AppCodec(), cantoApp.SimulationManager()),
102:         RandomAccounts, // replace with own random account function if using keys other than secp256k1
103:         simtestutil.SimulationOperations(cantoApp, cantoApp.AppCodec(), config),
104:         cantoApp.ModuleAccountAddrs(),
105:         config,
106:         cantoApp.AppCodec(),
107:     )

Contract: sim_test.go

146:     _, simParams, simErr := simulation.SimulateFromSeed(
147:         t,
148:         os.Stdout,
149:         app.BaseApp,
150:         AppStateFn(app.AppCodec(), app.SimulationManager()),
151:         RandomAccounts, // replace with own random account function if using keys other than secp256k1
152:         simtestutil.SimulationOperations(app, app.AppCodec(), config),
153:         app.ModuleAccountAddrs(),
154:         config,
155:         app.AppCodec(),
156:     )

Contract: sim_test.go

310: 
311:             _, _, err := simulation.SimulateFromSeed(
312:                 t,
313:                 os.Stdout,
314:                 app.BaseApp,
315:                 AppStateFn(app.AppCodec(), app.SimulationManager()),
316:                 RandomAccounts,
317:                 simtestutil.SimulationOperations(app, app.AppCodec(), config),
318:                 app.ModuleAccountAddrs(),
319:                 config,
320:                 app.AppCodec(),
321:             )

Contract: sim_test.go

366:     stopEarly, simParams, simErr := simulation.SimulateFromSeed(
367:         t,
368:         os.Stdout,
369:         app.BaseApp,
370:         AppStateFn(app.AppCodec(), app.SimulationManager()),
371:         RandomAccounts, // Replace with own random account function if using keys other than secp256k1
372:         simtestutil.SimulationOperations(app, app.AppCodec(), config),
373:         app.ModuleAccountAddrs(),
374:         config,
375:         app.AppCodec(),
376:     )

Contract: sim_test.go

415:     _, _, err = simulation.SimulateFromSeed(
416:         t,
417:         os.Stdout,
418:         newApp.BaseApp,
419:         AppStateFn(app.AppCodec(), app.SimulationManager()),
420:         RandomAccounts, // Replace with own random account function if using keys other than secp256k1
421:         simtestutil.SimulationOperations(newApp, newApp.AppCodec(), config),
422:         app.ModuleAccountAddrs(),
423:         config,
424:         app.AppCodec(),
425:     )
426:     require.NoError(t, err)

[L-17] CometBFT home directory setting

According to the docs:

CometBFT, by default, will consider its home directory in ~/.cometbft from now on instead of ~/.tendermint.

We couldn´t confirm the existing config for this requirement.

[L-18] Broken link is used for the proto files

Cosmos switched to CometBFT, while the Makefile still gets proto files from https://raw.githubusercontent.com/tendermint/tendermint/v0.34.15/proto/tendermint

canto-main/Makefile

TM_URL              = https://raw.githubusercontent.com/tendermint/tendermint/v0.34.15/proto/tendermint <====
GOGO_PROTO_URL      = https://raw.githubusercontent.com/regen-network/protobuf/cosmos
COSMOS_SDK_URL      = https://raw.githubusercontent.com/cosmos/cosmos-sdk/v0.45.1
ETHERMINT_URL          = https://raw.githubusercontent.com/Canto-Network/ethermint-v2/v0.10.0
IBC_GO_URL              = https://raw.githubusercontent.com/cosmos/ibc-go/v3.0.0-rc0
COSMOS_PROTO_URL    = https://raw.githubusercontent.com/regen-network/cosmos-proto/master

TM_CRYPTO_TYPES     = third_party/proto/tendermint/crypto
TM_ABCI_TYPES       = third_party/proto/tendermint/abci
TM_TYPES            = third_party/proto/tendermint/types

GOGO_PROTO_TYPES    = third_party/proto/gogoproto

COSMOS_PROTO_TYPES  = third_party/proto/cosmos_proto

proto-update-deps:
    @mkdir -p $(GOGO_PROTO_TYPES)
    @curl -sSL $(GOGO_PROTO_URL)/gogoproto/gogo.proto > $(GOGO_PROTO_TYPES)/gogo.proto

    @mkdir -p $(COSMOS_PROTO_TYPES)
    @curl -sSL $(COSMOS_PROTO_URL)/cosmos.proto > $(COSMOS_PROTO_TYPES)/cosmos.proto

## Importing of tendermint protobuf definitions currently requires the
## use of `sed` in order to build properly with cosmos-sdk's proto file layout
## (which is the standard Buf.build FILE_LAYOUT)
## Issue link: https://github.com/tendermint/tendermint/issues/5021
    @mkdir -p $(TM_ABCI_TYPES)
    @curl -sSL $(TM_URL)/abci/types.proto > $(TM_ABCI_TYPES)/types.proto <====

    @mkdir -p $(TM_TYPES)
    @curl -sSL $(TM_URL)/types/types.proto > $(TM_TYPES)/types.proto <====

    @mkdir -p $(TM_CRYPTO_TYPES)
    @curl -sSL $(TM_URL)/crypto/proof.proto > $(TM_CRYPTO_TYPES)/proof.proto <====
    @curl -sSL $(TM_URL)/crypto/keys.proto > $(TM_CRYPTO_TYPES)/keys.proto <====

However https://raw.githubusercontent.com/tendermint/tendermint/v0.34.15/proto/tendermint is not found

[NC-01] Usage of legacy amino is no longer needed

According to the docs

The amino codec was removed in v0.50+, this means there is not a need register legacyAminoCodec. To replace the amino codec, Amino protobuf annotations are used to provide information to the amino codec on how to encode and decode protobuf messages.

However, it's still used in Canto.

Exemplary proto file: https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/proto/canto/coinswap/v1/coinswap.proto

And auto generated .pg.go definition file from the proto: https://github.com/code-423n4/2024-05-canto/blob/main/canto-main/x/coinswap/types/coinswap.pb.go

[NC-02] Old way of passing StoreKey

Canto's custom module - group still uses old way of passing store to keeper:

x/group/keeper/keeper.go

func NewKeeper(storeKey storetypes.StoreKey, cdc codec.Codec, router baseapp.MessageRouter, accKeeper group.AccountKeeper, config group.Config) Keeper {
    k := Keeper{
        key:       storeKey,
        router:    router,
        accKeeper: accKeeper,
        cdc:       cdc,
    }

While this is not a problem, it is against the design decisions that Cosmos SDK took, according to the docs.

The following modules NewKeeper function now take a KVStoreService instead of a StoreKey

[NC-03] Missing checks for implementing all interfaces

According to docs https://github.com/cosmos/cosmos-sdk/blob/main/UPGRADING.md#all-1

:::tip It is possible to ensure that a module implements the correct interfaces by using compiler assertions in your x/{moduleName}/module.go: var ( _ module.AppModuleBasic = (*AppModule)(nil) _ module.AppModuleSimulation = (*AppModule)(nil) _ module.HasGenesis = (*AppModule)(nil)

_ appmodule.AppModule        = (*AppModule)(nil)
_ appmodule.HasBeginBlocker  = (*AppModule)(nil)
_ appmodule.HasEndBlocker    = (*AppModule)(nil)
...

)

While the protocol implements the interfaces implicitly in modules like epochs or feemarket, it's not explicit and may pose problems in the future, in case that Cosmos SDK interfaces change again, the the code is not accomodated for it.

[NC-04] Not upgrading to []byte from cometbft types

According to docs:

The usage of github.com/cometbft/cometbft/libs/bytes.HexByte has been replaced by []byte.

While this is Cosmos related, it would be good for the protocol to align with those changes. Canto is still using it, e.g.:

canto-main/x/onboarding/types/interfaces.go

  3 import (
//[...]
 12    tmbytes "github.com/cometbft/cometbft/libs/bytes"
//[...]
 70 type TransferKeeper interface {
 71    GetDenomTrace(ctx sdk.Context, denomTraceHash tmbytes.HexBytes) (transfertypes.DenomTrace, bool)
 72 }

The same situation exists for ethermint-main/x/evm/keeper/msg_server.go.

[NC-05] mismatch of storeUpgrades performed

As per the docs;

With the migrations of all modules away from x/params, the crisis module now has a store. The store must be created during a chain upgrade to v0.47.x.

  storetypes.StoreUpgrades{
    Added: []string{
        crisistypes.ModuleName,
    },
}

There are different stores created in canto-main/ibc/testing/simapp/upgrades.go:

    if upgradeInfo.Name == upgrades.V7 && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
        storeUpgrades := storetypes.StoreUpgrades{
            Added: []string{
                consensusparamtypes.StoreKey, // @audit this is loadedin V8 
                crisistypes.StoreKey,
            },
        }

        // configure store loader that checks if version == upgradeHeight and applies store upgrades
        app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
    }

    if upgradeInfo.Name == upgrades.V8 && !app.UpgradeKeeper.IsSkipHeight(upgradeInfo.Height) {
        storeUpgrades := storetypes.StoreUpgrades{
            Added: []string{
                circuittypes.ModuleName,
            },
        }
        // configure store loader that checks if version == upgradeHeight and applies store upgrades
        app.SetStoreLoader(upgradetypes.UpgradeStoreLoader(upgradeInfo.Height, &storeUpgrades))
    }

and canto-main/app/app.go:

    case v7.UpgradeName:
        storeUpgrades = &storetypes.StoreUpgrades{
            Added: []string{onboardingtypes.StoreKey, coinswaptypes.StoreKey},
        }
    case v8.UpgradeName:
        setupLegacyKeyTables(&app.ParamsKeeper)
        storeUpgrades = &storetypes.StoreUpgrades{
            Added: []string{crisistypes.StoreKey, consensusparamtypes.StoreKey},
        }
    }

Simapp is used for testing. If it's correct for testing and incorrect for mainnet, then when run it on mainnet it could result in issue.

Due to time constraints we weren't able to verify this fully, hence we leave this as NC. However we wanted to give Canto team heads up for possible issue, which we hope they will take a moment to double check