diff --git a/packages/multichain-account-service/CHANGELOG.md b/packages/multichain-account-service/CHANGELOG.md index d45eac14706..b35b4d4b9a6 100644 --- a/packages/multichain-account-service/CHANGELOG.md +++ b/packages/multichain-account-service/CHANGELOG.md @@ -25,6 +25,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Batch account creation with single `keyring.addAccounts` call. - Fetch all accounts in single `AccountsController:getAccounts` call instead of multiple `getAccount` calls. - Significantly reduces lock acquisitions and API calls for batch operations. +- Do not report `TimeoutError` errors ([#8249](https://github.com/MetaMask/core/pull/8249)) + - All other kind of errors are still reported as usual. ### Removed diff --git a/packages/multichain-account-service/src/MultichainAccountService.test.ts b/packages/multichain-account-service/src/MultichainAccountService.test.ts index 47b540dc0ae..51ec2fa8c67 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.test.ts @@ -15,6 +15,7 @@ import type { EthKeyring } from '@metamask/keyring-internal-api'; import type { MultichainAccountServiceOptions } from './MultichainAccountService'; import { MultichainAccountService } from './MultichainAccountService'; import type { Bip44AccountProvider } from './providers'; +import { TimeoutError } from './providers'; import { AccountProviderWrapper } from './providers/AccountProviderWrapper'; import { EVM_ACCOUNT_PROVIDER_NAME, @@ -1142,6 +1143,31 @@ describe('MultichainAccountService', () => { providerError, ); }); + + it('does not capture exception when provider throws a TimeoutError', async () => { + const rootMessenger = getRootMessenger(); + const captureExceptionSpy = jest.spyOn(rootMessenger, 'captureException'); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + const { service, mocks } = await setup({ + rootMessenger, + accounts: [MOCK_HD_ACCOUNT_1], + }); + + mocks.SolAccountProvider.resyncAccounts.mockRejectedValue( + new TimeoutError('Timed out after: 500ms'), + ); + + await service.resyncAccounts(); // Should not throw. + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(consoleWarnSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + + consoleWarnSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + }); }); describe('setBasicFunctionality', () => { diff --git a/packages/multichain-account-service/src/MultichainAccountService.ts b/packages/multichain-account-service/src/MultichainAccountService.ts index 874a9cdf8a0..5cf9ae3ea00 100644 --- a/packages/multichain-account-service/src/MultichainAccountService.ts +++ b/packages/multichain-account-service/src/MultichainAccountService.ts @@ -13,6 +13,7 @@ import type { InternalAccount } from '@metamask/keyring-internal-api'; import { areUint8ArraysEqual, assert } from '@metamask/utils'; import { traceFallback } from './analytics'; +import { reportError } from './errors'; import { projectLogger as log } from './logger'; import type { MultichainAccountGroup } from './MultichainAccountGroup'; import { MultichainAccountWallet } from './MultichainAccountWallet'; @@ -36,7 +37,6 @@ import type { MultichainAccountServiceConfig, MultichainAccountServiceMessenger, } from './types'; -import { createSentryError } from './utils'; export const serviceName = 'MultichainAccountService'; @@ -298,14 +298,14 @@ export class MultichainAccountService { try { await provider.resyncAccounts(accounts); } catch (error) { - const errorMessage = `Unable to re-sync provider "${provider.getName()}"`; - log(errorMessage); - console.error(errorMessage); - - const sentryError = createSentryError(errorMessage, error as Error, { - provider: provider.getName(), - }); - this.#messenger.captureException?.(sentryError); + reportError( + this.#messenger, + `Unable to re-sync provider "${provider.getName()}"`, + error, + { + provider: provider.getName(), + }, + ); } }), ); diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts index ff103d4446e..a2ca44a9b77 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.test.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.test.ts @@ -17,6 +17,7 @@ import type { InternalAccount } from '@metamask/keyring-internal-api'; import type { WalletState } from './MultichainAccountWallet'; import { MultichainAccountWallet } from './MultichainAccountWallet'; +import { TimeoutError } from './providers'; import type { MockAccountProvider, RootMessenger } from './tests'; import { MOCK_HD_ACCOUNT_1, @@ -307,6 +308,28 @@ describe('MultichainAccountWallet', () => { ); }); + it('does not capture exception when a provider times out creating accounts', async () => { + const groupIndex = 1; + const { wallet, providers, messenger } = setup({ + accounts: [[MOCK_HD_ACCOUNT_1]], + }); + const [provider] = providers; + provider.createAccounts.mockRejectedValueOnce( + new TimeoutError('Timed out after: 500ms'), + ); + const captureExceptionSpy = jest.spyOn(messenger, 'captureException'); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + await expect( + wallet.createMultichainAccountGroup(groupIndex), + ).rejects.toThrow('Timed out after: 500ms'); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(consoleWarnSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + }); + it('defers non-EVM account creation to alignment after group creation (waitForAllProvidersToFinishCreatingAccounts = false)', async () => { const groupIndex = 1; @@ -601,6 +624,31 @@ describe('MultichainAccountWallet', () => { ); }); + it('does not capture exception when a provider times out creating accounts in batch', async () => { + const { wallet, providers, messenger } = setup({ + accounts: [[]], + }); + + const [evmProvider] = providers; + evmProvider.createAccounts.mockRejectedValueOnce( + new TimeoutError('Timed out after: 500ms'), + ); + + const captureExceptionSpy = jest.spyOn(messenger, 'captureException'); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + + await expect( + wallet.createMultichainAccountGroups({ to: 2 }), + ).rejects.toThrow('Timed out after: 500ms'); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(consoleWarnSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + }); + it('creates accounts for all providers synchronously when waitForAllProvidersToFinishCreatingAccounts is true', async () => { const { wallet, providers } = setup({ accounts: [[], []], @@ -1062,8 +1110,11 @@ describe('MultichainAccountWallet', () => { // Thrown provider should have been called once and not rescheduled expect(providers[0].discoverAccounts).toHaveBeenCalledTimes(1); - expect(consoleSpy).toHaveBeenCalledWith(expect.any(Error)); - expect((consoleSpy.mock.calls[0][0] as Error).message).toBe( + expect(consoleSpy).toHaveBeenCalledWith( + expect.any(String), + expect.any(Error), + ); + expect((consoleSpy.mock.calls[0][1] as Error).message).toBe( 'Failed to discover accounts', ); @@ -1082,12 +1133,33 @@ describe('MultichainAccountWallet', () => { providers[1].discoverAccounts.mockResolvedValueOnce([]); await wallet.discoverAccounts(); expect(captureExceptionSpy).toHaveBeenCalledWith( - new Error('Unable to discover accounts'), + new Error( + 'Unable to discover accounts with provider "Mocked Provider 0"', + ), ); expect(captureExceptionSpy.mock.lastCall[0]).toHaveProperty( 'cause', providerError, ); }); + + it('does not capture exception when a provider times out during account discovery', async () => { + const { wallet, providers, messenger } = setup({ + accounts: [[], []], + }); + providers[0].discoverAccounts.mockRejectedValueOnce( + new TimeoutError('Timed out after: 500ms'), + ); + const captureExceptionSpy = jest.spyOn(messenger, 'captureException'); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + providers[1].discoverAccounts.mockResolvedValueOnce([]); + await wallet.discoverAccounts(); + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(consoleWarnSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + }); }); }); diff --git a/packages/multichain-account-service/src/MultichainAccountWallet.ts b/packages/multichain-account-service/src/MultichainAccountWallet.ts index d726874c7d7..eb71ee60b23 100644 --- a/packages/multichain-account-service/src/MultichainAccountWallet.ts +++ b/packages/multichain-account-service/src/MultichainAccountWallet.ts @@ -17,6 +17,7 @@ import type { EntropySourceId, KeyringAccount } from '@metamask/keyring-api'; import { assert } from '@metamask/utils'; import { Mutex } from 'async-mutex'; +import { reportError } from './errors'; import type { Logger } from './logger'; import { createModuleLogger, @@ -33,7 +34,6 @@ import type { MultichainAccountServiceMessenger } from './types'; import { assertGroupIndexIsValid, assertGroupIndexRangeIsValid, - createSentryError, GroupIndexRange, toErrorMessage, } from './utils'; @@ -243,29 +243,16 @@ export class MultichainAccountWallet< }, }); } catch (error) { - const modeDescription = isBatching - ? 'some accounts (batch)' - : 'some accounts'; - const rangeDescription = isBatching - ? `from group index ${from} to ${to}` - : `for group index ${to}`; - - const errorMessage = `Unable to create ${modeDescription} ${rangeDescription} with provider "${provider.getName()}". Error: ${(error as Error).message}`; - this.#log(`${ERROR_PREFIX} ${errorMessage}:`, error); - - const sentryError = createSentryError( - `Unable to create ${modeDescription} with provider "${provider.getName()}"`, - error as Error, + reportError( + this.#messenger, + `Unable to create ${isBatching ? 'some accounts (batch)' : 'some accounts'} with provider "${provider.getName()}"`, + error, { - range: { - from, - to, - }, + range: { from, to }, provider: provider.getName(), isBatching, }, ); - this.#messenger.captureException?.(sentryError); throw error; } } @@ -751,23 +738,23 @@ export class MultichainAccountWallet< }); } catch (error) { context.stopped = true; - console.error(error); + log( message( - `failed (with: "${(error as Error).message}")`, + `failed (with: "${toErrorMessage(error)}")`, targetGroupIndex, ), - error, ); - const sentryError = createSentryError( - 'Unable to discover accounts', - error as Error, + + reportError( + this.#messenger, + `Unable to discover accounts with provider "${providerName}"`, + error, { provider: providerName, groupIndex: targetGroupIndex, }, ); - this.#messenger.captureException?.(sentryError); break; } diff --git a/packages/multichain-account-service/src/errors.ts b/packages/multichain-account-service/src/errors.ts new file mode 100644 index 00000000000..3a35d6a70b4 --- /dev/null +++ b/packages/multichain-account-service/src/errors.ts @@ -0,0 +1,33 @@ +import { logErrorAs } from './logger'; +import { isTimeoutError } from './providers/utils'; +import { createSentryError } from './utils'; + +/** + * Reports an error by logging it and optionally capturing it in Sentry. + * + * Timeout errors are treated as warnings (not reported to Sentry). All other + * errors are logged as errors and captured via `captureException`. + * + * @param messenger - Object with an optional `captureException` method. + * @param messenger.captureException - Optional method to capture exceptions in Sentry. + * @param message - The static message describing what failed. + * @param error - The caught error. + * @param context - Optional context to attach to the Sentry error. + */ +export function reportError( + messenger: { captureException?: (error: Error) => void }, + message: string, + error: unknown, + context?: Record, +): void { + if (isTimeoutError(error)) { + logErrorAs('warn', message, error); + console.warn(message, error); + } else { + logErrorAs('error', message, error); + console.error(message, error); + + const sentryError = createSentryError(message, error as Error, context); + messenger.captureException?.(sentryError); + } +} diff --git a/packages/multichain-account-service/src/logger.ts b/packages/multichain-account-service/src/logger.ts index 03e92506d50..76dc8f6592f 100644 --- a/packages/multichain-account-service/src/logger.ts +++ b/packages/multichain-account-service/src/logger.ts @@ -1,5 +1,7 @@ import { createProjectLogger, createModuleLogger } from '@metamask/utils'; +import { toErrorMessage } from './utils'; + export const projectLogger = createProjectLogger('multichain-account-service'); export { createModuleLogger }; @@ -8,3 +10,19 @@ export const WARNING_PREFIX = 'WARNING --'; export const ERROR_PREFIX = 'ERROR --'; export type Logger = typeof projectLogger; + +/** + * Logs an error with either WARNING or ERROR prefix, appending the error message. + * + * @param level - 'warn' for WARNING prefix, 'error' for ERROR prefix. + * @param message - The static message describing what failed. + * @param error - The caught error. + */ +export function logErrorAs( + level: 'warn' | 'error', + message: string, + error: unknown, +): void { + const prefix = level === 'warn' ? WARNING_PREFIX : ERROR_PREFIX; + projectLogger(`${prefix} ${message}: ${toErrorMessage(error)}`); +} diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts index cc00eccf594..05721d34d57 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.test.ts @@ -28,6 +28,7 @@ import { } from './SnapAccountProvider'; import { SolAccountProvider } from './SolAccountProvider'; import { TrxAccountProvider } from './TrxAccountProvider'; +import { TimeoutError } from './utils'; import { traceFallback } from '../analytics'; import type { DeepPartial, RootMessenger } from '../tests'; import { @@ -799,6 +800,28 @@ describe('SnapAccountProvider', () => { ); }); + it('does not capture exception when deleteAccount times out', async () => { + const { provider, messenger, mocks } = setup({ + accounts: mockAccounts, + config: { resyncAccounts: { autoRemoveExtraSnapAccounts: true } }, + }); + + const captureExceptionSpy = jest.spyOn(messenger, 'captureException'); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + mocks.SnapController.handleKeyringRequest.deleteAccount.mockRejectedValue( + new TimeoutError('Timed out after: 500ms'), + ); + + await provider.resyncAccounts([mockAccounts[0]]); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(consoleWarnSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + }); + it('does not delete accounts that exist in both Snap and MetaMask', async () => { const { provider, mocks } = setup({ accounts: mockAccounts }); @@ -902,13 +925,33 @@ describe('SnapAccountProvider', () => { expect(createAccountsSpy).toHaveBeenCalled(); expect(captureExceptionSpy).toHaveBeenCalledWith( - new Error('Unable to re-sync account: 0'), + new Error('Unable to re-sync accounts'), ); expect(captureExceptionSpy.mock.lastCall[0]).toHaveProperty( 'cause', providerError, ); }); + + it('does not capture exception when re-sync times out', async () => { + const { provider, messenger } = setup({ accounts: [mockAccounts[0]] }); + + const captureExceptionSpy = jest.spyOn(messenger, 'captureException'); + const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation(); + const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(); + const createAccountsSpy = jest.spyOn(provider, 'createAccounts'); + createAccountsSpy.mockRejectedValue( + new TimeoutError('Timed out after: 500ms'), + ); + + await provider.resyncAccounts(mockAccounts); + + expect(captureExceptionSpy).not.toHaveBeenCalled(); + expect(consoleWarnSpy).toHaveBeenCalled(); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + consoleWarnSpy.mockRestore(); + consoleErrorSpy.mockRestore(); + }); }); describe('ensureCanUseSnapPlatform', () => { diff --git a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts index 58a9e2e4407..87b0fc80fc7 100644 --- a/packages/multichain-account-service/src/providers/SnapAccountProvider.ts +++ b/packages/multichain-account-service/src/providers/SnapAccountProvider.ts @@ -24,9 +24,9 @@ import { Semaphore } from 'async-mutex'; import { BaseBip44AccountProvider } from './BaseBip44AccountProvider'; import { withTimeout } from './utils'; import { traceFallback } from '../analytics'; +import { reportError } from '../errors'; import { projectLogger as log, WARNING_PREFIX } from '../logger'; import type { MultichainAccountServiceMessenger } from '../types'; -import { createSentryError } from '../utils'; export type RestrictedSnapKeyring = { createAccount: (options: Record) => Promise; @@ -230,12 +230,15 @@ export abstract class SnapAccountProvider extends BaseBip44AccountProvider { snapAccounts.delete(snapAccountId); } } catch (error) { - const sentryError = createSentryError( + reportError( + this.messenger, `Unable to delete de-synced Snap account: ${this.snapId}`, - error as Error, - { provider: this.getName(), snapAccountId }, + error, + { + provider: this.getName(), + snapAccountId, + }, ); - this.messenger.captureException?.(sentryError); } }), ); @@ -269,15 +272,10 @@ export abstract class SnapAccountProvider extends BaseBip44AccountProvider { }); } } catch (error) { - const sentryError = createSentryError( - `Unable to re-sync account: ${groupIndex}`, - error as Error, - { - provider: this.getName(), - groupIndex, - }, - ); - this.messenger.captureException?.(sentryError); + reportError(this.messenger, 'Unable to re-sync accounts', error, { + provider: this.getName(), + groupIndex, + }); } }), ); diff --git a/packages/multichain-account-service/src/providers/index.ts b/packages/multichain-account-service/src/providers/index.ts index f482c41d922..0504f99fea6 100644 --- a/packages/multichain-account-service/src/providers/index.ts +++ b/packages/multichain-account-service/src/providers/index.ts @@ -3,7 +3,7 @@ export * from './SnapAccountProvider'; export * from './AccountProviderWrapper'; // Errors that can bubble up outside of provider calls. -export { TimeoutError } from './utils'; +export { TimeoutError, isTimeoutError } from './utils'; // Concrete providers: export * from './SolAccountProvider'; diff --git a/packages/multichain-account-service/src/providers/utils.test.ts b/packages/multichain-account-service/src/providers/utils.test.ts index 1108ce96d77..cea24ca7854 100644 --- a/packages/multichain-account-service/src/providers/utils.test.ts +++ b/packages/multichain-account-service/src/providers/utils.test.ts @@ -1,4 +1,4 @@ -import { TimeoutError, withRetry, withTimeout } from './utils'; +import { TimeoutError, isTimeoutError, withRetry, withTimeout } from './utils'; describe('utils', () => { it('retries RPC request up to 3 times if it fails and throws the last error', async () => { @@ -33,4 +33,29 @@ describe('utils', () => { ), ).rejects.toThrow(TimeoutError); }); + + it('includes the timeout duration in the error message', async () => { + await expect( + withTimeout( + new Promise((resolve) => { + setTimeout(() => { + resolve(null); + }, 600); + }), + 500, + ), + ).rejects.toThrow('Timed out after: 500ms'); + }); + + it('isTimeoutError returns true for TimeoutError instances', () => { + expect(isTimeoutError(new TimeoutError('Timed out after: 500ms'))).toBe( + true, + ); + }); + + it('isTimeoutError returns false for non-TimeoutError instances', () => { + expect(isTimeoutError(new Error('some error'))).toBe(false); + expect(isTimeoutError('string')).toBe(false); + expect(isTimeoutError(null)).toBe(false); + }); }); diff --git a/packages/multichain-account-service/src/providers/utils.ts b/packages/multichain-account-service/src/providers/utils.ts index 8671da48b8f..6ead4026886 100644 --- a/packages/multichain-account-service/src/providers/utils.ts +++ b/packages/multichain-account-service/src/providers/utils.ts @@ -6,6 +6,16 @@ export class TimeoutError extends Error { } } +/** + * Check if an error is a `TimeoutError`. + * + * @param error - The error to check. + * @returns `true` if the error is a `TimeoutError`, otherwise `false`. + */ +export function isTimeoutError(error: unknown): error is TimeoutError { + return error instanceof TimeoutError; +} + /** * Execute a function with exponential backoff on transient failures. * @@ -58,7 +68,7 @@ export async function withTimeout( promise, new Promise((_resolve, reject) => { timer = setTimeout( - () => reject(new TimeoutError('Timed out')), + () => reject(new TimeoutError(`Timed out after: ${timeoutMs}ms`)), timeoutMs, ); }),