Skip to content
This repository has been archived by the owner on Apr 4, 2024. It is now read-only.

chore(evm): Deprecate x/params usage in x/evm #1472

Merged
merged 70 commits into from
Jan 4, 2023

Conversation

Vvaradinov
Copy link
Contributor

@Vvaradinov Vvaradinov commented Nov 17, 2022

Description

This PR is the first of a series of changes that need to be made to all modules in the Ethermint and Evmos repos.

The Cosmos SDK x/params module has been deprecated in favor of each module housing and providing way to modify their parameters. Each module that has parameters that are changeable during runtime have an authority, the authority can be a module or user account. The Cosmos-SDK team recommends migrating modules away from using the param module. An example of how this could look like can be found cosmos/cosmos-sdk#12363.

Notes

  • tx.proto - Added new tx proto message MsgUpdateParams

    • Now includes field authority which we set in app.go to the address of the Cosmos SDK governance module. This means only after a sucesfull governance proposal the x/gov module account can make the param change.
  • keeper/params.go - Where parameter getter and set functions live. This has been refactored so now parameters are set by the individual key directly in the store. A getter function for each of the parameters has been created for ease of use.

  • keeper/msg_server - A new UpdateParams function was added that chcekcs if the correct authority is provided and updates the parameters.

  • keeper/keeper.go - The now deprecated paramstore has been removed and an authority has been added to the Keeper struct

  • types/interfaces.go - This defines a new LegacyParams type with a Subspace interface from the now deprecated x/params module solely for the purpose of making an in-store migration.

  • types/codec.go - A new interface needs to be registered in the RegisterInterfaces function.

    • NOTE for Ethermint evm module an AminoCdc needs to be registered in order to support EIP-712
  • keeper/migrations.go - Where all migration functions live. We have added the legacySubspace types.Subspace to the Migrator struct in order to use in the migration process.

  • migrations - The migrations folder contains all store upgrade versions and their respective changes. A migrate.go file defines the changes and in this case where we obtain the current params from the depreacted x/params module and store the new params directly in the module store.

    • This folder may also contain a copy of the types and generated proto files to isolate and keep a copy of the state at that version.

    • NOTE - all previous version migrations were deleted because they were dependent on the x/params module

  • handler.go - A MsgUpdateParams route has been added that routes messages to the UpdateParams function.

  • module.go

    • The module should make sure the ConsensusVersion is set to the latest migration.
    • The Route should register the NewHandler for our messages.
    • The now legacy .Subspace has been added to the AppModule in order to use for the migration.
    • NOTE - The actual migration happens in RegisterServices and it panics if the migration encounters an error.
  • To manually test the new message you need to generate a json proposal. An easy way to generate one is with the CLI command ethermintd tx gov draft-proposal . Following the steps the proposal json file should look something like this:

{
 "messages": [
  {
   "@type": "/ethermint.evm.v1.MsgUpdateParams",
   "authority": "ethm10d07y265gmmuvt4z0w9aw880jnsr700jpva843",
   "params": {
    "evm_denom": "avlad",
    "enable_create": false,
    "enable_call": false,
    "extra_eips": {
     "extra_eips": []
    },
    "chain_config": {
     "homestead_block": null,
     "dao_fork_block": null,
     "dao_fork_support": false,
     "eip150_block": null,
     "eip150_hash": "",
     "eip155_block": null,
     "eip158_block": null,
     "byzantium_block": null,
     "constantinople_block": null,
     "petersburg_block": null,
     "istanbul_block": null,
     "muir_glacier_block": null,
     "berlin_block": null,
     "london_block": null,
     "arrow_glacier_block": null,
     "gray_glacier_block": null,
     "merge_netsplit_block": null,
     "shanghai_block": null,
     "cancun_block": null
    },
    "allow_unprotected_txs": false
   }
  }
 ],
 "metadata": "ipfs://CID",
 "deposit": "20aphoton"
}

Here I am changing the EVMDenom to avlad

Closes: ENG-1037

@Vvaradinov Vvaradinov requested a review from a team as a code owner November 17, 2022 14:56
@Vvaradinov Vvaradinov requested review from 4rgon4ut and adisaran64 and removed request for a team November 17, 2022 14:56
@github-actions github-actions bot added the C:Proto protobuf files (*.pb.go) label Nov 17, 2022
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Merging #1472 (aae45b2) into main (54000bf) will decrease coverage by 0.13%.
The diff coverage is 73.15%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1472      +/-   ##
==========================================
- Coverage   68.65%   68.52%   -0.14%     
==========================================
  Files         105      106       +1     
  Lines        9954    10077     +123     
==========================================
+ Hits         6834     6905      +71     
- Misses       2737     2775      +38     
- Partials      383      397      +14     
Impacted Files Coverage Δ
x/evm/handler.go 57.14% <0.00%> (-17.86%) ⬇️
x/evm/types/msg.go 80.88% <0.00%> (-4.16%) ⬇️
x/evm/genesis.go 53.33% <33.33%> (-3.34%) ⬇️
x/evm/keeper/keeper.go 87.13% <33.33%> (-2.34%) ⬇️
x/evm/keeper/msg_server.go 90.90% <66.66%> (-2.43%) ⬇️
x/evm/migrations/v4/migrate.go 78.57% <78.57%> (ø)
x/evm/types/params.go 87.32% <79.16%> (-12.68%) ⬇️
x/evm/module.go 53.73% <80.00%> (+1.27%) ⬆️
x/evm/keeper/params.go 81.60% <80.76%> (-18.40%) ⬇️
app/app.go 84.87% <100.00%> (+0.02%) ⬆️
... and 7 more

x/evm/genesis.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Show resolved Hide resolved
x/evm/migrations/v4/types/chain_config.go Show resolved Hide resolved
x/evm/types/msg.go Fixed Show resolved Hide resolved
@github-actions github-actions bot added the C:CLI label Nov 18, 2022
Copy link
Contributor

@ramacarlucho ramacarlucho left a comment

Choose a reason for hiding this comment

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

We should register amino encoding for the new messages

x/evm/types/key.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
@ramacarlucho
Copy link
Contributor

Also some tests are failing, can you please check
Tests / test-sim-nondeterminism
Tests / test-sim-random-genesis-fast

@github-actions github-actions bot removed the C:CLI label Nov 19, 2022
Copy link
Contributor

@MalteHerrmann MalteHerrmann left a comment

Choose a reason for hiding this comment

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

Great stuff @Vvaradinov. Left a couple of small comments and have this question:

When executing any CLI command with this branch checked out, I get the following warnings:

ethermintd keys list
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.Params
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.ExtraEIPs
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.ChainConfig
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.State
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.TransactionLogs
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.Log
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.TxResult
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.AccessTuple
2022/12/09 13:18:45 proto: duplicate proto type registered: ethermint.evm.v1.TraceConfig
...

Is this to be expected?

proto/ethermint/evm/v1/tx.proto Outdated Show resolved Hide resolved
proto/ethermint/evm/v1/evm.proto Outdated Show resolved Hide resolved
x/evm/keeper/msg_server_test.go Show resolved Hide resolved
x/evm/migrations/v4/types/chain_config.go Outdated Show resolved Hide resolved
x/evm/handler.go Show resolved Hide resolved
x/evm/types/msg.go Fixed Show resolved Hide resolved
x/evm/types/msg.go Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
rpc/backend/backend_suite_test.go Outdated Show resolved Hide resolved
rpc/backend/backend_suite_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@GAtom22 GAtom22 left a comment

Choose a reason for hiding this comment

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

Great work @Vvaradinov !! LGTM! Left a few nit comments

x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/migrations/v4/migrate.go Outdated Show resolved Hide resolved
x/evm/types/params.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

Great work! Requesting the same comments as the other PRs

x/evm/types/msg.go Outdated Show resolved Hide resolved
x/evm/types/msg.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
x/evm/keeper/keeper.go Outdated Show resolved Hide resolved
rpc/backend/backend_suite_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

ACK! Minor comments

x/evm/migrations/v4/migrate.go Outdated Show resolved Hide resolved
x/evm/migrations/v4/migrate.go Outdated Show resolved Hide resolved
x/evm/keeper/params.go Outdated Show resolved Hide resolved
rpc/backend/call_tx_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@danburck danburck left a comment

Choose a reason for hiding this comment

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

nice one @Vvaradinov utACK

@danburck danburck enabled auto-merge (squash) January 4, 2023 13:07
@danburck danburck merged commit 0f7bdce into main Jan 4, 2023
@danburck danburck deleted the Vvaradinov/refactor-deprecated-evm-params-logic branch January 4, 2023 14:28
GAtom22 added a commit that referenced this pull request Jan 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C:CLI C:Proto protobuf files (*.pb.go)
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants