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.

Sign up for GitHub

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

Jump to bottom

feat: make SafeEventEmitterProvider compatible with eth req libraries #4422

Merged
merged 32 commits into from
Jul 8, 2024
Merged

feat: make SafeEventEmitterProvider compatible with eth req libraries #4422

Show file tree
Hide file tree
Changes from all 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
1 change: 0 additions & 1 deletion packages/assets-controllers/src/AssetsContractController.ts
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,19 @@
},
"dependencies": {
"@metamask/json-rpc-engine": "^9.0.0",
"@metamask/rpc-errors": "^6.2.1",
"@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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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",
"@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
View file
Open in desktop
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',
);
});
});
26 changes: 14 additions & 12 deletions packages/eth-json-rpc-provider/src/provider-from-middleware.test.ts
View file
Open in desktop
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',
);
});
});
Loading
Loading