Skip to content
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

Add EVM support #221

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Add EVM support #221

wants to merge 16 commits into from

Conversation

philipsu522
Copy link
Contributor

@philipsu522 philipsu522 commented Aug 23, 2022

Description

Note: Please review this since the last commit introduces a lot of fmt changes
Add EVM support to SEI via ethermint module: https://github.com/evmos/ethermint
Essentially this PR replaces a lot of the scaffolding done by ignite to use ethermint instead of cosmos-sdk (e.g. https://github.com/sei-protocol/sei-chain/pull/221/files#diff-5b0b558d0a19d57e79af24fcfd5b1b099ced2eb2fe4447402069c892813478e9R11). Also created a 1-click init.sh file to run sei locally (with sane configs).

EVM support added are:

  • Add antehandler to discern b/w Ethermint messages vs Cosmos messageS
  • Add encoding/decoding for Ethermint types
  • Add scaffolding changes for Ethermint in app.go
  • Add support for secp256k1 and eth_secp256k1 signatures
  • Add
unsafe-export-eth-key **UNSAFE** Export an Ethereum private key
unsafe-import-eth-key **UNSAFE** Import Ethereum private keys into the local keybase

commands for seid keys

Testing

> seid q evm code 0x9c5AB47d2f67B1546248C46078C294F7232dfaA0
code:
 YIBgQFI0gBVhABBXYACA/VtQYAQ2EGEAQVdgADVg4ByAY08r6R8UYQBGV4Bjbe664xRhAFBXgGOK2gZuFGEAWldbYACA/VthAE5hAHhWWwBbYQBYYQCRVlsAW2EAYmEAqlZbYEBRYQBvkZBhAMJWW2BAUYCRA5DzW2AAgIFUgJKRkGEAipBhARFWW5GQUFVQVltgAICBVICSkZBhAKOQYQDnVluRkFBVUFZbYACAVJBQkFZbYQC8gWEA3VZbglJQUFZbYABgIIIBkFBhANdgAIMBhGEAs1ZbkpFQUFZbYACBkFCRkFBWW2AAYQDygmEA3VZbkVBgAIIUFWEBBldhAQVhAVpWW1tgAYIDkFCRkFBWW2AAYQEcgmEA3VZbkVB///////////////////////////////////////////+CFBVhAU9XYQFOYQFaVltbYAGCAZBQkZBQVlt/Tkh7cQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABgAFJgEWAEUmAkYAD9/qJkaXBmc1giEiALOUW8TkZUUnkxrfxjdn8+Po7nkORpMUYVZYQf4vPHF2Rzb2xjQwAIBwAz
  • Created ETH account and exported to Metamask
  • Executed commands. Verified they went through Screen Shot 2022-08-25 at 1 17 57 PM

@ratankaliani
Copy link

This is pretty cool! Does adding EVM & WASM support in the same app significantly increase strain on existing validator set-ups/add complexity at the app-layer by needing to have support for ERC-20s & cw20s?

@philipsu522
Copy link
Contributor Author

philipsu522 commented Aug 25, 2022

This is pretty cool! Does adding EVM & WASM support in the same app significantly increase strain on existing validator set-ups/add complexity at the app-layer by needing to have support for ERC-20s & cw20s?

Hey! Good question. We're still looking into this, since we ultimately would like to support Solidity < > Cosmwasm compatibility. That will likely be addressed in a separate PR

@philipsu522 philipsu522 changed the title Add ethermint/evm module to app Add EVM support Aug 25, 2022
@philipsu522 philipsu522 requested review from codchen and LCyson August 25, 2022 20:34
@ratankaliani
Copy link

Hey! Good question. We're still looking into this, since we ultimately would like to support Solidity < > Cosmwasm compatibility. That will likely be addressed in a separate PR

Yeah makes sense, integrating cw & erc-20 in tokenfactory feels like a great place to start

When the PR comes out would be great if you could link bc would love to take a look


// Ethermint keepers
EvmKeeper *evmkeeper.Keeper
FeeMarketKeeper feemarketkeeper.Keeper
Copy link
Contributor

Choose a reason for hiding this comment

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

how does the feemarket module interact with the original gas fee system here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for allowing for variable fee for Cosmos txns so that it behaves similarly to EIP-1559: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1559.md
Do you think it makes sense to remove it and keep the original gas fee mechanism? \cc @codchen

Copy link
Contributor

Choose a reason for hiding this comment

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

we can discuss this in a separate ticket^


###############################################################################
### JSON RPC Configuration ###
###############################################################################
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we put this in the root.go instead of the params store (from genesis.json)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean? Do you mind clarifying?

@@ -126,20 +133,27 @@ func initRootCmd(
)

// add server commands
server.AddCommands(
Copy link
Contributor

Choose a reason for hiding this comment

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

do we also need the sdkserver.AddCommands here to keep seid command working

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope, basically ethermintserver.AddCommands is a superset of server.AddCommands, but includes necessary ones for evm. See https://github.com/evmos/ethermint/blob/main/server/util.go#L24
Note that it has sdkserver which is what we have here as server

) (newCtx sdk.Context, err error) {
var anteHandler sdk.AnteHandler

defer Recover(ctx.Logger(), &err)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to remove the panic-recover here? we properly want to avoid recover panic as that may hide more severe bugs (and we may not be able to monitor this each time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a good question. I think it doesn't make sense for a single txn to bring down the whole chain though. Ill think it makes sense to add monitoring though

Copy link
Contributor

Choose a reason for hiding this comment

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

yea the thing i'm worried about is this may hide err and make the chain get into a corrupted state. I'm aligned with having this on testnet + monitor, and remove before mainnet

return nil, err
}

return func(
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to return built EthAnteHandler & CosmosAnteHandler map here? or register ethAnteHandler to the cosmos ante decorator? returning a func here seems to make each tx creates a new AnteHandler here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. Done!

@philipsu522 philipsu522 requested a review from LCyson August 31, 2022 17:28
) (newCtx sdk.Context, err error) {
var anteHandler sdk.AnteHandler

defer Recover(ctx.Logger(), &err)
Copy link
Contributor

Choose a reason for hiding this comment

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

yea the thing i'm worried about is this may hide err and make the chain get into a corrupted state. I'm aligned with having this on testnet + monitor, and remove before mainnet


// Ethermint keepers
EvmKeeper *evmkeeper.Keeper
FeeMarketKeeper feemarketkeeper.Keeper
Copy link
Contributor

Choose a reason for hiding this comment

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

we can discuss this in a separate ticket^

@BrandonWeng BrandonWeng added the enhancement New feature or request label May 4, 2023
@BrandonWeng
Copy link
Contributor

Leave open, we will revist soon

@BrandonWeng BrandonWeng added the on-hold Ticket is on hold label May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request on-hold Ticket is on hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants