diff --git a/packages/snap-networks-utils/CHANGELOG.md b/packages/snap-networks-utils/CHANGELOG.md index b6778d0ca..9003346f7 100644 --- a/packages/snap-networks-utils/CHANGELOG.md +++ b/packages/snap-networks-utils/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Add `SynchronizationError`, `formatAccountSyncFailures`, and the `AccountSyncFailure` type, for reporting account synchronization failures with per-account failure details embedded in the error message (details must live in the message because `snap_trackError` only serializes `name`, `message`, `stack`, and `cause`). ([#XXX](https://github.com/MetaMask/internal-snaps/pull/XXX)) - Add `wrapSnapHandlers` to wrap any Snap entrypoint handlers with `withCatchAndThrowSnapError`, with optional per-handler `logError` overrides ([#341](https://github.com/MetaMask/internal-snaps/pull/341)) - Add `noopAssetHandlers`, the no-op asset entrypoints network Snaps must export to keep the `endowment:assets` permission ([#341](https://github.com/MetaMask/internal-snaps/pull/341)) - Add a shared `AnalyticsService` and typed event properties for network Snap telemetry. ([#327](https://github.com/MetaMask/internal-snaps/pull/327)) diff --git a/packages/snap-networks-utils/src/index.ts b/packages/snap-networks-utils/src/index.ts index 105ce2fdd..4d43ea085 100644 --- a/packages/snap-networks-utils/src/index.ts +++ b/packages/snap-networks-utils/src/index.ts @@ -104,8 +104,12 @@ export { createSnapErrorHandling, createTrackError, createWithCatchAndThrowSnapError, + formatAccountSyncFailures, + getSyncFailuresFromSettledResult, isSnapRpcError, normalizeError, + stringifyReason, + SynchronizationError, } from './utils/errors'; export { InFlightCoalescer } from './utils/dedupe/InFlightCoalescer'; export { InMemoryCache } from './utils/cache/InMemoryCache'; @@ -137,6 +141,7 @@ export type { WithCatchAndThrowSnapError, } from './utils/handlers/wrapSnapHandlers'; export type { + AccountSyncFailure, CreateSnapErrorHandlingOptions, CreateTrackErrorOptions, CreateWithCatchAndThrowSnapErrorOptions, diff --git a/packages/snap-networks-utils/src/utils/errors/index.ts b/packages/snap-networks-utils/src/utils/errors/index.ts index f057effee..de11f671c 100644 --- a/packages/snap-networks-utils/src/utils/errors/index.ts +++ b/packages/snap-networks-utils/src/utils/errors/index.ts @@ -1,5 +1,12 @@ export { createSnapErrorHandling, createTrackError } from './trackError'; +export { + SynchronizationError, + formatAccountSyncFailures, + getSyncFailuresFromSettledResult, +} from './syncError'; +export type { AccountSyncFailure } from './syncError'; export { createWithCatchAndThrowSnapError, normalizeError } from './errors'; +export { stringifyReason } from './stringifyReason'; export { isSnapRpcError } from './snapRpcError'; export type { CreateSnapErrorHandlingOptions, diff --git a/packages/snap-networks-utils/src/utils/errors/stringifyReason.test.ts b/packages/snap-networks-utils/src/utils/errors/stringifyReason.test.ts new file mode 100644 index 000000000..21a586bba --- /dev/null +++ b/packages/snap-networks-utils/src/utils/errors/stringifyReason.test.ts @@ -0,0 +1,53 @@ +import { stringifyReason } from './stringifyReason'; + +describe('stringifyReason', () => { + it('stringifies a plain error as name and message', () => { + expect(stringifyReason(new Error('boom'))).toBe('Error: boom'); + }); + + it('appends the cause of wrapped errors', () => { + const error = new Error('Failed to synchronize account', { + cause: new Error('502 Bad Gateway'), + }); + + expect(stringifyReason(error)).toBe( + 'Error: Failed to synchronize account (Error: 502 Bad Gateway)', + ); + }); + + it('appends causes that are not errors', () => { + const error = new Error('boom', { cause: '502 Bad Gateway' }); + + expect(stringifyReason(error)).toBe('Error: boom (502 Bad Gateway)'); + }); + + it('does not append an explicit null cause', () => { + const error = new Error('boom', { cause: null }); + + expect(stringifyReason(error)).toBe('Error: boom'); + }); + + it('stringifies non-error reasons as-is', () => { + expect(stringifyReason('plain failure')).toBe('plain failure'); + expect(stringifyReason(42)).toBe('42'); + }); + + it('returns a placeholder when stringification throws', () => { + const hostile = { + toString(): string { + throw new Error('hostile toString'); + }, + }; + + expect(stringifyReason(hostile)).toBe('Unknown error'); + }); + + it('returns a placeholder when the error itself cannot be stringified', () => { + const hostileError = new Error('boom'); + hostileError.toString = (): string => { + throw new Error('hostile toString'); + }; + + expect(stringifyReason(hostileError)).toBe('Unknown error'); + }); +}); diff --git a/packages/snap-networks-utils/src/utils/errors/stringifyReason.ts b/packages/snap-networks-utils/src/utils/errors/stringifyReason.ts new file mode 100644 index 000000000..4beeb2d5d --- /dev/null +++ b/packages/snap-networks-utils/src/utils/errors/stringifyReason.ts @@ -0,0 +1,34 @@ +/** + * Stringifies a rejection reason without ever throwing: `String(reason)` + * throws when the reason has a hostile `toString`/`Symbol.toPrimitive`. If the + * reason is an error with a `cause`, the cause is appended, so the wrapped + * failure (e.g. the underlying network error) is part of the reported reason. + * + * Intended for building human-readable failure reasons (e.g. for + * `AccountSyncFailure.reason`), where wrapper errors would otherwise hide the + * original failure behind a generic message. + * + * @param reason - The rejection reason to stringify. + * @returns The reason as a string, or a placeholder when stringification fails. + */ +export function stringifyReason(reason: unknown): string { + try { + if (!(reason instanceof Error)) { + return String(reason); + } + + const message = String(reason); + const { cause } = reason; + + if (cause === null || cause === undefined) { + return message; + } + + // Deliberately permissive: any cause detail is worth reporting, and a + // pathological value still ends up as the placeholder via the outer catch. + // eslint-disable-next-line @typescript-eslint/no-base-to-string + return `${message} (${String(cause)})`; + } catch { + return 'Unknown error'; + } +} diff --git a/packages/snap-networks-utils/src/utils/errors/syncError.test.ts b/packages/snap-networks-utils/src/utils/errors/syncError.test.ts new file mode 100644 index 000000000..356d22ef6 --- /dev/null +++ b/packages/snap-networks-utils/src/utils/errors/syncError.test.ts @@ -0,0 +1,140 @@ +import { + SynchronizationError, + formatAccountSyncFailures, + getSyncFailuresFromSettledResult, +} from './syncError'; + +describe('formatAccountSyncFailures', () => { + it('formats a single failure', () => { + const failures = [{ accountId: 'account-1', reason: 'Error: boom' }]; + + expect(formatAccountSyncFailures(failures)).toBe('account-1: Error: boom'); + }); + + it('formats multiple failures separated by semicolons', () => { + const failures = [ + { accountId: 'account-1', reason: 'Error: boom' }, + { accountId: 'account-2', reason: 'TimeoutError: too slow' }, + ]; + + expect(formatAccountSyncFailures(failures)).toBe( + 'account-1: Error: boom; account-2: TimeoutError: too slow', + ); + }); + + it('truncates long reasons', () => { + const failures = [{ accountId: 'account-1', reason: 'x'.repeat(1000) }]; + + const formatted = formatAccountSyncFailures(failures); + + expect(formatted).toHaveLength('account-1: '.length + 300 + 1); + expect(formatted).toContain('…'); + }); + + it('truncates the list when there are many failures', () => { + const failures = Array.from({ length: 15 }, (_, index) => ({ + accountId: `account-${index}`, + reason: 'Error: boom', + })); + + const formatted = formatAccountSyncFailures(failures); + + expect(formatted).toContain('account-9: Error: boom'); + expect(formatted).not.toContain('account-10: Error: boom'); + expect(formatted.endsWith('+5 more')).toBe(true); + }); + + it('returns an empty string for no failures', () => { + expect(formatAccountSyncFailures([])).toBe(''); + }); +}); + +describe('getSyncFailuresFromSettledResult', () => { + it('maps rejections to failures with stringified reasons', () => { + const results = [ + { status: 'fulfilled', value: 'ok' }, + { + status: 'rejected', + reason: new Error('Failed to synchronize account', { + cause: new Error('502 Bad Gateway'), + }), + }, + { status: 'rejected', reason: 42 }, + ] as PromiseSettledResult[]; + + expect( + getSyncFailuresFromSettledResult(results, [ + 'account-1', + 'account-2', + 'account-3', + ]), + ).toStrictEqual([ + { + accountId: 'account-2', + reason: 'Error: Failed to synchronize account (Error: 502 Bad Gateway)', + }, + { accountId: 'account-3', reason: '42' }, + ]); + }); + + it('skips rejections without a matching account ID', () => { + const results = [ + { status: 'rejected', reason: new Error('boom') }, + { status: 'rejected', reason: new Error('also boom') }, + ] as PromiseSettledResult[]; + + expect( + getSyncFailuresFromSettledResult(results, ['account-1']), + ).toStrictEqual([{ accountId: 'account-1', reason: 'Error: boom' }]); + }); + + it('returns an empty list when nothing is rejected', () => { + const results = [ + { status: 'fulfilled', value: 'ok' }, + ] as PromiseSettledResult[]; + + expect( + getSyncFailuresFromSettledResult(results, ['account-1']), + ).toStrictEqual([]); + }); +}); + +describe('SynchronizationError', () => { + it('sets the name and the message', () => { + const error = new SynchronizationError('Account synchronization failures'); + + expect(error.name).toBe('SynchronizationError'); + expect(error.message).toBe('Account synchronization failures'); + expect(error.failures).toStrictEqual([]); + }); + + it('appends failure details to the message so they survive error tracking', () => { + const failures = [ + { accountId: 'account-1', reason: 'Error: boom' }, + { accountId: 'account-2', reason: 'TimeoutError: too slow' }, + ]; + + const error = new SynchronizationError( + 'Account synchronization failures', + failures, + ); + + expect(error.name).toBe('SynchronizationError'); + expect(error.message).toBe( + 'Account synchronization failures (2 failed): account-1: Error: boom; ' + + 'account-2: TimeoutError: too slow', + ); + expect(error.failures).toBe(failures); + }); + + it('supports a cause', () => { + const cause = new Error('root cause'); + const error = new SynchronizationError( + 'Account synchronization failures', + [{ accountId: 'account-1', reason: 'Error: boom' }], + { cause }, + ); + + expect(error.cause).toBe(cause); + }); +}); diff --git a/packages/snap-networks-utils/src/utils/errors/syncError.ts b/packages/snap-networks-utils/src/utils/errors/syncError.ts new file mode 100644 index 000000000..581d594e6 --- /dev/null +++ b/packages/snap-networks-utils/src/utils/errors/syncError.ts @@ -0,0 +1,125 @@ +import { stringifyReason } from './stringifyReason'; + +/** + * A single account synchronization failure, with enough context to understand + * which account failed and why. + */ +export type AccountSyncFailure = { + /** The ID of the account that failed to synchronize. */ + accountId: string; + /** The failure reason, as a human-readable string. */ + reason: string; +}; + +/** + * Maps settled synchronization results to account synchronization failures: + * each rejection is attributed to the account ID at the same index + * (`Promise.allSettled` preserves order). + * + * Rejection reasons are stringified with {@link stringifyReason}, so wrapped + * errors report their cause as well. + * + * @param results - The settled synchronization results. + * @param accountIds - The account IDs, in the same order as `results`. + * @returns The collected failures, in results order. + */ +export function getSyncFailuresFromSettledResult( + results: PromiseSettledResult[], + accountIds: string[], +): AccountSyncFailure[] { + const failures: AccountSyncFailure[] = []; + + results.forEach((result, index) => { + const accountId = accountIds[index]; + if (accountId && result.status === 'rejected') { + failures.push({ + accountId, + reason: stringifyReason(result.reason), + }); + } + }); + + return failures; +} + +/** + * Maximum number of characters kept from each failure reason. Reasons can be + * arbitrarily long (e.g. full network error messages), so they are truncated + * to keep the error message readable in Sentry. + */ +const MAX_REASON_LENGTH = 300; + +/** + * Maximum number of failures listed in the error message. When more accounts + * fail, the remaining ones are summarized with a `+N more` suffix. + */ +const MAX_LISTED_FAILURES = 10; + +/** + * Formats account synchronization failures as a compact, single-line summary. + * + * The summary is intended to be embedded in the error message of a + * `SynchronizationError`: error tracking via `snap_trackError` serializes only + * the error's `name`, `message`, `stack`, and `cause`, so any details stored in + * other properties (e.g. a `data` field) are lost before reaching Sentry. + * + * @param failures - The account synchronization failures to format. + * @returns A single-line summary of the failures, truncated to keep the + * message readable. + */ +export function formatAccountSyncFailures( + failures: AccountSyncFailure[], +): string { + const listed = failures.slice(0, MAX_LISTED_FAILURES); + const details = listed.map((failure) => { + const reason = + failure.reason.length > MAX_REASON_LENGTH + ? `${failure.reason.slice(0, MAX_REASON_LENGTH)}…` + : failure.reason; + + return `${failure.accountId}: ${reason}`; + }); + + const remaining = failures.length - listed.length; + const suffix = remaining > 0 ? `; +${remaining} more` : ''; + + return `${details.join('; ')}${suffix}`; +} + +/** + * Error thrown when one or more accounts fail to synchronize. + * + * The failure details are embedded in the message (not only in a property) so + * that they survive error tracking: `snap_trackError` serializes only the + * error's `name`, `message`, `stack`, and `cause`. + */ +export class SynchronizationError extends Error { + /** + * The structured list of account synchronization failures. Useful for logs; + * note that it is not preserved by error tracking. + */ + readonly failures: AccountSyncFailure[]; + + /** + * Construct a new synchronization error. + * + * @param message - The error message, e.g. `'Account synchronization failures'`. + * @param failures - The per-account failures to append to the message. + * @param options - Additional error options. + * @param options.cause - The underlying error that caused the synchronization failure, if any. + */ + constructor( + message: string, + failures: AccountSyncFailure[] = [], + options?: { cause?: unknown }, + ) { + const details = + failures.length > 0 + ? ` (${failures.length} failed): ${formatAccountSyncFailures(failures)}` + : ''; + + super(`${message}${details}`, options); + this.name = 'SynchronizationError'; + this.failures = failures; + } +} diff --git a/packages/tron-wallet-snap/CHANGELOG.md b/packages/tron-wallet-snap/CHANGELOG.md index 57d39ebd9..d5ad98021 100644 --- a/packages/tron-wallet-snap/CHANGELOG.md +++ b/packages/tron-wallet-snap/CHANGELOG.md @@ -12,6 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Add `signProofOfOwnershipBatch` for signing multiple proof-of-ownership messages in one request. ([#265](https://github.com/MetaMask/internal-snaps/pull/265)) - Emit `Transaction Added`, `Transaction Approved`, and `Transaction Rejected` for `signTransaction` confirmations ([#333](https://github.com/MetaMask/internal-snaps/pull/333)) - Only dApp-initiated transaction confirmations were affected; the unified send flow already emitted these events. +- Report account synchronization failures to Sentry with the failing account IDs and their failure reasons ([#374](https://github.com/MetaMask/internal-snaps/pull/374)) ### Changed diff --git a/packages/tron-wallet-snap/jest.config.js b/packages/tron-wallet-snap/jest.config.js index e3291d1d3..ca09b0997 100644 --- a/packages/tron-wallet-snap/jest.config.js +++ b/packages/tron-wallet-snap/jest.config.js @@ -19,10 +19,10 @@ module.exports = { // An object that configures minimum threshold enforcement for coverage results coverageThreshold: { global: { - branches: 72.42, - functions: 79.64, - lines: 85.79, - statements: 85.81, + branches: 72.63, + functions: 79.95, + lines: 85.84, + statements: 85.86, }, }, }; diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts index a473accb1..b11be2511 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.test.ts @@ -20,6 +20,7 @@ import type { SnapClient } from '../../clients/snap/SnapClient'; import { Network } from '../../constants'; import type { NativeAsset } from '../../entities/assets'; import { createTronBip44KeypairDeriver } from '../../utils/deriveTronFromCoinTypeNode'; +import { trackError } from '../../utils/errors'; import { mockLogger } from '../../utils/mockLogger'; import type { AssetsService } from '../assets/AssetsService'; import type { Config } from '../config/ConfigProvider'; @@ -27,6 +28,11 @@ import type { TransactionsService } from '../transactions/TransactionsService'; import type { AccountsRepository } from './AccountsRepository'; import { AccountsService, SUPPORTED_SCOPES } from './AccountsService'; +jest.mock('../../utils/errors', () => ({ + ...jest.requireActual('../../utils/errors'), + trackError: jest.fn().mockResolvedValue('tracked-error-id'), +})); + jest.mock('@metamask/keyring-snap-sdk', () => ({ getSelectedAccounts: jest.fn().mockResolvedValue([]), })); @@ -1015,8 +1021,8 @@ describe('AccountsService', () => { }); }); - describe('synchronizeAssets', () => { - it('calls fetch for each account and scope, then saveMany', async () => { + describe('synchronize (assets)', () => { + it('fetches assets for each account and scope, then saves', async () => { const account: ExtendedKeyringAccount = { id: 'sync-asset-id', address: 'TSyncAsset12345678901234567', @@ -1051,7 +1057,7 @@ describe('AccountsService', () => { mockAssets, ); - await accountsService.synchronizeAssets([account]); + await accountsService.synchronize([account]); expect( mockAssetsService.fetchAssetsAndBalancesForAccount, @@ -1065,6 +1071,149 @@ describe('AccountsService', () => { expect(mockAssetsService.saveMany).toHaveBeenCalledWith( expect.arrayContaining(mockAssets), ); + expect(trackError).not.toHaveBeenCalled(); + }, + ); + }); + + it('tracks save failures standalone, without failing', async () => { + const account: ExtendedKeyringAccount = { + id: 'sync-asset-fail-id', + address: 'TSyncFail123456789012345678', + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: "m/44'/195'/0'/0/0", + index: 0, + }; + + await withAccountsService( + async ({ accountsService, mockConfigProvider, mockAssetsService }) => { + mockConfigProvider.config = { + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet, Network.Shasta], + }; + const saveError = new Error('storage full'); + mockAssetsService.saveMany.mockRejectedValue(saveError); + + // Save failures are batch-level: tracked as their own error (not + // attributed to individual accounts), and the method still resolves. + await accountsService.synchronize([account]); + + expect(trackError).toHaveBeenCalledTimes(1); + const tracked = (trackError as jest.Mock).mock.calls[0][0] as Error; + expect(tracked.message).toBe('Failed to save assets'); + expect((tracked as Error & { cause?: unknown }).cause).toBe( + saveError, + ); + }, + ); + }); + + it('points at the specific account whose fetch failed', async () => { + const failingAccount: ExtendedKeyringAccount = { + id: 'sync-fail-id', + address: 'TSyncFail123456789012345678', + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: "m/44'/195'/0'/0/0", + index: 0, + }; + const healthyAccount: ExtendedKeyringAccount = { + id: 'sync-healthy-id', + address: 'TSyncHealthy123456789012345', + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: "m/44'/195'/0'/0/1", + index: 1, + }; + const healthyAssets: NativeAsset[] = [ + { + assetType: `${Network.Mainnet}/slip44:195`, + keyringAccountId: 'sync-healthy-id', + network: Network.Mainnet, + symbol: 'TRX', + decimals: 6, + rawAmount: '1000000', + uiAmount: '1', + iconUrl: '', + }, + ]; + + await withAccountsService( + async ({ accountsService, mockConfigProvider, mockAssetsService }) => { + mockConfigProvider.config = { + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }; + mockAssetsService.fetchAssetsAndBalancesForAccount + .mockRejectedValueOnce(new Error('grpc unavailable')) + .mockResolvedValueOnce(healthyAssets); + + await accountsService.synchronize([failingAccount, healthyAccount]); + + expect(trackError).toHaveBeenCalledTimes(1); + const tracked = (trackError as jest.Mock).mock.calls[0][0] as Error; + expect(tracked.name).toBe('SynchronizationError'); + expect(tracked.message).toBe( + 'Account synchronization failures (1 failed): ' + + 'sync-fail-id: Error: grpc unavailable', + ); + // The healthy account's assets are still saved. + expect(mockAssetsService.saveMany).toHaveBeenCalledWith( + healthyAssets, + ); + }, + ); + }); + + it('survives a hostile rejection reason that cannot be stringified', async () => { + const account: ExtendedKeyringAccount = { + id: 'sync-hostile-id', + address: 'TSyncHostile12345678901234567', + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: "m/44'/195'/0'/0/0", + index: 0, + }; + + await withAccountsService( + async ({ accountsService, mockConfigProvider, mockAssetsService }) => { + mockConfigProvider.config = { + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }; + const hostileReason = { + toString: (): never => { + throw new Error('toString boom'); + }, + }; + mockAssetsService.fetchAssetsAndBalancesForAccount.mockRejectedValue( + hostileReason, + ); + + // The hostile reason is defused to a placeholder and the failure is + // still reported; the sync completes and the save still runs. + await accountsService.synchronize([account]); + + expect(mockAssetsService.saveMany).toHaveBeenCalledWith([]); + expect(trackError).toHaveBeenCalledTimes(1); + const tracked = (trackError as jest.Mock).mock.calls[0][0] as Error; + expect(tracked.message).toBe( + `Account synchronization failures (1 failed): ` + + `${account.id}: Unknown error`, + ); }, ); }); @@ -1086,7 +1235,7 @@ describe('AccountsService', () => { index: 0, }; - await accountsService.synchronizeAssets([account]); + await accountsService.synchronize([account]); expect( mockAssetsService.fetchAssetsAndBalancesForAccount, @@ -1147,13 +1296,96 @@ describe('AccountsService', () => { expect(mockTransactionsService.saveMany).toHaveBeenCalledWith( mockTransactions, ); + expect(trackError).not.toHaveBeenCalled(); + }, + ); + }); + + it('tracks the failing account when a fetch fails, without failing', async () => { + const account: ExtendedKeyringAccount = { + id: 'sync-tx-fail-id', + address: 'TSyncTxFail12345678901234567', + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: "m/44'/195'/0'/0/0", + index: 0, + }; + + await withAccountsService( + async ({ + accountsService, + mockConfigProvider, + mockTransactionsService, + }) => { + mockConfigProvider.config = { + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }; + mockTransactionsService.fetchNewTransactionsForAccount.mockRejectedValue( + new Error('grpc unavailable'), + ); + + // Per-fetch failures are reported (attributed to the account), but + // the method still resolves. + expect( + await accountsService.synchronizeTransactions([account]), + ).toBeUndefined(); + expect(mockTransactionsService.saveMany).toHaveBeenCalledWith([]); + expect(trackError).toHaveBeenCalledTimes(1); + const tracked = (trackError as jest.Mock).mock.calls[0][0] as Error; + expect(tracked.name).toBe('SynchronizationError'); + expect(tracked.message).toBe( + `Account synchronization failures (1 failed): ` + + `${account.id}: Error: grpc unavailable`, + ); + }, + ); + }); + + it('tracks save failures standalone, without failing', async () => { + const account: ExtendedKeyringAccount = { + id: 'sync-tx-save-fail-id', + address: 'TSyncTxSaveFail12345678901234', + type: TrxAccountType.Eoa, + options: {}, + methods: [], + scopes: [], + entropySource: 'e1', + derivationPath: "m/44'/195'/0'/0/0", + index: 0, + }; + + await withAccountsService( + async ({ + accountsService, + mockConfigProvider, + mockTransactionsService, + }) => { + mockConfigProvider.config = { + ...MOCK_CONFIG, + activeNetworks: [Network.Mainnet], + }; + const saveError = new Error('storage full'); + mockTransactionsService.saveMany.mockRejectedValue(saveError); + + await accountsService.synchronizeTransactions([account]); + + expect(trackError).toHaveBeenCalledTimes(1); + const tracked = (trackError as jest.Mock).mock.calls[0][0] as Error; + expect(tracked.message).toBe('Failed to save transactions'); + expect((tracked as Error & { cause?: unknown }).cause).toBe( + saveError, + ); }, ); }); }); describe('synchronize', () => { - it('calls both synchronizeAssets and synchronizeTransactions', async () => { + it('fetches both assets and transactions', async () => { const account: ExtendedKeyringAccount = { id: 'sync-id', address: 'TSync12345678901234567890', diff --git a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts index c6476a84c..1bbab4f63 100644 --- a/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts +++ b/packages/tron-wallet-snap/src/services/accounts/AccountsService.ts @@ -12,9 +12,12 @@ import { import { getSelectedAccounts } from '@metamask/keyring-snap-sdk'; import { InFlightCoalescer, + SynchronizationError, asStrictKeyringAccount, + getSyncFailuresFromSettledResult, } from '@metamask/snap-networks-utils'; import type { + AccountSyncFailure, ExtendedKeyringAccount, Logger, } from '@metamask/snap-networks-utils'; @@ -31,7 +34,7 @@ import { createTronBip44AddressDeriver, createTronBip44KeypairDeriver, } from '../../utils/deriveTronFromCoinTypeNode'; -import { sanitizeSensitiveError } from '../../utils/errors'; +import { sanitizeSensitiveError, trackError } from '../../utils/errors'; import { DerivationPathStruct } from '../../validation/structs'; import type { AssetsService } from '../assets/AssetsService'; import type { ConfigProvider } from '../config'; @@ -507,6 +510,10 @@ export class AccountsService { * Synchronizes only assets for the given accounts. * This method can be called independently to sync assets without syncing transactions. * + * Fetch failures are reported to Sentry in one `SynchronizationError`, + * attributed to their account; save failures are tracked standalone. Neither + * is thrown. + * * @param accounts - The accounts to synchronize assets for. */ async synchronizeAssets(accounts: ExtendedKeyringAccount[]): Promise { @@ -528,9 +535,30 @@ export class AccountsService { response.status === 'fulfilled' ? response.value : [], ); - await this.#assetsService.saveMany(assets); + const failures = getSyncFailuresFromSettledResult( + assetResponses, + combinations.map(({ account }) => account.id), + ); + await this.#reportSyncFailures(failures); + + try { + await this.#assetsService.saveMany(assets); + } catch (error) { + // Save failures are batch-level (not attributable to one account), so + // they are tracked standalone. + await trackError(new Error('Failed to save assets', { cause: error })); + } } + /** + * Synchronizes only transactions for the given accounts. + * + * Fetch failures are reported to Sentry in one `SynchronizationError`, + * attributed to their account; save failures are tracked standalone. Neither + * is thrown. + * + * @param accounts - The accounts to synchronize transactions for. + */ async synchronizeTransactions( accounts: ExtendedKeyringAccount[], ): Promise { @@ -539,7 +567,7 @@ export class AccountsService { scopes.map((scope) => ({ account, scope })), ); - const transactionResponses = await Promise.allSettled( + const trxResponses = await Promise.allSettled( combinations.map(async ({ account, scope }) => { return this.#transactionsService.fetchNewTransactionsForAccount( scope, @@ -548,13 +576,33 @@ export class AccountsService { }), ); - const transactions = transactionResponses.flatMap((response) => + const transactions = trxResponses.flatMap((response) => response.status === 'fulfilled' ? response.value : [], ); - await this.#transactionsService.saveMany(transactions); + const failures = getSyncFailuresFromSettledResult( + trxResponses, + combinations.map(({ account }) => account.id), + ); + await this.#reportSyncFailures(failures); + + try { + await this.#transactionsService.saveMany(transactions); + } catch (error) { + // Save failures are batch-level (not attributable to one account), so + // they are tracked standalone. + await trackError( + new Error('Failed to save transactions', { cause: error }), + ); + } } + /** + * Synchronizes assets and transactions for the given accounts. Each sync + * reports its own failures to Sentry, attributed per account. + * + * @param accounts - The accounts to synchronize. + */ async synchronize(accounts: ExtendedKeyringAccount[]): Promise { // Sync triggers stack up (60s cronjob, a background event scheduled by // every `setSelectedAccounts` call, post-transaction refreshes), so @@ -573,6 +621,33 @@ export class AccountsService { }); } + /** + * Reports account synchronization failures to Sentry, with the failure + * reasons embedded in the message: error tracking (`snap_trackError`) does + * not preserve custom error properties, so the message is the only reliable + * channel for the details. + * + * @param failures - The account synchronization failures to report. + */ + async #reportSyncFailures(failures: AccountSyncFailure[]): Promise { + try { + if (failures.length === 0) { + return; + } + + const error = new SynchronizationError( + 'Account synchronization failures', + failures, + ); + + await trackError(error); + } catch (reportingError) { + this.#logger.warn('Failed to report synchronization failures', { + reportingError, + }); + } + } + async #createTronAddressDeriver( entropySource: EntropySourceId, ): Promise { diff --git a/packages/tron-wallet-snap/src/services/assets/AssetsService.test.ts b/packages/tron-wallet-snap/src/services/assets/AssetsService.test.ts index 57a36a90f..756208616 100644 --- a/packages/tron-wallet-snap/src/services/assets/AssetsService.test.ts +++ b/packages/tron-wallet-snap/src/services/assets/AssetsService.test.ts @@ -501,6 +501,51 @@ describe('AssetsService', () => { }, ); }); + + it('tracks unexpected account info failures that are treated as inactive', async () => { + await withAssetsService( + async ({ + assetsService, + mockTrongridApiClient, + mockTronHttpClient, + mockSnapClient, + }) => { + // An HTTP failure is not an "account not found", but the flow + // still treats it as an inactive account; it must be tracked. + const fetchError = new Error('HTTP error! status: 500'); + mockTrongridApiClient.getAccountInfoByAddress.mockRejectedValue( + fetchError, + ); + mockTronHttpClient.getAccountResources.mockResolvedValue( + emptyAccountResources, + ); + mockTrongridApiClient.getTrc20BalancesByAddress.mockResolvedValue( + [], + ); + + const assets = await assetsService.fetchAssetsAndBalancesForAccount( + Network.Mainnet, + mockAccount, + ); + + expect(mockSnapClient.trackError).toHaveBeenCalledTimes(1); + const tracked = (mockSnapClient.trackError as jest.Mock).mock + .calls[0][0] as Error; + expect(tracked.message).toBe( + 'Account info request failed; treating as inactive account', + ); + // The original error is preserved as the cause. + expect((tracked as Error & { cause?: unknown }).cause).toBe( + fetchError, + ); + // The inactive-account flow is unchanged. + expect( + mockTrongridApiClient.getTrc20BalancesByAddress, + ).toHaveBeenCalledWith(Network.Mainnet, mockAccount.address); + expect(assets.length).toBeGreaterThan(0); + }, + ); + }); }); describe('partial failure handling', () => { diff --git a/packages/tron-wallet-snap/src/services/assets/AssetsService.ts b/packages/tron-wallet-snap/src/services/assets/AssetsService.ts index 0b43b10c3..181a0555e 100644 --- a/packages/tron-wallet-snap/src/services/assets/AssetsService.ts +++ b/packages/tron-wallet-snap/src/services/assets/AssetsService.ts @@ -88,12 +88,7 @@ export class AssetsService { account: KeyringAccount, ): Promise { if (await this.#shouldReturnAssetsFromCore()) { - const assetsAndBalances = - await this.#coreAdapter.fetchAssetsAndBalancesForAccount( - scope, - account, - ); - return assetsAndBalances; + return this.#coreAdapter.fetchAssetsAndBalancesForAccount(scope, account); } return this.#snapAdapter.fetchAssetsAndBalancesForAccount(scope, account); diff --git a/packages/tron-wallet-snap/src/services/assets/adapters/SnapAssetsAdapter.ts b/packages/tron-wallet-snap/src/services/assets/adapters/SnapAssetsAdapter.ts index 592ff4c60..56757ebc3 100644 --- a/packages/tron-wallet-snap/src/services/assets/adapters/SnapAssetsAdapter.ts +++ b/packages/tron-wallet-snap/src/services/assets/adapters/SnapAssetsAdapter.ts @@ -16,6 +16,7 @@ import type { SnapClient } from '../../../clients/snap/SnapClient'; import type { TokenApiClient } from '../../../clients/token-api/TokenApiClient'; import type { AccountResources } from '../../../clients/tron-http'; import type { TronHttpClient } from '../../../clients/tron-http/TronHttpClient'; +import { TrongridAccountNotFoundError } from '../../../clients/trongrid/errors'; import type { TrongridApiClient } from '../../../clients/trongrid/TrongridApiClient'; import type { Trc20Balance, @@ -163,6 +164,10 @@ export class SnapAssetsAdapter { * 5. Enrich assets with metadata via `#enrichAssetsWithMetadata` * 6. Filter spam tokens via `#filterTokensWithoutPriceData` * + * Degradations along the way (unexpected account info failures, TRC20 + * fallback failures, spot price failures) are tracked to Sentry; the request + * itself only rejects when assets cannot be built at all. + * * @param scope - The network to query. * @param account - The keyring account. * @returns Promise - Array of assets with balances. @@ -188,17 +193,38 @@ export class SnapAssetsAdapter { const isInactiveAccount = tronAccountInfoRequest.status === 'rejected'; if (isInactiveAccount) { - this.#logger.info( - 'Account info request failed, treating as inactive account', - { account, scope }, - ); + const { reason } = tronAccountInfoRequest; + if (reason instanceof TrongridAccountNotFoundError) { + this.#logger.info( + 'Account not found on-chain, treating as inactive account', + { account, scope }, + ); + } else { + // Any rejection is currently treated as an inactive account. A + // rejection that is not "account not found" (HTTP error, timeout) + // zeroes the account's balances, so track it for visibility. + await this.#snapClient.trackError( + new Error( + 'Account info request failed; treating as inactive account', + { cause: reason }, + ), + ); + this.#logger.warn( + 'Account info request failed; treating as inactive account', + { error: reason, account, scope }, + ); + } } const trc20BalancesFallback = isInactiveAccount ? await this.#trongridApiClient .getTrc20BalancesByAddress(scope, account.address) .catch(async (error) => { - await this.#snapClient.trackError(error as Error); + await this.#snapClient.trackError( + new Error('Failed to fetch TRC20 balances for inactive account', { + cause: error, + }), + ); this.#logger.warn( 'Failed to fetch TRC20 balances for inactive account', { error, account, scope }, @@ -224,7 +250,12 @@ export class SnapAssetsAdapter { this.#priceApiClient .getMultipleSpotPrices(priceableAssetTypes, 'usd') .catch(async (error) => { - await this.#snapClient.trackError(error as Error); + await this.#snapClient.trackError( + new Error( + 'Failed to fetch spot prices; filtering tokens without price data', + { cause: error }, + ), + ); return {}; }), ]);