From 0ac32320806091c7310093575e41eeb2e1adbb69 Mon Sep 17 00:00:00 2001 From: Stanley Yuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Wed, 24 Jun 2026 18:19:50 +0800 Subject: [PATCH 1/3] refactor: TransacationService error handle --- packages/snap/src/context.ts | 4 +- .../clientRequest/confirmSend.test.ts | 46 ++++++- .../src/handlers/clientRequest/confirmSend.ts | 25 +++- .../clientRequest/onAddressInput.test.ts | 3 +- .../handlers/clientRequest/onAddressInput.ts | 18 +-- .../clientRequest/onAmountInput.test.ts | 20 ++- .../handlers/clientRequest/onAmountInput.ts | 29 ++-- .../src/services/sync/SynchronizeService.ts | 74 +++++----- .../src/services/transaction/Transaction.ts | 13 +- .../transaction/TransactionBuilder.ts | 10 +- .../services/transaction/TransactionMapper.ts | 101 +++++++------- .../transaction/TransactionService.ts | 14 +- .../TransactionSynchronizeService.ts | 17 ++- .../src/services/transaction/exceptions.ts | 64 +++------ .../transaction/simulation/simulators.ts | 15 +- .../snap/src/services/transaction/utils.ts | 10 +- .../services/transaction/xdrParser.test.ts | 25 +--- .../src/services/transaction/xdrParser.ts | 130 +++++++++--------- packages/snap/src/ui/confirmation/utils.ts | 6 +- 19 files changed, 317 insertions(+), 307 deletions(-) diff --git a/packages/snap/src/context.ts b/packages/snap/src/context.ts index fd29ea8c..698d91e3 100644 --- a/packages/snap/src/context.ts +++ b/packages/snap/src/context.ts @@ -266,9 +266,7 @@ const changeTrustOptHandler = new ChangeTrustOptHandler({ confirmationUIController, }); -const onAddressInputHandler = new OnAddressInputHandler({ - logger, -}); +const onAddressInputHandler = new OnAddressInputHandler(); const onAmountInputHandler = new OnAmountInputHandler({ logger, diff --git a/packages/snap/src/handlers/clientRequest/confirmSend.test.ts b/packages/snap/src/handlers/clientRequest/confirmSend.test.ts index 4a21e2b6..cea352e0 100644 --- a/packages/snap/src/handlers/clientRequest/confirmSend.test.ts +++ b/packages/snap/src/handlers/clientRequest/confirmSend.test.ts @@ -47,6 +47,7 @@ import { InsufficientBalanceException, InsufficientBalanceToCoverFeeException, TransactionValidationException, + XdrParseException, } from '../../services/transaction/exceptions'; import { KeyringTransactionType } from '../../services/transaction/KeyringTransactionBuilder'; import { WalletService } from '../../services/wallet'; @@ -522,13 +523,52 @@ describe('ConfirmSendHandler', () => { }); }); - it('rethrows unexpected errors from createValidatedSendTransaction', async () => { + it('returns invalid and tracks when createValidatedSendTransaction throws XdrParseException', async () => { + const { handler, createValidatedSendTransaction } = setup(); + const xdrParseError = new XdrParseException( + 'Invalid transfer function arguments', + ); + createValidatedSendTransaction.mockRejectedValueOnce(xdrParseError); + const trackErrorSpy = jest + .spyOn(snapUtils, 'trackError') + .mockResolvedValue(undefined); + + expect(await handler.handle(baseRequest())).toStrictEqual({ + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }); + expect(trackErrorSpy).toHaveBeenCalledWith(xdrParseError); + }); + + it('returns invalid for unexpected errors from createValidatedSendTransaction', async () => { + const { handler, createValidatedSendTransaction } = setup(); + const unexpectedError = new Error('unexpected'); + createValidatedSendTransaction.mockRejectedValueOnce(unexpectedError); + const trackErrorSpy = jest + .spyOn(snapUtils, 'trackError') + .mockResolvedValue(undefined); + + expect(await handler.handle(baseRequest())).toStrictEqual({ + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }); + expect(trackErrorSpy).toHaveBeenCalledWith(unexpectedError); + }); + + it('does not track expected validation errors from createValidatedSendTransaction', async () => { const { handler, createValidatedSendTransaction } = setup(); createValidatedSendTransaction.mockRejectedValueOnce( - new Error('unexpected'), + new TransactionValidationException('x'), ); + const trackErrorSpy = jest + .spyOn(snapUtils, 'trackError') + .mockResolvedValue(undefined); - await expect(handler.handle(baseRequest())).rejects.toThrow('unexpected'); + expect(await handler.handle(baseRequest())).toStrictEqual({ + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }); + expect(trackErrorSpy).not.toHaveBeenCalled(); }); it('continues successfully when saving pending transaction fails', async () => { diff --git a/packages/snap/src/handlers/clientRequest/confirmSend.ts b/packages/snap/src/handlers/clientRequest/confirmSend.ts index f5d0e0a0..e7f51b05 100644 --- a/packages/snap/src/handlers/clientRequest/confirmSend.ts +++ b/packages/snap/src/handlers/clientRequest/confirmSend.ts @@ -34,6 +34,7 @@ import { ConfirmationInterfaceKey } from '../../ui/confirmation/api'; import { hasDecimals, toSmallestUnit, + trackError, trackTransactionAdded, trackTransactionApproved, trackTransactionRejected, @@ -214,10 +215,7 @@ export class ConfirmSendHandler extends BaseClientRequestHandler< transactionId, }; } catch (error: unknown) { - this.#logger.logErrorWithDetails( - 'Failed to confirm send transaction', - error, - ); + // Expected validation failures are user-facing outcomes; return them without tracking. if (error instanceof InsufficientBalanceException) { return { valid: false, @@ -241,7 +239,24 @@ export class ConfirmSendHandler extends BaseClientRequestHandler< errors: [{ code: MultiChainSendErrorCodes.Invalid }], }; } - throw error; + + // User rejection must propagate so MetaMask can dismiss the send flow. + if (error instanceof UserRejectedRequestError) { + throw error; + } + + // Unexpected errors are swallowed into `{ valid: false }`, so track them for debugging. + await trackError(error); + + this.#logger.warn( + 'Failed to confirm send transaction due to unexpected issue', + { error }, + ); + + return { + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }; } } diff --git a/packages/snap/src/handlers/clientRequest/onAddressInput.test.ts b/packages/snap/src/handlers/clientRequest/onAddressInput.test.ts index f2bd3acf..a056881c 100644 --- a/packages/snap/src/handlers/clientRequest/onAddressInput.test.ts +++ b/packages/snap/src/handlers/clientRequest/onAddressInput.test.ts @@ -6,7 +6,6 @@ import { type OnAddressInputJsonRpcRequest, } from './api'; import { OnAddressInputHandler } from './onAddressInput'; -import { logger } from '../../utils/logger'; jest.mock('../../utils/logger'); @@ -14,7 +13,7 @@ const stellarAddress = 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN'; describe('OnAddressInputHandler', () => { - const handler = new OnAddressInputHandler({ logger }); + const handler = new OnAddressInputHandler(); it('returns valid when the address passes validation', async () => { const request: OnAddressInputJsonRpcRequest = { diff --git a/packages/snap/src/handlers/clientRequest/onAddressInput.ts b/packages/snap/src/handlers/clientRequest/onAddressInput.ts index 3cde6c3f..4ceb2c03 100644 --- a/packages/snap/src/handlers/clientRequest/onAddressInput.ts +++ b/packages/snap/src/handlers/clientRequest/onAddressInput.ts @@ -9,20 +9,9 @@ import { OnAddressInputJsonRpcRequestStruct, } from './api'; import type { IClientRequestHandler } from './base'; -import type { ILogger } from '../../utils'; -import { createPrefixedLogger, validateRequest } from '../../utils'; +import { validateRequest } from '../../utils'; export class OnAddressInputHandler implements IClientRequestHandler { - readonly #logger: ILogger; - - constructor({ logger }: { logger: ILogger }) { - const prefixedLogger = createPrefixedLogger( - logger, - '[📮 OnAddressInputHandler]', - ); - this.#logger = prefixedLogger; - } - /** * Handles the input of an address. * @@ -39,8 +28,9 @@ export class OnAddressInputHandler implements IClientRequestHandler { valid: true, errors: [], }; - } catch (error: unknown) { - this.#logger.logErrorWithDetails('Invalid address', error); + } catch { + // validateRequest will only throw InvalidParamsError, + // so we can safely ignore the error. return { valid: false, errors: [{ code: MultiChainSendErrorCodes.Invalid }], diff --git a/packages/snap/src/handlers/clientRequest/onAmountInput.test.ts b/packages/snap/src/handlers/clientRequest/onAmountInput.test.ts index 8bcdb1cc..7cc14215 100644 --- a/packages/snap/src/handlers/clientRequest/onAmountInput.test.ts +++ b/packages/snap/src/handlers/clientRequest/onAmountInput.test.ts @@ -40,6 +40,7 @@ import { InsufficientBalanceException, InsufficientBalanceToCoverFeeException, TransactionValidationException, + XdrParseException, } from '../../services/transaction/exceptions'; import { WalletService } from '../../services/wallet'; import { getTestWallet } from '../../services/wallet/__mocks__/wallet.fixtures'; @@ -300,13 +301,28 @@ describe('OnAmountInputHandler', () => { }); }); - it('rethrows unexpected errors from createValidatedSendTransaction', async () => { + it('returns invalid when createValidatedSendTransaction throws XdrParseException', async () => { + const { handler, createValidatedSendTransaction } = setup(); + createValidatedSendTransaction.mockRejectedValueOnce( + new XdrParseException('Invalid transfer function arguments'), + ); + + expect(await handler.handle(baseRequest())).toStrictEqual({ + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }); + }); + + it('returns invalid for unexpected errors from createValidatedSendTransaction', async () => { const { handler, createValidatedSendTransaction } = setup(); createValidatedSendTransaction.mockRejectedValueOnce( new Error('unexpected'), ); - await expect(handler.handle(baseRequest())).rejects.toThrow('unexpected'); + expect(await handler.handle(baseRequest())).toStrictEqual({ + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }); }); it('throws InvalidParamsError when the request fails struct validation', async () => { diff --git a/packages/snap/src/handlers/clientRequest/onAmountInput.ts b/packages/snap/src/handlers/clientRequest/onAmountInput.ts index bb9d9ac7..0f3c080e 100644 --- a/packages/snap/src/handlers/clientRequest/onAmountInput.ts +++ b/packages/snap/src/handlers/clientRequest/onAmountInput.ts @@ -11,12 +11,11 @@ import { } from './api'; import { BaseClientRequestHandler } from './base'; import type { AssetMetadataService } from '../../services/asset-metadata'; -import { AccountNotActivatedException } from '../../services/network/exceptions'; +import type { AccountNotActivatedException } from '../../services/network/exceptions'; import type { TransactionService } from '../../services/transaction'; import { InsufficientBalanceException, InsufficientBalanceToCoverFeeException, - TransactionValidationException, } from '../../services/transaction/exceptions'; import { hasDecimals, toSmallestUnit } from '../../utils'; import type { ILogger } from '../../utils/logger'; @@ -31,8 +30,6 @@ export class OnAmountInputHandler extends BaseClientRequestHandler< OnAmountInputJsonRpcRequest, OnAmountInputJsonRpcResponse > { - readonly #logger: ILogger; - readonly #assetMetadataService: AssetMetadataService; readonly #transactionService: TransactionService; @@ -61,7 +58,6 @@ export class OnAmountInputHandler extends BaseClientRequestHandler< }); this.#assetMetadataService = assetMetadataService; this.#transactionService = transactionService; - this.#logger = prefixedLogger; } /** @@ -112,10 +108,10 @@ export class OnAmountInputHandler extends BaseClientRequestHandler< errors: [], }; } catch (error: unknown) { - this.#logger.logErrorWithDetails( - 'Failed to validate amount input', - error, - ); + // Called on every amount change while the user types. Return structured + // validation errors without tracking to reduce Sentry noise — insufficient + // balance is expected during entry. Other failures also return invalid + // without tracking; confirmSend tracks unexpected errors on submit. if (error instanceof InsufficientBalanceException) { return { valid: false, @@ -130,16 +126,11 @@ export class OnAmountInputHandler extends BaseClientRequestHandler< ], }; } - if ( - error instanceof TransactionValidationException || - error instanceof AccountNotActivatedException - ) { - return { - valid: false, - errors: [{ code: MultiChainSendErrorCodes.Invalid }], - }; - } - throw error; + + return { + valid: false, + errors: [{ code: MultiChainSendErrorCodes.Invalid }], + }; } } diff --git a/packages/snap/src/services/sync/SynchronizeService.ts b/packages/snap/src/services/sync/SynchronizeService.ts index 2d63ade7..3fcc2cf8 100644 --- a/packages/snap/src/services/sync/SynchronizeService.ts +++ b/packages/snap/src/services/sync/SynchronizeService.ts @@ -1,5 +1,6 @@ import type { KnownCaip2ChainId } from '../../api'; import { AppConfig } from '../../config'; +import { trackError } from '../../utils'; import type { ILogger } from '../../utils/logger'; import { createPrefixedLogger } from '../../utils/logger'; import type { StellarKeyringAccount } from '../account'; @@ -75,8 +76,8 @@ export class SynchronizeService { // Both async loads are fail-safe, so we can use Promise.all. const [activatedAccountPairs, sep41Assets] = await Promise.all([ - this.#loadActivatedPairs(accounts, scope), - this.#loadSep41Assets(scope), + this.#loadActivatedPairsSafe(accounts, scope), + this.#loadSep41AssetsSafe(scope), ]); const tasks: { name: string; task: Promise }[] = []; @@ -107,14 +108,16 @@ export class SynchronizeService { tasks.map(async ({ task }) => task), ); - results.forEach((result, index) => { + for (const [index, result] of results.entries()) { if (result.status === 'rejected') { + await trackError(result.reason); + const taskName = tasks[index]?.name ?? 'synchronize'; - this.#logger.logErrorWithDetails(`Failed to ${taskName}`, { + this.#logger.warn(`Failed to ${taskName}`, { error: result.reason, }); } - }); + } } /** @@ -129,7 +132,9 @@ export class SynchronizeService { try { await this.#assetMetadataService.synchronize(scope); } catch (error: unknown) { - this.#logger.logErrorWithDetails('Failed to synchronize assets', { + await trackError(error); + + this.#logger.warn('Failed to synchronize assets', { error, }); } @@ -141,15 +146,18 @@ export class SynchronizeService { * @param scope - CAIP-2 network to load asset metadata for. * @returns The full asset catalog. */ - async #loadSep41Assets( + async #loadSep41AssetsSafe( scope: KnownCaip2ChainId, ): Promise { try { return await this.#assetMetadataService.fetchSep41AssetsOrSyncOnce(scope); } catch (error: unknown) { - this.#logger.logErrorWithDetails('Failed to load SEP-41 assets', { + await trackError(error); + + this.#logger.warn('Failed to load SEP-41 assets', { error, }); + return []; } } @@ -161,7 +169,7 @@ export class SynchronizeService { * @param scope - CAIP-2 network to query. * @returns Pairs keyed for SEP-41 sync and persistence. */ - async #loadActivatedPairs( + async #loadActivatedPairsSafe( accounts: StellarKeyringAccount[], scope: KnownCaip2ChainId, ): Promise { @@ -169,31 +177,33 @@ export class SynchronizeService { noOfAccounts: accounts.length, }); - const pairs: ActivatedAccountPair[] = []; - - const results = await Promise.allSettled( - accounts.map(async (account) => ({ - keyringAccount: account, - onChainAccount: await this.#onChainAccountService.resolveOnChainAccount( - account.address, - scope, - ), - })), + const results = await Promise.all( + accounts.map(async (account): Promise => { + try { + return { + keyringAccount: account, + onChainAccount: + await this.#onChainAccountService.resolveOnChainAccount( + account.address, + scope, + ), + }; + } catch (error: unknown) { + // Only capture the error if it is unexpected. + // AccountNotActivatedException is expected when the account is not activated yet. + if (!(error instanceof AccountNotActivatedException)) { + await trackError(error); + + this.#logger.warn('Failed to load account for sync', { error }); + } + return null; + } + }), ); - results.forEach((result, index) => { - if (result.status === 'fulfilled') { - pairs.push(result.value); - return; - } - if (result.reason instanceof AccountNotActivatedException) { - return; - } - this.#logger.logErrorWithDetails('Failed to load account for sync', { - accountId: accounts[index]?.id, - error: result.reason, - }); - }); + const pairs: ActivatedAccountPair[] = results.filter( + (result): result is ActivatedAccountPair => result !== null, + ); this.#logger.debug('number of activated account pairs', { noOfAccounts: pairs.length, diff --git a/packages/snap/src/services/transaction/Transaction.ts b/packages/snap/src/services/transaction/Transaction.ts index 5a3a21f0..1b15a3d9 100644 --- a/packages/snap/src/services/transaction/Transaction.ts +++ b/packages/snap/src/services/transaction/Transaction.ts @@ -11,7 +11,10 @@ import { import { BigNumber } from 'bignumber.js'; import { StellarOperationType } from './api'; -import { TransactionDeserializationException } from './exceptions'; +import { + TransactionDeserializationException, + TransactionServiceException, +} from './exceptions'; import { parseExpirationMaxTime } from './utils'; import type { KnownCaip2ChainId } from '../../api'; import { bufferToUint8Array } from '../../utils'; @@ -236,7 +239,7 @@ export class Transaction { } if (!feeSource) { - throw new Error('Fee source account is not set'); + throw new TransactionServiceException('Fee source account is not set'); } return feeSource; @@ -372,8 +375,10 @@ export class Transaction { caip2ChainIdToNetwork(scope), ); return Transaction.fromRaw(decoded); - } catch { - throw new TransactionDeserializationException(); + } catch (error: unknown) { + throw new TransactionDeserializationException({ + cause: error, + }); } } diff --git a/packages/snap/src/services/transaction/TransactionBuilder.ts b/packages/snap/src/services/transaction/TransactionBuilder.ts index d1b3507b..74f44996 100644 --- a/packages/snap/src/services/transaction/TransactionBuilder.ts +++ b/packages/snap/src/services/transaction/TransactionBuilder.ts @@ -89,12 +89,9 @@ export class TransactionBuilder { fee: baseFee, }); } catch (error: unknown) { - this.#logger.logErrorWithDetails( - 'Failed to build change trust transaction', - error, - ); throw new TransactionBuilderException( 'Failed to build change trust transaction', + { cause: error }, ); } } @@ -147,12 +144,9 @@ export class TransactionBuilder { fee: BASE_FEE.toString(), }); } catch (error: unknown) { - this.#logger.logErrorWithDetails( - 'Failed to build sep41 transfer transaction', - error, - ); throw new TransactionBuilderException( 'Failed to build sep41 transfer transaction', + { cause: error }, ); } } diff --git a/packages/snap/src/services/transaction/TransactionMapper.ts b/packages/snap/src/services/transaction/TransactionMapper.ts index 870d0d1c..4d4e4090 100644 --- a/packages/snap/src/services/transaction/TransactionMapper.ts +++ b/packages/snap/src/services/transaction/TransactionMapper.ts @@ -23,7 +23,8 @@ import { } from './utils'; import type { SuccessfulTransactionResult } from './xdrParser'; import { - parseSep41TransferInvokeSafe, + isSep41TransferInvoke, + parseSep41TransferInvoke, parseSuccessfulTransactionResult, TransactionResultType, } from './xdrParser'; @@ -101,8 +102,10 @@ export class TransactionMapper { return this.#mapTransaction(transaction, keyringAccount, assetMetadata); } catch (error) { + // Best effort to map a transaction. + // The error is not necessary to track. // Log and fall back to unknown so batch mapping can continue for other transactions. - this.#logger.logErrorWithDetails( + this.#logger.warn( 'Unable to map a transaction, mapping to an unknown transaction', { error, @@ -444,58 +447,49 @@ export class TransactionMapper { keyringAccount: StellarKeyringAccount, assetMetadata: Record, ): KeyringTransaction | undefined { - try { - const { scope } = transaction; - const [firstOperation] = transaction.transactionOperations; - - if (!isInvokeHostFunctionOperation(firstOperation)) { - return undefined; - } + const { scope } = transaction; + const [firstOperation] = transaction.transactionOperations; - const parsedSep41TransferInvoke = parseSep41TransferInvokeSafe( - firstOperation, - scope, - ); + if (!isInvokeHostFunctionOperation(firstOperation)) { + return undefined; + } - if (!parsedSep41TransferInvoke) { - return undefined; - } + if (!isSep41TransferInvoke(firstOperation)) { + return undefined; + } - const { assetId, amount, toAccountId, fromAccountId } = - parsedSep41TransferInvoke; + const parsedSep41TransferInvoke = parseSep41TransferInvoke( + firstOperation, + scope, + ); - if (fromAccountId !== keyringAccount.address) { - return undefined; - } + const { assetId, amount, toAccountId, fromAccountId } = + parsedSep41TransferInvoke; - // TODO: Fall back to NetworkService token metadata when state is missing (RPC cost per tx). - const asset = assetMetadata[assetId]; - if (!asset) { - return undefined; - } + if (fromAccountId !== keyringAccount.address) { + return undefined; + } - const assetRow = this.#toKeyringAssetRow( - asset.symbol, - asset.assetId, - toDisplayBalance(amount, asset.units[0].decimals), - ); - return this.#keyringTransactionBuilder.createTransaction({ - type: KeyringTransactionType.Send, - request: { - ...this.#commonOnChainFields(transaction, keyringAccount), - asset: assetRow, - toAddress: toAccountId, - }, - }); - } catch (error) { - this.#logger.logErrorWithDetails( - 'Unable to map a SEP-41 send transaction', - { - error, - }, - ); + // TODO: Fall back to NetworkService token metadata when state is missing (RPC cost per tx). + const asset = assetMetadata[assetId]; + if (!asset) { return undefined; } + + const assetRow = this.#toKeyringAssetRow( + asset.symbol, + asset.assetId, + toDisplayBalance(amount, asset.units[0].decimals), + ); + + return this.#keyringTransactionBuilder.createTransaction({ + type: KeyringTransactionType.Send, + request: { + ...this.#commonOnChainFields(transaction, keyringAccount), + asset: assetRow, + toAddress: toAccountId, + }, + }); } #getCreateTime(transaction: Transaction): number { @@ -630,9 +624,13 @@ export class TransactionMapper { destAmount, ); } - } catch { + } catch (error) { + this.#logger.warn('Failed to get receive operation asset', { + error, + }); + // Best effort to get the receive operation asset, + // The error is not necessary to track. // Skip unsupported or unparseable operations; keep collecting other receive assets. - return null; } return null; @@ -660,7 +658,12 @@ export class TransactionMapper { } return successfulTransactionResult; - } catch { + } catch (error) { + // Best effort to parse the successful transaction result, + // The error is not necessary to track. + this.#logger.warn('Failed to parse successful transaction result', { + error, + }); return null; } } diff --git a/packages/snap/src/services/transaction/TransactionService.ts b/packages/snap/src/services/transaction/TransactionService.ts index 4d6ebee4..20c8ce04 100644 --- a/packages/snap/src/services/transaction/TransactionService.ts +++ b/packages/snap/src/services/transaction/TransactionService.ts @@ -26,7 +26,12 @@ import type { KnownCaip19Slip44Id, KnownCaip2ChainId, } from '../../api'; -import { getSnapProvider, isSep41Id, isSlip44Id } from '../../utils'; +import { + getSnapProvider, + isSep41Id, + isSlip44Id, + trackError, +} from '../../utils'; import type { ILogger } from '../../utils/logger'; import { createPrefixedLogger } from '../../utils/logger'; import type { AccountService } from '../account'; @@ -486,10 +491,9 @@ export class TransactionService { try { return await this.savePendingKeyringTransaction(request); } catch (error: unknown) { - this.#logger.logErrorWithDetails( - 'Failed to save pending transaction', - error, - ); + await trackError(error); + + this.#logger.warn('Failed to save pending transaction', { error }); return null; } } diff --git a/packages/snap/src/services/transaction/TransactionSynchronizeService.ts b/packages/snap/src/services/transaction/TransactionSynchronizeService.ts index 72f4b496..f8f90684 100644 --- a/packages/snap/src/services/transaction/TransactionSynchronizeService.ts +++ b/packages/snap/src/services/transaction/TransactionSynchronizeService.ts @@ -355,6 +355,7 @@ export class TransactionSynchronizeService { transactionIdsToFetch, }); + // TODO: adding a max reconcile limit to avoid the sync running forever. const fetchResults = await batchesAllSettled( transactionIdsToFetch, 10, @@ -369,10 +370,9 @@ export class TransactionSynchronizeService { for (const fetchResult of fetchResults) { if (fetchResult.status === 'rejected') { - this.#logger.logErrorWithDetails( - 'Failed to fetch on-chain transaction', - fetchResult.reason, - ); + this.#logger.warn('Failed to fetch on-chain transaction', { + error: fetchResult.reason, + }); continue; } @@ -386,7 +386,7 @@ export class TransactionSynchronizeService { ); if (!keyringAccount) { - this.#logger.logErrorWithDetails( + this.#logger.warn( 'Failed to find keyring account for pending transaction', { transactionId, accountId: pendingFromState.account }, ); @@ -507,11 +507,10 @@ export class TransactionSynchronizeService { mappedReceive, ); } catch (error) { + // Best effort to append the SEP-41 receive mapping, + // The error is not necessary to track. // Not throwing the error to avoid blocking the sync. - this.#logger.logErrorWithDetails( - 'Failed to append SEP-41 receive mapping', - { error }, - ); + this.#logger.warn('Failed to append SEP-41 receive mapping', { error }); } } diff --git a/packages/snap/src/services/transaction/exceptions.ts b/packages/snap/src/services/transaction/exceptions.ts index de1b82b1..7bfb88fd 100644 --- a/packages/snap/src/services/transaction/exceptions.ts +++ b/packages/snap/src/services/transaction/exceptions.ts @@ -2,30 +2,23 @@ import type { KnownCaip19AssetIdOrSlip44Id, KnownCaip2ChainId, } from '../../api'; +import type { StellarSnapExceptionOptions } from '../../utils'; +import { StellarSnapException } from '../../utils'; + +export class TransactionServiceException extends StellarSnapException {} /** Thrown when building or rebuilding a transaction fails (e.g. invalid asset or SDK error). */ -export class TransactionBuilderException extends Error { - constructor(message: string) { - super(message); - this.name = 'TransactionBuilderException'; - } -} +export class TransactionBuilderException extends TransactionServiceException {} /** Thrown when deserializing a transaction fails. */ -export class TransactionDeserializationException extends Error { - constructor(message: string = 'Failed to deserialize transaction') { - super(message); - this.name = 'TransactionDeserializationException'; +export class TransactionDeserializationException extends TransactionServiceException { + constructor(options?: StellarSnapExceptionOptions) { + super('Failed to deserialize transaction', options); } } /** Base for all transaction validation errors (simulation, trustlines, balances). */ -export class TransactionValidationException extends Error { - constructor(message: string) { - super(message); - this.name = 'TransactionValidationException'; - } -} +export class TransactionValidationException extends TransactionServiceException {} /** * Thrown when a caller-supplied CAIP-2 scope does not match the transaction envelope's network. @@ -42,7 +35,6 @@ export class TransactionScopeNotMatchException extends TransactionValidationExce super( `Transaction scope ${transactionScope} does not match expected scope ${expectedScope}`, ); - this.name = 'TransactionScopeNotMatchException'; this.expectedScope = expectedScope; this.transactionScope = transactionScope; } @@ -51,7 +43,6 @@ export class TransactionScopeNotMatchException extends TransactionValidationExce export class TransactionExpireException extends TransactionValidationException { constructor(expirationTime: number) { super(`Transaction expired (maxTime: ${expirationTime})`); - this.name = 'TransactionExpireException'; } } @@ -76,8 +67,8 @@ export class InvalidAssetForSep41TransferException extends TransactionValidation /** Thrown when the account requires a memo. */ export class RequiresMemoException extends TransactionValidationException { - constructor(accountId: string) { - super(`Account ${accountId} requires a memo`); + constructor(destAccountAddress: string) { + super(`Account ${destAccountAddress} requires a memo`); } } @@ -92,7 +83,6 @@ export class InvalidAmountForCreateAccountException extends TransactionValidatio } } -/** Thrown when the trustline is not found. */ /** * Thrown when a payment uses a trustline that exists but is not authorized (`is_authorized` false). */ @@ -110,6 +100,7 @@ export class TrustlineNotAuthorizedException extends TransactionValidationExcept } } +/** Thrown when the trustline is not found. */ export class TrustlineNotFoundException extends TransactionValidationException { /** CAIP-19 asset id (or slip44 id for native) for the missing trustline. */ readonly assetId: KnownCaip19AssetIdOrSlip44Id; @@ -179,6 +170,7 @@ export class InsufficientBalanceToCoverFeeException extends TransactionValidatio } } +/** Thrown when the account's native balance is below the required base reserve. */ export class InsufficientBalanceToCoverBaseReserveException extends TransactionValidationException { readonly balance: string; @@ -192,6 +184,7 @@ export class InsufficientBalanceToCoverBaseReserveException extends TransactionV this.required = required; } } + /** * Thrown when the account's spendable balance for an asset is below the amount required * by the transaction. The asset can be native or non-native, and amounts are in @@ -214,36 +207,17 @@ export class InsufficientBalanceException extends TransactionValidationException } } -/** - * Thrown when the invoke host function transaction has more than one operation. - */ +/** Thrown when the invoke host function transaction has more than one operation. */ export class InvalidInvokeContractStructureException extends TransactionValidationException { constructor() { super(`Invoke host function transaction must have exactly one operation`); } } -/** - * Thrown when the keyring transaction builder fails to create a transaction. - */ -export class KeyringTransactionBuilderException extends Error { - constructor(message: string) { - super(message); - this.name = 'KeyringTransactionBuilderException'; - } -} +/** Thrown when the keyring transaction builder fails to create a transaction. */ +export class KeyringTransactionBuilderException extends TransactionServiceException {} /** Thrown when transaction mapping cannot proceed (invalid shape, missing metadata, etc.). */ -export class TransactionMapperException extends Error { - constructor(message: string) { - super(message); - this.name = 'TransactionMapperException'; - } -} +export class TransactionMapperException extends TransactionServiceException {} /** Thrown when Soroban XDR or ScVal parsing fails. */ -export class XdrParseException extends Error { - constructor(message: string) { - super(message); - this.name = 'XdrParseException'; - } -} +export class XdrParseException extends TransactionServiceException {} diff --git a/packages/snap/src/services/transaction/simulation/simulators.ts b/packages/snap/src/services/transaction/simulation/simulators.ts index bf9f5f6f..a7b934ef 100644 --- a/packages/snap/src/services/transaction/simulation/simulators.ts +++ b/packages/snap/src/services/transaction/simulation/simulators.ts @@ -30,7 +30,6 @@ import { InvalidTrustlineException, RemoveTrustlineWithNonZeroBalanceException, TransactionValidationException, - XdrParseException, TrustlineNotAuthorizedException, TrustlineNotFoundException, UpdateTrustlineException, @@ -632,15 +631,11 @@ export class InvokeHostFunctionOPSimulator implements OperationSimulator { return; } - let parsed; - try { - parsed = parseSep41TransferInvoke(op, scope); - } catch (error) { - if (error instanceof XdrParseException) { - throw new TransactionValidationException(error.message); - } - throw error; - } + // Let XdrParseException propagate as-is: it signals an internal XDR/ScVal + // parsing failure, not a user-facing validation outcome like insufficient + // balance. confirmSend tracks these via trackError; onAmountInput returns + // invalid without tracking to reduce Sentry noise during amount entry. + const parsed = parseSep41TransferInvoke(op, scope); const { fromAccountId, assetId, amount } = parsed; diff --git a/packages/snap/src/services/transaction/utils.ts b/packages/snap/src/services/transaction/utils.ts index 740f3def..35a2ccd8 100644 --- a/packages/snap/src/services/transaction/utils.ts +++ b/packages/snap/src/services/transaction/utils.ts @@ -167,13 +167,13 @@ export function assertTransactionTimeBound(transaction: Transaction): void { * Throws when `destRequiresMemo` is true and the envelope has no memo (SEP-29). * * @param transaction - Wrapped Stellar transaction. - * @param destAccountId - Payment or path-payment destination. + * @param destAccountAddress - Payment or path-payment destination. * @param destRequiresMemo - From {@link OnChainAccount.requiresMemo} or simulation state. * @throws {RequiresMemoException} When a memo is required but missing or blank. */ export function assertMemoWhenDestinationRequires( transaction: Transaction, - destAccountId: string, + destAccountAddress: string, destRequiresMemo: boolean, ): void { const memo = transaction.getMemo(); @@ -182,7 +182,7 @@ export function assertMemoWhenDestinationRequires( if (!destRequiresMemo || (memo !== null && /\S/u.test(memo))) { return; } - throw new RequiresMemoException(destAccountId); + throw new RequiresMemoException(destAccountAddress); } /** @@ -193,7 +193,7 @@ export function assertMemoWhenDestinationRequires( * @returns The CAIP-19 id, or `null` when the reference cannot be parsed * (e.g. liquidity pool ids that arrive on `setTrustLineFlags` / `revokeSponsorship`). */ -export function parseOperationAssetReference( +export function parseOperationAssetReferenceSafe( scope: KnownCaip2ChainId, assetReference: string, ): KnownCaip19AssetIdOrSlip44Id | null { @@ -246,7 +246,7 @@ export function collectTransactionAssetCaipIds( if (reference === null) { continue; } - const assetId = parseOperationAssetReference(scope, reference); + const assetId = parseOperationAssetReferenceSafe(scope, reference); if (assetId !== null) { ids.add(assetId); } diff --git a/packages/snap/src/services/transaction/xdrParser.test.ts b/packages/snap/src/services/transaction/xdrParser.test.ts index 57afae7a..d246c346 100644 --- a/packages/snap/src/services/transaction/xdrParser.test.ts +++ b/packages/snap/src/services/transaction/xdrParser.test.ts @@ -15,7 +15,6 @@ import { XdrParseException } from './exceptions'; import { isSep41TransferInvoke, parseSep41TransferInvoke, - parseSep41TransferInvokeSafe, parseSuccessfulTransactionResult, TransactionResultType, xdrAssetToCaip19, @@ -35,10 +34,10 @@ describe('transaction-xdr-decoder', () => { const usdcIssuer = 'GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN'; describe('parseSuccessfulTransactionResult', () => { - it('returns null for invalid xdr', () => { - expect( + it('throws XdrParseException for invalid xdr', () => { + expect(() => parseSuccessfulTransactionResult('not-valid-xdr', scope), - ).toBeNull(); + ).toThrow(XdrParseException); }); it('parses pathPaymentStrictSendSuccess from single-op swap', () => { @@ -246,23 +245,5 @@ describe('transaction-xdr-decoder', () => { 'Invalid transfer function arguments', ); }); - - it('returns null from parseSep41TransferInvokeSafe for non-transfer invoke', () => { - const operation = buildTransferInvokeOperation('balance', [ - fromAccountId, - ]); - - expect(parseSep41TransferInvokeSafe(operation, scope)).toBeNull(); - }); - - it('returns null from parseSep41TransferInvokeSafe when from address is invalid', () => { - const operation = buildTransferInvokeOperation( - 'transfer', - [42, toAccountId, '1'], - [{ type: 'u32' }, { type: 'address' }, { type: 'i128' }], - ); - - expect(parseSep41TransferInvokeSafe(operation, scope)).toBeNull(); - }); }); }); diff --git a/packages/snap/src/services/transaction/xdrParser.ts b/packages/snap/src/services/transaction/xdrParser.ts index 26ee28da..fbbf76a2 100644 --- a/packages/snap/src/services/transaction/xdrParser.ts +++ b/packages/snap/src/services/transaction/xdrParser.ts @@ -18,6 +18,7 @@ import { } from '../../api'; import { getSlip44AssetId, + rethrowIfInstanceElseThrow, toCaip19ClassicAssetId, toCaip19Sep41AssetId, toDisplayBalance, @@ -97,7 +98,8 @@ export type ParsedSep41TransferInvoke = { * * @param xdrString - Base64-encoded `TransactionResult` XDR from Horizon. * @param scope - CAIP-2 chain used to encode parsed asset ids. - * @returns Parsed fee and per-operation path payment results, or `null` when parsing fails. + * @returns Parsed fee and per-operation path payment results, or `null` when the result is not `txSuccess`. + * @throws {XdrParseException} When the XDR envelope cannot be parsed. */ export function parseSuccessfulTransactionResult( xdrString: string, @@ -164,8 +166,11 @@ export function parseSuccessfulTransactionResult( operationResults, feeCharged, }; - } catch { - return null; + } catch (error) { + throw new XdrParseException( + 'Failed to parse successful transaction result', + { cause: error }, + ); } } @@ -201,69 +206,58 @@ export function parseSep41TransferInvoke( op: Operation.InvokeHostFunction, scope: KnownCaip2ChainId, ): ParsedSep41TransferInvoke { - const { func } = op; - if (!func || func.switch().name !== 'hostFunctionTypeInvokeContract') { - throw new XdrParseException('Not an invoke contract operation'); - } - const ic = func.invokeContract(); - if (ic.functionName().toString() !== 'transfer') { - throw new XdrParseException('Contract is not a transfer function'); - } - - const args = ic.args(); - if ( - args.length !== 3 || - args[0] === undefined || - args[1] === undefined || - args[2] === undefined - ) { - throw new XdrParseException('Invalid transfer function arguments'); - } + try { + const { func } = op; + if (!func || func.switch().name !== 'hostFunctionTypeInvokeContract') { + throw new XdrParseException('Not an invoke contract operation'); + } + const ic = func.invokeContract(); + if (ic.functionName().toString() !== 'transfer') { + throw new XdrParseException('Contract is not a transfer function'); + } - const contractAddr = Address.fromScAddress(ic.contractAddress()).toString(); + const args = ic.args(); + if ( + args.length !== 3 || + args[0] === undefined || + args[1] === undefined || + args[2] === undefined + ) { + throw new XdrParseException('Invalid transfer function arguments'); + } - const fromNative = scValToNative(args[0]); - const toNative = scValToNative(args[1]); - const amountNative = scValToNative(args[2]); - if ( - typeof fromNative !== 'string' || - !StellarAddressOrContractStruct.is(fromNative) - ) { - throw new XdrParseException('Invalid from address'); - } - if ( - typeof toNative !== 'string' || - !StellarAddressOrContractStruct.is(toNative) - ) { - throw new XdrParseException('Invalid to address'); - } + const contractAddr = Address.fromScAddress(ic.contractAddress()).toString(); - return { - assetId: toCaip19Sep41AssetId(scope, contractAddr), - fromAccountId: fromNative, - toAccountId: toNative, - amount: parseScValToNative(amountNative), - }; -} + const fromNative = scValToNative(args[0]); + const toNative = scValToNative(args[1]); + const amountNative = scValToNative(args[2]); + if ( + typeof fromNative !== 'string' || + !StellarAddressOrContractStruct.is(fromNative) + ) { + throw new XdrParseException('Invalid from address'); + } + if ( + typeof toNative !== 'string' || + !StellarAddressOrContractStruct.is(toNative) + ) { + throw new XdrParseException('Invalid to address'); + } -/** - * Parses a SEP-41 transfer invoke without throwing. - * - * @param op - Parsed `invokeHostFunction` operation. - * @param scope - CAIP-2 chain id (must match the envelope network when matching preload keys). - * @returns Parsed transfer metadata, or `null` if the shape does not match. - */ -export function parseSep41TransferInvokeSafe( - op: Operation.InvokeHostFunction, - scope: KnownCaip2ChainId, -): ParsedSep41TransferInvoke | null { - if (!isSep41TransferInvoke(op)) { - return null; - } - try { - return parseSep41TransferInvoke(op, scope); - } catch { - return null; + return { + assetId: toCaip19Sep41AssetId(scope, contractAddr), + fromAccountId: fromNative, + toAccountId: toNative, + amount: parseScValToNative(amountNative), + }; + } catch (error) { + return rethrowIfInstanceElseThrow( + error, + [XdrParseException], + new XdrParseException('Failed to parse SEP-41 transfer invoke', { + cause: error, + }), + ); } } @@ -409,11 +403,13 @@ export function extractAssetDataFromContractData( return assetData; } catch (error) { - if (error instanceof XdrParseException) { - throw error; - } - throw new XdrParseException( - `Error extracting asset data from contract ${contractAddress}`, + return rethrowIfInstanceElseThrow( + error, + [XdrParseException], + new XdrParseException( + `Error extracting asset data from contract ${contractAddress}`, + { cause: error }, + ), ); } } diff --git a/packages/snap/src/ui/confirmation/utils.ts b/packages/snap/src/ui/confirmation/utils.ts index 55c641d2..a9ad3b30 100644 --- a/packages/snap/src/ui/confirmation/utils.ts +++ b/packages/snap/src/ui/confirmation/utils.ts @@ -7,7 +7,7 @@ import type { KnownCaip19AssetIdOrSlip44Id } from '../../api'; import { KnownCaip2ChainId } from '../../api'; import { AppConfig } from '../../config'; import { getNativeAssetMetadata } from '../../services/asset-metadata/utils'; -import { parseOperationAssetReference } from '../../services/transaction/utils'; +import { parseOperationAssetReferenceSafe } from '../../services/transaction/utils'; import { TransactionScanValidationType, type TransactionScanResult, @@ -336,7 +336,7 @@ export function resolveAssetDisplay( scope: KnownCaip2ChainId, assetReference: string, ): ResolvedAssetDisplay | null { - const assetId = parseOperationAssetReference(scope, assetReference); + const assetId = parseOperationAssetReferenceSafe(scope, assetReference); if (assetId === null) { return null; } @@ -349,7 +349,7 @@ export function resolveAssetDisplay( iconUrl: xlmIcon, }; } - // Safe: parseOperationAssetReference returned non-null for a non-native ref, + // Safe: parseOperationAssetReferenceSafe returned non-null for a non-native ref, // so the reference is a parseable classic CODE-ISSUER pair. const { assetCode } = parseClassicAssetCodeIssuer(assetReference); // TODO: resolve classic-asset iconUrl via AssetMetadataService once From 15535cadebc5c2543a74e9120d5d0c6e477adcd4 Mon Sep 17 00:00:00 2001 From: Stanley Yuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Wed, 24 Jun 2026 18:35:44 +0800 Subject: [PATCH 2/3] fix: comment --- packages/snap/src/services/sync/SynchronizeService.ts | 6 +++++- packages/snap/src/services/transaction/TransactionMapper.ts | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/snap/src/services/sync/SynchronizeService.ts b/packages/snap/src/services/sync/SynchronizeService.ts index 3fcc2cf8..9354ba6b 100644 --- a/packages/snap/src/services/sync/SynchronizeService.ts +++ b/packages/snap/src/services/sync/SynchronizeService.ts @@ -194,7 +194,11 @@ export class SynchronizeService { if (!(error instanceof AccountNotActivatedException)) { await trackError(error); - this.#logger.warn('Failed to load account for sync', { error }); + this.#logger.warn('Failed to load account for sync', { + error, + accountAddress: account.address, + scope, + }); } return null; } diff --git a/packages/snap/src/services/transaction/TransactionMapper.ts b/packages/snap/src/services/transaction/TransactionMapper.ts index 4d4e4090..d2e6d0b5 100644 --- a/packages/snap/src/services/transaction/TransactionMapper.ts +++ b/packages/snap/src/services/transaction/TransactionMapper.ts @@ -627,6 +627,8 @@ export class TransactionMapper { } catch (error) { this.#logger.warn('Failed to get receive operation asset', { error, + transactionId: transaction.id, + operationIndex: index, }); // Best effort to get the receive operation asset, // The error is not necessary to track. @@ -663,6 +665,7 @@ export class TransactionMapper { // The error is not necessary to track. this.#logger.warn('Failed to parse successful transaction result', { error, + transactionId: transaction.id, }); return null; } From 7a16b85edff537f5cc4c9d2af69aee99be0cf3f9 Mon Sep 17 00:00:00 2001 From: Stanley Yuen <102275989+stanleyyconsensys@users.noreply.github.com> Date: Thu, 25 Jun 2026 16:08:34 +0800 Subject: [PATCH 3/3] feat: reconcile txn with max age and attempt --- packages/snap/.env.example | 8 +- packages/snap/snap.config.ts | 2 + packages/snap/src/config.ts | 16 + .../snap/src/services/network/exceptions.ts | 3 + .../transaction/TransactionRepository.test.ts | 203 +++++++++++++ .../transaction/TransactionRepository.ts | 60 +++- .../TransactionSynchronizeService.test.ts | 126 +++++++- .../TransactionSynchronizeService.ts | 286 +++++++++--------- packages/snap/src/services/transaction/api.ts | 10 + .../snap/src/services/transaction/utils.ts | 85 +++++- 10 files changed, 627 insertions(+), 172 deletions(-) create mode 100644 packages/snap/src/services/transaction/TransactionRepository.test.ts diff --git a/packages/snap/.env.example b/packages/snap/.env.example index e4dd7855..99862ed4 100644 --- a/packages/snap/.env.example +++ b/packages/snap/.env.example @@ -74,4 +74,10 @@ SECURITY_ALERTS_API_BASE_URL=https://security-alerts.api.cx.metamask.io #MAX_FEE_THRESHOLD_IN_XLM= # Maximum background reschedules for the track-transaction cron job while Horizon has not -#TRACK_TRANSACTION_MAX_RESCHEDULES=10 \ No newline at end of file +#TRACK_TRANSACTION_MAX_RESCHEDULES=10 + +# Maximum number of reconcile attempts for a pending transaction +#MAX_RECONCILE_ATTEMPTS=5 + +# Maximum age of a pending transaction in milliseconds +#MAX_PENDING_TRANSACTION_AGE=30000 \ No newline at end of file diff --git a/packages/snap/snap.config.ts b/packages/snap/snap.config.ts index 65816dd3..8d1fc1e2 100644 --- a/packages/snap/snap.config.ts +++ b/packages/snap/snap.config.ts @@ -44,6 +44,8 @@ const config: SnapConfig = { BASE_FEE_MULTIPLIER: process.env.BASE_FEE_MULTIPLIER ?? '', SIMULATION_FEE_MULTIPLIER: process.env.SIMULATION_FEE_MULTIPLIER ?? '', MAX_FEE_THRESHOLD_IN_XLM: process.env.MAX_FEE_THRESHOLD_IN_XLM ?? '', + MAX_RECONCILE_ATTEMPTS: process.env.MAX_RECONCILE_ATTEMPTS ?? '', + MAX_PENDING_TRANSACTION_AGE: process.env.MAX_PENDING_TRANSACTION_AGE ?? '', }, polyfills: true, }; diff --git a/packages/snap/src/config.ts b/packages/snap/src/config.ts index 1b281b3e..312ec059 100644 --- a/packages/snap/src/config.ts +++ b/packages/snap/src/config.ts @@ -110,6 +110,20 @@ const ConfigStruct = object({ * The maximum fee threshold in XLM for the Stellar network. */ maxFeeThresholdInXLM: parseFloatStruct(1, 1), + /** + * The maximum number of Horizon not-found reconcile attempts for a pending transaction. + * Used with `maxPendingTransactionAge` to evict stale pending txs from snap state; + * both limits must be exceeded before a pending tx is dropped. + * Minimum value is 2 to avoid dropping the pending transaction too early. + */ + maxReconcileAttempts: parseIntegerStruct(2, 5), + /** + * The maximum age of a pending transaction in milliseconds. + * Used with `maxReconcileAttempts` to evict stale pending txs from snap state; + * both limits must be exceeded before a pending tx is dropped. + * Minimum value is 15000 to avoid dropping the pending transaction too early. + */ + maxPendingTransactionAge: parseIntegerStruct(15000, 30000), }), api: object({ tokenApi: object({ @@ -185,6 +199,8 @@ export const AppConfig = create( maxFeeThresholdInXLM: process.env.MAX_FEE_THRESHOLD_IN_XLM, trackTransactionMaxReschedules: process.env.TRACK_TRANSACTION_MAX_RESCHEDULES, + maxReconcileAttempts: process.env.MAX_RECONCILE_ATTEMPTS, + maxPendingTransactionAge: process.env.MAX_PENDING_TRANSACTION_AGE, }, api: { tokenApi: { diff --git a/packages/snap/src/services/network/exceptions.ts b/packages/snap/src/services/network/exceptions.ts index 8b622610..c17b4c62 100644 --- a/packages/snap/src/services/network/exceptions.ts +++ b/packages/snap/src/services/network/exceptions.ts @@ -73,8 +73,11 @@ export class TransactionRetryableException extends TransactionSendException {} /** Thrown when a transaction is not found. */ export class TransactionNotFoundException extends NetworkServiceException { + readonly transactionHash: string; + constructor(transactionHash: string, options?: StellarSnapExceptionOptions) { super(`Transaction ${transactionHash} not found`, options); + this.transactionHash = transactionHash; } } diff --git a/packages/snap/src/services/transaction/TransactionRepository.test.ts b/packages/snap/src/services/transaction/TransactionRepository.test.ts new file mode 100644 index 00000000..89dd7d83 --- /dev/null +++ b/packages/snap/src/services/transaction/TransactionRepository.test.ts @@ -0,0 +1,203 @@ +import { TransactionStatus } from '@metamask/keyring-api'; + +import { KnownCaip2ChainId } from '../../api'; +import { AppConfig } from '../../config'; +import { generateMockTransactions } from './__mocks__/transaction.fixtures'; +import type { StellarKeyringTransaction } from './api'; +import type { TransactionStateValue } from './TransactionRepository'; +import { TransactionRepository } from './TransactionRepository'; +import { getSnapProvider } from '../../utils/snap'; +import { State } from '../state/State'; + +jest.mock('../../utils/snap'); + +describe('TransactionRepository', () => { + const scope = KnownCaip2ChainId.Mainnet; + const accountId = 'account-1'; + + const defaultState: TransactionStateValue = { + transactions: {}, + lastScanTokens: {}, + }; + + const recentTimestampSeconds = () => Math.floor(Date.now() / 1000); + + const expiredTimestampSeconds = () => + Math.floor( + (Date.now() - AppConfig.transaction.maxPendingTransactionAge - 1000) / + 1000, + ); + + let mockState: TransactionStateValue; + + const createRepository = () => + new TransactionRepository( + new State({ + encrypted: false, + defaultState, + }), + ); + + beforeEach(() => { + mockState = structuredClone(defaultState); + const snapProvider = getSnapProvider() as { request: jest.Mock }; + snapProvider.request.mockImplementation(async ({ method, params }) => { + if (method === 'snap_getState') { + if (params.key) { + return mockState[params.key as keyof TransactionStateValue]; + } + return mockState; + } + + if (method === 'snap_manageState' && params.operation === 'update') { + mockState = params.newState as TransactionStateValue; + } + + return null; + }); + }); + + afterEach(() => { + (getSnapProvider() as { request: jest.Mock }).request.mockReset(); + }); + + it('removes confirmed incoming transactions from snap state', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + })[0] as StellarKeyringTransaction; + + await repository.saveMany([pendingTransaction]); + + const confirmedTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Confirmed, + })[0] as StellarKeyringTransaction; + + await repository.saveMany([confirmedTransaction]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([]); + }); + + it('keeps incoming pending over existing when reconcileAttemptCount is higher', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: 100, + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 1, + } as StellarKeyringTransaction, + ]); + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 2, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([ + expect.objectContaining({ + id: 'tx-hash-1', + reconcileAttemptCount: 2, + }), + ]); + }); + + it('drops pending transactions when reconcile attempts and max age are both exceeded', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: expiredTimestampSeconds(), + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 5, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([]); + }); + + it('keeps pending transactions when reconcile attempts exceeded but max age is not', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: recentTimestampSeconds(), + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 5, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([ + expect.objectContaining({ + id: 'tx-hash-1', + reconcileAttemptCount: 5, + }), + ]); + }); + + it('keeps pending transactions when max age exceeded but reconcile attempts are not', async () => { + const repository = createRepository(); + const pendingTransaction = generateMockTransactions(1, { + id: 'tx-hash-1', + account: accountId, + scope, + status: TransactionStatus.Unconfirmed, + timestamp: expiredTimestampSeconds(), + })[0] as StellarKeyringTransaction; + + await repository.saveMany([ + { + ...pendingTransaction, + reconcileAttemptCount: 1, + } as StellarKeyringTransaction, + ]); + + const stored = await repository.findStellarTransactionsByAccountIds([ + accountId, + ]); + expect(stored).toStrictEqual([ + expect.objectContaining({ + id: 'tx-hash-1', + reconcileAttemptCount: 1, + }), + ]); + }); +}); diff --git a/packages/snap/src/services/transaction/TransactionRepository.ts b/packages/snap/src/services/transaction/TransactionRepository.ts index fdb7381c..db5d8151 100644 --- a/packages/snap/src/services/transaction/TransactionRepository.ts +++ b/packages/snap/src/services/transaction/TransactionRepository.ts @@ -3,12 +3,17 @@ import { groupBy } from 'lodash'; import sortBy from 'lodash/sortBy'; import uniqBy from 'lodash/uniqBy'; -import { isPendingTransactionStatus } from './utils'; +import type { StellarKeyringTransaction } from './api'; +import { + isPendingTransactionStatus, + shouldDropPendingTransaction, + toKeyringTransactions, +} from './utils'; import type { KnownCaip2ChainId } from '../../api'; import type { State } from '../state/State'; export type TransactionStateValue = { - transactions: Record; + transactions: Record; lastScanTokens: Record>; }; @@ -28,18 +33,29 @@ export class TransactionRepository { TransactionStateValue['transactions'] >(this.#stateKey); - return Object.values(transactionsByAccount ?? {}).flat(); + return toKeyringTransactions( + Object.values(transactionsByAccount ?? {}).flat(), + ); } async findByAccountIds( accountIds: string[], scope?: KnownCaip2ChainId, ): Promise { + return toKeyringTransactions( + await this.findStellarTransactionsByAccountIds(accountIds, scope), + ); + } + + async findStellarTransactionsByAccountIds( + accountIds: string[], + scope?: KnownCaip2ChainId, + ): Promise { const transactionsByAccount = await this.#state.getKey< TransactionStateValue['transactions'] >(this.#stateKey); - const transactions: KeyringTransaction[] = []; + const transactions: StellarKeyringTransaction[] = []; for (const accountId of accountIds) { const accountTransactions = transactionsByAccount?.[accountId] ?? []; transactions.push( @@ -58,7 +74,7 @@ export class TransactionRepository { TransactionStateValue['transactions'] >(this.#stateKey); - return transactionsByAccount?.[accountId] ?? []; + return toKeyringTransactions(transactionsByAccount?.[accountId] ?? []); } async findLastScanTokenByAccountIds( @@ -78,7 +94,7 @@ export class TransactionRepository { return lastScanTokenByAccountId; } - async save(transaction: KeyringTransaction): Promise { + async save(transaction: StellarKeyringTransaction): Promise { await this.saveMany([transaction]); } @@ -87,13 +103,14 @@ export class TransactionRepository { * * Submitted transactions are upserted locally. Confirmed and failed transactions * remove any matching id from snap state — durable history lives in the controller - * after AccountTransactionsUpdated is emitted. + * after AccountTransactionsUpdated is emitted. Pending transactions that exceed both + * `maxReconcileAttempts` and `maxPendingTransactionAge` are also evicted. * * @param transactions - Transactions to reconcile into snap state. * @param lastScanTokens - Optional scan cursors to persist per account and scope. */ async saveMany( - transactions: KeyringTransaction[], + transactions: StellarKeyringTransaction[], lastScanTokens?: Record>, ): Promise { if (transactions.length === 0 && !lastScanTokens) { @@ -142,16 +159,31 @@ export class TransactionRepository { } #applyTransactionUpdate( - existingTransactions: KeyringTransaction[], - incomingTransactions: KeyringTransaction[], - ): KeyringTransaction[] { + existingTransactions: StellarKeyringTransaction[], + incomingTransactions: StellarKeyringTransaction[], + ): StellarKeyringTransaction[] { const merged = [...incomingTransactions, ...existingTransactions]; - // Merge the transactions based on transaction id and keep the latest one + // Merge the transactions based on transaction id and choose the incoming one if there are duplicates // Sort it by timestamp in descending order - // Filter out the completed transactions + // Filter out completed transactions and pending txs eligible for eviction return sortBy( uniqBy(merged, 'id'), (item) => -(item.timestamp ?? 0), - ).filter((transaction) => isPendingTransactionStatus(transaction.status)); + ).filter((transaction) => this.#canSave(transaction)); + } + + /** + * Checks if the transaction can be saved to snap state. + * + * Keeps pending transactions unless both reconcile attempts and max age are exceeded. + * + * @param transaction - The transaction to check. + * @returns Whether the transaction can be saved to snap state. + */ + #canSave(transaction: StellarKeyringTransaction): boolean { + return ( + isPendingTransactionStatus(transaction.status) && + !shouldDropPendingTransaction(transaction) + ); } } diff --git a/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts b/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts index 3585e5d2..2e99da96 100644 --- a/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts +++ b/packages/snap/src/services/transaction/TransactionSynchronizeService.test.ts @@ -14,6 +14,7 @@ import { buildMockClassicTransaction, generateMockTransactions, } from './__mocks__/transaction.fixtures'; +import type { StellarKeyringTransaction } from './api'; import { KeyringTransactionBuilder } from './KeyringTransactionBuilder'; import { Transaction } from './Transaction'; import { TransactionMapper } from './TransactionMapper'; @@ -27,7 +28,7 @@ import type { AccountService } from '../account/AccountService'; import type { StellarAssetMetadata } from '../asset-metadata/api'; import { toStellarAssetMetadata } from '../asset-metadata/utils'; import { createMemoryCache } from '../cache/__mocks__/cache.fixtures'; -import { NetworkService } from '../network'; +import { NetworkService, TransactionNotFoundException } from '../network'; import { createMockAccountWithBalances, DEFAULT_MOCK_ACCOUNT_WITH_BALANCES, @@ -140,9 +141,9 @@ describe('TransactionSynchronizeService', () => { accountService, getTransactionsSpy: jest.spyOn(networkService, 'getTransactions'), getTransactionSpy: jest.spyOn(networkService, 'getTransaction'), - findByAccountIdsSpy: jest.spyOn( + findStellarTransactionsByAccountIdsSpy: jest.spyOn( transactionRepository, - 'findByAccountIds', + 'findStellarTransactionsByAccountIds', ), findLastScanTokenSpy: jest.spyOn( transactionRepository, @@ -204,7 +205,7 @@ describe('TransactionSynchronizeService', () => { const { service, getTransactionsSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, saveManySpy, emitSnapKeyringEventSpy, @@ -219,7 +220,7 @@ describe('TransactionSynchronizeService', () => { amount: '3', }); - findByAccountIdsSpy.mockResolvedValue([]); + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([]); findLastScanTokenSpy.mockResolvedValue({ [keyringAccount.id]: null, }); @@ -270,7 +271,7 @@ describe('TransactionSynchronizeService', () => { service, getTransactionsSpy, getTransactionSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, saveManySpy, } = setup(); @@ -290,7 +291,7 @@ describe('TransactionSynchronizeService', () => { status: TransactionStatus.Unconfirmed, }); - findByAccountIdsSpy.mockResolvedValue([ + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([ pendingTransaction as KeyringTransaction, ]); findLastScanTokenSpy.mockResolvedValue({ @@ -343,7 +344,7 @@ describe('TransactionSynchronizeService', () => { const { service, getTransactionsSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, saveManySpy, emitSnapKeyringEventSpy, @@ -359,7 +360,7 @@ describe('TransactionSynchronizeService', () => { scope, }); - findByAccountIdsSpy.mockResolvedValue([]); + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([]); findLastScanTokenSpy.mockResolvedValue({ [senderAccount.id]: null, }); @@ -438,7 +439,7 @@ describe('TransactionSynchronizeService', () => { const { service, getTransactionsSpy, - findByAccountIdsSpy, + findStellarTransactionsByAccountIdsSpy, findLastScanTokenSpy, emitSnapKeyringEventSpy, } = setup({ @@ -456,7 +457,7 @@ describe('TransactionSynchronizeService', () => { scope, }); - findByAccountIdsSpy.mockResolvedValue([]); + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([]); findLastScanTokenSpy.mockResolvedValue({ [senderAccount.id]: null, }); @@ -485,4 +486,107 @@ describe('TransactionSynchronizeService', () => { `transactions.${recipientAccount.id}`, ); }); + + it('increments reconcile attempt when pending transaction is not found on Horizon', async () => { + const { + service, + getTransactionsSpy, + getTransactionSpy, + findStellarTransactionsByAccountIdsSpy, + findLastScanTokenSpy, + saveManySpy, + } = setup(); + const { keyringAccount, activatedAccountPair } = buildActivatedPair( + 'account-sync-attempt', + 'GA7UCNSASSOPQYTRGJ2NC7TDBSXHMWK6JHS7AO6X2ZQAIQSTB5ELNFSO', + ); + const [pendingTransaction] = generateMockTransactions(1, { + id: 'missing-tx-hash', + account: keyringAccount.id, + scope, + status: TransactionStatus.Unconfirmed, + }); + + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([ + { + ...pendingTransaction, + reconcileAttemptCount: 1, + } as StellarKeyringTransaction, + ]); + findLastScanTokenSpy.mockResolvedValue({ + [keyringAccount.id]: 'existing-token', + }); + getTransactionsSpy.mockResolvedValue([]); + getTransactionSpy.mockRejectedValue( + new TransactionNotFoundException('missing-tx-hash'), + ); + + await service.synchronize([activatedAccountPair], scope, []); + + expect(getTransactionSpy).toHaveBeenCalledWith('missing-tx-hash', scope); + expect(saveManySpy).toHaveBeenCalledWith( + [ + expect.objectContaining({ + id: 'missing-tx-hash', + reconcileAttemptCount: 2, + }), + ], + { + [keyringAccount.id]: { + [scope]: 'existing-token', + }, + }, + ); + }); + + it('increments reconcile attempt to max when one below limit', async () => { + const { + service, + getTransactionsSpy, + getTransactionSpy, + findStellarTransactionsByAccountIdsSpy, + findLastScanTokenSpy, + saveManySpy, + } = setup(); + const { keyringAccount, activatedAccountPair } = buildActivatedPair( + 'account-sync-drop', + 'GA7UCNSASSOPQYTRGJ2NC7TDBSXHMWK6JHS7AO6X2ZQAIQSTB5ELNFSO', + ); + const [pendingTransaction] = generateMockTransactions(1, { + id: 'missing-tx-hash-drop', + account: keyringAccount.id, + scope, + status: TransactionStatus.Unconfirmed, + }); + + findStellarTransactionsByAccountIdsSpy.mockResolvedValue([ + { + ...pendingTransaction, + reconcileAttemptCount: 4, + } as StellarKeyringTransaction, + ]); + findLastScanTokenSpy.mockResolvedValue({ + [keyringAccount.id]: 'existing-token', + }); + getTransactionsSpy.mockResolvedValue([]); + getTransactionSpy.mockRejectedValue( + new TransactionNotFoundException('missing-tx-hash-drop'), + ); + + await service.synchronize([activatedAccountPair], scope, []); + + expect(saveManySpy).toHaveBeenCalledWith( + [ + expect.objectContaining({ + id: 'missing-tx-hash-drop', + reconcileAttemptCount: 5, + }), + ], + { + [keyringAccount.id]: { + [scope]: 'existing-token', + }, + }, + ); + }); }); diff --git a/packages/snap/src/services/transaction/TransactionSynchronizeService.ts b/packages/snap/src/services/transaction/TransactionSynchronizeService.ts index f8f90684..aa86c328 100644 --- a/packages/snap/src/services/transaction/TransactionSynchronizeService.ts +++ b/packages/snap/src/services/transaction/TransactionSynchronizeService.ts @@ -29,21 +29,20 @@ import type { KeyringAccountId, StellarKeyringAccount, } from '../account'; +import type { StellarKeyringTransaction } from './api'; import { TransactionOrder } from './api'; import type { Transaction } from './Transaction'; import type { TransactionMapper } from './TransactionMapper'; import type { TransactionRepository } from './TransactionRepository'; import type { StellarAssetMetadata } from '../asset-metadata'; -import type { NetworkService } from '../network'; -import { isPendingTransactionStatus } from './utils'; +import { TransactionNotFoundException, type NetworkService } from '../network'; +import { + isPendingTransactionStatus, + shouldDropPendingTransaction, + toKeyringTransactions, +} from './utils'; import type { ActivatedAccountPair } from '../sync/api'; -/** Pending snap-state txs keyed by account, then by on-chain transaction hash. */ -type PendingTransactionsByAccount = Map< - KeyringAccountId, - Map ->; - /** * Mutable state shared across fetch, map, and persist steps within one sync run. */ @@ -53,10 +52,10 @@ type SyncContext = { keyringAccountsById: Map; /** All snap-managed accounts on `scope`, keyed by address for SEP-41 receive mapping. */ allAccountsByAddress: Map; - pendingByAccount: PendingTransactionsByAccount; + pendingTransactionsById: Map; lastScanTokenByAccountId: Record; /** Mapped keyring txs collected during steps 2–3; persisted and emitted in step 4. */ - transactionsToSave: Record; + transactionsToSave: Record; sep41AssetsMetadata: Record; }; @@ -151,6 +150,7 @@ export class TransactionSynchronizeService { }); } + // #region -------------------- Context Creation -------------------- async #createSyncContext( activatedAccountPairs: ActivatedAccountPair[], scope: KnownCaip2ChainId, @@ -169,12 +169,15 @@ export class TransactionSynchronizeService { } // Use Promise.all (not allSettled): if state cannot be loaded, sync cannot continue. - const [pendingByAccount, lastScanTokenByAccountId, allAccountsByAddress] = - await Promise.all([ - this.#loadPendingTransactionsFromState(keyringAccountIds, scope), - this.#fetchLastScanTokens(keyringAccountIds, scope), - this.#loadAllAccountsByAddress(scope), - ]); + const [ + pendingTransactionsById, + lastScanTokenByAccountId, + allAccountsByAddress, + ] = await Promise.all([ + this.#loadPendingTransactionsFromState(keyringAccountIds, scope), + this.#fetchLastScanTokens(keyringAccountIds, scope), + this.#loadAllAccountsByAddress(scope), + ]); const sep41AssetsMetadata = this.#toSep41AssetsMetadata(sep41Assets); return { @@ -182,7 +185,7 @@ export class TransactionSynchronizeService { keyringAccounts, keyringAccountsById, allAccountsByAddress, - pendingByAccount, + pendingTransactionsById, lastScanTokenByAccountId, transactionsToSave: {}, sep41AssetsMetadata, @@ -230,31 +233,37 @@ export class TransactionSynchronizeService { async #loadPendingTransactionsFromState( keyringAccountIds: KeyringAccountId[], scope: KnownCaip2ChainId, - ): Promise { - const transactions = await this.#transactionRepository.findByAccountIds( - keyringAccountIds, - scope, - ); + ): Promise> { + const transactions = + await this.#transactionRepository.findStellarTransactionsByAccountIds( + keyringAccountIds, + scope, + ); - const pendingByAccount: PendingTransactionsByAccount = new Map(); + const pendingTransactionsById: Map< + TransactionId, + StellarKeyringTransaction + > = new Map(); - // Transaction ids can be shared across accounts (send vs receive), so index by account first. + // Pending txs are unique and belong to the submitting account only. + // Hence we dont need to group by account id. for (const transaction of transactions) { if (isPendingTransactionStatus(transaction.status)) { - this.#setPendingInState(pendingByAccount, transaction); + pendingTransactionsById.set(transaction.id, transaction); } } this.#logger.debug('Pending transactions loaded from snap state', { noOfAccounts: keyringAccountIds.length, noOfStoredTransactions: transactions.length, - noOfPendingTransactions: - this.#getPendingTransactionCount(pendingByAccount), + noOfPendingTransactions: pendingTransactionsById.size, }); - return pendingByAccount; + return pendingTransactionsById; } + // #endregion + // #region -------------------- Main Step to scan transactions -------------------- async #scanAccountTransactionsAndMap(context: SyncContext): Promise { // We use promise.allSettled here because we want to continue the sync even if some accounts fail to fetch. const fetchResults = await batchesAllSettled( @@ -288,26 +297,23 @@ export class TransactionSynchronizeService { let noOfResolved = 0; for (const transaction of transactions) { - const pendingFromState = this.#getPendingFromState( - context.pendingByAccount, - keyringAccount.id, + const pendingTransaction = context.pendingTransactionsById.get( transaction.id, ); + const pendingForAccount = + pendingTransaction?.account === keyringAccount.id + ? pendingTransaction + : undefined; - // Pending tx seen in this scan — skip the reconcile pass below. - if (pendingFromState) { - this.#deletePendingFromState( - context.pendingByAccount, - keyringAccount.id, - transaction.id, - ); + if (pendingForAccount) { + context.pendingTransactionsById.delete(transaction.id); noOfResolved += 1; } this.#appendMappedTransaction(context, { keyringAccount, onChainTransaction: transaction, - pendingFromState, + pendingTransaction: pendingForAccount, }); } @@ -327,110 +333,105 @@ export class TransactionSynchronizeService { async #reconcilePendingTransactionsAndMap( context: SyncContext, ): Promise { - const pendingCountBeforeReconcile = this.#getPendingTransactionCount( - context.pendingByAccount, - ); + const pendingCountBeforeReconcile = context.pendingTransactionsById.size; if (pendingCountBeforeReconcile === 0) { return; } - const pendingByTransactionId: Record = - {}; - - // Group by transaction id so each on-chain hash is fetched once when shared across accounts. - for (const pendingById of context.pendingByAccount.values()) { - for (const pendingFromState of pendingById.values()) { - pushToRecordArray( - pendingByTransactionId, - pendingFromState.id, - pendingFromState, - ); - } - } - - const transactionIdsToFetch = Object.keys(pendingByTransactionId); + const transactionIdsToFetch = Array.from( + context.pendingTransactionsById.keys(), + ); this.#logger.debug('Reconciling pending transactions', { transactionIdsToFetch, }); - // TODO: adding a max reconcile limit to avoid the sync running forever. const fetchResults = await batchesAllSettled( transactionIdsToFetch, 10, - async (transactionId) => ({ - transactionId, - onChainTransaction: await this.#fetchOnChainTransaction( - transactionId, - context.scope, - ), - }), + async (transactionId) => + this.#fetchOnChainTransaction(transactionId, context.scope), ); for (const fetchResult of fetchResults) { if (fetchResult.status === 'rejected') { + const error = fetchResult.reason; + // if the error is a TransactionNotFoundException, + // increment the reconcile attempt and continue. + if (error instanceof TransactionNotFoundException) { + this.#incrementReconcileAttempt(context, error.transactionHash); + continue; + } + // for other errors, log and continue. this.#logger.warn('Failed to fetch on-chain transaction', { error: fetchResult.reason, }); continue; } - const { transactionId, onChainTransaction } = fetchResult.value; - - // Map the on-chain transaction for every account that still has it pending. - for (const pendingFromState of pendingByTransactionId[transactionId] ?? - []) { - const keyringAccount = context.keyringAccountsById.get( - pendingFromState.account, - ); - - if (!keyringAccount) { - this.#logger.warn( - 'Failed to find keyring account for pending transaction', - { transactionId, accountId: pendingFromState.account }, - ); - continue; - } + const onChainTransaction = fetchResult.value; - this.#appendMappedTransaction(context, { - keyringAccount, - onChainTransaction, - pendingFromState, + // Get the pending transaction from the sync context. + // OnChain Transaction Id should be the same as the Pending Transaction Id. + const pendingTransaction = context.pendingTransactionsById.get( + onChainTransaction.id, + ); + if (!pendingTransaction) { + this.#logger.warn('Pending transaction not found after fetch', { + transactionId: onChainTransaction.id, }); + continue; + } - this.#deletePendingFromState( - context.pendingByAccount, - pendingFromState.account, - transactionId, + // Find the keyring account that the pending transaction belongs to. + // The pending transaction should only belong to one single account. + const keyringAccount = context.keyringAccountsById.get( + pendingTransaction.account, + ); + if (!keyringAccount) { + this.#logger.warn( + 'Failed to find keyring account for pending transaction', + { + transactionId: pendingTransaction.id, + accountId: pendingTransaction.account, + }, ); + continue; } + + this.#appendMappedTransaction(context, { + keyringAccount, + onChainTransaction, + pendingTransaction, + }); + + context.pendingTransactionsById.delete(pendingTransaction.id); } - const remainingPendingCount = this.#getPendingTransactionCount( - context.pendingByAccount, - ); + const remainingPendingCount = context.pendingTransactionsById.size; this.#logger.debug('Pending transactions reconciled', { noOfResolved: pendingCountBeforeReconcile - remainingPendingCount, noOfRemaining: remainingPendingCount, }); } + // #endregion #appendMappedTransaction( context: SyncContext, params: { keyringAccount: StellarKeyringAccount; onChainTransaction: Transaction; - pendingFromState?: KeyringTransaction; + pendingTransaction?: KeyringTransaction; }, ): void { - const { keyringAccount, onChainTransaction, pendingFromState } = params; + const { keyringAccount, onChainTransaction, pendingTransaction } = params; const mappedTransaction = this.#transactionMapper.mapTransactionSafe({ transaction: onChainTransaction, keyringAccount, - transactionFromState: pendingFromState, + transactionFromState: pendingTransaction, assetMetadata: context.sep41AssetsMetadata, }); @@ -583,6 +584,9 @@ export class TransactionSynchronizeService { async #saveTransactions(context: SyncContext): Promise { this.#logger.debug('Saving transactions'); const transactions = Object.values(context.transactionsToSave).flat(); + const pendingTransactions = Array.from( + context.pendingTransactionsById.values(), + ); const lastScanTokensByAccountIdWithScope: Record< KeyringAccountId, @@ -598,7 +602,7 @@ export class TransactionSynchronizeService { } await this.#transactionRepository.saveMany( - transactions, + transactions.concat(pendingTransactions), lastScanTokensByAccountIdWithScope, ); this.#logger.debug('Transactions saved'); @@ -612,60 +616,20 @@ export class TransactionSynchronizeService { getSnapProvider(), KeyringEvent.AccountTransactionsUpdated, { - transactions: context.transactionsToSave, + // Strip snap-internal reconcile metadata before keyring emit. + transactions: Object.fromEntries( + Object.entries(context.transactionsToSave).map( + ([accountId, transactions]) => [ + accountId, + toKeyringTransactions(transactions), + ], + ), + ), }, ); this.#logger.debug('Transactions updated event emitted'); } - #getPendingTransactionCount( - pendingByAccount: PendingTransactionsByAccount, - ): number { - let count = 0; - for (const pendingById of pendingByAccount.values()) { - count += pendingById.size; - } - return count; - } - - #getPendingFromState( - pendingByAccount: PendingTransactionsByAccount, - accountId: KeyringAccountId, - transactionId: TransactionId, - ): KeyringTransaction | undefined { - return pendingByAccount.get(accountId)?.get(transactionId); - } - - #deletePendingFromState( - pendingByAccount: PendingTransactionsByAccount, - accountId: KeyringAccountId, - transactionId: TransactionId, - ): void { - const pendingById = pendingByAccount.get(accountId); - if (!pendingById) { - return; - } - - pendingById.delete(transactionId); - - if (pendingById.size === 0) { - pendingByAccount.delete(accountId); - } - } - - #setPendingInState( - pendingByAccount: PendingTransactionsByAccount, - transaction: KeyringTransaction, - ): void { - let pendingById = pendingByAccount.get(transaction.account); - if (!pendingById) { - pendingById = new Map(); - pendingByAccount.set(transaction.account, pendingById); - } - - pendingById.set(transaction.id, transaction); - } - /** * Returns the recipient address when `transaction` is a confirmed SEP-41 send from * `senderKeyringAccount`. @@ -705,4 +669,36 @@ export class TransactionSynchronizeService { return to.address; } + + /** + * Records a Horizon not-found reconcile attempt on a pending transaction. + * Eviction is deferred to {@link TransactionRepository.saveMany} when both + * `maxReconcileAttempts` and `maxPendingTransactionAge` are exceeded. + * + * @param context - Mutable sync run state including pending transactions. + * @param transactionId - Pending transaction hash that Horizon did not return. + */ + #incrementReconcileAttempt( + context: SyncContext, + transactionId: TransactionId, + ): void { + const transaction = context.pendingTransactionsById.get(transactionId); + if (!transaction) { + return; + } + + const reconcileAttemptCount = (transaction.reconcileAttemptCount ?? 0) + 1; + transaction.reconcileAttemptCount = reconcileAttemptCount; + + if (shouldDropPendingTransaction(transaction)) { + this.#logger.debug( + 'Pending transaction eligible for eviction on save after max reconcile attempts and max age are exceeded', + { + transactionId, + accountId: transaction.account, + reconcileAttemptCount, + }, + ); + } + } } diff --git a/packages/snap/src/services/transaction/api.ts b/packages/snap/src/services/transaction/api.ts index 30c982a9..b23892bf 100644 --- a/packages/snap/src/services/transaction/api.ts +++ b/packages/snap/src/services/transaction/api.ts @@ -1,3 +1,13 @@ +import type { Transaction as KeyringTransaction } from '@metamask/keyring-api'; + +/** + * Snap-state transaction with optional reconcile metadata. + * `reconcileAttemptCount` is internal-only and must be stripped before keyring emit/read APIs. + */ +export type StellarKeyringTransaction = KeyringTransaction & { + reconcileAttemptCount?: number; +}; + /** * The order of onChain transactions to fetch. */ diff --git a/packages/snap/src/services/transaction/utils.ts b/packages/snap/src/services/transaction/utils.ts index 35a2ccd8..71bcd28c 100644 --- a/packages/snap/src/services/transaction/utils.ts +++ b/packages/snap/src/services/transaction/utils.ts @@ -7,7 +7,7 @@ import type { Operation } from '@stellar/stellar-sdk'; import { Asset } from '@stellar/stellar-sdk'; import { BigNumber } from 'bignumber.js'; -import { StellarOperationType } from './api'; +import { StellarOperationType, type StellarKeyringTransaction } from './api'; import { InvalidInvokeContractStructureException, RequiresMemoException, @@ -25,6 +25,7 @@ import type { KnownCaip2ChainId, } from '../../api'; import { SwapTransactionXdrStruct } from '../../api'; +import { AppConfig } from '../../config'; import { DUST_XLM_AMOUNT } from '../../constants'; import { getSlip44AssetId, @@ -576,3 +577,85 @@ export function isCompletedTransactionStatus( status === `${TransactionStatus.Confirmed}` ); } + +/** + * Strips snap-internal reconcile metadata before exposing a transaction via keyring APIs. + * + * @param transaction - Snap-state transaction that may include `reconcileAttemptCount`. + * @returns A pure keyring transaction. + */ +export function toKeyringTransaction( + transaction: StellarKeyringTransaction, +): KeyringTransaction { + const { + reconcileAttemptCount: _reconcileAttemptCount, + ...keyringTransaction + } = transaction; + return keyringTransaction; +} + +/** + * Converts a list of Stellar transactions to a list of pure keyring transactions. + * + * @param transactions - Snap-state transactions that may include `reconcileAttemptCount`. + * @returns Pure keyring transactions. + */ +export function toKeyringTransactions( + transactions: StellarKeyringTransaction[], +): KeyringTransaction[] { + return transactions.map(toKeyringTransaction); +} + +/** + * Checks if the pending transaction has exceeded the max reconcile attempts. + * + * @param reconcileAttemptCount - Number of Horizon not-found reconcile attempts for a pending tx. + * @returns Whether the reconcile attempt limit has been reached. + */ +export function isReconcileAttemptExceeded( + reconcileAttemptCount: number = 0, +): boolean { + return reconcileAttemptCount >= AppConfig.transaction.maxReconcileAttempts; +} + +/** + * Checks if the pending transaction has exceeded the max pending transaction age. + * + * @param timestamp - Transaction creation time in seconds since epoch. + * @returns Whether the pending transaction is older than the configured max age. + */ +export function isExceedMaxPendingTransactionAge( + timestamp?: number | null, +): boolean { + if (timestamp === undefined || timestamp === null) { + return false; + } + + return ( + Date.now() - timestamp * 1000 > + AppConfig.transaction.maxPendingTransactionAge + ); +} + +/** + * Whether a pending transaction should be evicted from snap state. + * + * Both reconcile attempts and max age must be exceeded so frequent account-switch + * syncs cannot drop a pending tx before it has had enough wall-clock time. + * + * @param transaction - Pending transaction with optional reconcile metadata. + * @param transaction.reconcileAttemptCount - Horizon not-found reconcile attempts for this pending tx. + * @param transaction.timestamp - Transaction creation time in seconds since epoch. + * @returns Whether the pending tx should be dropped from snap state. + */ +export function shouldDropPendingTransaction( + transaction: Pick< + StellarKeyringTransaction, + 'reconcileAttemptCount' | 'timestamp' + >, +): boolean { + return ( + isReconcileAttemptExceeded(transaction.reconcileAttemptCount ?? 0) && + isExceedMaxPendingTransactionAge(transaction.timestamp) + ); +}