From 47f4a940f061eeafb66e09ce83c042e26a16d33f Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Mon, 21 Oct 2024 13:13:47 +0200 Subject: [PATCH] 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. --- .../services/AbstractExecutionService.test.ts | 27 ---------------- .../src/services/AbstractExecutionService.ts | 18 ++++++----- .../src/snaps/SnapController.test.tsx | 32 +++++++++++++++++++ .../coverage.json | 4 +-- .../src/common/BaseSnapExecutor.ts | 2 ++ .../src/common/utils.test.ts | 22 ------------- .../src/common/utils.ts | 21 +----------- .../src/common/validation.ts | 6 +++- 8 files changed, 52 insertions(+), 80 deletions(-) diff --git a/packages/snaps-controllers/src/services/AbstractExecutionService.test.ts b/packages/snaps-controllers/src/services/AbstractExecutionService.test.ts index 49dc43740c..b1b817f4a5 100644 --- a/packages/snaps-controllers/src/services/AbstractExecutionService.test.ts +++ b/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); diff --git a/packages/snaps-controllers/src/services/AbstractExecutionService.ts b/packages/snaps-controllers/src/services/AbstractExecutionService.ts index 31c6e0d9cf..12b2a2632c 100644 --- a/packages/snaps-controllers/src/services/AbstractExecutionService.ts +++ b/packages/snaps-controllers/src/services/AbstractExecutionService.ts @@ -410,13 +410,17 @@ export abstract class AbstractExecutionService 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 jobId: string, message: JsonRpcRequest, ): Promise { - assertIsJsonRpcRequest(message); - const job = this.jobs.get(jobId); if (!job) { throw new Error(`Job with id "${jobId}" not found.`); diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.tsx b/packages/snaps-controllers/src/snaps/SnapController.test.tsx index 7072ee6efe..dad16aed3f 100644 --- a/packages/snaps-controllers/src/snaps/SnapController.test.tsx +++ b/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); diff --git a/packages/snaps-execution-environments/coverage.json b/packages/snaps-execution-environments/coverage.json index a60ace0c3e..bb7c7347df 100644 --- a/packages/snaps-execution-environments/coverage.json +++ b/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 } diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts index a6de058aa9..49d9f83f99 100644 --- a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.ts +++ b/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( diff --git a/packages/snaps-execution-environments/src/common/utils.test.ts b/packages/snaps-execution-environments/src/common/utils.test.ts index 2acacc371a..8edf9a538a 100644 --- a/packages/snaps-execution-environments/src/common/utils.test.ts +++ b/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', () => { diff --git a/packages/snaps-execution-environments/src/common/utils.ts b/packages/snaps-execution-environments/src/common/utils.ts index db71429f5c..e6e51ecaab 100644 --- a/packages/snaps-execution-environments/src/common/utils.ts +++ b/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, - ); } /** diff --git a/packages/snaps-execution-environments/src/common/validation.ts b/packages/snaps-execution-environments/src/common/validation.ts index e28c6c3f62..166c9df8c7 100644 --- a/packages/snaps-execution-environments/src/common/validation.ts +++ b/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())), }), ), ]);