fix(bitcoin-wallet-snap): repair drifted wallets with a one-time full rescan

packages/bitcoin-wallet-snap/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Run a one-time full scan of every existing account after the update, so funds on previously unwatched addresses are found ([#226](https://github.com/MetaMask/internal-snaps/pull/226))
+- Emit a `Scan Discovered Missed Transactions` tracking event when the repair scan finds transactions that routine sync did not know about ([#226](https://github.com/MetaMask/internal-snaps/pull/226))
+
 ### Changed
 
 - Split the chain `stopGap` configuration into `{ discovery: 5, scan: 20 }` so account discovery keeps the cheap probe while full account scans use the BIP44 gap limit ([#224](https://github.com/MetaMask/internal-snaps/pull/224))

packages/bitcoin-wallet-snap/src/entities/snap.ts

@@ -51,6 +51,7 @@ export const TrackingSnapEvent = {
   TransactionReceived: 'Transaction Received',
   TransactionReorged: 'Transaction Reorged',
   TransactionSubmitted: 'Transaction Submitted',
+  ScanDiscoveredMissedTransactions: 'Scan Discovered Missed Transactions',
 } as const;
 
 export type TrackingSnapEvent =

packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts

@@ -32,6 +32,9 @@ describe('CronHandler', () => {
       clientVersion: '1.0.0',
       platformVersion: '1.0.0',
     });
+    // Default the one-time rescan to already-done so existing tests don't
+    // trigger background-event scheduling.
+    mockSnapClient.getState.mockResolvedValue(true);
   });
 
   describe('synchronizeAccounts', () => {
@@ -148,6 +151,63 @@ describe('CronHandler', () => {
         mockSnapClient.emitAccountBalancesUpdatedEvent,
       ).toHaveBeenCalledWith([mockAccounts[0]]);
     });
+
+    describe('one-time rescan repair', () => {
+      beforeEach(() => {
+        (getSelectedAccounts as jest.Mock).mockResolvedValue([
+          'account-1',
+          'account-2',
+        ]);
+        mockAccountUseCases.list.mockResolvedValue(mockAccounts);
+        mockAccountUseCases.synchronize.mockResolvedValue({
+          account: mockAccount1,
+          transactionsToNotify: [],
+        });
+      });
+
+      it('schedules one full scan per existing account and marks the repair done when not yet run', async () => {
+        mockSnapClient.getState.mockResolvedValue(null);
+
+        await handler.route(request);
+
+        expect(mockSnapClient.getState).toHaveBeenCalledWith('rescanV1');
+        expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledTimes(
+          mockAccounts.length,
+        );
+        expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({
+          duration: 'PT5S',
+          method: CronMethod.FullScanAccount,
+          params: { accountId: 'account-1', trackMissed: true },
+        });
+        expect(mockSnapClient.scheduleBackgroundEvent).toHaveBeenCalledWith({
+          duration: 'PT5S',
+          method: CronMethod.FullScanAccount,
+          params: { accountId: 'account-2', trackMissed: true },
+        });
+        expect(mockSnapClient.setState).toHaveBeenCalledWith('rescanV1', true);
+
+        // Scheduling happens before the state is marked done.
+        const scheduleOrder =
+          mockSnapClient.scheduleBackgroundEvent.mock.invocationCallOrder[0];
+        const setStateOrder =
+          mockSnapClient.setState.mock.invocationCallOrder[0];
+        expect(scheduleOrder).toBeLessThan(setStateOrder as number);
+
+        // The normal sync flow still runs afterwards.
+        expect(mockAccountUseCases.synchronize).toHaveBeenCalled();
+      });
+
+      it('does not schedule or update state when the repair already ran', async () => {
+        mockSnapClient.getState.mockResolvedValue(true);
+
+        await handler.route(request);
+
+        expect(mockSnapClient.getState).toHaveBeenCalledWith('rescanV1');
+        expect(mockSnapClient.scheduleBackgroundEvent).not.toHaveBeenCalled();
+        expect(mockSnapClient.setState).not.toHaveBeenCalled();
+        expect(mockAccountUseCases.synchronize).toHaveBeenCalled();
+      });
+    });
   });
 
   describe('refreshRates', () => {
@@ -349,5 +409,70 @@ describe('CronHandler', () => {
 
       await expect(handler.route(request)).rejects.toThrow(error);
     });
+
+    describe('trackMissed', () => {
+      const buildTx = (txid: string): WalletTx =>
+        mock<WalletTx>({
+          txid: mock<WalletTx['txid']>({ toString: () => txid }),
+        });
+
+      const trackMissedRequest = {
+        method: CronMethod.FullScanAccount,
+        params: { accountId: 'account-1', trackMissed: true },
+      } as unknown as JsonRpcRequest;
+
+      it('passes trackMissed through from the route params', async () => {
+        mockAccountUseCases.get.mockResolvedValue(mockAccount);
+        mockAccountUseCases.fullScan.mockResolvedValue({
+          account: mockAccount,
+          transactionsToNotify: [],
+        });
+        mockAccount.listTransactions.mockReturnValue([]);
+
+        await handler.route(trackMissedRequest);
+
+        expect(mockAccountUseCases.get).toHaveBeenCalledWith('account-1');
+        // Called once for the before-scan set and once for the after-scan set,
+        // proving trackMissed was honored.
+        expect(mockAccount.listTransactions).toHaveBeenCalledTimes(2);
+      });
+
+      it('emits a tracking event only for transactions discovered by the scan', async () => {
+        const txBefore = buildTx('txid-existing');
+        const txNew = buildTx('txid-new');
+
+        mockAccountUseCases.get.mockResolvedValue(mockAccount);
+        mockAccountUseCases.fullScan.mockResolvedValue({
+          account: mockAccount,
+          transactionsToNotify: [],
+        });
+        mockAccount.listTransactions
+          .mockReturnValueOnce([txBefore])
+          .mockReturnValueOnce([txBefore, txNew]);
+
+        await handler.route(trackMissedRequest);
+
+        expect(mockSnapClient.emitTrackingEvent).toHaveBeenCalledTimes(1);
+        expect(mockSnapClient.emitTrackingEvent).toHaveBeenCalledWith(
+          'Scan Discovered Missed Transactions',
+          mockAccount,
+          txNew,
+          'cron',
+        );
+      });
+
+      it('never emits the tracking event when trackMissed is false or undefined', async () => {
+        mockAccountUseCases.get.mockResolvedValue(mockAccount);
+        mockAccountUseCases.fullScan.mockResolvedValue({
+          account: mockAccount,
+          transactionsToNotify: [],
+        });
+        mockAccount.listTransactions.mockReturnValue([buildTx('txid-new')]);
+
+        await handler.route(request);
+
+        expect(mockSnapClient.emitTrackingEvent).not.toHaveBeenCalled();
+      });
+    });
   });
 });

packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts

@@ -1,9 +1,10 @@
 import { getSelectedAccounts } from '@metamask/keyring-snap-sdk';
-import type { JsonRpcRequest, SnapsProvider } from '@metamask/snaps-sdk';
-import { array, assert, object, string } from 'superstruct';
+import type { Json, JsonRpcRequest, SnapsProvider } from '@metamask/snaps-sdk';
+import { array, assert, boolean, object, optional, string } from 'superstruct';
 
 import { InexistentMethodError, SynchronizationError } from '../entities';
-import type { SnapClient, SyncResult } from '../entities';
+import type { BitcoinAccount, SnapClient, SyncResult } from '../entities';
+import { TrackingSnapEvent } from '../entities';
 import type { SendFlowUseCases, AccountUseCases } from '../use-cases';
 
 export const CronMethod = {
@@ -25,6 +26,7 @@ export const SyncSelectedAccountsRequest = object({
 
 export const FullScanAccountRequest = object({
   accountId: string(),
+  trackMissed: optional(boolean()),
 });
 
 export class CronHandler {
@@ -70,14 +72,26 @@ export class CronHandler {
       }
       case CronMethod.FullScanAccount: {
         assert(params, FullScanAccountRequest);
-        return this.fullScanAccount(params.accountId);
+        return this.fullScanAccount(params.accountId, params.trackMissed);
       }
       default:
         throw new InexistentMethodError(`Method not found: ${method}`);
     }
   }
 
   async synchronizeAccounts(): Promise<void> {
+    if ((await this.#snapClient.getState('rescanV1')) !== true) {
+      const allAccounts = await this.#accountsUseCases.list();
+      for (const account of allAccounts) {
+        await this.#snapClient.scheduleBackgroundEvent({
+          duration: 'PT5S',
+          method: CronMethod.FullScanAccount,
+          params: { accountId: account.id, trackMissed: true },
+        });
+      }
+      await this.#snapClient.setState('rescanV1', true);
+    }
+
     const selectedAccounts: Set<string> = new Set(
       await getSelectedAccounts(this.#snap),
     );
@@ -92,30 +106,43 @@ export class CronHandler {
       ),
     );
 
-    const successfulResults: SyncResult[] = [];
+    await this.#finishSync(
+      accounts,
+      results,
+      'Account synchronization failures',
+    );
+  }
 
-    // TODO: Replace `any` with type
-    // eslint-disable-next-line @typescript-eslint/no-explicit-any
-    const errors: Record<string, any> = {};
+  /**
+   * Aggregate settled sync results, emit events for successes, and throw for failures.
+   *
+   * @param accounts - The accounts that were synchronized, in the same order as `results`.
+   * @param results - The settled synchronization results.
+   * @param message - The error message to use if any synchronization failed.
+   */
+  async #finishSync(
+    accounts: BitcoinAccount[],
+    results: PromiseSettledResult<SyncResult>[],
+    message: string,
+  ): Promise<void> {
+    const successfulResults: SyncResult[] = [];
+    const errors: Record<string, Json> = {};
 
     results.forEach((result, index) => {
       if (result.status === 'fulfilled') {
         successfulResults.push(result.value);
       } else {
         const id = accounts[index]?.id;
         if (id) {
-          errors[id] = result.reason;
+          errors[id] = String(result.reason);
         }
       }
     });
 
     await this.#emitSyncEvents(successfulResults);
 
     if (Object.keys(errors).length > 0) {
-      throw new SynchronizationError(
-        'Account synchronization failures',
-        errors,
-      );
+      throw new SynchronizationError(message, errors);
     }
   }
 
@@ -183,10 +210,31 @@ export class CronHandler {
     }
   }
 
-  async fullScanAccount(accountId: string): Promise<void> {
+  async fullScanAccount(
+    accountId: string,
+    trackMissed?: boolean,
+  ): Promise<void> {
     const account = await this.#accountsUseCases.get(accountId);
+
+    const before = trackMissed
+      ? new Set(account.listTransactions().map((tx) => tx.txid.toString()))
+      : undefined;
+
     const result = await this.#accountsUseCases.fullScan(account);
 
+    if (before) {
+      for (const tx of account.listTransactions()) {
+        if (!before.has(tx.txid.toString())) {
+          await this.#snapClient.emitTrackingEvent(
+            TrackingSnapEvent.ScanDiscoveredMissedTransactions,
+            account,
+            tx,
+            'cron',
+          );
+        }
+      }
+    }
+
     await this.#emitSyncEvents([result]);
   }
 }