feat: Add createMethodMiddleware function#8506
Conversation
| messenger: Messenger<string, MessengerActions>; | ||
| hooks: Hooks; | ||
| }) => Promise<Result> | Result; | ||
| hookNames?: { [Key in keyof Hooks]: true }; |
There was a problem hiding this comment.
I'm wondering why this wasn't just a list of names in our current production implementations. Open to changing it here.
There was a problem hiding this comment.
This is because, at the time it was first implemented, you couldn't create an array type like [Key in keyof Hooks]. Idk if this has changed.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 9ad6497. Configure here.
| messenger, | ||
| }); | ||
|
|
||
| const engine = JsonRpcEngineV2.create({ middleware: [middleware] }); |
There was a problem hiding this comment.
Should we be sharing values across tests like this? Just curious whether it would be less error-prone in the future to create them inside of each test, perhaps using a function to encapsulate the setup.
There was a problem hiding this comment.
I don't think it's ever justified. Even if I know that the value is stateless, I still create a makeX() utility or do let x; beforeEach(() => { x = ... });.
| options: CreateMethodMiddlewareOptions<Handlers>, | ||
| ): JsonRpcMiddleware<JsonRpcRequest, Json, Context> { | ||
| const { messenger: rootMessenger } = options; | ||
| const allHooks = options.hooks as Record<string, unknown>; |
There was a problem hiding this comment.
I was curious why we needed this type assertion. I asked Claude, and if I understand correctly, because we are using a conditional type in HandlerHooks, TypeScript doesn't have enough information to resolve UnionToIntersection<HandlerHooks<Handlers[keyof Handlers]>> to Record<string, unknown> at this point in time. I'm not sure what TypeScript would need or how else we could set up the types for createMethodMiddleware. But the suggestion I was given was that we could change CreateMethodMiddlewareOptions from this:
export type CreateMethodMiddlewareOptions<
Handlers extends Record<string, AnyMethodHandler>,
> = {
handlers: Handlers;
messenger: Messenger<string, HandlerActions<Handlers[keyof Handlers]>>;
hooks: UnionToIntersection<HandlerHooks<Handlers[keyof Handlers]>>;
};to this:
export type CreateMethodMiddlewareOptions<
Handlers extends Record<string, AnyMethodHandler>,
> = {
handlers: Handlers;
messenger: Messenger<string, HandlerActions<Handlers[keyof Handlers]>>;
hooks: UnionToIntersection<HandlerHooks<Handlers[keyof Handlers]>> &
Record<string, unknown>;
};and then here we can remove the type assertion:
| const allHooks = options.hooks as Record<string, unknown>; | |
| const allHooks = options.hooks; |
My interpretation of this is we are telling TypeScript, "hey, just so you know, hooks is also a Record<string, unknown>, so treat it like that too".
What do you think?
|
|
||
| ### Added | ||
|
|
||
| - Add `createMethodMiddleware` ([#8506](https://github.com/MetaMask/core/pull/8506)) |
There was a problem hiding this comment.
Should we be explicit that this was added to the v2 path? Something like:
| - Add `createMethodMiddleware` ([#8506](https://github.com/MetaMask/core/pull/8506)) | |
| - Add `createMethodMiddleware` to `@metamask/json-rpc-engine/v2` ([#8506](https://github.com/MetaMask/core/pull/8506)) |
or even just
| - Add `createMethodMiddleware` ([#8506](https://github.com/MetaMask/core/pull/8506)) | |
| - Add `createMethodMiddleware` to `v2` ([#8506](https://github.com/MetaMask/core/pull/8506)) |
| messenger: Messenger<string, MessengerActions>; | ||
| hooks: Hooks; |
There was a problem hiding this comment.
Should these be optional? Could they be optional?
| messenger: Messenger<string, MessengerActions>; | ||
| hooks: Hooks; | ||
| }) => Promise<Result> | Result; | ||
| hookNames?: { [Key in keyof Hooks]: true }; |
There was a problem hiding this comment.
This is because, at the time it was first implemented, you couldn't create an array type like [Key in keyof Hooks]. Idk if this has changed.
3 participants
Explanation
This PR implements a
createMethodMiddlewarefunction which allows usage of both hooks and the messenger. Currently hooks are the only way JSON-RPC implementations can leverage controllers, services etc. They require more glue code to be written and are more difficult to type. This PR aims to provide a shared utility for constructing middlewares with multiple method implementations, while also allowing these implementations to use the messenger, cutting down the amount of glue code required in each client significantly.References
https://consensyssoftware.atlassian.net/browse/WPC-981
Checklist
Note
Medium Risk
Introduces new middleware construction and messenger delegation behavior; while additive, mistakes could cause incorrect action routing or over-broad hook exposure across RPC methods.
Overview
Adds
createMethodMiddlewareto@metamask/json-rpc-engine/v2, enabling per-method JSON-RPC handlers to receive a least-privilege subset of declared hooks and a delegated@metamask/messengerinstance for calling registered actions.Updates exports/tests to include
createMethodMiddleware,selectHooks, and theMethodHandlertype, and wires in@metamask/messengeras a dependency (including TS project references) plus a small README dependency-graph edge and changelog entry.Reviewed by Cursor Bugbot for commit fb74b9f. Bugbot is set up for automated code reviews on this repo. Configure here.