fix: decode pre-v9 state via height-dispatched legacy adapter#12
fix: decode pre-v9 state via height-dispatched legacy adapter#12aluque-peersyst wants to merge 1 commit into
Conversation
JordiParraCrespo
left a comment
JordiParraCrespo
left a comment
There was a problem hiding this comment.
For me the concept ACK.
A few things I would consider:
- Move the adapter from
/rpc/backend/ethinto the legacy module. - Isolate the tests in
<file_name>_legacy_test.go. This should help reduce future test conflicts and make the legacy code easier to maintain. - As mentioned in some of the comments, I’d avoid naming things around
v9where possible and use names that are semantically tied to Evmos or Ethermint instead. This would make it easier for other chains to reuse the ideas from this module. - For methods that cannot realistically be supported, such as tracing, I’d make sure we return an explicit “not supported” error rather than leaving the behavior ambiguous.
| // The adapter reads the v9 upgrade record from the state mounted at | ||
| // `height`; at pre-v9 heights the record does not exist yet, so exporting | ||
| // legacy-layout state is unsupported. | ||
| return app.legacyAdapter.Load(app.NewUncachedContext(true, cmtproto.Header{})) |
There was a problem hiding this comment.
Why do we need to do it here? The legacyAdapter is already loaded in the app.go no?
| cdc codec.BinaryCodec | ||
| uk UpgradeKeeper | ||
| ak AccountKeeper | ||
| v9Height int64 |
There was a problem hiding this comment.
I would name this as: cosmosEvmHeight
| ) | ||
|
|
||
| // V9 is the upgrade that migrated Ethermint state layouts to cosmos/evm. | ||
| const V9 = "v9.0.0" |
There was a problem hiding this comment.
I would use a name like: COSMOS_EVM_UPGRADE_VERSION
| @@ -0,0 +1,79 @@ | |||
| // Package legacy reads EVM state written in layouts that predate the v9 | |||
There was a problem hiding this comment.
Here instead of always like assuming that the version is V9 we can have a more generic approach in order that other chains can plug and play this utils
| // loadedAdapter builds an adapter and reads its upgrade height once | ||
| func loadedAdapter(t *testing.T, v9Height int64) *legacy.LegacyAdapter { |
There was a problem hiding this comment.
I would name this function as newLoadedTestLegacyAdapter
|
|
||
| // UnmarshalLegacyParams converts pre-v9 (Ethermint) EVM params bytes to the | ||
| // current schema. | ||
| func (l *LegacyAdapter) UnmarshalLegacyParams(bz []byte) (evmtypes.Params, error) { |
There was a problem hiding this comment.
Here I would unify the naming with the GetCodeHash that we do not use the GetLegacyCodeHash.
When using it we use legacyAdapter.UnmarshalParams
| // to the current schema. The encodings are wire-compatible (current-schema | ||
| // decoding silently rescales base_fee by 10^-18) so the height dispatch is | ||
| // the only safeguard. | ||
| func (l *LegacyAdapter) UnmarshalFeeMarketParams(bz []byte) (fmtypes.Params, error) { |
There was a problem hiding this comment.
Here we should unify with the naming of the previous function
| return evmtypes.Params{}, fmt.Errorf("unmarshal legacy EVM params: %w", err) | ||
| } | ||
|
|
||
| // "ethereum_3855" → 3855, mirroring the v9 upgrade migration. |
There was a problem hiding this comment.
I would not include → in the comments
2 participants
No description provided.