refactor: TransacationService and ClientRequest handler error handle #133
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors transaction-related error handling across the snap by standardizing exception types, making XDR parsing failures explicit (via thrown domain exceptions), and updating client-request and sync flows to log/track unexpected failures while keeping user-facing validation responses structured.
Changes:
- Standardize transaction exceptions on
StellarSnapException(withcausesupport) and propagate domain exceptions more consistently. - Refactor XDR parsing behavior (e.g.,
parseSuccessfulTransactionResult) to throwXdrParseExceptionon parse failures and remove “safe” wrappers in favor of centralized handling. - Update client request handlers and sync services to reduce noisy logging while selectively tracking unexpected errors via
trackError.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/snap/src/ui/confirmation/utils.ts | Uses the renamed safe asset-reference parser for confirmation UI asset display resolution. |
| packages/snap/src/services/transaction/xdrParser.ts | Throws domain exceptions for parse failures; refactors SEP-41 invoke parsing error handling; wraps/propagates causes. |
| packages/snap/src/services/transaction/xdrParser.test.ts | Updates tests to expect thrown parse exceptions and removes tests for deleted safe wrapper. |
| packages/snap/src/services/transaction/utils.ts | Renames asset reference parser to parseOperationAssetReferenceSafe and updates call sites. |
| packages/snap/src/services/transaction/TransactionSynchronizeService.ts | Downgrades some error logs to warnings; adds a TODO for reconcile limits. |
| packages/snap/src/services/transaction/TransactionService.ts | Tracks unexpected save failures (trackError) before warning and returning null. |
| packages/snap/src/services/transaction/TransactionMapper.ts | Removes SEP-41 “safe” parsing wrapper usage; adds warning logs on best-effort parsing failures. |
| packages/snap/src/services/transaction/TransactionBuilder.ts | Adds exception causes when wrapping build errors; removes redundant logging in some catch blocks. |
| packages/snap/src/services/transaction/Transaction.ts | Uses domain exceptions for internal failures and attaches causes on deserialization errors. |
| packages/snap/src/services/transaction/simulation/simulators.ts | Lets XdrParseException propagate (treated as internal parsing failures rather than user validation outcomes). |
| packages/snap/src/services/transaction/exceptions.ts | Refactors exceptions to extend StellarSnapException for consistent naming and cause/data handling. |
| packages/snap/src/services/sync/SynchronizeService.ts | Adds trackError for rejected tasks and asset loading; refactors account loading to be per-account fail-safe. |
| packages/snap/src/handlers/clientRequest/onAmountInput.ts | Simplifies amount validation error handling to always return structured invalid responses without tracking. |
| packages/snap/src/handlers/clientRequest/onAmountInput.test.ts | Updates tests to expect invalid responses (not rethrows) for XDR/unexpected failures. |
| packages/snap/src/handlers/clientRequest/onAddressInput.ts | Removes logger dependency; treats request validation failures as { valid: false }. |
| packages/snap/src/handlers/clientRequest/onAddressInput.test.ts | Updates construction to match handler signature changes. |
| packages/snap/src/handlers/clientRequest/confirmSend.ts | Tracks unexpected failures (trackError) and returns structured invalid results; keeps user rejection propagation. |
| packages/snap/src/handlers/clientRequest/confirmSend.test.ts | Adds/updates tests for tracking on XDR/unexpected failures and non-tracking of expected validation errors. |
| packages/snap/src/context.ts | Updates handler wiring to match the new OnAddressInputHandler constructor signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { error }, | ||
| ); | ||
|
|
||
| return { |
There was a problem hiding this comment.
we dont need to throw error to caller, as it will catch the error and convert the same result if there is a error or code is invalid
it only special handle for UserRejectedRequestError
| scope, | ||
| ), | ||
| })), | ||
| const results = await Promise.all( |
There was a problem hiding this comment.
change from Promise.allSettled to Promise.all to make it easier to implement track error, as track error is a async func too
| // 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); |
There was a problem hiding this comment.
in short, we dont wrap the error, as XdrParseException is not a expected error, we should log it, please see confirmSend
| keyringAccount: StellarKeyringAccount, | ||
| assetMetadata: Record<KnownCaip19Sep41AssetId, StellarAssetMetadata>, | ||
| ): KeyringTransaction | undefined { | ||
| try { |
There was a problem hiding this comment.
remove try catch on mapSep41SendTransaction
so if it throws error, we will map to a unknown txn, it is the same for catch it and return undefine
| export function assertMemoWhenDestinationRequires( | ||
| transaction: Transaction, | ||
| destAccountId: string, | ||
| destAccountAddress: string, |
There was a problem hiding this comment.
better naming
| } catch { | ||
| return null; | ||
| } catch (error) { | ||
| throw new XdrParseException( |
There was a problem hiding this comment.
we dont need return null
becoz it is using from getSuccessfulTransactionResultSafe
it already has a try catch and return null logic exist
| * @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( |
There was a problem hiding this comment.
this func is no longer need,
it was using in mapSep41SendTransaction
as the caller will handle it already, so we dont need it anymore
| error: result.reason, | ||
| }); | ||
| }); | ||
| const pairs: ActivatedAccountPair[] = results.filter( |
There was a problem hiding this comment.
as we change to promise.all, so we need to filter the result
| // 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; |
There was a problem hiding this comment.
redundant null
Explanation
This PR refactors transaction-related error handling across the snap by standardizing exception types, making XDR parsing failures explicit (via thrown domain exceptions), and updating client-request and sync flows to log/track unexpected failures while keeping user-facing validation responses structured.
References
Checklist