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

Make SafeEventEmitterProvider type-compatible with Ethereum request libraries #4095

Closed
mcmire opened this issue Mar 20, 2024 · 1 comment · Fixed by #4422
Closed

Make SafeEventEmitterProvider type-compatible with Ethereum request libraries #4095

mcmire opened this issue Mar 20, 2024 · 1 comment · Fixed by #4422
Assignees

Comments

@mcmire
Copy link
Contributor

mcmire commented Mar 20, 2024

Problem

Currently, when an instance of SafeEventEmitterProvider is passed to the Web3Provider constructor from Ethers v5 or the BrowserProvider constructor from Ethers v6, it produces a type error. Web3Provider produces:

Argument of type 'SafeEventEmitterProvider' is not assignable to parameter of type 'ExternalProvider | JsonRpcFetchFunc'.
  Type 'SafeEventEmitterProvider' is not assignable to type 'ExternalProvider'.
    Types of property 'sendAsync' are incompatible.
      Type '(req: JsonRpcRequest<JsonRpcParams>, callback: (error: unknown, providerRes?: any) => void) => void' is not assignable to type '(request: { method: string; params?: any[]; }, callback: (error: any, response: any) => void) => void'.
        Types of parameters 'req' and 'request' are incompatible.
          Type '{ method: string; params?: any[]; }' is not assignable to type 'JsonRpcRequest<JsonRpcParams>'.
            Type '{ method: string; params?: any[]; }' is missing the following properties from type '{ params?: (Record<string, Json> | Json[]) & ExactOptionalGuard; id: string | number | null; method: string; jsonrpc: "2.0"; }': id, jsonrpctypescript(2345)

and BrowserProvider produces:

Argument of type 'SafeEventEmitterProvider' is not assignable to parameter of type 'Eip1193Provider'.
  Property 'request' is missing in type 'SafeEventEmitterProvider' but required in type 'Eip1193Provider'.typescript(2345)

The first error happens because Web3Provider allows jsonrpc and id to be missing from the request object, whereas JsonRpcRequest does not; the second error happens because BrowserProvider expects the provider to conform to EIP-1193, which specifies that it ought to have a request method.

Proposed Solution

We can address both of these issues by:

  • Aligning SafeEventEmitterProvider to EIP-1193 by adding a request method and deprecating send and sendAsync (which aren't part of the spec)
  • Ensuring that request does not require id and jsonrpc to be set, filling them in if they are missing

Acceptance Criteria

  • We should be able to pass an instance of SafeEventEmitter to the constructor from each of these libraries without type errors:
    • Web3Provider constructor from Ethers v5
    • BrowserProvider constructor from Ethers v6
    • eth-query constructor
    • ethjs-query constructor
  • Using send and sendAsync should produce deprecation warnings
  • eth-json-rpc-provider tests should not use send and sendAsync except where necessary to test the above
@desi
Copy link
Contributor

desi commented Mar 21, 2024

Refer to technical proposal located in notion. If you can't find it ask @desi or @mcmire

@cryptodev-2s cryptodev-2s self-assigned this May 22, 2024
cryptodev-2s added a commit that referenced this issue Jul 9, 2024
## Explanation

After updating the [`SafeEventEmitterProvider` to support
EIP-1193](#4095), I have made the
following changes:

- Deprecated the use of `sendAsync` and replaced it with the `request`
method.
  - Updated instances in the `core` repository:
    - `NetworkController` tests
- Modified areas where we previously expected the provider returned by
NetworkController to have a `sendAsync` method. These now use the
`request` method with the correct method signature:
    - `SelectedNetworkController` tests
    - `createAutoManagedNetworkClient` tests

## References

* Fixes #4100

## Changelog

## Checklist

- [ ] I've updated the test suite for new or updated code as appropriate
- [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [ ] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants