feat(snap-account-service): report migration error

packages/snap-account-service/CHANGELOG.md

@@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Report migration error in case of failure ([#9696](https://github.com/MetaMask/core/pull/9696))
+  - The migration is a mandatory and it is not supposed to fail.
+  - Reporting should help identifying which step could have failed and help addressing the actual issue.
+
 ### Changed
 
 - Bump `@metamask/account-tree-controller` from `^7.5.4` to `^7.5.5` ([#9470](https://github.com/MetaMask/core/pull/9470))

packages/snap-account-service/src/SnapAccountService.test.ts

@@ -39,6 +39,7 @@ import type { SnapControllerState } from '@metamask/snaps-controllers';
 import type { SnapId } from '@metamask/snaps-sdk';
 import type { TruncatedSnap } from '@metamask/snaps-utils';
 
+import { SafeError } from './errors.js';
 import type {
   SnapAccountServiceMessenger,
   SnapAccountServiceOptions,
@@ -89,10 +90,13 @@ type Mocks = {
 /**
  * Constructs the root messenger for the service under test.
  *
+ * @param captureException - Optional method to capture exceptions in Sentry.
  * @returns The root messenger.
  */
-function getRootMessenger(): RootMessenger {
-  return new Messenger({ namespace: MOCK_ANY_NAMESPACE });
+function getRootMessenger(
+  captureException?: (error: Error) => void,
+): RootMessenger {
+  return new Messenger({ namespace: MOCK_ANY_NAMESPACE, captureException });
 }
 
 /**
@@ -400,23 +404,26 @@ function mockWithKeyringV2Unsafe(
  * @param args.snapIsReady - Initial value of `SnapController.isReady`.
  * @param args.runnableSnaps - Snaps returned by `SnapController:getRunnableSnaps`.
  * @param args.config - Optional service config.
+ * @param args.captureException - Optional method to capture exceptions in Sentry.
  * @returns The new service, root messenger, service messenger, and mocks.
  */
 async function setup({
   snapIsReady = true,
   runnableSnaps = [],
   config,
+  captureException,
 }: {
   snapIsReady?: boolean;
   runnableSnaps?: TruncatedSnap[];
   config?: SnapAccountServiceOptions['config'];
+  captureException?: (error: Error) => void;
 } = {}): Promise<{
   service: SnapAccountService;
   rootMessenger: RootMessenger;
   messenger: SnapAccountServiceMessenger;
   mocks: Mocks;
 }> {
-  const rootMessenger = getRootMessenger();
+  const rootMessenger = getRootMessenger(captureException);
   const messenger = getMessenger(rootMessenger);
 
   const mocks: Mocks = {
@@ -614,6 +621,116 @@ describe('SnapAccountService', () => {
       expect(removeKeyring).toHaveBeenCalledWith(legacyKeyringId);
     });
 
+    it('wraps unknown addNewKeyring failures as SafeError', async () => {
+      const legacyKeyringId = 'legacy-id';
+      const legacyKeyring = {
+        type: SNAP_KEYRING_TYPE,
+        listAccounts: jest.fn().mockReturnValue([
+          {
+            id: 'a1',
+            address: '0x1',
+            metadata: { snap: { id: MOCK_SNAP_ID } },
+          },
+        ]),
+      };
+      const { service, mocks } = await setup();
+      const addNewKeyring = jest
+        .fn()
+        .mockRejectedValue(new Error('superstruct: address 0x1 invalid'));
+      mocks.KeyringController.withController.mockImplementation(
+        async (operation) =>
+          operation({
+            keyrings: [
+              { keyring: legacyKeyring, metadata: { id: legacyKeyringId } },
+            ],
+            addNewKeyring,
+            removeKeyring: jest.fn(),
+          }),
+      );
+
+      await expect(service.ensureMigrated()).rejects.toThrow(SafeError);
+    });
+
+    it('re-throws KeyringControllerError from addNewKeyring as-is', async () => {
+      const legacyKeyringId = 'legacy-id';
+      const legacyKeyring = {
+        type: SNAP_KEYRING_TYPE,
+        listAccounts: jest.fn().mockReturnValue([
+          {
+            id: 'a1',
+            address: '0x1',
+            metadata: { snap: { id: MOCK_SNAP_ID } },
+          },
+        ]),
+      };
+      const { service, mocks } = await setup();
+      const error = new KeyringControllerError(
+        KeyringControllerErrorMessage.DuplicatedAccount,
+      );
+      const addNewKeyring = jest.fn().mockRejectedValue(error);
+      mocks.KeyringController.withController.mockImplementation(
+        async (operation) =>
+          operation({
+            keyrings: [
+              { keyring: legacyKeyring, metadata: { id: legacyKeyringId } },
+            ],
+            addNewKeyring,
+            removeKeyring: jest.fn(),
+          }),
+      );
+
+      await expect(service.ensureMigrated()).rejects.toThrow(error);
+    });
+
+    it('wraps unknown removeKeyring failures as SafeError', async () => {
+      const legacyKeyringId = 'legacy-id';
+      const legacyKeyring = {
+        type: SNAP_KEYRING_TYPE,
+        listAccounts: jest.fn().mockReturnValue([]),
+      };
+      const { service, mocks } = await setup();
+      const removeKeyring = jest
+        .fn()
+        .mockRejectedValue(new Error('unexpected internal error'));
+      mocks.KeyringController.withController.mockImplementation(
+        async (operation) =>
+          operation({
+            keyrings: [
+              { keyring: legacyKeyring, metadata: { id: legacyKeyringId } },
+            ],
+            addNewKeyring: jest.fn(),
+            removeKeyring,
+          }),
+      );
+
+      await expect(service.ensureMigrated()).rejects.toThrow(SafeError);
+    });
+
+    it('re-throws KeyringControllerError from removeKeyring as-is', async () => {
+      const legacyKeyringId = 'legacy-id';
+      const legacyKeyring = {
+        type: SNAP_KEYRING_TYPE,
+        listAccounts: jest.fn().mockReturnValue([]),
+      };
+      const { service, mocks } = await setup();
+      const error = new KeyringControllerError(
+        KeyringControllerErrorMessage.KeyringNotFound,
+      );
+      const removeKeyring = jest.fn().mockRejectedValue(error);
+      mocks.KeyringController.withController.mockImplementation(
+        async (operation) =>
+          operation({
+            keyrings: [
+              { keyring: legacyKeyring, metadata: { id: legacyKeyringId } },
+            ],
+            addNewKeyring: jest.fn(),
+            removeKeyring,
+          }),
+      );
+
+      await expect(service.ensureMigrated()).rejects.toThrow(error);
+    });
+
     it('does not re-run after a successful migration', async () => {
       const { service, mocks } = await setup();
       mocks.KeyringController.withController.mockResolvedValue(undefined);
@@ -1390,9 +1507,24 @@ describe('SnapAccountService', () => {
       expect(service).toBeDefined();
     });
 
-    it('logs an error when migration fails', async () => {
-      const { service, rootMessenger, mocks } = await setup();
-      const error = new Error('migration boom');
+    it.each([
+      {
+        name: 'SafeError',
+        error: new SafeError(
+          'Adding v2 Snap keyring for "npm:test-snap": Validation failed at "accounts.id.type" (expected: enums)',
+        ),
+      },
+      {
+        name: 'KeyringControllerError',
+        error: new KeyringControllerError(
+          KeyringControllerErrorMessage.DuplicatedAccount,
+        ),
+      },
+    ])('passes a $name through to Sentry as-is', async ({ error }) => {
+      const captureException = jest.fn();
+      const { service, rootMessenger, mocks } = await setup({
+        captureException,
+      });
       mocks.KeyringController.withController.mockRejectedValueOnce(error);
       const consoleErrorSpy = jest
         .spyOn(console, 'error')
@@ -1402,9 +1534,49 @@ describe('SnapAccountService', () => {
       await flushMicrotasks();
 
       expect(consoleErrorSpy).toHaveBeenCalledWith(
-        'Migration failed after unlock:',
+        'Migration failed after unlock',
         error,
       );
+      expect(captureException).toHaveBeenCalledWith(
+        expect.objectContaining({
+          message: 'Migration failed after unlock',
+          cause: error,
+        }),
+      );
+      expect(service).toBeDefined();
+      consoleErrorSpy.mockRestore();
+    });
+
+    it('replaces an unknown error with a generic SafeError before reporting to Sentry', async () => {
+      const captureException = jest.fn();
+      const { service, rootMessenger, mocks } = await setup({
+        captureException,
+      });
+      // An error from snap keyring internals or superstruct could contain
+      // account addresses in its message — withSafeError replaces it with a safe one.
+      mocks.KeyringController.withController.mockRejectedValueOnce(
+        new Error('internal error with address 0x1234'),
+      );
+      const consoleErrorSpy = jest
+        .spyOn(console, 'error')
+        .mockImplementation(() => undefined);
+
+      publishUnlock(rootMessenger);
+      await flushMicrotasks();
+
+      const safeError = expect.objectContaining({
+        message: 'Snap keyring v2 migration: An unexpected error occurred',
+      });
+      expect(consoleErrorSpy).toHaveBeenCalledWith(
+        'Migration failed after unlock',
+        safeError,
+      );
+      expect(captureException).toHaveBeenCalledWith(
+        expect.objectContaining({
+          message: 'Migration failed after unlock',
+          cause: safeError,
+        }),
+      );
       expect(service).toBeDefined();
       consoleErrorSpy.mockRestore();
     });
@@ -1423,8 +1595,9 @@ describe('SnapAccountService', () => {
       mocks.AccountTreeController.getAccountGroupObject.mockReturnValue(
         buildGroup(MOCK_GROUP_ID, MOCK_ACCOUNTS),
       );
-      const error = new Error('migration boom');
-      mocks.KeyringController.withController.mockRejectedValueOnce(error);
+      mocks.KeyringController.withController.mockRejectedValueOnce(
+        new Error('migration boom'),
+      );
       const consoleErrorSpy = jest
         .spyOn(console, 'error')
         .mockImplementation(() => undefined);
@@ -1437,8 +1610,8 @@ describe('SnapAccountService', () => {
         mocks.KeyringController.withKeyringV2Unsafe,
       ).not.toHaveBeenCalled();
       expect(consoleErrorSpy).toHaveBeenCalledWith(
-        'Migration failed after unlock:',
-        error,
+        'Migration failed after unlock',
+        expect.any(Error),
       );
       expect(service).toBeDefined();
       consoleErrorSpy.mockRestore();

packages/snap-account-service/src/SnapAccountService.ts

@@ -65,6 +65,7 @@ import { SnapId } from '@metamask/snaps-sdk';
 import type { Json } from '@metamask/utils';
 import { assertStruct } from '@metamask/utils';
 
+import { reportError, withSafeError } from './errors.js';
 import { projectLogger as log } from './logger.js';
 import type {
   SnapAccountServiceEnsureReadyAction,
@@ -343,18 +344,23 @@ export class SnapAccountService {
    */
   #handleUnlock(): void {
     // eslint-disable-next-line no-void
-    void this.ensureMigrated()
-      .then(async () => {
+    void withSafeError('Snap keyring v2 migration', () =>
+      this.ensureMigrated(),
+    ).then(
+      async () => {
         // If the migration is successful, we re-forward the current groups to each new keyrings!
         const groupId = this.#getSelectedAccountGroupId();
+        // NOTE: This cannot throw, all errors are swallowed under the hood, so we don't need to
+        // catch anything here.
         return await this.#forwardSelectedAccounts(
           groupId,
           this.#getAccountGroup(groupId)?.accounts,
         );
-      })
-      .catch((error) => {
-        console.error('Migration failed after unlock:', error);
-      });
+      },
+      (error) => {
+        reportError(this.#messenger, 'Migration failed after unlock', error);
+      },
+    );
   }
 
   /**
@@ -508,18 +514,24 @@ export class SnapAccountService {
         // accounts over.
         for (const state of states.values()) {
           log(`Migrating accounts for Snap "${state.snapId}"...`);
-          await controller.addNewKeyring(
-            // IMPORTANT: The Snap keyring (v2) can also be used as a v1
-            // keyring. So the builder associated with the v2 keyring type is
-            // able to build both v1 and v2 keyrings.
-            KeyringType.Snap,
-            state,
+          await withSafeError(
+            `Adding v2 Snap keyring for "${state.snapId}"`,
+            async () =>
+              controller.addNewKeyring(
+                // IMPORTANT: The Snap keyring (v2) can also be used as a v1
+                // keyring. So the builder associated with the v2 keyring type
+                // is able to build both v1 and v2 keyrings.
+                KeyringType.Snap,
+                state,
+              ),
           );
         }
 
         // Remove the legacy Snap keyring after migration.
         log('Removing legacy Snap keyring...');
-        await controller.removeKeyring(legacySnapKeyringEntry.metadata.id);
+        await withSafeError('Removing legacy Snap keyring', async () =>
+          controller.removeKeyring(legacySnapKeyringEntry.metadata.id),
+        );
 
         log('Migration completed!');
       },

packages/snap-account-service/src/SnapTracker.test.ts

@@ -347,5 +347,13 @@ describe('SnapTracker', () => {
 
       expect(tracker.getSnaps()).toStrictEqual([]);
     });
+
+    it('does nothing when a removal event fires for an untracked Snap', () => {
+      const { tracker, rootMessenger } = setup();
+
+      publishSnapDisabled(rootMessenger, buildSnap(MOCK_SNAP_ID, true));
+
+      expect(tracker.getSnaps()).toStrictEqual([]);
+    });
   });
 });