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

fix: avoid import conflicts between v4-sdk and ethers-v6 #187

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

Conversation

Amxx
Copy link

@Amxx Amxx commented Oct 29, 2024

Description

v4-sdk uses ethers v5, which is slowly going away. Many project that import v4-sdk may also have ethers-v6 in their dependency. This creates conflicts, because v4-sdk imports ethers directly (instead of importing @ethersproject/xxx).

How Has This Been Tested?

This is only a change to the imports. There should be no feature changes. Was tested with yarn test

Are there any breaking changes?

There shouldn't be any.

Feedback Focus

The dependencies of the v4-sdk package do not include ethers. It only contains @ethersproject/solidity. However, v4-sdk does use features from all of the following

@ethersproject/abi
@ethersproject/address
@ethersproject/bignumber
@ethersproject/constants
@ethersproject/hash
@ethersproject/solidity
@ethersproject/units
@ethersproject/abstract-signer

Maybe they should be added to the dependencies in a more explicit way

Follow Ups

Migration to ethers v6: #141

@Amxx Amxx requested review from jsy1218 and a team as code owners October 29, 2024 08:59
@jsy1218
Copy link
Member

jsy1218 commented Oct 29, 2024

I will give a test to smart order router and routing-api on my local to make sure this PR is good

@jsy1218
Copy link
Member

jsy1218 commented Oct 29, 2024

I'm hitting weird issue on my local https://app.warp.dev/block/QhX8rzl8rm2lPoL2m7ysSQ. Although peeking this PR, I don't think it's causing the weird issue.

@Amxx
Copy link
Author

Amxx commented Oct 29, 2024

Vscode did some linting changes automatically. I just reverted them.

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