perf: Reduce unnecessary JSON validation (#2844)

Reduces unnecessary JSON validation when handling JSON-RPC requests to
and from Snaps. For large payloads, JSON validation may be expensive, so
we should attempt to reduce additional validation where inputs are
already validated.

packages/snaps-controllers/src/services/AbstractExecutionService.test.ts

@@ -106,33 +106,6 @@ describe('AbstractExecutionService', () => {
     );
   });
 
-  it('throws an error if RPC request is non JSON serializable', async () => {
-    const { service } = createService(MockExecutionService);
-    await service.executeSnap({
-      snapId: MOCK_SNAP_ID,
-      sourceCode: `
-         module.exports.onRpcRequest = () => null;
-      `,
-      endowments: [],
-    });
-
-    await expect(
-      service.handleRpcRequest(MOCK_SNAP_ID, {
-        origin: 'foo.com',
-        handler: HandlerType.OnRpcRequest,
-        request: {
-          id: 6,
-          method: 'bar',
-          params: undefined,
-        },
-      }),
-    ).rejects.toThrow(
-      'Invalid JSON-RPC request: At path: params -- Expected the value to satisfy a union of `record | array`, but received: [object Object].',
-    );
-
-    await service.terminateAllSnaps();
-  });
-
   it('throws an error if execution environment fails to respond to ping', async () => {
     const { service } = createService(MockExecutionService);
 

packages/snaps-controllers/src/services/AbstractExecutionService.ts

@@ -410,13 +410,17 @@ export abstract class AbstractExecutionService<WorkerType>
 
     const remainingTime = timer.remaining;
 
+    const request = {
+      jsonrpc: '2.0',
+      method: 'executeSnap',
+      params: { snapId, sourceCode, endowments },
+      id: nanoid(),
+    };
+
+    assertIsJsonRpcRequest(request);
+
     const result = await withTimeout(
-      this.command(job.id, {
-        jsonrpc: '2.0',
-        method: 'executeSnap',
-        params: { snapId, sourceCode, endowments },
-        id: nanoid(),
-      }),
+      this.command(job.id, request),
       remainingTime,
     );
 
@@ -433,8 +437,6 @@ export abstract class AbstractExecutionService<WorkerType>
     jobId: string,
     message: JsonRpcRequest,
   ): Promise<Json | undefined> {
-    assertIsJsonRpcRequest(message);
-
     const job = this.jobs.get(jobId);
     if (!job) {
       throw new Error(`Job with id "${jobId}" not found.`);

packages/snaps-controllers/src/snaps/SnapController.test.tsx

@@ -3764,6 +3764,38 @@ describe('SnapController', () => {
       snapController.destroy();
     });
 
+    it('handlers throw if the request is not valid JSON', async () => {
+      const fakeSnap = getPersistedSnapObject({ status: SnapStatus.Running });
+      const snapId = fakeSnap.id;
+      const snapController = getSnapController(
+        getSnapControllerOptions({
+          state: {
+            snaps: {
+              [snapId]: fakeSnap,
+            },
+          },
+        }),
+      );
+      await expect(
+        snapController.handleRequest({
+          snapId,
+          origin: 'foo.com',
+          handler: HandlerType.OnRpcRequest,
+          request: {
+            method: 'bar',
+            params: BigInt(0),
+          },
+        }),
+      ).rejects.toThrow(
+        rpcErrors.invalidRequest({
+          message:
+            'Invalid JSON-RPC request: At path: params -- Expected the value to satisfy a union of `record | array`, but received: 0.',
+        }),
+      );
+
+      snapController.destroy();
+    });
+
     it('handlers will throw if there are too many pending requests before a snap has started', async () => {
       const rootMessenger = getControllerMessenger();
       const messenger = getSnapControllerMessenger(rootMessenger);

packages/snaps-execution-environments/coverage.json

@@ -1,6 +1,6 @@
 {
   "branches": 80,
   "functions": 90.06,
-  "lines": 90.77,
-  "statements": 90.15
+  "lines": 90.75,
+  "statements": 90.12
 }

packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts

@@ -476,6 +476,7 @@ export class BaseSnapExecutor {
     const originalRequest = provider.request.bind(provider);
 
     const request = async (args: RequestArguments) => {
+      // As part of the sanitization, we validate that the args are valid JSON.
       const sanitizedArgs = sanitizeRequestArguments(args);
       assertSnapOutboundRequest(sanitizedArgs);
       return await withTeardown(
@@ -512,6 +513,7 @@ export class BaseSnapExecutor {
     const originalRequest = provider.request.bind(provider);
 
     const request = async (args: RequestArguments) => {
+      // As part of the sanitization, we validate that the args are valid JSON.
       const sanitizedArgs = sanitizeRequestArguments(args);
       assertEthereumOutboundRequest(sanitizedArgs);
       return await withTeardown(

packages/snaps-execution-environments/src/common/utils.test.ts

@@ -32,17 +32,6 @@ describe('assertSnapOutboundRequest', () => {
       'The method does not exist / is not available.',
     );
   });
-
-  it('throws for invalid JSON values', () => {
-    expect(() =>
-      assertSnapOutboundRequest({
-        method: 'snap_notify',
-        params: [undefined],
-      }),
-    ).toThrow(
-      'Provided value is not JSON-RPC compatible: Expected the value to satisfy a union of `literal | boolean | finite number | string | array | record`, but received: [object Object].',
-    );
-  });
 });
 
 describe('assertEthereumOutboundRequest', () => {
@@ -66,17 +55,6 @@ describe('assertEthereumOutboundRequest', () => {
       'The method does not exist / is not available.',
     );
   });
-
-  it('throws for invalid JSON values', () => {
-    expect(() =>
-      assertEthereumOutboundRequest({
-        method: 'eth_blockNumber',
-        params: [undefined],
-      }),
-    ).toThrow(
-      'Provided value is not JSON-RPC compatible: Expected the value to satisfy a union of `literal | boolean | finite number | string | array | record`, but received: [object Object].',
-    );
-  });
 });
 
 describe('isValidResponse', () => {

packages/snaps-execution-environments/src/common/utils.ts

@@ -1,13 +1,6 @@
 import type { StreamProvider, RequestArguments } from '@metamask/providers';
 import { rpcErrors } from '@metamask/rpc-errors';
-import {
-  assert,
-  assertStruct,
-  getJsonSize,
-  getSafeJson,
-  isObject,
-  JsonStruct,
-} from '@metamask/utils';
+import { assert, getJsonSize, getSafeJson, isObject } from '@metamask/utils';
 
 import { log } from '../logging';
 
@@ -122,12 +115,6 @@ export function assertSnapOutboundRequest(args: RequestArguments) {
       },
     }),
   );
-  assertStruct(
-    args,
-    JsonStruct,
-    'Provided value is not JSON-RPC compatible',
-    rpcErrors.invalidParams,
-  );
 }
 
 /**
@@ -153,12 +140,6 @@ export function assertEthereumOutboundRequest(args: RequestArguments) {
       },
     }),
   );
-  assertStruct(
-    args,
-    JsonStruct,
-    'Provided value is not JSON-RPC compatible',
-    rpcErrors.invalidParams,
-  );
 }
 
 /**

packages/snaps-execution-environments/src/common/validation.ts

@@ -6,6 +6,7 @@ import {
 import { ChainIdStruct, HandlerType } from '@metamask/snaps-utils';
 import type { Infer } from '@metamask/superstruct';
 import {
+  any,
   array,
   assign,
   enums,
@@ -87,7 +88,10 @@ export const SnapRpcRequestArgumentsStruct = tuple([
   assign(
     JsonRpcRequestWithoutIdStruct,
     object({
-      params: optional(record(string(), JsonStruct)),
+      // Previously this would validate that the parameters were valid JSON.
+      // This is already validated for all messages received by the executor.
+      // If that assumption changes, this should once again validate JSON.
+      params: optional(record(string(), any())),
     }),
   ),
 ]);