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

clients/js: wrap ethers6 #140

Merged
merged 1 commit into from Jun 29, 2023
Merged

clients/js: wrap ethers6 #140

merged 1 commit into from Jun 29, 2023

Conversation

nhynes
Copy link
Contributor

@nhynes nhynes commented Jun 28, 2023

This PR adds ethers6 support to the compat lib.

The compat lib is getting unmaintainable*, but it should be mostly fine for another few years until the ecosystem changes again. What I would recommend is to split this compat lib into a stub that auto-detects the kind of provider it's getting, and then dynamically import the specific wrapper for it.

* The signed calls module over-exports, which makes it harder to retain backwards compatibility. The provider detection code needs to be centralized. The overall idea of hooks and wrap is working well, but tree-shakeability is becoming an issue, as ethers5 and ethers6 are both large codebases.

@nhynes nhynes requested review from kostko and CedarMist June 28, 2023 09:17
@nhynes nhynes force-pushed the nhynes/ethers-v6 branch 3 times, most recently from 598549f to fcdb8ef Compare June 28, 2023 09:25
@CedarMist
Copy link
Member

CedarMist commented Jun 28, 2023

@nhynes re: maintainability, what is your opinion on https://github.com/MetaMask/eth-json-rpc-middleware ?

More recent example of the rpc middleware: https://github.com/ArkaneNetwork/web3-arkane-provider/blob/develop/src/index.ts where they have their own custom provider.

Will give this a more thorough review later, am concerned about compatibility with latest versions of Truffle or Hardhat.

@nhynes
Copy link
Contributor Author

nhynes commented Jun 28, 2023

what is your opinion on https://github.com/MetaMask/eth-json-rpc-middleware

I think it's the right kind of thing, but after a cursory inspection, I do not see any obvious place where it integrates with any existing provider like hardhat or truffle. Is your expectation that this will itself interoperate with those providers?

@CedarMist
Copy link
Member

Is your expectation that this will itself interoperate with those providers?

Yup, seems like it handles a variety of different wrapping types, e.g. sapphire.wrap could use eth-json-rpc-middleware behind the scenes to use an existing provider as an upstream, so would work with the existing hardhat/truffle integration pattern.

@nhynes
Copy link
Contributor Author

nhynes commented Jun 28, 2023

@CedarMist can you either link to or write a quick function that demonstrates wrap(s: ethers.Signer): ethers.Signer using eth-json-rpc-middleware? You can stub out the actual hook implementations. I'm probably just missing the repo where they have the adapters.

@CedarMist
Copy link
Member

can you either link to or write a quick function that demonstrates wrap(s: ethers.Signer): ethers.Signer using eth-json-rpc-middleware

Have made a ticket #141 - is on my mental todo list as I only found the project yesterday and still need to have a more in-depth look at it.

@nhynes
Copy link
Contributor Author

nhynes commented Jun 28, 2023

I heard there were issues with truffle? Maybe someone could try upgrading the versions of that by following this PR as a template. This code isn't actually bad, but it could use a bit of reorganization to include a centralized detection function and a zero-bundle-size ethers5->ethers6 upgrade function.

@CedarMist
Copy link
Member

CedarMist commented Jun 28, 2023

I heard there were issues with truffle?

I've made a temporary non-optimal hacky fix in #142 which tests against several versions of Truffle (and the pinned versions of Hardhat).

Ideally should probably expand the version matrix to include several versions of Hardhat too, including whatever :latest is.

The problem with wrapping providers it's always going to have to deal with the underlying implementation, and there are still fundamental differences in how Truffle (e.g. HDWalletProvider) vs Ethers (either 5 or 6), metamask and whatever the window.ethereum provider is which conflict with one another when it comes to .send and .sendAsync which seems to be the root of the problem.

A good test would be trying to move everything to .request which is defined by EIP-1193, and seeing what breaks? Then... deprecating usage with whatever doesn't adhere to EIP-1193 and doing a version bump.

Copy link
Member

@CedarMist CedarMist left a comment

Choose a reason for hiding this comment

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

Looks good, will need to think of some more tests to run. But that's not blocking this.

@nhynes nhynes merged commit e877623 into main Jun 29, 2023
13 checks passed
@nhynes nhynes deleted the nhynes/ethers-v6 branch June 29, 2023 16:35
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.

None yet

3 participants