From deecfb5e7c476bff5c4c04ea244f17860fd9109a Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 10 Oct 2023 13:15:32 +0200 Subject: [PATCH 1/4] Fix snaps-controllers tests --- packages/snaps-controllers/coverage.json | 2 +- .../src/services/AbstractExecutionService.ts | 29 +++++++++++++++---- .../src/services/browser.test.ts | 1 + .../node/NodeProcessExecutionService.test.ts | 24 ++++++++++----- .../node/NodeThreadExecutionService.test.ts | 24 ++++++++++----- 5 files changed, 60 insertions(+), 20 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 42a887dfc6..8fdf6f7439 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,5 +1,5 @@ { - "branches": 89.28, + "branches": 89.36, "functions": 95.6, "lines": 96.97, "statements": 96.64 diff --git a/packages/snaps-controllers/src/services/AbstractExecutionService.ts b/packages/snaps-controllers/src/services/AbstractExecutionService.ts index fc115e0092..a8a93a5d2e 100644 --- a/packages/snaps-controllers/src/services/AbstractExecutionService.ts +++ b/packages/snaps-controllers/src/services/AbstractExecutionService.ts @@ -9,7 +9,12 @@ import type { JsonRpcRequest, PendingJsonRpcResponse, } from '@metamask/utils'; -import { Duration, isJsonRpcNotification, isObject } from '@metamask/utils'; +import { + Duration, + hasProperty, + isJsonRpcNotification, + isObject, +} from '@metamask/utils'; import { createStreamMiddleware } from 'json-rpc-middleware-stream'; import { nanoid } from 'nanoid'; import { pipeline } from 'stream'; @@ -47,6 +52,10 @@ export type Job = { worker: WorkerType; }; +export class ExecutionEnvironmentError extends Error { + cause?: Json; +} + export abstract class AbstractExecutionService implements ExecutionService { @@ -373,12 +382,22 @@ export abstract class AbstractExecutionService } log('Parent: Sending Command', message); - const response: PendingJsonRpcResponse = - // eslint-disable-next-line @typescript-eslint/await-thenable - await job.rpcEngine.handle(message); + const response: PendingJsonRpcResponse = await job.rpcEngine.handle( + message, + ); + if (response.error) { - throw new Error(response.error.message); + const error = new ExecutionEnvironmentError(response.error.message); + if ( + isObject(response.error.data) && + hasProperty(response.error.data, 'cause') + ) { + error.cause = response.error.data.cause; + } + + throw error; } + return response.result; } diff --git a/packages/snaps-controllers/src/services/browser.test.ts b/packages/snaps-controllers/src/services/browser.test.ts index caaeff1548..a20f0ded01 100644 --- a/packages/snaps-controllers/src/services/browser.test.ts +++ b/packages/snaps-controllers/src/services/browser.test.ts @@ -8,6 +8,7 @@ describe('browser entrypoint', () => { 'OffscreenExecutionService', 'WebWorkerExecutionService', 'ProxyPostMessageStream', + 'ExecutionEnvironmentError', ]; it('entrypoint has expected exports', () => { diff --git a/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts b/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts index 131a1acce5..465e380343 100644 --- a/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts +++ b/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts @@ -2,6 +2,7 @@ import type { SnapId } from '@metamask/snaps-utils'; import { HandlerType } from '@metamask/snaps-utils'; import { createService, MOCK_BLOCK_NUMBER } from '../../test-utils'; +import { ExecutionEnvironmentError } from '../AbstractExecutionService'; import type { SnapErrorJson } from '../ExecutionService'; import { NodeProcessExecutionService } from './NodeProcessExecutionService'; @@ -47,7 +48,7 @@ describe('NodeProcessExecutionService', () => { }); it('can handle errors in request handler', async () => { - expect.assertions(1); + expect.assertions(2); const { service } = createService(NodeProcessExecutionService); const snapId = 'TestSnap'; await service.executeSnap({ @@ -58,8 +59,8 @@ describe('NodeProcessExecutionService', () => { endowments: [], }); - await expect( - service.handleRpcRequest(snapId, { + const result = await service + .handleRpcRequest(snapId, { origin: 'fooOrigin', handler: ON_RPC_REQUEST, request: { @@ -68,8 +69,17 @@ describe('NodeProcessExecutionService', () => { params: {}, id: 1, }, - }), - ).rejects.toThrow('foobar'); + }) + .catch((error) => error); + + expect(result).toBeInstanceOf(ExecutionEnvironmentError); + + // eslint-disable-next-line jest/prefer-strict-equal + expect((result as ExecutionEnvironmentError).cause).toEqual({ + message: 'foobar', + stack: expect.any(String), + }); + await service.terminateAllSnaps(); }); @@ -126,9 +136,9 @@ describe('NodeProcessExecutionService', () => { code: -32603, data: { snapId: 'TestSnap', - stack: expect.any(String), + stack: expect.stringContaining('Error: random error inside'), }, - message: 'random error inside', + message: 'Execution Environment Error', }); await service.terminateAllSnaps(); diff --git a/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts b/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts index 15f1cce4dc..f878d7ccff 100644 --- a/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts +++ b/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts @@ -2,6 +2,7 @@ import type { SnapId } from '@metamask/snaps-utils'; import { HandlerType } from '@metamask/snaps-utils'; import { createService, MOCK_BLOCK_NUMBER } from '../../test-utils'; +import { ExecutionEnvironmentError } from '../AbstractExecutionService'; import type { SnapErrorJson } from '../ExecutionService'; import { NodeThreadExecutionService } from './NodeThreadExecutionService'; @@ -47,7 +48,7 @@ describe('NodeThreadExecutionService', () => { }); it('can handle errors in request handler', async () => { - expect.assertions(1); + expect.assertions(2); const { service } = createService(NodeThreadExecutionService); const snapId = 'TestSnap'; await service.executeSnap({ @@ -58,8 +59,8 @@ describe('NodeThreadExecutionService', () => { endowments: [], }); - await expect( - service.handleRpcRequest(snapId, { + const result = await service + .handleRpcRequest(snapId, { origin: 'fooOrigin', handler: ON_RPC_REQUEST, request: { @@ -68,8 +69,17 @@ describe('NodeThreadExecutionService', () => { params: {}, id: 1, }, - }), - ).rejects.toThrow('foobar'); + }) + .catch((error) => error); + + expect(result).toBeInstanceOf(ExecutionEnvironmentError); + + // eslint-disable-next-line jest/prefer-strict-equal + expect((result as ExecutionEnvironmentError).cause).toEqual({ + message: 'foobar', + stack: expect.any(String), + }); + await service.terminateAllSnaps(); }); @@ -126,9 +136,9 @@ describe('NodeThreadExecutionService', () => { code: -32603, data: { snapId: 'TestSnap', - stack: expect.any(String), + stack: expect.stringContaining('Error: random error inside'), }, - message: 'random error inside', + message: 'Execution Environment Error', }); await service.terminateAllSnaps(); From be95c713d50f7fa50ca1287f6d3fdd768d976c1a Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 10 Oct 2023 13:31:29 +0200 Subject: [PATCH 2/4] Only add cause if it's non-null --- packages/snaps-controllers/coverage.json | 2 +- .../snaps-controllers/src/services/AbstractExecutionService.ts | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 8fdf6f7439..4d7ba0c10f 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -1,5 +1,5 @@ { - "branches": 89.36, + "branches": 89.39, "functions": 95.6, "lines": 96.97, "statements": 96.64 diff --git a/packages/snaps-controllers/src/services/AbstractExecutionService.ts b/packages/snaps-controllers/src/services/AbstractExecutionService.ts index a8a93a5d2e..ad23f14aea 100644 --- a/packages/snaps-controllers/src/services/AbstractExecutionService.ts +++ b/packages/snaps-controllers/src/services/AbstractExecutionService.ts @@ -390,7 +390,8 @@ export abstract class AbstractExecutionService const error = new ExecutionEnvironmentError(response.error.message); if ( isObject(response.error.data) && - hasProperty(response.error.data, 'cause') + hasProperty(response.error.data, 'cause') && + response.error.data.cause !== null ) { error.cause = response.error.data.cause; } From 404f97d77d1d0dff07ee8c4600d2d78e05153693 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Tue, 10 Oct 2023 16:00:00 +0200 Subject: [PATCH 3/4] Fix execution environments errors --- .../snaps-execution-environments/coverage.json | 6 +++--- .../src/common/BaseSnapExecutor.test.browser.ts | 15 ++++++++++----- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/snaps-execution-environments/coverage.json b/packages/snaps-execution-environments/coverage.json index 3955b04bb7..34fcf6a6bc 100644 --- a/packages/snaps-execution-environments/coverage.json +++ b/packages/snaps-execution-environments/coverage.json @@ -1,6 +1,6 @@ { - "branches": 80.41, + "branches": 79.72, "functions": 91.91, - "lines": 91.41, - "statements": 91.12 + "lines": 91.08, + "statements": 90.8 } diff --git a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts index 4c6bbac97f..08739ff4c2 100644 --- a/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts +++ b/packages/snaps-execution-environments/src/common/BaseSnapExecutor.test.browser.ts @@ -541,7 +541,8 @@ describe('BaseSnapExecutor', () => { jsonrpc: '2.0', error: { code: -32603, - message: expect.stringContaining('undefined'), + // TODO: Unwrap errors, and change this to the actual error message. + message: 'Execution Environment Error', data: expect.any(Object), }, id: 2, @@ -1022,7 +1023,8 @@ describe('BaseSnapExecutor', () => { stack: error.stack, snapId: MOCK_SNAP_ID, }, - message: error.message, + // TODO: Unwrap errors, and change this to the actual error message. + message: 'Execution Environment Error', }, }, }); @@ -1082,7 +1084,8 @@ describe('BaseSnapExecutor', () => { stack: error.stack, snapId: MOCK_SNAP_ID, }, - message: error.message, + // TODO: Unwrap errors, and change this to the actual error message. + message: 'Execution Environment Error', }, }, }); @@ -1388,7 +1391,8 @@ describe('BaseSnapExecutor', () => { error: { code: -32603, data: expect.anything(), - message: expect.stringContaining('undefined'), + // TODO: Unwrap errors, and change this to the actual error message. + message: 'Execution Environment Error', }, }); @@ -1744,7 +1748,8 @@ describe('BaseSnapExecutor', () => { error: { code: -32603, data: expect.any(Object), - message: 'JSON-RPC responses must be JSON serializable objects.', + // TODO: Unwrap errors, and change this to the actual error message. + message: 'Execution Environment Error', }, }); }); From 1889e590c7ba8ddf343fa02334fc9fc8355b80d7 Mon Sep 17 00:00:00 2001 From: Maarten Zuidhoorn Date: Wed, 11 Oct 2023 10:40:36 +0200 Subject: [PATCH 4/4] Add more TODOs --- .../src/services/node/NodeProcessExecutionService.test.ts | 2 ++ .../src/services/node/NodeThreadExecutionService.test.ts | 2 ++ 2 files changed, 4 insertions(+) diff --git a/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts b/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts index 465e380343..eddc8ca799 100644 --- a/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts +++ b/packages/snaps-controllers/src/services/node/NodeProcessExecutionService.test.ts @@ -76,6 +76,7 @@ describe('NodeProcessExecutionService', () => { // eslint-disable-next-line jest/prefer-strict-equal expect((result as ExecutionEnvironmentError).cause).toEqual({ + // TODO: Unwrap errors, and change this to the actual error message. message: 'foobar', stack: expect.any(String), }); @@ -138,6 +139,7 @@ describe('NodeProcessExecutionService', () => { snapId: 'TestSnap', stack: expect.stringContaining('Error: random error inside'), }, + // TODO: Unwrap errors, and change this to the actual error message. message: 'Execution Environment Error', }); diff --git a/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts b/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts index f878d7ccff..437af71f8e 100644 --- a/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts +++ b/packages/snaps-controllers/src/services/node/NodeThreadExecutionService.test.ts @@ -76,6 +76,7 @@ describe('NodeThreadExecutionService', () => { // eslint-disable-next-line jest/prefer-strict-equal expect((result as ExecutionEnvironmentError).cause).toEqual({ + // TODO: Unwrap errors, and change this to the actual error message. message: 'foobar', stack: expect.any(String), }); @@ -138,6 +139,7 @@ describe('NodeThreadExecutionService', () => { snapId: 'TestSnap', stack: expect.stringContaining('Error: random error inside'), }, + // TODO: Unwrap errors, and change this to the actual error message. message: 'Execution Environment Error', });