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

feat: make SafeEventEmitterProvider compatible with eth req libraries #4422

Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
fe4e75b
feat: make SafeEventEmitterProvider compatible with eth req libraries
cryptodev-2s Jun 13, 2024
fd56e13
feat: add tsd for type testing
cryptodev-2s Jun 13, 2024
d754397
fix: unit tests
cryptodev-2s Jun 13, 2024
72a25cd
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s Jun 13, 2024
e734a5d
fix: build
cryptodev-2s Jun 14, 2024
bd6070f
fix: test coverage
cryptodev-2s Jun 18, 2024
3bbf4db
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s Jun 18, 2024
2bd3b24
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s Jun 20, 2024
5338acc
fix: override request in fake provider
cryptodev-2s Jun 20, 2024
dc2f57b
fix: helper function unit tests
cryptodev-2s Jun 20, 2024
c99a02c
fix: remove tsd
cryptodev-2s Jun 20, 2024
2285231
fix: remove useless jest config update
cryptodev-2s Jun 20, 2024
7783d95
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s Jun 25, 2024
403a862
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s Jun 25, 2024
9f40d0e
Merge remote-tracking branch 'origin/main' into feature/make-SafeEven…
cryptodev-2s Jul 2, 2024
58443b4
fix: add smoke tests
cryptodev-2s Jul 2, 2024
c96d681
fix: move @metamask/utils to devDependencies
cryptodev-2s Jul 2, 2024
e87a334
fix: move back metamsk utils as dependencies
cryptodev-2s Jul 3, 2024
662b20e
fix: dependencies order
cryptodev-2s Jul 3, 2024
cf969dc
fix: request returned type and result
cryptodev-2s Jul 5, 2024
efda64b
Merge branch 'main' into feature/make-SafeEventEmitterProvider-type-c…
cryptodev-2s Jul 5, 2024
5a7887f
fix: remove useless export of Eip1193Request
cryptodev-2s Jul 5, 2024
640e821
fix: fake-provider
cryptodev-2s Jul 5, 2024
5ac0498
fix: test handle thrown error
cryptodev-2s Jul 5, 2024
17118e6
fix: remove optional chaining operator
cryptodev-2s Jul 5, 2024
8a9dd51
fix: thrown error
cryptodev-2s Jul 5, 2024
b3c426a
fix: add more tests for request method
cryptodev-2s Jul 5, 2024
88162ec
fix: add more tests for sendAsync and send
cryptodev-2s Jul 5, 2024
43608ec
fix: add more request failure tests
cryptodev-2s Jul 5, 2024
38dbabd
fix: request error unit tests
cryptodev-2s Jul 6, 2024
e19d25a
fix: drop error line
cryptodev-2s Jul 6, 2024
e9c69bb
fix: add :
cryptodev-2s Jul 6, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ export class AssetsContractController extends BaseControllerV1<
throw new Error(MISSING_PROVIDER_ERROR);
}

// @ts-expect-error TODO: remove this annotation once the `Eip1193Provider` class is released
return new Web3Provider(provider);
}

Expand Down
8 changes: 7 additions & 1 deletion packages/eth-json-rpc-provider/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,18 @@
"dependencies": {
"@metamask/json-rpc-engine": "^9.0.0",
"@metamask/safe-event-emitter": "^3.0.0",
"@metamask/utils": "^8.3.0"
"@metamask/utils": "^8.3.0",
Copy link
Contributor

@legobeat legobeat Jul 2, 2024

Choose a reason for hiding this comment

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

It looks like this is only used for dependencies?

Could the types be re-exported in this package, thus delegating @metamask/utils to a devDependency?

Copy link
Contributor

@legobeat legobeat Jul 2, 2024

Choose a reason for hiding this comment

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

To complete it, the types used in exports also have to be exported from packages/eth-json-rpc-provider/src/index.ts

Copy link
Contributor

@MajorLift MajorLift Jul 3, 2024

Choose a reason for hiding this comment

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

@legobeat I'm not sure I understand the reasoning for this change, based on the criteria for categorizing dep vs. devDep discussed here: #4449 (comment).

The types imported from @metamask/utils always need to be present for the user, since they are dependencies for the exported types and functions Eip1193Request, convertEip1193RequestToJsonRpcRequest, SafeEventEmitterProvider['request'], SafeEventEmitterProvider['sendAsync'], SafeEventEmitterProvider['send']. These exports will always directly reference the imported types, and "any project using this package would need to have the package the types come from as well, or it would be an invalid reference."

The type imports from @metamask/utils are also present in the built type declaration files for this package, which wouldn't happen if they were only being used internally at build time.

Re-exporting the imports would give the user access to them, but wouldn't it also require the user to manually import the re-exported types in order to use the exports from this package -- even if the re-exported types are not used anywhere else? It seems like that would effectively make @metamask/utils a package dependency, just with extra steps.

Copy link
Contributor

@mcmire mcmire Jul 3, 2024

Choose a reason for hiding this comment

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

The type imports from @metamask/utils are also present in the built type declaration files for this package, which wouldn't happen if they were only being used internally at build time.

I see this as the key criterion for deciding that @metamask/utils should be in dependencies and not devDependencies, so that TypeScript users are able to use the type declarations that ship with this package.

I see that we are exporting Eip1193Request in this PR. If we were not doing this, then it would be less obvious that the type declaration files referenced types imported from @metamask/utils, but I believe that even without this, providerFromMiddleware takes types that reference Json and JsonRpcParams. So I believe that @metamask/utils still needs to be included in dependencies.

Thoughts?

Copy link
Contributor

@legobeat legobeat Jul 3, 2024

Choose a reason for hiding this comment

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

My suggestion here is that the used types (Json, JsonRpcParams) would get re-export-ed from this package.

This will be equivalent from the TypeScript user perspective (the exact same types are available for them) but without requiring the downstream user to pull in the not-actually used js files. So effectively the built .d.ts files in @metamask/eth-json-rpc-provider would include the necessary definitions which would otherwise get pulled in from @metamask/utils.

TypeScript will transparently see that the types are identical and not distinguish between them when imported from different sources downstream.

This also has a side-benefit that any future otherwise non-breaking module refactors (renaming or moving initial type definitions between different packages, for example) becomes transparent for users of this package.

Copy link
Contributor

Choose a reason for hiding this comment

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

This will be equivalent from the TypeScript user perspective (the exact same types are available for them) but without requiring the downstream user to pull in the not-actually used js files. So effectively the built .d.ts files in @metamask/eth-json-rpc-provider would include the necessary definitions which would otherwise get pulled in from @metamask/utils.

Hmm. Is the thought here that if we re-export types from @metamask/utils, the TypeScript compiler will recognize this and embed those types into the type declarations for @metamask/eth-json-rpc-provider? If so, I'm not sure it works that way. I believe the type declaration files would still include import lines from @metamask/utils, even if those types are being re-exported. Therefore, the consumer would still need @metamask/utils somewhere in the dependency tree for TypeScript to "see" those types.

"uuid": "^8.3.2"
},
"devDependencies": {
"@ethersproject/providers": "^5.7.0",
"@metamask/auto-changelog": "^3.4.4",
"@metamask/eth-query": "^4.0.0",
"@metamask/ethjs-query": "^0.5.3",
"@metamask/rpc-errors": "^6.2.1",
"@types/jest": "^27.4.1",
"deepmerge": "^4.2.2",
"ethers": "^6.12.0",
"jest": "^27.5.1",
"jest-it-up": "^2.0.2",
"ts-jest": "^27.1.4",
Expand Down
23 changes: 13 additions & 10 deletions packages/eth-json-rpc-provider/src/provider-from-engine.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { JsonRpcEngine } from '@metamask/json-rpc-engine';
import { promisify } from 'util';
import { providerErrors } from '@metamask/rpc-errors';

import { providerFromEngine } from './provider-from-engine';

Expand All @@ -11,33 +11,36 @@ describe('providerFromEngine', () => {
end();
});
const provider = providerFromEngine(engine);
const promisifiedSendAsync = promisify(provider.sendAsync);
const exampleRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'test',
};

const response = await promisifiedSendAsync(exampleRequest);
const response = await provider.request(exampleRequest);

expect(response.result).toBe(42);
expect(response).toBe(42);
});

it('handle a failed request', async () => {
const engine = new JsonRpcEngine();
engine.push((_req, _res, _next, _end) => {
throw new Error('Test error');
engine.push((_req, _res, _next, end) => {
end(
providerErrors.custom({
code: 1001,
message: 'Test error',
}),
);
});
const provider = providerFromEngine(engine);
const promisifiedSendAsync = promisify(provider.sendAsync);
const exampleRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'test',
};

await expect(async () =>
promisifiedSendAsync(exampleRequest),
).rejects.toThrow('Test error');
await expect(async () => provider.request(exampleRequest)).rejects.toThrow(
'Test error',
);
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { JsonRpcMiddleware } from '@metamask/json-rpc-engine';
import { promisify } from 'util';
import { providerErrors } from '@metamask/rpc-errors';

import { providerFromMiddleware } from './provider-from-middleware';

Expand All @@ -12,32 +12,34 @@ describe('providerFromMiddleware', () => {
end();
};
const provider = providerFromMiddleware(middleware);
const promisifiedSendAsync = promisify(provider.sendAsync);
const exampleRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'test',
};

const response = await promisifiedSendAsync(exampleRequest);
const response = await provider.request(exampleRequest);

expect(response.result).toBe(42);
expect(response).toBe(42);
});

it('handle a failed request', async () => {
const middleware = () => {
throw new Error('Test error');
};
const provider = providerFromMiddleware(middleware);
const promisifiedSendAsync = promisify(provider.sendAsync);
const provider = providerFromMiddleware((_req, _res, _next, end) => {
end(
providerErrors.custom({
code: 1001,
message: 'Test error',
}),
);
});
const exampleRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'test',
};

await expect(async () =>
promisifiedSendAsync(exampleRequest),
).rejects.toThrow('Test error');
await expect(async () => provider.request(exampleRequest)).rejects.toThrow(
'Test error',
);
});
});
Original file line number Diff line number Diff line change
@@ -1,7 +1,35 @@
import { Web3Provider } from '@ethersproject/providers';
import EthQuery from '@metamask/eth-query';
import EthJsQuery from '@metamask/ethjs-query';
import { JsonRpcEngine } from '@metamask/json-rpc-engine';
import { providerErrors } from '@metamask/rpc-errors';
import { type Json } from '@metamask/utils';
import { BrowserProvider } from 'ethers';
import { promisify } from 'util';

import { SafeEventEmitterProvider } from './safe-event-emitter-provider';
import {
SafeEventEmitterProvider,
convertEip1193RequestToJsonRpcRequest,
} from './safe-event-emitter-provider';

/**
* Creates a mock JSON-RPC engine that returns a predefined response for a specific method.
*
* @param method - The RPC method to mock.
* @param response - The response to return for the mocked method.
* @returns A JSON-RPC engine instance with the mocked method.
*/
function createMockEngine(method: string, response: Json) {
const engine = new JsonRpcEngine();
engine.push((req, res, next, end) => {
if (req.method === method) {
res.result = response;
return end();
}
return next();
});
return engine;
}

describe('SafeEventEmitterProvider', () => {
describe('constructor', () => {
Expand Down Expand Up @@ -30,6 +58,92 @@ describe('SafeEventEmitterProvider', () => {
});
});

it('returns the correct block number with @metamask/eth-query', async () => {
const provider = new SafeEventEmitterProvider({
engine: createMockEngine('eth_blockNumber', 42),
});
const ethQuery = new EthQuery(provider);

ethQuery.sendAsync({ method: 'eth_blockNumber' }, (_error, response) => {
expect(response).toBe(42);
});
});

it('returns the correct block number with @metamask/ethjs-query', async () => {
const provider = new SafeEventEmitterProvider({
engine: createMockEngine('eth_blockNumber', 42),
});
const ethJsQuery = new EthJsQuery(provider);

const response = await ethJsQuery.blockNumber();

expect(response.toNumber()).toBe(42);
});

it('returns the correct block number with Web3Provider', async () => {
const provider = new SafeEventEmitterProvider({
engine: createMockEngine('eth_blockNumber', 42),
});
const web3Provider = new Web3Provider(provider);

const response = await web3Provider.send('eth_blockNumber', []);

expect(response).toBe(42);
});

it('returns the correct block number with BrowserProvider', async () => {
const provider = new SafeEventEmitterProvider({
engine: createMockEngine('eth_blockNumber', 42),
});
const browserProvider = new BrowserProvider(provider);

const response = await browserProvider.send('eth_blockNumber', []);

expect(response).toBe(42);
});

describe('request', () => {
it('handles a successful request', async () => {
const engine = new JsonRpcEngine();
engine.push((_req, res, _next, end) => {
res.result = 42;
end();
});
const provider = new SafeEventEmitterProvider({ engine });
const exampleRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'test',
cryptodev-2s marked this conversation as resolved.
Show resolved Hide resolved
};

const response = await provider.request(exampleRequest);
cryptodev-2s marked this conversation as resolved.
Show resolved Hide resolved

expect(response).toBe(42);
});

it('handles a failed request', async () => {
cryptodev-2s marked this conversation as resolved.
Show resolved Hide resolved
const engine = new JsonRpcEngine();
engine.push((_req, _res, _next, end) => {
end(
providerErrors.custom({
code: 1001,
message: 'Test error',
}),
);
});
const provider = new SafeEventEmitterProvider({ engine });
const exampleRequest = {
id: 1,
jsonrpc: '2.0' as const,
method: 'test',
};

await expect(async () =>
provider.request(exampleRequest),
).rejects.toThrow('Test error');
});
});

describe('sendAsync', () => {
cryptodev-2s marked this conversation as resolved.
Show resolved Hide resolved
it('handles a successful request', async () => {
const engine = new JsonRpcEngine();
Expand Down Expand Up @@ -122,3 +236,90 @@ describe('SafeEventEmitterProvider', () => {
});
});
});

describe('convertEip1193RequestToJsonRpcRequest', () => {
it('generates a unique id if id is not provided', () => {
const eip1193Request = {
method: 'test',
params: { param1: 'value1', param2: 'value2' },
};

const jsonRpcRequest =
convertEip1193RequestToJsonRpcRequest(eip1193Request);

expect(jsonRpcRequest).toStrictEqual({
id: expect.any(String),
jsonrpc: '2.0',
method: 'test',
params: { param1: 'value1', param2: 'value2' },
});
});

it('uses the provided id if id is provided', () => {
const eip1193Request = {
id: '123',
method: 'test',
params: { param1: 'value1', param2: 'value2' },
};
const jsonRpcRequest =
convertEip1193RequestToJsonRpcRequest(eip1193Request);

expect(jsonRpcRequest).toStrictEqual({
id: '123',
jsonrpc: '2.0',
method: 'test',
params: { param1: 'value1', param2: 'value2' },
});
});

it('uses the default jsonrpc version if not provided', () => {
const eip1193Request = {
method: 'test',
params: { param1: 'value1', param2: 'value2' },
};

const jsonRpcRequest =
convertEip1193RequestToJsonRpcRequest(eip1193Request);

expect(jsonRpcRequest).toStrictEqual({
id: expect.any(String),
jsonrpc: '2.0',
method: 'test',
params: { param1: 'value1', param2: 'value2' },
});
});

it('uses the provided jsonrpc version if provided', () => {
const eip1193Request = {
jsonrpc: '2.0' as const,
method: 'test',
params: { param1: 'value1', param2: 'value2' },
};

const jsonRpcRequest =
convertEip1193RequestToJsonRpcRequest(eip1193Request);

expect(jsonRpcRequest).toStrictEqual({
id: expect.any(String),
jsonrpc: '2.0',
method: 'test',
params: { param1: 'value1', param2: 'value2' },
});
});

it('uses an empty object as params if not provided', () => {
const eip1193Request = {
method: 'test',
};

const jsonRpcRequest =
convertEip1193RequestToJsonRpcRequest(eip1193Request);

expect(jsonRpcRequest).toStrictEqual({
id: expect.any(String),
jsonrpc: '2.0',
method: 'test',
params: {},
});
});
});
Loading
Loading