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/2] 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/2] 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; }