-
Notifications
You must be signed in to change notification settings - Fork 94
refactor!: consolidate and clean up chain config #436
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
base: main
Are you sure you want to change the base?
Conversation
1f0d4d4
to
cda76f9
Compare
evmd/cmd/evmd/config/evmd_config.go
Outdated
@@ -92,6 +93,7 @@ type EVMAppConfig struct { | |||
EVM cosmosevmserverconfig.EVMConfig | |||
JSONRPC cosmosevmserverconfig.JSONRPCConfig | |||
TLS cosmosevmserverconfig.TLSConfig | |||
Chain evmtypes.EvmCoinInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels confusing that all of the other configs are defined in cosmosevmserverconfig
but this one comes from another package (evmtypes
). Then below, the DefaultEvmCoinInfo
function actually does come from the cosmosevmserverconfig
package
server/config/toml.go
Outdated
############################################################################### | ||
### Chain Configuration ### | ||
############################################################################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folks generate this toml
when they run appd init
so this would be run at whatever initial version they are using. So if i was on v0.4
when i ran appd init
i would not have this section in my config. What would happen in that scenario?
evmd/cmd/evmd/config/config.go
Outdated
Decimals: evmtypes.EighteenDecimals, | ||
}, | ||
// GetEvmCoinInfo returns appropriate EvmCoinInfo for evmd based on chainID. | ||
func GetEvmCoinInfo(chainID uint64) evmtypes.EvmCoinInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we just export something like this once from the library instead of here and how it is duplicated in testutil
?
@@ -328,6 +328,24 @@ certificate-path = "" | |||
# Key path defines the key.pem file path for the TLS configuration. | |||
key-path = "" | |||
|
|||
############################################################################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this file?
testutil/config/example_migration.md
Outdated
@@ -0,0 +1,119 @@ | |||
# Chain Configuration Migration Guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title of this file is pretty generic. How are we pointing people to this file?
Feels like it needs to be linked in the changelog or in an overall "migration guide"
we have stuff here for previous releases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should definitely put what versions this is relevant for here
testutil/constants/constants.go
Outdated
// Deprecated: Use testutil/config.CreateEvmCoinInfoFromDynamicConfig() instead. | ||
// This function is kept for backward compatibility but should be replaced | ||
// with dynamic configuration generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are deprecating this and keeping it for compatibility, then why not just keep the global map type?
Folks will be broken because we've changed the API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would also make the diff much smaller
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree here, it seems counter intuitive to break something and immediately deprecate it. its also still used extensively throughout the code. should that be updated to the non-deprecated version?
x/vm/types/config.go
Outdated
@@ -11,6 +11,7 @@ import ( | |||
"fmt" | |||
|
|||
"github.com/ethereum/go-ethereum/core/vm" | |||
"github.com/ethereum/go-ethereum/log" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this file need to be behind test
build flags?
i think the PR title should be "refactor!" since it is a breaking change and a restructure of how users will interact with the software |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly questions on stuff. am a little confused about the map -> function changes. it kinda blew up the diff and its unclear to me if it was necessary for this refactor
CHANGELOG.md
Outdated
@@ -14,6 +14,7 @@ | |||
- [\#467](https://github.com/cosmos/evm/pull/467) Replace GlobalEVMMempool by passing to JSONRPC on initiate. | |||
- [\#352](https://github.com/cosmos/evm/pull/352) Remove the creation of a Geth EVM instance, stateDB during the AnteHandler balance check. | |||
- [\#496](https://github.com/cosmos/evm/pull/496) Simplify mempool instantiation by using configs instead of objects. | |||
- [\#436](https://github.com/cosmos/evm/pull/436) Consolidate and clean up chain config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels a bit too ambiguous for the change being made. if i was an evm user, i wouldn't really know what needs to be actionized from this.
evmd/cmd/evmd/config/config.go
Outdated
default: | ||
// Default fallback - return the default configuration | ||
return *config.DefaultEvmCoinInfo() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it could be a potential footgun. what types of issues could cascade from this?
server/config/toml.go
Outdated
############################################################################### | ||
### Chain Configuration ### | ||
############################################################################### |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we can automate the updating of their app.toml in an upgrade handler, we should definitely do that. im still going through the PR as i write this comment so i'm not sure what is possible yet.
testutil/config/example_migration.md
Outdated
@@ -0,0 +1,119 @@ | |||
# Chain Configuration Migration Guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should definitely put what versions this is relevant for here
testutil/config/example_migration.md
Outdated
} | ||
``` | ||
|
||
## Dynamic Configuration for Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems a bit confusing to me. is there a reason we have all this dynamic stuff -- it seems like you could just skirt all this and directly construct an EvmCoinInfo type, no?
testutil/constants/constants.go
Outdated
// Deprecated: Use testutil/config.CreateEvmCoinInfoFromDynamicConfig() instead. | ||
// This function is kept for backward compatibility but should be replaced | ||
// with dynamic configuration generation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree here, it seems counter intuitive to break something and immediately deprecate it. its also still used extensively throughout the code. should that be updated to the non-deprecated version?
@@ -118,14 +118,7 @@ type ConfigOption func(*Config) | |||
// WithChainID sets a custom chainID for the network. Changing the chainID | |||
// change automatically also the EVM coin used. It panics if the chainID is invalid. | |||
func WithChainID(chainID testconstants.ChainID) ConfigOption { | |||
evmCoinInfo, found := testconstants.ExampleChainCoinInfo[chainID] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
regarding the change of maps to functions for the coin info, the map version did have the benefit of giving the user a presence check for free. with the function as is, it doesnt seem like there is a way for the user to check that the thing they were looking for is actually there. but, im not too sure how important that is.
|
||
// If the base denom has different decimals, we need to create it properly | ||
denom := evmtypes.CreateDenomStr(defaultDecimals, defaultDisplayDenom) | ||
if defaultDecimals == evmtypes.EighteenDecimals { |
Check warning
Code scanning / CodeQL
Comparison of identical values Warning
expression
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 days ago
To fix the issue, we need to remove the unnecessary comparison between two identical values. Since defaultDecimals
is set unconditionally to evmtypes.Decimals(18)
, which is in all likelihood equal to evmtypes.EighteenDecimals
, the conditional on line 160 will always execute. The ramifications are that the nominal else block (base and extended denom creation) is never used, and the denom
/extendedDenom
are both simply set to the default string. The fix should be either to use the default values unconditionally, or (if conditional behaviour is needed) to ensure the comparison actually checks for something meaningful. If the intent was to allow variable decimal configurations, defaultDecimals
should be set from configuration rather than hardcoded. For now, the best minimal fix is to remove the condition and set denom
and extendedDenom
to the default values unconditionally, and remove the dead code.
-
Copy modified lines R155-R157
@@ -152,17 +152,10 @@ | ||
defaultDisplayDenom := "test" | ||
defaultDecimals := evmtypes.Decimals(18) | ||
|
||
// Create extended denom (atto version for 18-decimal representation) | ||
extendedDenom := evmtypes.CreateDenomStr(defaultDecimals, defaultDisplayDenom) | ||
// For 18 decimals, base and extended are the same | ||
denom := defaultDenom | ||
extendedDenom := defaultDenom | ||
|
||
// If the base denom has different decimals, we need to create it properly | ||
denom := evmtypes.CreateDenomStr(defaultDecimals, defaultDisplayDenom) | ||
if defaultDecimals == evmtypes.EighteenDecimals { | ||
// For 18 decimals, base and extended are the same | ||
denom = defaultDenom | ||
extendedDenom = defaultDenom | ||
} | ||
|
||
coinInfo = evmtypes.EvmCoinInfo{ | ||
Denom: denom, | ||
ExtendedDenom: extendedDenom, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments.
Especially, configuring issue with integration test suite looks important.
|
||
// EvmAppOptionsFromConfig creates an EVM options function that uses the provided chain configuration. | ||
func EvmAppOptionsFromConfig(chainConfig ChainConfig) evmconfig.EVMOptionsFn { | ||
return func(chainID uint64) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument chainID
is not used.
I think we should modify evmconfig.EVMOptionsFn as func() error
type and remove this argument.
Currently, it is still confusing for devs in app.goL271-L274
Because now, we are using EVMChainID from config, but looking at app.go, devs can confused that the other chainID input is used.
evmd/cmd/evmd/cmd/root.go
Outdated
} | ||
|
||
// createBasicApp creates a basic app instance for extracting encoding configuration | ||
func createBasicApp(nodeHome string) *evmd.EVMD { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument nodeHome is not used.
How about removing it?
I think HomeDir is set in L82.
evmd/cmd/evmd/cmd/root.go
Outdated
|
||
return cfg | ||
// loadChainConfigFromContext loads chain configuration from the command context and flags. | ||
func loadChainConfigFromContext(cmd *cobra.Command, clientCtx client.Context) (evmdconfig.ChainConfig, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument clientCtx is not used.
How about removing it?
evmd/cmd/evmd/cmd/root.go
Outdated
true, | ||
simtestutil.EmptyAppOptions{}, | ||
1, // Temporary chain ID, only for codec extraction | ||
noOpEvmAppOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have suggestions.
Option 1. Lets make evmAppOptions as one of baseAppOptions.
Because evmAppOptions is used for global variable configuration and it returns nothing, I think we can add this function as one of baseAppOptions and let is executed in BaseApp instantiation.
evmAppOptions := func(_ *baseapp.BaseApp) {
if err := evmdconfig.EvmAppOptions(chainConfig); err != nil {
panic(err)
}
}
And we can remove evmAppOptions config.EVMOptionsFn
argument from NewExampleApp method.
Then we don't have to make weird noOpAppOptions for cmd client instantiation.
In noremal case, we can input evmAppOptions
as one of baseAppOptions like below.
evmAppOptions := func(_ *baseapp.BaseApp) {
if err := evmdconfig.EvmAppOptions(chainConfig); err != nil {
panic(err)
}
}
baseappOptions := []func(*baseapp.BaseApp){
// ...
evmAppOptions
}
app := evmd.NewExampleApp(
//...
baseappOptions
)
Option 2. Let's just add condition in app.go
// initialize the Cosmos EVM application configuration
if evmAppOptions != nil && err := evmAppOptions(evmChainID); err != nil {
panic(err)
}
In this case, we can just set nil for evmAppOptions argument.
@@ -88,14 +88,20 @@ func (a appCreator) newApp( | |||
baseapp.SetIAVLCacheSize(cast.ToInt(appOpts.Get(server.FlagIAVLCacheSize))), | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is comment about newApp
and exportApp
functions.
These functions are not used.
Instead, newApp
and exportApp
in evmd/cmd/evmd/root.go are used.
The two set of functions are functionally identical.
I think we can remove this file entirely.
chainConfig := testconfig.DefaultChainConfig | ||
configurator := evmtypes.NewEVMConfigurator() | ||
configurator.ResetTestConfig() | ||
return configurator.WithEVMCoinInfo(chainConfig.CoinInfo).Configure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@almk-dev (cc. @vladjdk)
ChainConfig including chainID should be set, but currently, it is not set.
I think this is the reason why invalid chainID 0
panic occurs in all integration tests.
On the other hand, start cmd works well.
I guess It is because, chainConfig is configured well when evmd starts from cmd client.
You can refer to how cmd client configure chainConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left additional comments.
Overall, because the config values are declared and configured as global variables, I think code duplication occurs in various places and maintenance becomes difficult. This refactoring PR has already grown in size, so making additional large-scale changes here doesn’t seem appropriate. However, at some point, it would be better to separate the config and pass it as an argument individually to the app, gRPC server, and JSON-RPC server, etc...
@@ -463,12 +463,15 @@ func setDefaultMintGenesisState(cosmosEVMApp evm.EvmApp, genesisState cosmosevmt | |||
} | |||
|
|||
func setDefaultErc20GenesisState(cosmosEVMApp evm.EvmApp, evmChainID uint64, genesisState cosmosevmtypes.GenesisState) cosmosevmtypes.GenesisState { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Argument evmChainID is not used.
How about removing it?
x/vm/types/denom.go
Outdated
ExtendedDenom string | ||
DisplayDenom string | ||
Decimals Decimals | ||
func (d Decimals) GetDenomPrefix() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should decide between 2 options and modify other parts for consistency.
- Only support 1, 2, 3, 6, 9, 12, 15, 18 decimals
- Support <= 18 decimals
For example, Validate() method regards decimals less than or equal to 18 as valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the Validate method to make use of this. The <18 is too simplistic and has gaps in usability as many of those decimals are non standard and don't even have SI prefixes.
My opinion is that we should strictly stick to 1, 2, 3, 6, 9, 12, 15, 18 decimals.
@@ -27,13 +27,17 @@ import ( | |||
func (s *EvmUnitAnteTestSuite) TestVerifyAccountBalance() { | |||
// Setup | |||
keyring := testkeyring.New(2) | |||
chainConfig := testconfig.ChainConfig{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These setup Code are repeated in many test files.
I suggest adding (s *EvmUnitAnteTestSuite) Setup()
method.
server/config/toml.go
Outdated
|
||
[coin] | ||
|
||
# Denom defines the base denomination used in the chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd better adding detailed description.
# Denom defines the base denomination used in the chain | |
# Denom is the base denomination of the native coin used in the chain’s EVM for paying gas fees. If the native coin has 18 decimals, then Denom must be the same denomination as ExtendedDenom. |
chainConfig := testconfig.DefaultChainConfig | ||
configurator := evmtypes.NewEVMConfigurator() | ||
configurator.ResetTestConfig() | ||
return configurator.WithEVMCoinInfo(chainConfig.CoinInfo).Configure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#436 (comment))
chainConfig := testconfig.DefaultChainConfig | ||
configurator := evmtypes.NewEVMConfigurator() | ||
configurator.ResetTestConfig() | ||
return configurator.WithEVMCoinInfo(chainConfig.CoinInfo).Configure() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#436 (comment))
@@ -144,8 +146,16 @@ func toMsgSlice(msgs []*evmsdktypes.MsgEthereumTx) []sdk.Msg { | |||
} | |||
|
|||
func TestMonoDecorator(t *testing.T) { | |||
chainID := uint64(config.EighteenDecimalsChainID) | |||
require.NoError(t, config.EvmAppOptions(chainID)) | |||
chainConfig := testconfig.DefaultChainConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion] I think we could commonize default setup code (L149-L156) in testutil/somewhere dir.
@@ -57,9 +56,15 @@ func TestSDKTxFeeChecker(t *testing.T) { | |||
// with extension option | |||
// without extension option | |||
// london hardfork enableness | |||
chainID := uint64(config.EighteenDecimalsChainID) | |||
chainConfig := testconfig.DefaultChainConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto (#436 (comment))
if denom != types.GetEVMCoinDenom() { | ||
panic(fmt.Sprintf("expected evm denom %s, received %s", types.GetEVMCoinDenom(), denom)) | ||
if denom != w.evmCfg.CoinInfo.GetDenom() { | ||
panic(fmt.Sprintf("expected evm denom %s, received %s", w.evmCfg.CoinInfo.GetDenom(), denom)) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
type ChainConfig struct { | ||
ChainInfo ChainInfo | ||
CoinInfo evmtypes.EvmCoinInfo | ||
} | ||
|
||
type ChainInfo struct { | ||
ChainID string | ||
EVMChainID uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing as we also have similarly named and purposed types in the vm module types.
// EvmConfig contains the specific EVM configuration details
type EvmConfig struct {
ChainConfig *ChainConfig
CoinInfo *EvmCoinInfo
ExtendedEIPs map[int]func(*vm.JumpTable)
}
type ChainConfig struct { | ||
ChainID string | ||
EvmConfig *evmtypes.EvmConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to redefine this here--we have similar types in evmd and in vm types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is directionally correct, but it could be improved if it was made a bit more generic to fit whatever params we want to pull from app options/flags.
I think dydx's setup is pretty clean. Maybe if we organized it similarly, it would be easier to track how parameters were initialized and passed into the app--and eliminate some of our duplicated fields tracking chain IDs, denom, decimals, etc. in multiple places.
type EvmCoinInfo struct { | ||
// DisplayDenom defines the display denomination shown to users | ||
DisplayDenom string `mapstructure:"display-denom"` | ||
// Decimals defines the precision/decimals for the base denomination (1-18) | ||
Decimals Decimals `mapstructure:"decimals"` | ||
// ExtendedDecimals defines the precision/decimals for the extended denomination (typically 18 decimals for atto-denom) | ||
ExtendedDecimals Decimals `mapstructure:"extended-decimals"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do consider these as things that should just be in module state--and potentially already are. Maybe you can pull things like Decimals or the Bond Denom from the bank/staking modules? Or maybe we should be storing these in precisebank and having an explicit dependency on that in the vm module?
But it probably it makes sense to have these in module Params somewhere, and we can keep things like the eip-155 chain id and hardfork rules in user-supplied params.
Description
STACK-1409
EVM-176
Please review that all chain config redundancy has been removed and that all tests and functionality is working exactly the same as before.
Author Checklist
I have...
main
branch