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

Fix #1451, option 1: Simply upgrade ethers #1452

Merged
merged 5 commits into from
Jul 7, 2023
Merged

Fix #1451, option 1: Simply upgrade ethers #1452

merged 5 commits into from
Jul 7, 2023

Conversation

ChihChengLiang
Copy link
Collaborator

Description

Change ethers-rs related packages from 0.17.0 to 2.0.7

Issue Link

#1451

Type of change

  • New feature (non-breaking change which adds functionality)

Rationale

We need nothing more than simply upgrading the package.

@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-eth-types Issues related to the eth-types workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-mock Issues related to the mock workspace member T-bench Type: benchmark improvements labels Jun 5, 2023
@ChihChengLiang ChihChengLiang mentioned this pull request Jun 5, 2023
Closed
@adria0
Copy link
Member

adria0 commented Jun 28, 2023

Or now alloy? https://www.paradigm.xyz/2023/06/alloy#ethers-rs-is-now-alloy

@ChihChengLiang
Copy link
Collaborator Author

@adria0 I'm also excited about alloy, but I feel it's too soon to introduce alloy. The reasonings are as follows.

  • Alloy was released few days ago, the API might be unstable. Our codebase depends on ethers-rs heavily and when some bug is introduced by the unstable API it might be hard for us to fix.
  • We use providers and signers crates from ethers-rs. In Alloy these APIs seem unimplemented or redesigned. Migrating to Alloy requires some work. I suggest we wait a bit and see if we can introduce Alloy in the near future.
  • There are two main purposes to upgrade ethers-rs.
    • First, we can remove the unmaintained Parity dependencies.
    • Second, we can get the latest update like withdrawal root in the block.

Let me know if you have thoughts on this.

@ChihChengLiang ChihChengLiang marked this pull request as ready for review July 4, 2023 13:40
This was referenced Jul 4, 2023
Closed
Closed
@ChihChengLiang ChihChengLiang linked an issue Jul 4, 2023 that may be closed by this pull request
Upgrade Ethers to 2.0.7 and boundary control #1451
Closed
@ChihChengLiang ChihChengLiang force-pushed the upgrade-ethers branch 2 times, most recently from 9d7bf93 to 5a7db3c Compare July 5, 2023 15:08
KimiWu123
KimiWu123 approved these changes Jul 6, 2023
View reviewed changes
Copy link
Contributor

@KimiWu123 KimiWu123 left a comment

Choose a reason for hiding this comment

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

LGTM. Just have a question.

integration-tests/src/bin/gen_blockchain_data.rs
.first_mut()
.expect("first exists")
.clone()
.evm_version(EvmVersion::London);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you have some comments to explain why we don't need to set evm version before but we need it now? Or we just have to set evm version here bcs it's part of rules of 2.0.7? Just would like to know what kind of situations we might need to upgrade the version in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point that we should add comments here. I added the comment in e118683

I checked ethers.rs and found it defaulted evm version to Shanghai, which is incompatible with my local 0.8.16 version.

I printed out the CI solidity version, which is 0.8.19. The Solidity doc for 0.8.19 says Paris is the latest supported EVM version. So if the ethers-solc selects Shanghai as EVM version, the Solidity compiler would not recognize it.

@hero78119 hero78119 self-requested a review July 7, 2023 02:09
hero78119
hero78119 approved these changes Jul 7, 2023
View reviewed changes
Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

With a comment decoration suggestions.
Other LGTM!

integration-tests/src/bin/gen_blockchain_data.rs Outdated
let compiled = Solc::default()
.compile_source(&path_sol)
let inputs = CompilerInput::new(&path_sol).expect("Compile success");
// ethers-solc defaults evm version to Shanghai
Copy link
Member

Choose a reason for hiding this comment

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

This comment will be easily out of date

Maybe can just have one line comment

- // ethers-solc defaults evm version to Shanghai
- // We explicitly specify EvmVersion here for some old Solidity version to work.
- // We can change to Shanghai later.
+ // ethers-solc: explicitly indicate the EvmVersion that corresponds to the zkevm circuit's supported Upgrade, e.g. `London/Shanghai/...` specifications.

Copy link
Collaborator Author

@ChihChengLiang ChihChengLiang Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Simplifying the comment sounds good. I applied the suggestion in 548c876

@ChihChengLiang ChihChengLiang added this pull request to the merge queue Jul 7, 2023
Merged via the queue into main with commit e9eb6f2 Jul 7, 2023
@ChihChengLiang ChihChengLiang deleted the upgrade-ethers branch July 7, 2023 08:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@KimiWu123 KimiWu123 KimiWu123 approved these changes

@hero78119 hero78119 hero78119 approved these changes

Assignees
No one assigned
Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-eth-types Issues related to the eth-types workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-mock Issues related to the mock workspace member T-bench Type: benchmark improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade Ethers to 2.0.7 and boundary control
4 participants
@ChihChengLiang @adria0 @hero78119 @KimiWu123