-
-
Notifications
You must be signed in to change notification settings - Fork 254
refactor!: Migrate eth-json-rpc-middleware to JsonRpcEngineV2
#7065
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
Conversation
8bf6c31 to
a345102
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
1e01c22 to
50e1c81
Compare
… methods (#7061) ## Explanation Adds a `MiddlewareContext` parameter to `InternalProvider.request()` and `JsonRpcServer.handle()`. This is essentially a missing feature from the `JsonRpcEngineV2` implementation / migration, since callers are no longer able to add non-JSON-RPC properties to request objects and instead need to use the `context` object. As part of this, permit specifying a plain object as the context to avoid forcing callers to import `MiddlewareContext` whenever they want to make a JSON-RPC call with some particular context. Also opportunistically fixes a bug with the static `isInstance` methods of V2 engine classes, where we wouldn't walk the prototype chain when checking for the symbol property. ## References - Related to #6327 ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've communicated my changes to consumers by [updating changelogs for packages I've changed](https://github.com/MetaMask/core/tree/main/docs/contributing.md#updating-changelogs), highlighting breaking changes as necessary - ~I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes~ - These changes are non-breaking, and will in any event be exhaustively covered via preview builds through #7065. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Adds a context option to `InternalProvider.request()`/`JsonRpcServer.handle()`, enables passing plain-object contexts across the V2 engine, makes `PollingBlockTracker` context-generic, and narrows Network Controller’s `Provider` type; updates tests/docs accordingly. > > - **JSON-RPC Engine (v2)**: > - Add `HandleOptions` with `context` (accepts `MiddlewareContext` or plain object) and forward it through `handle()` and `JsonRpcServer.handle()`. > - Enhance `MiddlewareContext` (construct from KeyValues object, `isInstance`) and export `InferKeyValues`; add shared `isInstance` util; minor server refactor (static request coercion). > - Update README and tests for context passing and utils. > - **eth-json-rpc-provider**: > - Make `InternalProvider` generic over `Context`; `request()` accepts `{ context }` and forwards to engine. > - Update tests and CHANGELOG. > - **eth-block-tracker**: > - Genericize `PollingBlockTracker` with `Context`; type provider as `InternalProvider<Context>`; update CHANGELOG. > - **Network Controller**: > - Narrow `Provider` to `InternalProvider<MiddlewareContext<{ origin: string; skipCache: boolean } & Record<string, unknown>>>`. > - Type updates in `create-network-client`, tests, and helpers; update CHANGELOG. > - **Tests/Utilities**: > - Update fakes (`FakeProvider`, `FakeBlockTracker`) and consumers to new generic/context types. > - Bridge controller test now uses `@metamask/network-controller` `Provider`. > - **Deps**: > - Add `@metamask/network-controller` to root `package.json`. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit c853283. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
7e2d0c0 to
78988ea
Compare
78988ea to
536ed51
Compare
| } | ||
| ], | ||
| "include": ["../../types", "./src", "./test", "./types"] | ||
| "include": ["../../types", "./src", "./test"] |
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.
There is no ./types directory.
| headers, | ||
| }, | ||
| ); | ||
| const jsonRpcResponse = await rpcService.request(klona(request), { |
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.
I naively cloned this, but I'm questioning whether we actually need to. The request is not mutated in rpc-service.ts. Can we just pass the frozen request?
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.
I think we can: 92a672c
| "@metamask/eth-json-rpc-provider": "^5.0.1", | ||
| "@metamask/eth-sig-util": "^8.2.0", | ||
| "@metamask/json-rpc-engine": "^10.1.1", | ||
| "@metamask/message-manager": "^14.0.0", |
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.
Type-only import, but the types appear in the public interface.
3225e4a to
eb33694
Compare
jeffsmale90
left a comment
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.
I only reviewed the EIP-7715 related wallet-request-execution-permission and wallet-revoke-execution-permission middlewares, which look good!
mcmire
left a comment
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.
LGTM! I found some other minor changes we could make, but I can prepare a new PR for those.
|
|
||
| ## [Unreleased] | ||
|
|
||
| ### Changed |
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.
It looks like we also added a new export: providerAsMiddlewareV2.
I can make sure to add this when we make a new release.
|
|
||
| - **BREAKING:** Migrate to `JsonRpcEngineV2` ([#7065](https://github.com/MetaMask/core/pull/7065)) | ||
| - Migrates all middleware from `JsonRpcEngine` to `JsonRpcEngineV2`. | ||
| - Signatures of various middleware dependencies, e.g. `processTransaction` of `createWalletMiddleware`, have changed |
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.
It might be worth it to detail this further to make upgrading easier. I'll do another pass when making a new release.
| // allow request to be handled normally | ||
| log('Carrying original request forward %o', request); | ||
| try { | ||
| const result = (await next()) as Json; |
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.
I feel that we should use Readonly<Json> here rather than Json, to better match Next<...>. I can make this change in a followup.
| activeRequestHandlers.length, | ||
| request, | ||
| ); | ||
| runRequestHandlers({ error: error as Error }, activeRequestHandlers); |
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.
I don't think we need the type assertion here. I can make a change in a followup.
| export function createScaffoldMiddleware( | ||
| handlers: MiddlewareScaffold, | ||
| ): JsonRpcMiddleware<JsonRpcRequest, Json> { | ||
| export function createScaffoldMiddleware<Context extends ContextConstraint>( |
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.
Perhaps worth documenting this in the changelog? Need to investigate further.
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.
I guess this is covered under the existing bullet.
|
|
||
| ### Changed | ||
|
|
||
| - Rename `OriginalRequest` type to `MessageRequest` and permit string `id` values ([#7065](https://github.com/MetaMask/core/pull/7065)) |
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.
This is a breaking change due to the rename. We should probably retain the old name for backward compatibility, for now. I can handle this in a followup.
|
|
||
| async request<Params extends JsonRpcParams, Result extends Json>( | ||
| jsonRpcRequest: JsonRpcRequest<Params>, | ||
| // The request object may be frozen and must not be mutated. |
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.
We should consider adding this to AbstractRpcService as well. I can do this in a followup.
Explanation
Migrates
@metamask/eth-json-rpc-middlewareto theJsonRpcEngineV2conventions in a total overhaul of the package. The semantics of each middleware should be the same as before.One potential behavioral difference is that request cloning within middleware now occurs with
klonainstead ofklona/full. This is becauseklona/fullcopies property descriptors such that cloned requests are immutable but not frozen in theObject.isFrozen()sense. This is unlikely to make a difference in practice to us, since our request objects have historically been "plain" objects.As with all #6327-related work up to this point,
json-rpc-enginereceived minor refactors to facilitate the use of the new abstractions in situ.Incidentally, all lint rule exceptions have been removed and all lint warnings and errors have been resolved for
eth-json-rpc-middleware.Several other packages are changed to support or in consequence of the migration:
network-controllerNetworkClientis migrated toJsonRpcEngineV2.RpcServicenow usesReadonly<JsonRpcRequest>internally for request objects received via itsrequest()method, since the "fetch" middleware now passes it deeply frozen request objects directly.message-managerandsignature-controllerReferences
JsonRpcEngineV2#6327Checklist
Note
Migrates
@metamask/eth-json-rpc-middlewareand the Network Controller’s client toJsonRpcEngineV2, updates APIs to use context and deeply frozen requests, and aligns dependent packages/types and tests.@metamask/eth-json-rpc-middlewaretoJsonRpcEngineV2(createBlockCache/Ref/RefRewrite/TrackerInspector/InflightCache/RetryOnEmpty/Fetch,createWalletMiddleware).providerAsMiddlewareV2; retain legacy adapter for back-compat.origin,skipCache), return values instead of mutating responses; requests are deeply frozen.context; useInternalProvider.NetworkClientpipelines rebuilt onJsonRpcEngineV2; compose V2 middleware; provider created viaproviderFromMiddlewareV2.RpcService.requestnow acceptsReadonly<JsonRpcRequest>and handles frozen requests; tests expanded.OriginalRequest→MessageRequest; allow stringidvalues in message/signature requests.deep-freeze-strict,@metamask/message-manager); minor coverage threshold tweak.Written by Cursor Bugbot for commit e7a7768. This will update automatically on new commits. Configure here.