-
Notifications
You must be signed in to change notification settings - Fork 73
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
Conversation
009d741
to
f1c7140
Compare
Pull Request Test Coverage Report for Build 6613336725
💛 - Coveralls |
f1c7140
to
dca88e1
Compare
dca88e1
to
1051fe2
Compare
"overrides": { | ||
"@safe-global/safe-contracts": { | ||
"ethers": "^5.6.8" | ||
"ethers": "^6.8.0" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job sir
Thanks for your help 😄 |
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 byethereal-waffle
🎉).Note that, in order to use the latest version of
hardhat
andhardhat-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:
solc
: A follow up to Feature: Adjust compiler settings and document optimizer usage #99, where the directsolc
version didn't change to reflect the hardhat compiler settings@types/node
: Updated to the LTS version of node, which we intend to use.Additionally, for testing I was able to execute the
runOp
script (tx).