Nd unify legacy evm subspace usage (#22)

* enable registration of feemarket legacy param key table registration to
prevent panics when using the feemarket keeper with a vanilla subspace

* touch up some comments for clarity, remove unused line

* add some tests that enforce keeper registration of the key table in
addition to ensuring that the migration stays compatible when a subpsace
is shared between the module (and thus the migration) and the keeper
which is a common setup in app.go

* add test for current bug in legacy merge fork block fetching where
value is not fetched and converted into new params structure

* get tests passing by using true legacy parameters in keeper and
converting them to new params;  this fixes key table incompatibility
with the migration and a bug where the merge split block renamed value
is not set from the historical state

* refactor legacy param handling in migration (non-state breaking) and in
keeper to simplify logic; avoid additional registration of types on
proto codec; remove unused v2 migration code; and avoid copying of
EIP712 data in parameters during conversion by using the same concrete
type

* remove dead code

* add test to ensure nil params do not panic for legacy historical queries

* fix test error from rebasing onto migration paramstore refactor

* remove oboselete test now that store migrations use an indepent subpsace
and it is not passed as an argument anymore

* refactor to use a shared legacy param store to avoid injecting param keys
and amino codec through keeper to migrator.  Both keeper and migrator
are tested to ensure proper key table registraion that does not conflict
and to ensure historical params are fetchable

* add additional test to ensure store registors it's own key table when
needed

* remove unused private member

app/app.go

@@ -124,6 +124,7 @@ import (
 	"github.com/evmos/ethermint/x/evm"
 	evmkeeper "github.com/evmos/ethermint/x/evm/keeper"
 	evmtypes "github.com/evmos/ethermint/x/evm/types"
+	legacyevmtypes "github.com/evmos/ethermint/x/evm/types/legacy"
 	"github.com/evmos/ethermint/x/evm/vm/geth"
 	"github.com/evmos/ethermint/x/feemarket"
 	feemarketkeeper "github.com/evmos/ethermint/x/feemarket/keeper"
@@ -418,7 +419,7 @@ func NewEthermintApp(
 	// Set authority to x/gov module account to only expect the module account to update params
 	evmSs := app.GetSubspace(evmtypes.ModuleName)
 	app.EvmKeeper = evmkeeper.NewKeeper(
-		appCodec, cdc, keys[evmtypes.StoreKey], tkeys[evmtypes.TransientKey], keys[paramstypes.StoreKey], tkeys[paramstypes.TStoreKey],
+		appCodec, keys[evmtypes.StoreKey], tkeys[evmtypes.TransientKey],
 		authtypes.NewModuleAddress(govtypes.ModuleName),
 		app.AccountKeeper, app.BankKeeper, app.StakingKeeper, app.FeeMarketKeeper,
 		nil, geth.NewEVM, tracer, evmSs,
@@ -861,7 +862,7 @@ func initParamsKeeper(appCodec codec.BinaryCodec, legacyAmino *codec.LegacyAmino
 	paramsKeeper.Subspace(ibctransfertypes.ModuleName)
 	paramsKeeper.Subspace(ibchost.ModuleName)
 	// ethermint subspaces
-	paramsKeeper.Subspace(evmtypes.ModuleName).WithKeyTable(evmtypes.ParamKeyTable()) //nolint: staticcheck
+	paramsKeeper.Subspace(evmtypes.ModuleName).WithKeyTable(legacyevmtypes.ParamKeyTable()) //nolint: staticcheck
 	paramsKeeper.Subspace(feemarkettypes.ModuleName).WithKeyTable(feemarkettypes.ParamKeyTable())
 	return paramsKeeper
 }

x/evm/keeper/keeper.go

@@ -34,15 +34,14 @@ import (
 	ethermint "github.com/evmos/ethermint/types"
 	"github.com/evmos/ethermint/x/evm/statedb"
 	"github.com/evmos/ethermint/x/evm/types"
+	legacytypes "github.com/evmos/ethermint/x/evm/types/legacy"
 	evm "github.com/evmos/ethermint/x/evm/vm"
 )
 
 // Keeper grants access to the EVM module state and implements the go-ethereum StateDB interface.
 type Keeper struct {
 	// Protobuf codec
 	cdc codec.BinaryCodec
-	// Amino Codec used for legacy parameters
-	legacyAmino *codec.LegacyAmino
 	// Store key required for the EVM Prefix KVStore. It is required by:
 	// - storing account's Storage State
 	// - storing account's Code
@@ -53,10 +52,6 @@ type Keeper struct {
 	// key to access the transient store, which is reset on every block during Commit
 	transientKey storetypes.StoreKey
 
-	// keys used by migrator and interaction with legacy parameter store
-	paramStoreKey  storetypes.StoreKey
-	paramStoreTKey storetypes.StoreKey
-
 	// the address capable of executing a MsgUpdateParams message. Typically, this should be the x/gov module account.
 	authority sdk.AccAddress
 	// access to account state
@@ -89,9 +84,7 @@ type Keeper struct {
 // NewKeeper generates new evm module keeper
 func NewKeeper(
 	cdc codec.BinaryCodec,
-	legacyAmino *codec.LegacyAmino,
 	storeKey, transientKey storetypes.StoreKey,
-	paramStoreKey, paramStoreTKey storetypes.StoreKey,
 	authority sdk.AccAddress,
 	ak types.AccountKeeper,
 	bankKeeper types.BankKeeper,
@@ -112,19 +105,20 @@ func NewKeeper(
 		panic(err)
 	}
 
+	if !ss.HasKeyTable() {
+		ss = ss.WithKeyTable(legacytypes.ParamKeyTable())
+	}
+
 	// NOTE: we pass in the parameter space to the CommitStateDB in order to use custom denominations for the EVM operations
 	return &Keeper{
 		cdc:               cdc,
-		legacyAmino:       legacyAmino,
 		authority:         authority,
 		accountKeeper:     ak,
 		bankKeeper:        bankKeeper,
 		stakingKeeper:     sk,
 		feeMarketKeeper:   fmk,
 		storeKey:          storeKey,
 		transientKey:      transientKey,
-		paramStoreKey:     paramStoreKey,
-		paramStoreTKey:    paramStoreTKey,
 		customPrecompiles: customPrecompiles,
 		evmConstructor:    evmConstructor,
 		tracer:            tracer,

x/evm/keeper/migrations.go

@@ -36,10 +36,8 @@ func NewMigrator(keeper Keeper) Migrator {
 func (m Migrator) Migrate2to3(ctx sdk.Context) error {
 	return v3.MigrateStore(
 		ctx,
-		m.keeper.cdc,
-		m.keeper.legacyAmino,
+		m.keeper.ss,
 		m.keeper.storeKey,
-		m.keeper.paramStoreKey,
-		m.keeper.paramStoreTKey,
+		m.keeper.cdc,
 	)
 }

x/evm/keeper/params.go

@@ -18,6 +18,7 @@ package keeper
 import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/evmos/ethermint/x/evm/types"
+	legacytypes "github.com/evmos/ethermint/x/evm/types/legacy"
 )
 
 // GetParams returns the total set of evm parameters.
@@ -49,7 +50,44 @@ func (k Keeper) SetParams(ctx sdk.Context, params types.Params) error {
 
 // GetLegacyParams returns param set for version before migrate
 func (k Keeper) GetLegacyParams(ctx sdk.Context) types.Params {
-	var params types.Params
-	k.ss.GetParamSetIfExists(ctx, &params)
+	var legacyParams legacytypes.LegacyParams
+	k.ss.GetParamSetIfExists(ctx, &legacyParams)
+
+	newChainConfig := types.ChainConfig{
+		HomesteadBlock:      legacyParams.ChainConfig.HomesteadBlock,
+		DAOForkBlock:        legacyParams.ChainConfig.DAOForkBlock,
+		DAOForkSupport:      legacyParams.ChainConfig.DAOForkSupport,
+		EIP150Block:         legacyParams.ChainConfig.EIP150Block,
+		EIP150Hash:          legacyParams.ChainConfig.EIP150Hash,
+		EIP155Block:         legacyParams.ChainConfig.EIP155Block,
+		EIP158Block:         legacyParams.ChainConfig.EIP158Block,
+		ByzantiumBlock:      legacyParams.ChainConfig.ByzantiumBlock,
+		ConstantinopleBlock: legacyParams.ChainConfig.ConstantinopleBlock,
+		PetersburgBlock:     legacyParams.ChainConfig.PetersburgBlock,
+		IstanbulBlock:       legacyParams.ChainConfig.IstanbulBlock,
+		MuirGlacierBlock:    legacyParams.ChainConfig.MuirGlacierBlock,
+		BerlinBlock:         legacyParams.ChainConfig.BerlinBlock,
+		LondonBlock:         legacyParams.ChainConfig.LondonBlock,
+		ArrowGlacierBlock:   legacyParams.ChainConfig.ArrowGlacierBlock,
+
+		// This is an old field, but renamed from mergeForkBlock
+		MergeNetsplitBlock: legacyParams.ChainConfig.MergeForkBlock,
+
+		// New fields are nil
+		GrayGlacierBlock: nil,
+		ShanghaiBlock:    nil,
+		CancunBlock:      nil,
+	}
+
+	params := types.Params{
+		EvmDenom:            legacyParams.EvmDenom,
+		EnableCreate:        legacyParams.EnableCreate,
+		EnableCall:          legacyParams.EnableCall,
+		ExtraEIPs:           legacyParams.ExtraEIPs,
+		ChainConfig:         newChainConfig,
+		EIP712AllowedMsgs:   legacyParams.EIP712AllowedMsgs,
+		AllowUnprotectedTxs: false, // Upstream v1 to v2
+	}
+
 	return params
 }

x/evm/keeper/params_test.go

@@ -3,7 +3,18 @@ package keeper_test
 import (
 	"reflect"
 
+	sdkmath "cosmossdk.io/math"
+	storetypes "github.com/cosmos/cosmos-sdk/store/types"
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
+	"github.com/evmos/ethermint/app"
+	"github.com/evmos/ethermint/encoding"
+	"github.com/evmos/ethermint/x/evm/keeper"
 	"github.com/evmos/ethermint/x/evm/types"
+	legacytypes "github.com/evmos/ethermint/x/evm/types/legacy"
+	legacytestutil "github.com/evmos/ethermint/x/evm/types/legacy/testutil"
+	"github.com/evmos/ethermint/x/evm/vm/geth"
 )
 
 func (suite *KeeperTestSuite) TestParams() {
@@ -98,3 +109,141 @@ func (suite *KeeperTestSuite) TestParams() {
 		})
 	}
 }
+
+func (suite *KeeperTestSuite) TestLegacyParamsKeyTableRegistration() {
+	encCfg := encoding.MakeConfig(app.ModuleBasics)
+	cdc := encCfg.Codec
+	storeKey := sdk.NewKVStoreKey(types.ModuleName)
+	tKey := sdk.NewTransientStoreKey(types.TransientKey)
+	paramStoreKey := sdk.NewKVStoreKey(paramtypes.ModuleName)
+	paramStoreTKey := sdk.NewTransientStoreKey(paramtypes.TStoreKey)
+	ctx := legacytestutil.NewDBContext([]storetypes.StoreKey{storeKey, paramStoreKey}, []storetypes.StoreKey{tKey, paramStoreTKey})
+	ak := suite.app.AccountKeeper
+
+	// paramspace used only for setting legacy parameters (not given to keeper)
+	setParamSpace := paramtypes.NewSubspace(
+		cdc,
+		encCfg.Amino,
+		paramStoreKey,
+		paramStoreTKey,
+		"evm",
+	).WithKeyTable(legacytypes.ParamKeyTable())
+	params := legacytypes.DefaultParams()
+	params.EIP712AllowedMsgs = legacytestutil.TestEIP712AllowedMsgs
+	setParamSpace.SetParamSet(ctx, &params)
+
+	// param space that has not been created with a key table
+	unregisteredSubspace := paramtypes.NewSubspace(
+		cdc,
+		encCfg.Amino,
+		paramStoreKey,
+		paramStoreTKey,
+		"evm",
+	)
+
+	// assertion required to ensure we are testing correctness
+	// of a keeper receiving a subpsace without a key table registration
+	suite.Require().False(unregisteredSubspace.HasKeyTable())
+
+	newKeeper := func() *keeper.Keeper {
+		// create a keeper, mimicking an app.go which has not registered the key table
+		return keeper.NewKeeper(
+			cdc, storeKey, tKey, authtypes.NewModuleAddress("gov"),
+			ak,
+			nil, nil, nil, nil, // OK to pass nil in for these since we only instantiate and use params
+			geth.NewEVM,
+			"",
+			unregisteredSubspace,
+		)
+	}
+	k := newKeeper()
+
+	// the keeper must set the key table
+	var fetchedParams types.Params
+	suite.Require().NotPanics(func() { fetchedParams = k.GetParams(ctx) })
+	// this modifies the internal data of the subspace, so we should see the key table registered
+	suite.Require().True(unregisteredSubspace.HasKeyTable())
+	// ensure returned params are equal to the set legacy parameters
+	legacytestutil.AssertParamsEqual(suite.T(), params, fetchedParams)
+	// ensure we do not attempt to override any existing key tables to keep compatibility
+	// when passing a subpsace to the keeper that has already been used to work with parameters
+	suite.Require().NotPanics(func() { newKeeper() })
+}
+
+func (suite *KeeperTestSuite) TestRenamedFieldReturnsProperValueForLegacyParams() {
+	encCfg := encoding.MakeConfig(app.ModuleBasics)
+	cdc := encCfg.Codec
+	storeKey := sdk.NewKVStoreKey(types.ModuleName)
+	tKey := sdk.NewTransientStoreKey(types.TransientKey)
+	paramStoreKey := sdk.NewKVStoreKey(paramtypes.ModuleName)
+	paramStoreTKey := sdk.NewTransientStoreKey(paramtypes.TStoreKey)
+	ctx := legacytestutil.NewDBContext([]storetypes.StoreKey{storeKey, paramStoreKey}, []storetypes.StoreKey{tKey, paramStoreTKey})
+	ak := suite.app.AccountKeeper
+
+	// paramspace used only for setting legacy parameters (not given to keeper)
+	legacyParamstore := paramtypes.NewSubspace(
+		cdc,
+		encCfg.Amino,
+		paramStoreKey,
+		paramStoreTKey,
+		"evm",
+	).WithKeyTable(legacytypes.ParamKeyTable())
+
+	oldParams := legacytypes.DefaultParams()
+	// ensure this is set regardless of default param refactoring
+	mergeBlock := sdkmath.NewInt(9999)
+	oldParams.ChainConfig.MergeForkBlock = &mergeBlock
+	// set legacy params with merge block set
+	legacyParamstore.SetParamSet(ctx, &oldParams)
+
+	// new subspace for keeper, mimicking what a new binary would do
+	subspace := paramtypes.NewSubspace(
+		cdc,
+		encCfg.Amino,
+		paramStoreKey,
+		paramStoreTKey,
+		"evm",
+	)
+	k := keeper.NewKeeper(
+		cdc, storeKey, tKey, authtypes.NewModuleAddress("gov"),
+		ak,
+		nil, nil, nil, nil,
+		geth.NewEVM,
+		"",
+		subspace,
+	)
+
+	params := k.GetParams(ctx)
+
+	suite.Require().Equal(params.ChainConfig.MergeNetsplitBlock, oldParams.ChainConfig.MergeForkBlock)
+}
+
+func (suite *KeeperTestSuite) TestNilLegacyParamsDoNotPanic() {
+	encCfg := encoding.MakeConfig(app.ModuleBasics)
+	cdc := encCfg.Codec
+	storeKey := sdk.NewKVStoreKey(types.ModuleName)
+	tKey := sdk.NewTransientStoreKey(types.TransientKey)
+	paramStoreKey := sdk.NewKVStoreKey(paramtypes.ModuleName)
+	paramStoreTKey := sdk.NewTransientStoreKey(paramtypes.TStoreKey)
+	ctx := legacytestutil.NewDBContext([]storetypes.StoreKey{storeKey, paramStoreKey}, []storetypes.StoreKey{tKey, paramStoreTKey})
+	ak := suite.app.AccountKeeper
+
+	subspace := paramtypes.NewSubspace(
+		cdc,
+		encCfg.Amino,
+		paramStoreKey,
+		paramStoreTKey,
+		"evm",
+	)
+
+	k := keeper.NewKeeper(
+		cdc, storeKey, tKey, authtypes.NewModuleAddress("gov"),
+		ak,
+		nil, nil, nil, nil, // OK to pass nil in for these since we only instantiate and use params
+		geth.NewEVM,
+		"",
+		subspace,
+	)
+
+	suite.Require().NotPanics(func() { k.GetParams(ctx) })
+}