refactor(json-rpc-engine): Genericize JsonRpcServer middleware / engine

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.
 

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<JsonRpcNotification> = /* ... */;
@@ -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).

packages/json-rpc-engine/src/v2/JsonRpcEngineV2.ts

@@ -73,7 +73,7 @@ type ConstructorOptions<
   >;
 };
 
-type RequestOf<Middleware> =
+export type RequestOf<Middleware> =
   Middleware extends JsonRpcMiddleware<
     infer Request,
     ResultConstraint<infer Request>,
@@ -91,7 +91,7 @@ type ContextOf<Middleware> =
     ? C
     : never;
 
-type MergedContextOf<
+export type MergedContextOf<
   // Non-polluting `any` constraint.
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
   Middleware extends JsonRpcMiddleware<any, any, any>,

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,53 @@ describe('JsonRpcServer', () => {
     });
   });
 
+  it('errors if passed a notification when only requests are supported', async () => {
+    const onError = jest.fn();
+    const server = new JsonRpcServer<JsonRpcMiddleware<JsonRpcRequest>>({
+      middleware: [() => null],
+      onError,
+    });
+
+    const notification = { jsonrpc, method: 'hello' };
+    const response = await server.handle(notification);
+
+    expect(response).toBeUndefined();
+    expect(onError).toHaveBeenCalledTimes(1);
+    expect(onError).toHaveBeenCalledWith(
+      new JsonRpcEngineError(
+        `Result returned for notification: ${stringify(notification)}`,
+      ),
+    );
+  });
+
+  it('errors if passed a request when only notifications are supported', async () => {
+    const onError = jest.fn();
+    const server = new JsonRpcServer<JsonRpcMiddleware<JsonRpcNotification>>({
+      middleware: [() => undefined],
+      onError,
+    });
+
+    const request = { jsonrpc, id: 1, method: 'hello' };
+    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({
+        // 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) => {
@@ -253,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({

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<Middleware extends JsonRpcMiddleware> = {
   onError?: OnError;
 } & (
   | {
       engine: JsonRpcEngineV2;
     }
   | {
-      middleware: NonEmptyArray<JsonRpcMiddleware>;
+      middleware: NonEmptyArray<Middleware>;
     }
 );
 
@@ -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>,
+    any
+    /* eslint-enable @typescript-eslint/no-explicit-any */
+  > = JsonRpcMiddleware,
+> {
+  readonly #engine: JsonRpcEngineV2<
+    RequestOf<Middleware>,
+    MergedContextOf<Middleware>
+  >;
 
   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<Middleware>) {
     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,20 +119,26 @@ 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<void>;
 
   /**
    * 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];
 }

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';