Improve keyring endowment error messaging (#884)

* Improve keyring endowment error messaging

* Fix coverage

* Use assertIsNamespacesObject and make assertStruct more generic

* Fix small things

* Fix coverage

packages/controllers/jest.config.js

@@ -8,10 +8,10 @@ module.exports = {
   coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'],
   coverageThreshold: {
     global: {
-      branches: 86,
+      branches: 85.97,
       functions: 95.33,
       lines: 94.88,
-      statements: 94.98,
+      statements: 94.97,
     },
   },
   projects: [

packages/controllers/src/snaps/endowments/keyring.test.ts

@@ -181,7 +181,9 @@ describe('keyringCaveatSpecifications', () => {
             namespaces: undefined,
           },
         }),
-      ).toThrow('Expected a valid namespaces object.');
+      ).toThrow(
+        'Invalid namespaces object: Expected an object, but received: undefined.',
+      );
     });
   });
 });

packages/controllers/src/snaps/endowments/keyring.ts

@@ -1,5 +1,5 @@
 import {
-  isNamespacesObject,
+  assertIsNamespacesObject,
   Namespaces,
   SnapCaveatType,
 } from '@metamask/snap-utils';
@@ -92,11 +92,7 @@ function validateCaveatNamespace(caveat: Caveat<string, any>): void {
     });
   }
 
-  if (!isNamespacesObject(value.namespaces)) {
-    throw ethErrors.rpc.invalidParams({
-      message: 'Expected a valid namespaces object.',
-    });
-  }
+  assertIsNamespacesObject(value.namespaces, ethErrors.rpc.invalidParams);
 }
 
 /**

packages/utils/jest.config.js

@@ -15,10 +15,10 @@ module.exports = {
   coverageReporters: ['clover', 'json', 'lcov', 'text', 'json-summary'],
   coverageThreshold: {
     global: {
-      branches: 86.05,
-      functions: 97.14,
-      lines: 96.79,
-      statements: 96.86,
+      branches: 86.54,
+      functions: 97.19,
+      lines: 96.82,
+      statements: 96.89,
     },
   },
   moduleFileExtensions: ['js', 'json', 'jsx', 'ts', 'tsx', 'node'],

packages/utils/src/assert.test.ts

@@ -10,19 +10,51 @@ describe('assertStruct', () => {
 
   it('throws meaningful error messages for an invalid value', () => {
     expect(() => assertStruct({ data: 'foo' }, EventStruct)).toThrow(
-      'Assertion failed: At path: name -- Expected a string, but received: undefined',
+      'Assertion failed: At path: name -- Expected a string, but received: undefined.',
     );
 
     expect(() => assertStruct({ name: 1, data: 'foo' }, EventStruct)).toThrow(
-      'Assertion failed: At path: name -- Expected a string, but received: 1',
+      'Assertion failed: At path: name -- Expected a string, but received: 1.',
     );
   });
 
   it('throws with a custom error prefix', () => {
     expect(() =>
       assertStruct({ data: 'foo' }, EventStruct, 'Invalid event'),
     ).toThrow(
-      'Invalid event: At path: name -- Expected a string, but received: undefined',
+      'Invalid event: At path: name -- Expected a string, but received: undefined.',
+    );
+  });
+
+  it('throws with a custom error class', () => {
+    class CustomError extends Error {
+      constructor({ message }: { message: string }) {
+        super(message);
+        this.name = 'CustomError';
+      }
+    }
+
+    expect(() =>
+      assertStruct({ data: 'foo' }, EventStruct, 'Invalid event', CustomError),
+    ).toThrow(
+      new CustomError({
+        message:
+          'Invalid event: At path: name -- Expected a string, but received: undefined.',
+      }),
+    );
+  });
+
+  it('throws with a custom error function', () => {
+    const CustomError = ({ message }: { message: string }) =>
+      new Error(message);
+
+    expect(() =>
+      assertStruct({ data: 'foo' }, EventStruct, 'Invalid event', CustomError),
+    ).toThrow(
+      CustomError({
+        message:
+          'Invalid event: At path: name -- Expected a string, but received: undefined.',
+      }),
     );
   });
 });

packages/utils/src/assert.ts

@@ -1,25 +1,50 @@
 import { AssertionError } from '@metamask/utils';
 import { Struct, assert as assertSuperstruct } from 'superstruct';
 
+export type AssertionErrorConstructor =
+  | (new (args: { message: string }) => Error)
+  | ((args: { message: string }) => Error);
+
+/**
+ * Check if a value is a constructor.
+ *
+ * @param fn - The value to check.
+ * @returns `true` if the value is a constructor, or `false` otherwise.
+ */
+function isConstructable(
+  fn: AssertionErrorConstructor,
+): fn is new (args: { message: string }) => Error {
+  return Boolean(typeof fn?.prototype?.constructor?.name === 'string');
+}
+
 /**
  * Assert a value against a Superstruct struct.
  *
  * @param value - The value to validate.
  * @param struct - The struct to validate against.
  * @param errorPrefix - A prefix to add to the error message. Defaults to
  * "Assertion failed".
+ * @param ErrorWrapper - The error class to throw if the assertion fails.
+ * Defaults to {@link AssertionError}.
  * @throws If the value is not valid.
  */
 export function assertStruct<T, S>(
   value: unknown,
   struct: Struct<T, S>,
   errorPrefix = 'Assertion failed',
+  ErrorWrapper: AssertionErrorConstructor = AssertionError,
 ): asserts value is T {
   try {
     assertSuperstruct(value, struct);
   } catch (error) {
-    throw new AssertionError({
-      message: `${errorPrefix}: ${error.message}`,
-    });
+    if (isConstructable(ErrorWrapper)) {
+      throw new ErrorWrapper({
+        message: `${errorPrefix}: ${error.message}.`,
+      });
+    } else {
+      throw ErrorWrapper({
+        message: `${errorPrefix}: ${error.message}.`,
+      });
+    }
   }
 }

packages/utils/src/namespace.test.ts

@@ -7,6 +7,7 @@ import {
 import {
   assertIsConnectArguments,
   assertIsMultiChainRequest,
+  assertIsNamespacesObject,
   assertIsSession,
   isAccountId,
   isAccountIdArray,
@@ -645,3 +646,40 @@ describe('isNamespacesObject', () => {
     expect(isNamespacesObject(object)).toBe(false);
   });
 });
+
+describe('assertIsNamespacesObject', () => {
+  it.each([
+    {},
+    { eip155: getNamespace() },
+    { bip122: getNamespace() },
+    { eip155: getNamespace(), bip122: getNamespace() },
+  ])('does not throw for a valid namespaces object', (object) => {
+    expect(() => assertIsNamespacesObject(object)).not.toThrow();
+  });
+
+  it.each([
+    true,
+    false,
+    null,
+    undefined,
+    1,
+    'foo',
+    { eip155: {} },
+    { eip155: [], bip122: [] },
+    { eip155: true, bip122: true },
+    { eip155: false, bip122: false },
+    { eip155: null, bip122: null },
+    { eip155: undefined, bip122: undefined },
+    { eip155: 1, bip122: 1 },
+    { eip155: 'foo', bip122: 'foo' },
+    { eip155: { methods: [] }, bip122: { methods: [] } },
+    { eip155: { chains: ['foo'] }, bip122: { chains: ['foo'] } },
+    { a: getNamespace() },
+    { eip155: getNamespace(), a: getNamespace() },
+    { foobarbaz: getNamespace() },
+  ])('throws for an invalid namespaces object', (object) => {
+    expect(() => assertIsNamespacesObject(object)).toThrow(
+      'Invalid namespaces object:',
+    );
+  });
+});

packages/utils/src/namespace.ts

@@ -14,7 +14,7 @@ import {
   pick,
 } from 'superstruct';
 import { JsonRpcRequestStruct } from '@metamask/utils';
-import { assertStruct } from './assert';
+import { AssertionErrorConstructor, assertStruct } from './assert';
 
 export const CHAIN_ID_REGEX =
   /^(?<namespace>[-a-z0-9]{3,8}):(?<reference>[-a-zA-Z0-9]{1,32})$/u;
@@ -276,3 +276,23 @@ export function isNamespace(value: unknown): value is Namespace {
 export function isNamespacesObject(value: unknown): value is Namespaces {
   return is(value, NamespacesStruct);
 }
+
+/**
+ * Assert that the given value is a {@link Namespaces} object.
+ *
+ * @param value - The value to check.
+ * @param ErrorWrapper - The error wrapper to use. Defaults to
+ * {@link AssertionError}.
+ * @throws If the value is not a valid {@link Namespaces} object.
+ */
+export function assertIsNamespacesObject(
+  value: unknown,
+  ErrorWrapper?: AssertionErrorConstructor,
+): asserts value is Namespaces {
+  assertStruct(
+    value,
+    NamespacesStruct,
+    'Invalid namespaces object',
+    ErrorWrapper,
+  );
+}