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

[Flow EVM] Fix the evm chainID bug for mainnet #5443

Merged
merged 17 commits into from
Mar 5, 2024

Conversation

ramtinms
Copy link
Contributor

@ramtinms ramtinms commented Feb 22, 2024

closes: #5441

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2024

Codecov Report

Attention: Patch coverage is 61.90476% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 55.83%. Comparing base (26a5cc0) to head (7ae2c70).

Files Patch % Lines
fvm/evm/types/chainIDs.go 0.00% 7 Missing ⚠️
fvm/evm/types/emulator.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5443      +/-   ##
==========================================
- Coverage   55.84%   55.83%   -0.01%     
==========================================
  Files        1029     1030       +1     
  Lines      100798   100807       +9     
==========================================
- Hits        56291    56289       -2     
- Misses      40179    40186       +7     
- Partials     4328     4332       +4     
Flag Coverage Δ
unittests 55.83% <61.90%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ramtinms ramtinms marked this pull request as ready for review February 26, 2024 19:55
@ramtinms ramtinms requested a review from sideninja February 26, 2024 19:55
@ramtinms ramtinms changed the title [Flow EVM] fix the evm chainID bug for mainnet [Flow EVM] Fix the evm chainID bug for mainnet Feb 26, 2024
)

var (
FlowEVMTestnetChainID = big.NewInt(646)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: why is Flow in the name, wouldn't this just be EVMTestnetID and EVMMainnetID

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could name it EVMTestnetID , I used the Flow in case someone externally is using it and we use name FlowEVM for the evm chain

// WithMainnetChainID sets the chain ID to flow evm testnet
func WithTestnetChainID() Option {
// WithChainID sets the evm chain ID
func WithChainID(chainID *big.Int) Option {
Copy link
Contributor

Choose a reason for hiding this comment

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

could we make this a type instead of *big.Int

)

var (
FlowEVMTestnetChainID = big.NewInt(646)
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above: could this be

type EVMChainID big.Int

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 start with that change first but we directly use this value for a lot of internal EVM stuff that needs the big.Int value and there was a couple of typecasting that we had to do. I might revisit this later

Copy link
Contributor

@sideninja sideninja left a comment

Choose a reason for hiding this comment

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

minor comments, but looks good!

@ramtinms ramtinms enabled auto-merge March 5, 2024 19:12
@ramtinms ramtinms added this pull request to the merge queue Mar 5, 2024
Merged via the queue into master with commit 53aaae3 Mar 5, 2024
51 checks passed
@ramtinms ramtinms deleted the ramtin/5441-use-evm-mainnet-chainID-for-flow-mainnet branch March 5, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Flow EVM] Use EVM chain ID for mainnet when deployed on mainnet
6 participants