Fix transaction insight snaps returning null (#1952)

When RPC result assertions were added, we forgot to account for transaction insight snaps returning null to signify no insight. This PR fixes this problem. --------- Co-authored-by: Maarten Zuidhoorn <[email protected]>
MetaMask · Nov 10, 2023 · f621a33 · f621a33
1 parent 70269b6
commit f621a33
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 7 deletions.
diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json
@@ -1,5 +1,5 @@
 {
-  "branches": 90.23,
+  "branches": 90.26,
   "functions": 96.32,
   "lines": 97.22,
   "statements": 96.9

diff --git a/packages/snaps-controllers/src/snaps/SnapController.test.ts b/packages/snaps-controllers/src/snaps/SnapController.test.ts
@@ -2247,6 +2247,58 @@ describe('SnapController', () => {
     });
   });
 
+  it(`doesn't throw if onTransaction handler returns null`, async () => {
+    const rootMessenger = getControllerMessenger();
+    const messenger = getSnapControllerMessenger(rootMessenger);
+    const snapController = getSnapController(
+      getSnapControllerOptions({
+        messenger,
+        state: {
+          snaps: getPersistedSnapsState(),
+        },
+      }),
+    );
+
+    rootMessenger.registerActionHandler(
+      'PermissionController:getPermissions',
+      () => ({
+        [SnapEndowments.TransactionInsight]: {
+          caveats: [{ type: SnapCaveatType.TransactionOrigin, value: false }],
+          date: 1664187844588,
+          id: 'izn0WGUO8cvq_jqvLQuQP',
+          invoker: MOCK_SNAP_ID,
+          parentCapability: SnapEndowments.TransactionInsight,
+        },
+      }),
+    );
+
+    rootMessenger.registerActionHandler(
+      'SubjectMetadataController:getSubjectMetadata',
+      () => MOCK_SNAP_SUBJECT_METADATA,
+    );
+
+    rootMessenger.registerActionHandler(
+      'ExecutionService:handleRpcRequest',
+      async () => Promise.resolve(null),
+    );
+
+    expect(
+      await snapController.handleRequest({
+        snapId: MOCK_SNAP_ID,
+        origin: 'foo.com',
+        handler: HandlerType.OnTransaction,
+        request: {
+          jsonrpc: '2.0',
+          method: ' ',
+          params: {},
+          id: 1,
+        },
+      }),
+    ).toBeNull();
+
+    snapController.destroy();
+  });
+
   it('throws if onHomePage handler returns a phishing link', async () => {
     const rootMessenger = getControllerMessenger();
     const messenger = getSnapControllerMessenger(rootMessenger);

diff --git a/packages/snaps-controllers/src/snaps/SnapController.ts b/packages/snaps-controllers/src/snaps/SnapController.ts
@@ -2701,8 +2701,12 @@ export class SnapController extends BaseController<
    */
   async #assertSnapRpcRequestResult(handlerType: HandlerType, result: unknown) {
     switch (handlerType) {
-      case HandlerType.OnTransaction:
+      case HandlerType.OnTransaction: {
         assertStruct(result, OnTransactionResponseStruct);
+        // Null is an allowed return value here
+        if (result === null) {
+          return;
+        }
 
         await this.#triggerPhishingListUpdate();
 
@@ -2711,6 +2715,7 @@ export class SnapController extends BaseController<
           this.#checkPhishingList.bind(this),
         );
         break;
+      }
       case HandlerType.OnHomePage:
         assertStruct(result, OnHomePageResponseStruct);
 

diff --git a/packages/snaps-utils/src/handlers.ts b/packages/snaps-utils/src/handlers.ts
@@ -10,7 +10,7 @@ import type {
 } from '@metamask/snaps-sdk';
 import { SeverityLevel } from '@metamask/snaps-sdk';
 import { ComponentStruct } from '@metamask/snaps-ui';
-import { literal, object, optional } from 'superstruct';
+import { literal, nullable, object, optional } from 'superstruct';
 
 import type { SnapHandler } from './handler-types';
 import { HandlerType } from './handler-types';
@@ -80,10 +80,12 @@ export const SNAP_EXPORTS = {
   },
 } as const;
 
-export const OnTransactionResponseStruct = object({
-  content: ComponentStruct,
-  severity: optional(literal(SeverityLevel.Critical)),
-});
+export const OnTransactionResponseStruct = nullable(
+  object({
+    content: ComponentStruct,
+    severity: optional(literal(SeverityLevel.Critical)),
+  }),
+);
 
 export const OnHomePageResponseStruct = object({
   content: ComponentStruct,