From b3441ebffcdddbf3d2b8265371a33d43c11e1047 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 29 Oct 2025 00:07:42 -0700 Subject: [PATCH 1/4] refactor: Genericize JsonRpcServer middleware / engine --- .../json-rpc-engine/src/v2/JsonRpcEngineV2.ts | 4 +- .../src/v2/JsonRpcServer.test.ts | 40 ++++++++++++++- .../json-rpc-engine/src/v2/JsonRpcServer.ts | 50 +++++++++++++++---- packages/json-rpc-engine/src/v2/index.ts | 8 ++- 4 files changed, 88 insertions(+), 14 deletions(-) diff --git a/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts b/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts index 24a41d82929..fe89e800ef0 100644 --- a/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts +++ b/packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts @@ -73,7 +73,7 @@ type ConstructorOptions< >; }; -type RequestOf = +export type RequestOf = Middleware extends JsonRpcMiddleware< infer Request, ResultConstraint, @@ -91,7 +91,7 @@ type ContextOf = ? C : never; -type MergedContextOf< +export type MergedContextOf< // Non-polluting `any` constraint. // eslint-disable-next-line @typescript-eslint/no-explicit-any Middleware extends JsonRpcMiddleware, diff --git a/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts b/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts index 88d90d2e7d3..605513c2ed5 100644 --- a/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts +++ b/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts @@ -3,7 +3,8 @@ import { rpcErrors } from '@metamask/rpc-errors'; import type { JsonRpcMiddleware } from './JsonRpcEngineV2'; import { JsonRpcEngineV2 } from './JsonRpcEngineV2'; import { JsonRpcServer } from './JsonRpcServer'; -import { isRequest } from './utils'; +import type { JsonRpcNotification, JsonRpcRequest } from './utils'; +import { isRequest, JsonRpcEngineError, stringify } from './utils'; const jsonrpc = '2.0' as const; @@ -218,6 +219,43 @@ describe('JsonRpcServer', () => { }); }); + it('throws if passed a notification when only requests are supported', async () => { + const onError = jest.fn(); + const server = new JsonRpcServer>({ + middleware: [() => null], + onError, + }); + + const notification = { jsonrpc, method: 'hello' }; + await server.handle(notification); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith( + new JsonRpcEngineError( + `Result returned for notification: ${stringify(notification)}`, + ), + ); + }); + + it('throws if passed a request when only notifications are supported', async () => { + const onError = jest.fn(); + const server = new JsonRpcServer>({ + middleware: [() => undefined], + onError, + }); + + const request = { jsonrpc, id: 1, method: 'hello' }; + await server.handle(request); + + expect(onError).toHaveBeenCalledTimes(1); + expect(onError).toHaveBeenCalledWith( + expect.objectContaining({ + // Using a regex match because id in the error message is not predictable. + message: expect.stringMatching(/^Nothing ended request: /u), + }), + ); + }); + it.each([undefined, Symbol('test'), null, true, false, {}, []])( 'accepts requests with malformed ids', async (id) => { diff --git a/packages/json-rpc-engine/src/v2/JsonRpcServer.ts b/packages/json-rpc-engine/src/v2/JsonRpcServer.ts index bd10a67bf4a..3630ad64577 100644 --- a/packages/json-rpc-engine/src/v2/JsonRpcServer.ts +++ b/packages/json-rpc-engine/src/v2/JsonRpcServer.ts @@ -8,21 +8,26 @@ import type { } from '@metamask/utils'; import { hasProperty, isObject } from '@metamask/utils'; -import type { JsonRpcMiddleware } from './JsonRpcEngineV2'; +import type { + JsonRpcMiddleware, + MergedContextOf, + RequestOf, + ResultConstraint, +} from './JsonRpcEngineV2'; import { JsonRpcEngineV2 } from './JsonRpcEngineV2'; import type { JsonRpcCall } from './utils'; import { getUniqueId } from '../getUniqueId'; type OnError = (error: unknown) => void; -type Options = { +type Options = { onError?: OnError; } & ( | { engine: JsonRpcEngineV2; } | { - middleware: NonEmptyArray; + middleware: NonEmptyArray; } ); @@ -34,6 +39,9 @@ const jsonrpc = '2.0' as const; * Essentially wraps a {@link JsonRpcEngineV2} in order to create a conformant * yet permissive JSON-RPC 2.0 server. * + * Note that the server will accept both requests and notifications via {@link handle}, + * even if the underlying engine is only able to handle one or the other. + * * @example * ```ts * const server = new JsonRpcServer({ @@ -49,8 +57,20 @@ const jsonrpc = '2.0' as const; * } * ``` */ -export class JsonRpcServer { - readonly #engine: JsonRpcEngineV2; +export class JsonRpcServer< + Middleware extends JsonRpcMiddleware< + // Non-polluting `any` constraint. + /* eslint-disable @typescript-eslint/no-explicit-any */ + any, + ResultConstraint, + any + /* eslint-enable @typescript-eslint/no-explicit-any */ + > = JsonRpcMiddleware, +> { + readonly #engine: JsonRpcEngineV2< + RequestOf, + MergedContextOf + >; readonly #onError?: OnError | undefined; @@ -67,13 +87,14 @@ export class JsonRpcServer { * @param options.middleware - The middleware to use. Mutually exclusive with * `engine`. */ - constructor(options: Options) { + constructor(options: Options) { this.#onError = options.onError; if (hasProperty(options, 'engine')) { // @ts-expect-error - hasProperty fails to narrow the type. this.#engine = options.engine; } else { + // @ts-expect-error - TypeScript complains that engine is of the wrong type, but clearly it's not. this.#engine = JsonRpcEngineV2.create({ middleware: options.middleware }); } } @@ -84,6 +105,9 @@ export class JsonRpcServer { * This method never throws. For requests, a response is always returned. * All errors are passed to the engine's `onError` callback. * + * **WARNING**: This method is unaware of the request type of the underlying + * engine. The request will fail if the engine can only handle notifications. + * * @param request - The request to handle. * @returns The JSON-RPC response. */ @@ -95,6 +119,9 @@ export class JsonRpcServer { * This method never throws. For notifications, `undefined` is always returned. * All errors are passed to the engine's `onError` callback. * + * **WARNING**: This method is unaware of the request type of the underlying + * engine. The request will fail if the engine cannot handle notifications. + * * @param notification - The notification to handle. */ async handle(notification: JsonRpcNotification): Promise; @@ -102,13 +129,16 @@ export class JsonRpcServer { /** * Handle an alleged JSON-RPC request or notification. Permits any plain * object with `{ method: string }`, so long as any present JSON-RPC 2.0 - * properties are valid. If the object has no `id`, it will be treated as - * a notification and vice versa. + * properties are valid. If the object has an `id` property, it will be + * treated as a request, otherwise it will be treated as a notification. * * This method never throws. All errors are passed to the engine's * `onError` callback. A JSON-RPC response is always returned for requests, * and `undefined` is returned for notifications. * + * **WARNING**: The request will fail if its coerced type (i.e. request or + * response) is not of the type expected by the underlying engine. + * * @param rawRequest - The raw request to handle. * @returns The JSON-RPC response, or `undefined` if the request is a * notification. @@ -123,6 +153,8 @@ export class JsonRpcServer { try { const request = this.#coerceRequest(rawRequest, isRequest); + // @ts-expect-error - The request may not be of the type expected by the engine, + // and we intentionally allow this to happen. const result = await this.#engine.handle(request); if (result !== undefined) { @@ -213,7 +245,6 @@ function hasValidParams( if (hasProperty(rawRequest, 'params')) { return Array.isArray(rawRequest.params) || isObject(rawRequest.params); } - return true; } @@ -228,6 +259,5 @@ function getOriginalId(rawRequest: unknown): [unknown, boolean] { if (isObject(rawRequest) && hasProperty(rawRequest, 'id')) { return [rawRequest.id, true]; } - return [undefined, false]; } diff --git a/packages/json-rpc-engine/src/v2/index.ts b/packages/json-rpc-engine/src/v2/index.ts index 29ea635dca6..29560da7df6 100644 --- a/packages/json-rpc-engine/src/v2/index.ts +++ b/packages/json-rpc-engine/src/v2/index.ts @@ -1,7 +1,13 @@ export { asLegacyMiddleware } from './asLegacyMiddleware'; export { getUniqueId } from '../getUniqueId'; export { createScaffoldMiddleware } from './createScaffoldMiddleware'; -export * from './JsonRpcEngineV2'; +export { JsonRpcEngineV2 } from './JsonRpcEngineV2'; +export type { + JsonRpcMiddleware, + MiddlewareParams, + Next, + ResultConstraint, +} from './JsonRpcEngineV2'; export { JsonRpcServer } from './JsonRpcServer'; export { MiddlewareContext } from './MiddlewareContext'; export type { EmptyContext } from './MiddlewareContext'; From 3654a2714599a8293187f2f8ff579e3ae3cf8881 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 29 Oct 2025 00:14:49 -0700 Subject: [PATCH 2/4] docs: Update changelog --- packages/json-rpc-engine/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/json-rpc-engine/CHANGELOG.md b/packages/json-rpc-engine/CHANGELOG.md index 1dea6cc6675..e3617409f25 100644 --- a/packages/json-rpc-engine/CHANGELOG.md +++ b/packages/json-rpc-engine/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- `JsonRpcEngineV2` ([#6176](https://github.com/MetaMask/core/pull/6176), [#6971](https://github.com/MetaMask/core/pull/6971), [#6975](https://github.com/MetaMask/core/pull/6975)) +- `JsonRpcEngineV2` ([#6176](https://github.com/MetaMask/core/pull/6176), [#6971](https://github.com/MetaMask/core/pull/6971), [#6975](https://github.com/MetaMask/core/pull/6975), [#6990](https://github.com/MetaMask/core/pull/6990)) - This is a complete rewrite of `JsonRpcEngine`, intended to replace the original implementation. See the readme for details. From b77bf6bb33646639bfc218a4edafac42a276dd83 Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 29 Oct 2025 00:19:14 -0700 Subject: [PATCH 3/4] docs: Update readme --- packages/json-rpc-engine/README.md | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/json-rpc-engine/README.md b/packages/json-rpc-engine/README.md index 1113ede22df..464755feaf8 100644 --- a/packages/json-rpc-engine/README.md +++ b/packages/json-rpc-engine/README.md @@ -551,8 +551,8 @@ The following middleware are incompatible due to mismatched request types: > [!WARNING] > Providing `JsonRpcRequest`- and `JsonRpcNotification`-only middleware to the same engine is -> unsound and should be avoided. However, doing so will **not** cause a type error, and it -> is the programmer's responsibility to prevent it from happening. +> generally unsound and should be avoided. However, doing so will **not** cause a type error, +> and it is the programmer's responsibility to prevent it from happening. ```ts const middleware1: JsonRpcMiddleware = /* ... */; @@ -722,6 +722,12 @@ Errors thrown by the underlying engine are always passed to `onError` unmodified If the request is not a notification, the error is subsequently serialized and attached to the response object via the `error` property. +> [!WARNING] +> It is possible to construct a `JsonRpcServer` the only accepts either requests or notifications, +> but not both. If you do so, it is your responsibility to ensure that the server is only used with the +> appropriate request objects. `JsonRpcServer.handle()` will not type error at compile time if you attempt to pass +> it an unsupported request object. + ## Contributing This package is part of a monorepo. Instructions for contributing can be found in the [monorepo README](https://github.com/MetaMask/core#readme). From e68ff036f23fcc0f1137a7c8f805393ecea3ad5b Mon Sep 17 00:00:00 2001 From: Erik Marks <25517051+rekmarks@users.noreply.github.com> Date: Wed, 29 Oct 2025 09:52:43 -0700 Subject: [PATCH 4/4] refactor: Respond to review --- .../src/v2/JsonRpcServer.test.ts | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts b/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts index 605513c2ed5..df6a6ce3e2d 100644 --- a/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts +++ b/packages/json-rpc-engine/src/v2/JsonRpcServer.test.ts @@ -219,7 +219,7 @@ describe('JsonRpcServer', () => { }); }); - it('throws if passed a notification when only requests are supported', async () => { + it('errors if passed a notification when only requests are supported', async () => { const onError = jest.fn(); const server = new JsonRpcServer>({ middleware: [() => null], @@ -227,8 +227,9 @@ describe('JsonRpcServer', () => { }); const notification = { jsonrpc, method: 'hello' }; - await server.handle(notification); + const response = await server.handle(notification); + expect(response).toBeUndefined(); expect(onError).toHaveBeenCalledTimes(1); expect(onError).toHaveBeenCalledWith( new JsonRpcEngineError( @@ -237,7 +238,7 @@ describe('JsonRpcServer', () => { ); }); - it('throws if passed a request when only notifications are supported', async () => { + it('errors if passed a request when only notifications are supported', async () => { const onError = jest.fn(); const server = new JsonRpcServer>({ middleware: [() => undefined], @@ -245,8 +246,17 @@ describe('JsonRpcServer', () => { }); const request = { jsonrpc, id: 1, method: 'hello' }; - await server.handle(request); + const response = await server.handle(request); + expect(response).toStrictEqual({ + jsonrpc, + id: 1, + error: { + code: -32603, + message: expect.stringMatching(/^Nothing ended request: /u), + data: { cause: expect.any(Object) }, + }, + }); expect(onError).toHaveBeenCalledTimes(1); expect(onError).toHaveBeenCalledWith( expect.objectContaining({ @@ -291,7 +301,7 @@ describe('JsonRpcServer', () => { { jsonrpc }, { id: 1 }, ])( - 'throws if the request is not minimally conformant', + 'errors if the request is not minimally conformant', async (malformedRequest) => { const onError = jest.fn(); const server = new JsonRpcServer({