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

Migrate to hardhat-toolbox and Ethers.js v6 #103

Merged
merged 5 commits into from
Oct 23, 2023

Conversation

nlordell
Copy link
Collaborator

@nlordell nlordell commented Oct 19, 2023

Supersedes #100

This PR implements the suggestion from #100 (review) (thanks @mmv08 for the suggestion!), and migrates the 4337 package to use the latest hardhat with @nomicfoundation/hardhat-toolbox in order to simplify dependency management (it also removes a lot of extra dependencies that were pulled in by ethereal-waffle 🎉).

Note that, in order to use the latest version of hardhat and hardhat-toolbox, the codebase needed to migrate to Ethers.js v6 which caused some fairly small changes to the existing TypeScript codebase.

Some additional dependency version changes that may seem unrelated but worth clarifying:

Additionally, for testing I was able to execute the runOp script (tx).

@coveralls
Copy link

coveralls commented Oct 19, 2023

Pull Request Test Coverage Report for Build 6613336725

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.8%) to 69.856%

Totals Coverage Status
Change from base Build 6575637387: -0.8%
Covered Lines: 102
Relevant Lines: 129

💛 - Coveralls

4337/src/utils/execution.ts Outdated Show resolved Hide resolved
@nlordell nlordell marked this pull request as ready for review October 23, 2023 08:33
@nlordell nlordell requested a review from a team as a code owner October 23, 2023 08:33
@nlordell nlordell requested review from rmeissner, Uxio0, akshay-ap and mmv08 and removed request for a team October 23, 2023 08:33
"overrides": {
"@safe-global/safe-contracts": {
"ethers": "^5.6.8"
"ethers": "^6.8.0"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I think the version 1.4.1 still uses ethers v5. I'm wondering if this override is safe. Or does npm not allow installing dependencies because of incompatible packages in case there's no override?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or does npm not allow installing dependencies because of incompatible packages in case there's no override?

The issue I ran into is that both @nomicfoundation/hardhat-toolbox and @safe-global/safe-contracts have an ethers peer dependency with non-overlapping version ranges. AFAICT, NPM does not support this (looked for a bit online but didn't find anything conclusive, I may be wrong though).

Furthermore, I noticed that we use @safe-global/safe-contracts only for contracts and not exported TypeScript/JavaScript code, so even if we override the ethers version, it should not cause any problems.

And finally, the other thing that I found confusing was that the package.json in the safe-contracts repo does not include an ethers dependency or peer dependency, while the one published to NPM does.

Copy link
Member

Choose a reason for hiding this comment

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

And finally, the other thing that I found confusing was that the package.json in the safe-contracts repo does not include an ethers dependency or peer dependency, while the one published to NPM does.

I think it was a result of the upgrade to the hardhat-toolbox as well since ethers is already a dependency of the hardhat-toolbox package. But this upgrade happened after 1.4.1, which has not been released yet.

4337/src/utils/execution.ts Outdated Show resolved Hide resolved
4337/src/utils/execution.ts Outdated Show resolved Hide resolved
4337/src/utils/userOp.ts Outdated Show resolved Hide resolved
4337/test/eip4337/EIP4337ModuleExisting.spec.ts Outdated Show resolved Hide resolved
@nlordell nlordell requested a review from mmv08 October 23, 2023 12:43
Copy link
Member

@mmv08 mmv08 left a comment

Choose a reason for hiding this comment

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

Good job sir

@nlordell
Copy link
Collaborator Author

Thanks for your help 😄

@nlordell nlordell merged commit 0206ae9 into master Oct 23, 2023
4 checks passed
@nlordell nlordell deleted the migrate-to-hardhat-toolbox branch October 23, 2023 13:21
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.

3 participants