diff --git a/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.test.ts b/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.test.ts index 8ca6d59d..76553a0b 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.test.ts @@ -1,9 +1,14 @@ import type { GetPreferencesResult } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; -import { BaseError, ExternalServiceError, UserActionError } from '../entities'; +import { BaseError, ExternalServiceError } from '../entities'; import type { Logger, SnapClient, Translator } from '../entities'; -import { HandlerMiddleware, shouldTrackError } from './HandlerMiddleware'; +import { trackError } from '../utils/errors'; +import { HandlerMiddleware } from './HandlerMiddleware'; + +jest.mock('../utils/errors', () => ({ + trackError: jest.fn(), +})); describe('HandlerMiddleware', () => { const mockLogger = mock(); @@ -13,6 +18,7 @@ describe('HandlerMiddleware', () => { const mockTranslator = mock({ load: jest.fn(), }); + const mockTrackError = jest.mocked(trackError); const middleware = new HandlerMiddleware( mockLogger, @@ -22,33 +28,13 @@ describe('HandlerMiddleware', () => { beforeEach(() => { jest.clearAllMocks(); + mockTrackError.mockResolvedValue(undefined); mockSnapClient.getPreferences.mockResolvedValue({ locale: 'en', } as GetPreferencesResult); mockTranslator.load.mockResolvedValue({}); }); - describe('shouldTrackError', () => { - it('returns false for canceled confirmation errors', () => { - expect( - shouldTrackError( - new UserActionError('User canceled the confirmation'), - mockLogger, - ), - ).toBe(false); - }); - - it('returns true for other errors', () => { - expect(shouldTrackError(new Error('boom'), mockLogger)).toBe(true); - expect( - shouldTrackError( - new UserActionError('Another user action'), - mockLogger, - ), - ).toBe(true); - }); - }); - describe('handle', () => { it('executes the function successfully', async () => { const mockFn = jest.fn().mockResolvedValue('success'); @@ -66,6 +52,7 @@ describe('HandlerMiddleware', () => { expect(mockSnapClient.getPreferences).toHaveBeenCalled(); expect(mockTranslator.load).toHaveBeenCalledWith('en'); expect(mockLogger.error).toHaveBeenCalledWith(error); + expect(mockTrackError).toHaveBeenCalledWith(error); }); it('tracks an unexpected Error before rethrowing it as a SnapError', async () => { @@ -73,16 +60,16 @@ describe('HandlerMiddleware', () => { const mockFn = jest.fn().mockRejectedValue(error); await expect(middleware.handle(mockFn)).rejects.toThrow('tracked boom'); - expect(mockSnapClient.emitTrackingError).toHaveBeenCalledWith(error); + expect(mockTrackError).toHaveBeenCalledWith(error); }); - it('continues to throw a SnapError when emitTrackingError fails', async () => { + it('continues to throw a SnapError when trackError is invoked', async () => { const error = new Error('boom after tracking failure'); const mockFn = jest.fn().mockRejectedValue(error); await expect(middleware.handle(mockFn)).rejects.toThrow(error); - expect(mockSnapClient.emitTrackingError).toHaveBeenCalledWith(error); + expect(mockTrackError).toHaveBeenCalledWith(error); expect(mockSnapClient.getPreferences).toHaveBeenCalled(); }); @@ -91,6 +78,7 @@ describe('HandlerMiddleware', () => { await expect(middleware.handle(mockFn)).rejects.toThrow('string failure'); expect(mockLogger.error).toHaveBeenCalledWith('string failure'); + expect(mockTrackError).toHaveBeenCalledWith('string failure'); }); it('wraps a thrown plain object by stringifying it', async () => { @@ -101,6 +89,7 @@ describe('HandlerMiddleware', () => { '[object Object]', ); expect(mockLogger.error).toHaveBeenCalledWith(thrown); + expect(mockTrackError).toHaveBeenCalledWith(thrown); }); it('uses the message property if it exists on a thrown plain object', async () => { @@ -111,6 +100,7 @@ describe('HandlerMiddleware', () => { 'InsufficientFunds', ); expect(mockLogger.error).toHaveBeenCalledWith(thrown); + expect(mockTrackError).toHaveBeenCalledWith(thrown); }); it('handles error successfully if instance of BaseError', async () => { @@ -124,7 +114,7 @@ describe('HandlerMiddleware', () => { expect(mockSnapClient.getPreferences).toHaveBeenCalled(); expect(mockTranslator.load).toHaveBeenCalledWith('en'); expect(mockLogger.error).toHaveBeenCalledWith(error, error.data); - expect(mockSnapClient.emitTrackingError).toHaveBeenCalledWith(error); + expect(mockTrackError).toHaveBeenCalledWith(error); }); it('includes the concrete external service failure in the returned error message', async () => { @@ -139,7 +129,7 @@ describe('HandlerMiddleware', () => { await expect(middleware.handle(mockFn)).rejects.toThrow( 'Connection error: Failed to synchronize account', ); - expect(mockSnapClient.emitTrackingError).toHaveBeenCalledWith(error); + expect(mockTrackError).toHaveBeenCalledWith(error); }); }); }); diff --git a/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.ts b/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.ts index 99faf8df..2b7a7c55 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/HandlerMiddleware.ts @@ -26,24 +26,7 @@ import { WalletError, AssertionError, } from '../entities'; - -/** - * Determines whether an error should be reported through `snap_trackError`. - * - * @param error - The error to evaluate. - * @param logger - logger for error - * @returns `true` when the error should be tracked. - */ -export function shouldTrackError(error: unknown, logger: Logger): boolean { - try { - return !( - (error as UserActionError)?.message === 'User canceled the confirmation' - ); - } catch { - logger.error(error, 'Failed to determine if error should be tracked'); - return false; - } -} +import { trackError } from '../utils/errors'; export class HandlerMiddleware { readonly #logger: Logger; @@ -62,9 +45,7 @@ export class HandlerMiddleware { try { return await fn(); } catch (error) { - if (shouldTrackError(error, this.#logger)) { - await this.#snapClient.emitTrackingError(error as Error); - } + await trackError(error); const { locale } = await this.#snapClient.getPreferences(); const messages = await this.#translator.load(locale); diff --git a/packages/bitcoin-wallet-snap/src/index.ts b/packages/bitcoin-wallet-snap/src/index.ts index 92076b77..c0e6036c 100644 --- a/packages/bitcoin-wallet-snap/src/index.ts +++ b/packages/bitcoin-wallet-snap/src/index.ts @@ -1,5 +1,4 @@ import { handleKeyringRequest } from '@metamask/keyring-snap-sdk/v2'; -import { Logger } from '@metamask/snap-networks-utils'; import type { OnAssetsConversionHandler, OnAssetsLookupHandler, @@ -37,9 +36,9 @@ import { ConfirmationUseCases, SendFlowUseCases, } from './use-cases'; +import logger from './utils/logger'; // Infra layer -const logger = new Logger({ level: Config.logLevel }); const snapClient = new SnapClientAdapter(logger, Config.encrypt); const chainClient = new EsploraClientAdapter(Config.chain); const assetRatesClient = new PriceApiClientAdapter(Config.priceApi); diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts index a4498f87..95802e8b 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts @@ -4,6 +4,7 @@ import { mock } from 'jest-mock-extended'; import type { BitcoinAccount, Logger } from '../entities'; import { TrackingSnapEvent } from '../entities'; +import logger from '../utils/logger'; import { SnapClientAdapter } from './SnapClientAdapter'; jest.mock('@metamask/bitcoindevkit', () => ({ @@ -16,6 +17,13 @@ jest.mock('@metamask/bitcoindevkit', () => ({ }, })); +jest.mock('../utils/logger', () => ({ + __esModule: true, + default: { + error: jest.fn(), + }, +})); + const setupTest = () => { const mockLogger = mock(); const mockRequest = jest.fn(); @@ -27,10 +35,19 @@ const setupTest = () => { writable: true, }); - return { snapClient, mockLogger, mockRequest }; + return { + snapClient, + mockLogger, + mockRequest, + mockTrackingLogger: jest.mocked(logger), + }; }; describe('SnapClientAdapter', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + describe('emitTrackingEvent', () => { it("doesn't throw and logs when event tracking fails", async () => { const { snapClient, mockLogger, mockRequest } = setupTest(); @@ -79,7 +96,7 @@ describe('SnapClientAdapter', () => { describe('emitTrackingError', () => { it('sends the tracking error payload to the snap client', async () => { - const { snapClient, mockRequest } = setupTest(); + const { snapClient, mockRequest, mockTrackingLogger } = setupTest(); const error = new Error('boom'); mockRequest.mockResolvedValue(undefined); @@ -90,19 +107,25 @@ describe('SnapClientAdapter', () => { method: 'snap_trackError', params: { error: getJsonError(error) }, }); + expect(mockTrackingLogger.error).not.toHaveBeenCalled(); }); it("doesn't break execution when error tracking fails", async () => { - const { snapClient, mockLogger, mockRequest } = setupTest(); + const { snapClient, mockRequest, mockTrackingLogger } = setupTest(); const error = new Error('boom'); const trackingError = new Error('track failed'); mockRequest.mockRejectedValue(trackingError); expect(await snapClient.emitTrackingError(error)).toBeUndefined(); - expect(mockLogger.error).toHaveBeenCalledWith( + + expect(mockRequest).toHaveBeenCalledWith({ + method: 'snap_trackError', + params: { error: getJsonError(error) }, + }); + expect(mockTrackingLogger.error).toHaveBeenCalledWith( + { error: trackingError }, 'Failed to track error', - trackingError, ); }); }); diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts index d6efbe8a..955f1b57 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts @@ -12,7 +12,7 @@ import type { GetPreferencesResult, Json, } from '@metamask/snaps-sdk'; -import { DialogType, getJsonError } from '@metamask/snaps-sdk'; +import { DialogType } from '@metamask/snaps-sdk'; import type { BitcoinAccount, Logger, SnapClient } from '../entities'; import { @@ -27,6 +27,7 @@ import { networkToScope, } from '../handlers'; import { mapToTransaction } from '../handlers/mappings'; +import { trackError } from '../utils/errors'; export class SnapClientAdapter implements SnapClient { readonly #encrypt: boolean; @@ -286,14 +287,7 @@ export class SnapClientAdapter implements SnapClient { } async emitTrackingError(error: Error): Promise { - try { - await snap.request({ - method: 'snap_trackError', - params: { error: getJsonError(error) }, - }); - } catch (trackingError) { - this.#logger.error('Failed to track error', trackingError); - } + await trackError(error); } async startTrace(name: string): Promise { diff --git a/packages/bitcoin-wallet-snap/src/infra/getSnapProvider.ts b/packages/bitcoin-wallet-snap/src/infra/getSnapProvider.ts new file mode 100644 index 00000000..fffb00f1 --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/infra/getSnapProvider.ts @@ -0,0 +1,11 @@ +import type { SnapsProvider } from '@metamask/snaps-sdk'; + +/** + * Returns the Snap provider. + * + * @returns The Snap provider. + */ +export function getSnapProvider(): SnapsProvider { + // snap is a global variable provided by the Snap SDK + return snap; +} diff --git a/packages/bitcoin-wallet-snap/src/utils/errors.test.ts b/packages/bitcoin-wallet-snap/src/utils/errors.test.ts new file mode 100644 index 00000000..5cc8b5e4 --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/errors.test.ts @@ -0,0 +1,117 @@ +import { UserRejectedRequestError } from '@metamask/snaps-sdk'; + +import { UserActionError } from '../entities'; +import { shouldTrackError, trackError } from './errors'; +import logger from './logger'; + +jest.mock('./logger', () => ({ + error: jest.fn(), +})); + +type SetupTestResult = { + mockSnapRequest: jest.Mock; + mockLogger: jest.Mocked; +}; + +const setupTest = (): SetupTestResult => { + jest.clearAllMocks(); + + const mockSnapRequest = jest.fn(); + Object.defineProperty(globalThis, 'snap', { + configurable: true, + value: { request: mockSnapRequest }, + writable: true, + }); + + return { mockSnapRequest, mockLogger: jest.mocked(logger) }; +}; + +describe('errors', () => { + describe('shouldTrackError', () => { + it('returns false for canceled confirmation errors', () => { + expect( + shouldTrackError(new UserActionError('User canceled the confirmation')), + ).toBe(false); + }); + + it('returns false for UserRejectedRequestError', () => { + expect(shouldTrackError(new UserRejectedRequestError())).toBe(false); + }); + + it('returns true for other errors', () => { + expect(shouldTrackError(new Error('boom'))).toBe(true); + expect(shouldTrackError(new UserActionError('Another user action'))).toBe( + true, + ); + }); + + it('returns false and logs when error inspection fails', () => { + const { mockLogger } = setupTest(); + const brokenError = { + get message(): string { + throw new Error('broken getter'); + }, + }; + + expect(shouldTrackError(brokenError)).toBe(false); + expect(mockLogger.error).toHaveBeenCalledWith( + expect.objectContaining({ message: 'broken getter' }), + 'Failed to determine if error should be tracked', + ); + }); + }); + + describe('trackError', () => { + it('does not track canceled confirmation UserActionError', async () => { + const { mockSnapRequest } = setupTest(); + + expect( + await trackError(new UserActionError('User canceled the confirmation')), + ).toBeUndefined(); + + expect(mockSnapRequest).not.toHaveBeenCalled(); + }); + + it('does not throw if error tracking fails', async () => { + const { mockLogger, mockSnapRequest } = setupTest(); + + const originalError = new Error('Test error'); + const trackingError = new Error('Tracking failed'); + mockSnapRequest.mockRejectedValue(trackingError); + + expect(await trackError(originalError)).toBeUndefined(); + + expect(mockSnapRequest).toHaveBeenCalledWith({ + method: 'snap_trackError', + params: { + error: expect.objectContaining({ + message: originalError.message, + }), + }, + }); + expect(mockLogger.error).toHaveBeenCalledWith( + { error: trackingError }, + 'Failed to track error', + ); + }); + + it('tracks errors', async () => { + const { mockLogger, mockSnapRequest } = setupTest(); + + const originalError = new Error('Test error'); + mockSnapRequest.mockResolvedValue('tracked-error-id'); + + expect(await trackError(originalError)).toBe('tracked-error-id'); + + expect(mockSnapRequest).toHaveBeenCalledWith({ + method: 'snap_trackError', + params: { + error: expect.objectContaining({ + message: originalError.message, + }), + }, + }); + expect(mockLogger.error).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/packages/bitcoin-wallet-snap/src/utils/errors.ts b/packages/bitcoin-wallet-snap/src/utils/errors.ts new file mode 100644 index 00000000..82c4d14f --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/errors.ts @@ -0,0 +1,44 @@ +import { createTrackError } from '@metamask/snap-networks-utils'; +import { UserRejectedRequestError } from '@metamask/snaps-sdk'; + +import { UserActionError } from '../entities'; +import { getSnapProvider } from '../infra/getSnapProvider'; +import logger from './logger'; + +/** + * Determines whether an error should be reported through `snap_trackError`. + * + * @param error - The error to evaluate. + * @returns `true` when the error should be tracked. + */ +export function shouldTrackError(error: unknown): boolean { + try { + if (error instanceof UserRejectedRequestError) { + return false; + } + + return !( + (error as UserActionError)?.message === 'User canceled the confirmation' + ); + } catch (checkError) { + logger.error(checkError, 'Failed to determine if error should be tracked'); + return false; + } +} + +/** + * Tracks an error in MetaMask via Sentry (`snap_trackError`). + * + * Skips errors that {@link shouldTrackError} filters out. RPC failures are + * caught and logged but never rethrown, so this is safe to call from + * already-failing error-handling paths without masking the original failure. + * + * @param error - The error to report to Sentry. + * @returns The Sentry event ID on success, or `undefined` on failure or if the + * error is skipped. + */ +export const trackError = createTrackError({ + getSnapProvider, + logError: logger.error.bind(logger), + shouldTrack: shouldTrackError, +}); diff --git a/packages/bitcoin-wallet-snap/src/utils/logger.ts b/packages/bitcoin-wallet-snap/src/utils/logger.ts new file mode 100644 index 00000000..de72f67b --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/utils/logger.ts @@ -0,0 +1,7 @@ +import { Logger } from '@metamask/snap-networks-utils'; + +import { Config } from '../config'; + +const logger = new Logger({ level: Config.logLevel }); + +export default logger;