feat: Add selectiveUnion for improved Superstruct error messaging (#…

…2696) Adds `selectiveUnion` and rewrites a bunch of structs to use this pattern. This helps alleviate a problem where Superstruct errors would be impossible to read. The idea of `selectiveUnion` is to be able to guide Superstruct validation more with simple checks that can be used to choose between validation paths. This combined with `typedUnion` makes it possible to have drastically improved error messages for structs that use unions. This PR rewrites some of the UI structs to use these new patterns, but is not necessarily exhaustive. It also adds support for refinements and coercion to `typedUnion`. Generally, this PR turns errors like this: ``` Invalid params: At path: ui -- Expected the value to satisfy a union of `union | union`, but received: "foo".' ``` into errors like this: ``` Invalid params: At path: ui -- Expected type to be one of: "Address", "Bold", "Box", "Button", "Copyable", "Divider", "Dropdown", "RadioGroup", "FileInput", "Form", "Heading", "Input", "Image", "Italic", "Link", "Row", "Spinner", "Text", "Tooltip", "Checkbox", "Card", "Icon", "Selector", "Section", "Container", but received: undefined. ``` This PR does not solve this problem for structs that use the `children()` utility function with more than one struct as the input (e.g. `Button`). These components will fail with something like: ``` Invalid params: At path: ui.props.children.1.props.children.props.children -- Expected the value to satisfy a union of `union | object | object`, but received: [object Object]. ``` There may also still be other UI components that fail with similar errors, however, this is a huge step forward in readability and therefore dev-ex. Progresses #2692 --------- Co-authored-by: Maarten Zuidhoorn <[email protected]>
MetaMask · Sep 10, 2024 · d21f57b · d21f57b
1 parent 8275454
commit d21f57b
Show file tree

Hide file tree

Showing 23 changed files with 479 additions and 158 deletions.
diff --git a/packages/examples/packages/bip32/snap.manifest.json b/packages/examples/packages/bip32/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "oIth1iwHikuY19tkSxTGtl4YzdX1XAiGn2RJl+DAeCs=",
+    "shasum": "5o3Zg3/1/23XKZdIzzo1Sbwmp2uQNF1EpnzyLYqhfqo=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/bip44/snap.manifest.json b/packages/examples/packages/bip44/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "Ta9rEMnKSdYz7ecxZh++RZyMSW9JVsQVqa+zFLLO87k=",
+    "shasum": "K0uZKIJkRUzXrIrbcNY/hjouFPSEVScyeJc0D5SvuIs=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/browserify-plugin/snap.manifest.json b/packages/examples/packages/browserify-plugin/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "q9RIkIkHMu7cVWgMRnqa0iLb7gzeYAk9LA6WXlEJkUg=",
+    "shasum": "zjj26VuJEMPd17W0vAdpVONDt/JeYPacP5GwbRAiQZo=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/browserify/snap.manifest.json b/packages/examples/packages/browserify/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "Sehc3xVJmoP4CBbyL64schgPUfHu7Labnrtk5i5LH88=",
+    "shasum": "WEO5ozYWnfy690WSCYKkexaQnlkDbks1f+vXrRVQRTM=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/cronjobs/snap.manifest.json b/packages/examples/packages/cronjobs/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "o5qiOur9Zl+yAjHScLzh0FfK7/jcKx7rGqotitpJ6Uo=",
+    "shasum": "jcA6NL+j9BPrnOiv5FOl+KnTyxAJ5UdThThk4zCoiu8=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/dialogs/snap.manifest.json b/packages/examples/packages/dialogs/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "77wtrsk6BY8OSjG5pWALMSJzOmznWaaR7EiM3LGigjI=",
+    "shasum": "P+jpppDqo3e+Dsyk2KP+26NlKDyGwW+CxztRYiDkqnQ=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/ethers-js/snap.manifest.json b/packages/examples/packages/ethers-js/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "fwMTtk4KJjak+yf6666Vv/+syXmFVAnVPAsBkalK2Qc=",
+    "shasum": "ARrlf69swjj7vnhGGRLajr4MFr4hbKbh9G5R987WFoo=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/get-entropy/snap.manifest.json b/packages/examples/packages/get-entropy/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "ur2Jxp1kniP1jYLeexXHTB/+dgfJom1volcPQgSNX6c=",
+    "shasum": "dPssrjujrDEsuDMxogdHlFNJbGtuZjTpwwU5Lirbr6A=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/images/snap.manifest.json b/packages/examples/packages/images/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "ewEvZGj7D/wUtBH8bDutuDuJ1leq8n9Penuu8yDAykA=",
+    "shasum": "lNV3t5b0qgpl6fMuTUdFA75wWL3zFzm3MvEokB8VF5U=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/invoke-snap/packages/core-signer/snap.manifest.json b/packages/examples/packages/invoke-snap/packages/core-signer/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "Y0k3QSqHBYytNfYgmtrjwhbZ3e8WzXhOQFYPJCIuJfg=",
+    "shasum": "nBYyvFGnEv4kGTgEAvmhbfAafd85Z1dxbQTczGahxvA=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/lifecycle-hooks/snap.manifest.json b/packages/examples/packages/lifecycle-hooks/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "Huxm0NrX6zvh0k9iGnStycNNcmHSGRK1EK/+jmv7nyI=",
+    "shasum": "Mgf0SLyA7yvGJ3+3y8f/Nf2GuMUzLHt4zRj8ZwVoC08=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/signature-insights/snap.manifest.json b/packages/examples/packages/signature-insights/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "mxW5zlXyghmveLeZIwJMZkwjqb7Y8peJut9uvbxgLCU=",
+    "shasum": "DdjMLFOqo+SfRA4nNJflLvSX3v/IwM23QtjPYzOLG1o=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/examples/packages/transaction-insights/snap.manifest.json b/packages/examples/packages/transaction-insights/snap.manifest.json
@@ -7,7 +7,7 @@
     "url": "https://github.com/MetaMask/snaps.git"
   },
   "source": {
-    "shasum": "2HeDCuxgl9zngyHAGEyikHFuM3Rcek5Jc0lCeuPNFqA=",
+    "shasum": "psh5WYTtiqN1PD18OI3JLmBiMFuo8Uw0V56yiBMVZ4M=",
     "location": {
       "npm": {
         "filePath": "dist/bundle.js",

diff --git a/packages/snaps-rpc-methods/jest.config.js b/packages/snaps-rpc-methods/jest.config.js
@@ -10,7 +10,7 @@ module.exports = deepmerge(baseConfig, {
   ],
   coverageThreshold: {
     global: {
-      branches: 92.07,
+      branches: 92.62,
       functions: 96.96,
       lines: 97.51,
       statements: 96.97,

diff --git a/packages/snaps-rpc-methods/src/permitted/createInterface.test.tsx b/packages/snaps-rpc-methods/src/permitted/createInterface.test.tsx
@@ -1,7 +1,15 @@
 import { JsonRpcEngine } from '@metamask/json-rpc-engine';
 import { text, type CreateInterfaceResult } from '@metamask/snaps-sdk';
 import type { JSXElement } from '@metamask/snaps-sdk/jsx';
-import { Text, Box } from '@metamask/snaps-sdk/jsx';
+import {
+  Text,
+  Box,
+  Field,
+  Input,
+  Form,
+  Container,
+  Footer,
+} from '@metamask/snaps-sdk/jsx';
 import type { JsonRpcRequest, PendingJsonRpcResponse } from '@metamask/utils';
 
 import type { CreateInterfaceParameters } from './createInterface';
@@ -132,7 +140,114 @@ describe('snap_createInterface', () => {
       error: {
         code: -32602,
         message:
-          'Invalid params: At path: ui -- Expected the value to satisfy a union of `union | union`, but received: "foo".',
+          'Invalid params: At path: ui -- Expected type to be one of: "Address", "Bold", "Box", "Button", "Copyable", "Divider", "Dropdown", "RadioGroup", "FileInput", "Form", "Heading", "Input", "Image", "Italic", "Link", "Row", "Spinner", "Text", "Tooltip", "Checkbox", "Card", "Icon", "Selector", "Section", "Container", but received: undefined.',
+        stack: expect.any(String),
+      },
+      id: 1,
+      jsonrpc: '2.0',
+    });
+  });
+
+  it('throws on invalid UI', async () => {
+    const { implementation } = createInterfaceHandler;
+
+    const createInterface = jest.fn().mockReturnValue('foo');
+
+    const hooks = {
+      createInterface,
+    };
+
+    const engine = new JsonRpcEngine();
+
+    engine.push((request, response, next, end) => {
+      const result = implementation(
+        request as JsonRpcRequest<CreateInterfaceParameters>,
+        response as PendingJsonRpcResponse<CreateInterfaceResult>,
+        next,
+        end,
+        hooks,
+      );
+
+      result?.catch(end);
+    });
+
+    const response = await engine.handle({
+      jsonrpc: '2.0',
+      id: 1,
+      method: 'snap_createInterface',
+      params: {
+        ui: (
+          <Box>
+            <Field label="Input">
+              <Input name="input" />
+            </Field>
+          </Box>
+        ) as JSXElement,
+      },
+    });
+
+    expect(response).toStrictEqual({
+      error: {
+        code: -32602,
+        message:
+          'Invalid params: At path: ui.props.children -- Expected type to be one of: "Address", "Bold", "Box", "Button", "Copyable", "Divider", "Dropdown", "RadioGroup", "FileInput", "Form", "Heading", "Input", "Image", "Italic", "Link", "Row", "Spinner", "Text", "Tooltip", "Checkbox", "Card", "Icon", "Selector", "Section", but received: "Field".',
+        stack: expect.any(String),
+      },
+      id: 1,
+      jsonrpc: '2.0',
+    });
+  });
+
+  it('throws on invalid nested UI', async () => {
+    const { implementation } = createInterfaceHandler;
+
+    const createInterface = jest.fn().mockReturnValue('foo');
+
+    const hooks = {
+      createInterface,
+    };
+
+    const engine = new JsonRpcEngine();
+
+    engine.push((request, response, next, end) => {
+      const result = implementation(
+        request as JsonRpcRequest<CreateInterfaceParameters>,
+        response as PendingJsonRpcResponse<CreateInterfaceResult>,
+        next,
+        end,
+        hooks,
+      );
+
+      result?.catch(end);
+    });
+
+    const response = await engine.handle({
+      jsonrpc: '2.0',
+      id: 1,
+      method: 'snap_createInterface',
+      params: {
+        ui: (
+          <Container>
+            <Box>
+              <Form name="form">
+                <Field label="Input">
+                  <Input name="input" />
+                </Field>
+              </Form>
+            </Box>
+            <Footer>
+              <Text>Foo</Text>
+            </Footer>
+          </Container>
+        ) as JSXElement,
+      },
+    });
+
+    expect(response).toStrictEqual({
+      error: {
+        code: -32602,
+        message:
+          'Invalid params: At path: ui.props.children.1.props.children.type -- Expected the literal `"Button"`, but received: "Text".',
         stack: expect.any(String),
       },
       id: 1,

diff --git a/packages/snaps-rpc-methods/src/restricted/dialog.test.tsx b/packages/snaps-rpc-methods/src/restricted/dialog.test.tsx
@@ -358,6 +358,30 @@ describe('implementation', () => {
         },
       });
     });
+
+    it('handles confirmations using an ID', async () => {
+      const hooks = getMockDialogHooks();
+      const implementation = getDialogImplementation(hooks);
+      await implementation({
+        context: { origin: 'foo' },
+        method: 'snap_dialog',
+        params: {
+          type: DialogType.Confirmation,
+          id: 'baz',
+        },
+      });
+
+      expect(hooks.requestUserApproval).toHaveBeenCalledTimes(1);
+      expect(hooks.requestUserApproval).toHaveBeenCalledWith({
+        id: undefined,
+        origin: 'foo',
+        type: DIALOG_APPROVAL_TYPES[DialogType.Confirmation],
+        requestData: {
+          id: 'baz',
+          placeholder: undefined,
+        },
+      });
+    });
   });
 
   describe('prompts', () => {
@@ -414,6 +438,31 @@ describe('implementation', () => {
         },
       });
     });
+
+    it('handles prompts using an ID', async () => {
+      const hooks = getMockDialogHooks();
+      const implementation = getDialogImplementation(hooks);
+      await implementation({
+        context: { origin: 'foo' },
+        method: 'snap_dialog',
+        params: {
+          type: DialogType.Prompt,
+          id: 'baz',
+          placeholder: 'foobar',
+        },
+      });
+
+      expect(hooks.requestUserApproval).toHaveBeenCalledTimes(1);
+      expect(hooks.requestUserApproval).toHaveBeenCalledWith({
+        id: undefined,
+        origin: 'foo',
+        type: DIALOG_APPROVAL_TYPES[DialogType.Prompt],
+        requestData: {
+          id: 'baz',
+          placeholder: 'foobar',
+        },
+      });
+    });
   });
 
   describe('validation', () => {
@@ -446,7 +495,7 @@ describe('implementation', () => {
           params: {} as any,
         }),
       ).rejects.toThrow(
-        'Invalid params: Expected the value to satisfy a union of `object | object`, but received: [object Object]',
+        /Invalid params: At path: .* -- Expected type to be one of: .*, but received: .*/u,
       );
     });
 
@@ -487,7 +536,7 @@ describe('implementation', () => {
           params: value as any,
         }),
       ).rejects.toThrow(
-        /Invalid params: At path: .* — Expected a value of type .*, but received: .*\./u,
+        /Invalid params: At path: .* -- Expected type to be one of: .*, but received: .*/u,
       );
     });
 
@@ -508,7 +557,7 @@ describe('implementation', () => {
             },
           }),
         ).rejects.toThrow(
-          /Invalid params: At path: placeholder — Expected a value of type string, but received: .*\./u,
+          /Invalid params: At path: placeholder -- Expected a string, but received: .*/u,
         );
       },
     );
@@ -528,7 +577,7 @@ describe('implementation', () => {
           },
         }),
       ).rejects.toThrow(
-        'Invalid params: At path: placeholder — Expected a string with a length between 1 and 40, but received one with a length of 0.',
+        'Invalid params: At path: placeholder -- Expected a string with a length between `1` and `40` but received one with a length of `0`',
       );
     });
 
@@ -548,7 +597,7 @@ describe('implementation', () => {
             },
           }),
         ).rejects.toThrow(
-          'Invalid params: Unknown key: placeholder, received: "foobar".',
+          'Invalid params: At path: placeholder -- Expected a value of type `never`, but received: `"foobar"`',
         );
       },
     );