-
-
Notifications
You must be signed in to change notification settings - Fork 10
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
base: main
Are you sure you want to change the base?
Changes from 8 commits
1ea71a6
35ec38a
de8d6ef
28a8129
a53994f
e3c3771
47f7023
a8ddce8
bbffb39
7b176aa
6a4ab9b
90c23d6
199f1de
fe10d3c
940bdc9
82ea00b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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), | ||
}); |
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>( | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
* | ||||||
* @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>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we type params with
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 The Is there still a reason to prefer There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should still use We can always use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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>; | ||||||
*/ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does defining |
||||||
}; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we also need to include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ethers itself (which this is intended to represent) only has |
||||||
|
||||||
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>, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, this seems overly strict. The params here can be anything — Where did you derive this type from? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 As for extraneous parameters: On one hand, the more backwards-compatible approach would be something like Are we aware of any places where that could already be a thing? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this type? Our provider technically supports |
||||||
/** | ||||||
* 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
|
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.
do we need a sendAsync scenario?