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

types: export EIP1193Provider and LegacyEthereumProvider interfaces #140

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,16 @@
"superstruct": "^1.0.3"
},
"devDependencies": {
"@ethersproject/providers": "^5.7.2",
"@lavamoat/allow-scripts": "^2.3.1",
"@lavamoat/preinstall-always-fail": "^1.0.0",
"@metamask/auto-changelog": "^3.1.0",
"@metamask/eslint-config": "^12.0.0",
"@metamask/eslint-config-jest": "^12.0.0",
"@metamask/eslint-config-nodejs": "^12.0.0",
"@metamask/eslint-config-typescript": "^12.0.0",
"@metamask/eth-query": "^3.0.1",
"@metamask/safe-event-emitter": "^3.0.0",
"@types/jest": "^28.1.7",
"@types/jest-when": "^3.5.3",
"@types/node": "^17.0.23",
Expand Down
1 change: 1 addition & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ export * from './collections';
export * from './encryption-types';
export * from './errors';
export * from './hex';
export * from './json-rpc-provider-types';
export * from './json';
export * from './keyring';
export * from './logging';
Expand Down
34 changes: 34 additions & 0 deletions src/json-rpc-provider-types.test-d.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need a sendAsync scenario?

Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Web3Provider } from '@ethersproject/providers';
import EthQuery from '@metamask/eth-query';
import { expectAssignable, expectNotAssignable } from 'tsd';

import type { JsonRpcRequest, LegacyEthereumProvider } from '.';

// Known legacy providers
expectAssignable<LegacyEthereumProvider>(new EthQuery({} as any));
MajorLift marked this conversation as resolved.
Show resolved Hide resolved
expectAssignable<LegacyEthereumProvider>(new Web3Provider({} as any));
expectAssignable<LegacyEthereumProvider>({
send: async (method: string, params: string[]) =>
Promise.resolve([method, params]),
});
expectAssignable<LegacyEthereumProvider>({
// eslint-disable-next-line @typescript-eslint/no-empty-function
send: (_req: JsonRpcRequest, _cb: () => void) => {},
});
expectAssignable<LegacyEthereumProvider>({
send: async (req: JsonRpcRequest, _cb: (_x: null, _result: null) => void) =>
Promise.resolve(req),
});

expectNotAssignable<LegacyEthereumProvider>({ foo: '123' });
expectNotAssignable<LegacyEthereumProvider>({ send: '123' });

expectNotAssignable<LegacyEthereumProvider>({
send: (method: string, params: string[]) => [method, params],
});
expectNotAssignable<LegacyEthereumProvider>({
send: async (
req: JsonRpcRequest,
_cb: (_x: null, _result: undefined) => void,
) => Promise.resolve(req),
});
92 changes: 92 additions & 0 deletions src/json-rpc-provider-types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import type SafeEventEmitter from '@metamask/safe-event-emitter';

import type { JsonRpcParams, JsonRpcRequest, Json } from './json';
import type { PartialOrAbsent } from './misc';

/**
* An interface for the EIP-1193 specification for an Ethereum JavaScript Provider.
*
* @see [EIP-1193]{@link https://eips.ethereum.org/EIPS/eip-1193}.
* @see [BaseProvider]{@link https://github.com/MetaMask/providers/blob/main/src/BaseProvider.ts} in package [@metamask/providers]{@link https://www.npmjs.com/package/@metamask/providers}.
*/
export type EIP1193Provider = SafeEventEmitter & {
/**
* Submits an RPC request for the given method, with the given params.
* Resolves with the result of the method call, or rejects on error.
*
* @param args - The RPC request arguments.
* @param args.method - The RPC method name.
* @param args.params - The parameters for the RPC method.
* @returns A Promise that resolves with the result of the RPC method,
* or rejects if an error is encountered.
*/
request<Params extends JsonRpcParams, Result extends Json>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this method signature always compatible with the one in BaseProvider? If not, do we plan on changing BaseProvider to match this?

args: Params,
): Promise<PartialOrAbsent<Result>>;
};

/**
* The interface for a legacy Ethereum provider.
*
* A provider of this type should be acceptable by either `eth-query`, `ethjs-query`, or Ethers' v5 `Web3Provider`.
*/
export type LegacyEthereumProvider =
| LegacyEthersProvider
| LegacyEthJsQueryProvider
| LegacyWeb3Provider;

type LegacyEthersProvider = {
/**
* Send a provider request asynchronously. (ethers v5 Web3Provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

To make this a little cleaner, what do you think about adding a comment above LegacyEthersProvider which explains what it represents? My understanding is that this interface represents the portion of Ethers v5's ExternalProvider interface which intersects with our legacy provider — is that correct?

*
* @param method - The RPC method to call.
* @param params - Array with method parameters.
* @returns A promise resolving with the result of the RPC call, or rejecting on failure.
*/
send(method: string, params: any[]): Promise<Json>;
Copy link
Contributor

@MajorLift MajorLift Oct 4, 2023

Choose a reason for hiding this comment

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

Could we type params with unknown[] or string[]?

Suggested change
send(method: string, params: any[]): Promise<Json>;
send(method: string, params: unknown[]): Promise<Json>;

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we trying to align with Ethers v5 here? If so, its signature for send is here: https://github.com/ethers-io/ethers.js/blob/v5.7.2/packages/providers/src.ts/web3-provider.ts#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but here it should be the Ethers providers themselves (as opposed to the ExternalProvider, which maps to what can be passed into ethers for wrapping)

The any is lifted straight from there (they don't have an interface for it AFAICTbut they're all identical): https://github.com/ethers-io/ethers.js/blob/0bfa7f497dc5793b66df7adfb42c6b846c51d794/packages/providers/src.ts/web3-provider.ts#L166

Is there still a reason to prefer unknown?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes but here it should be the Ethers providers themselves (as opposed to the ExternalProvider, which maps to what can be passed into ethers for wrapping)

Ah. I must be misunderstanding how this type is intended to be used, then. Sorry for being dense, but would you mind providing an example?

Copy link
Contributor

@MajorLift MajorLift Oct 16, 2023

Choose a reason for hiding this comment

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

I think we should still use unknown here. The range of types that can be assigned to params is exactly the same whether we use any[] or unknown[]. Any errors resulting from unknown will also cause any to fail -- just at runtime and silently.

We can always use @ts-expect-error if an error blocks something. At least then we can be aware that there's a dragon to account for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mcmire It's roughly the same difference as between MetaMaskInPageProvider and EthQuery on the MM side - one (the signature of send you're linking) is what the library exposes externally. The other (intended to be represented here) is the externally constructed provider that gets passed in as constructor argument.

So an Ethers provider shouldn't (necessarily) satisfy this interface. But an object passed into Ethers as an ExternalProvider should.

/*
send<Result extends Json = Json>(
method: string,
params: any[],
): Promise<Result>;
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

guessing this commented fn signature doesn't work for some reason?

Comment on lines +47 to +52
Copy link
Contributor

@MajorLift MajorLift Oct 4, 2023

Choose a reason for hiding this comment

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

Does defining send as a generic cause issues? I think this comment should be removed either way.

};
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also need to include sendAsync in this type? Or do we need to define two types for Ethers v5, one that contains send, another that contains sendAsync?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ethers itself (which this is intended to represent) only has send, not sendAsync: https://docs.ethers.org/v5/api/providers/jsonrpc-provider/#JsonRpcProvider-send


type LegacyEthJsQueryProvider = {
/**
* Send a provider request asynchronously. (ethjs-query)
*
* @param req - The request to send.
* @param callback - A function that is called upon the success or failure of the request.
*/
sendAsync<Result extends Json = Json>(
req: Partial<JsonRpcRequest>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this seems overly strict. The params here can be anything — eth-query / ethjs-query doesn't put a restriction on what they can be.

Where did you derive this type from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From here: https://github.com/MetaMask/eth-query/blob/main/index.d.ts

First, we can see here that the missing parts are auto-filled in ethjs-rpc (actual provider for eth-query) so we can at least see that the same interface is implicit.

As for extraneous parameters: On one hand, the more backwards-compatible approach would be something like Json, EverythingButNull or even unknown. But at the same time I'm thinking that users of this type could benefit from spotting unintentionally unsupported API-usage (as any non-standard fields would have to be recognized by the actual provider).

Are we aware of any places where that could already be a thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... I'm wary of doing this, but I don't know why yet. I'll have to think about this.

callback: SendAsyncCallback<Result>,
): void;
};

type LegacyWeb3Provider = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this type? Our provider technically supports send, but it's even more deprecated than sendAsync is. I can't recall a time where we've used it internally.

/**
* Send a provider request asynchronously.
*
* @param req - The request to send.
* @param callback - A function that is called upon the success or failure of the request.
*/
send(req: Partial<JsonRpcRequest>, callback: SendAsyncCallback<Json>): void;
};

type SendAsyncCallback<Result extends Json> = (
...args:
| [error: EverythingButNull, result: undefined]
| [error: null, result: Result]
) => void;

// What it says on the tin. We omit `null`, as that value is used for a
// successful response to indicate a lack of an error.
type EverythingButNull =
| string
| number
| boolean
| object
| symbol
| undefined;
MajorLift marked this conversation as resolved.
Show resolved Hide resolved
legobeat marked this conversation as resolved.
Show resolved Hide resolved
Loading