Skip to content

Don't attach volume telemetry to pre-submission failures - #2991

Merged
JakeUrban merged 3 commits into
masterfrom
claude/send-swap-volume-telemetry-20yl5j
Sep 2, 2026
Merged

Don't attach volume telemetry to pre-submission failures#2991
JakeUrban merged 3 commits into
masterfrom
claude/send-swap-volume-telemetry-20yl5j

Conversation

@JakeUrban

@JakeUrban JakeUrban commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #2984. Don't attach volume telemetry to a pre-submission failure event.

Found while implementing the same telemetry on mobile (stellar/freighter-mobile#996) and checking the two clients against each other.

🤖 Generated with Claude Code

A signing failure never reached `submitFreighterTransaction`'s guard, so it
fell through to submission and its terminal event carried volume data for a
transaction that never left the device.

`signFreighterTransaction` is dispatched without `.unwrap()`, so a rejection
does not throw. Execution continued to the submit call with whatever `signedXDR`
held, which failed and landed in the `rejected` branch — the volume-bearing
emit site. Two shapes, both wrong:

- Classic payment: `signedXDR` is still `""`, so the submit fails on a
  client-side XDR parse error. No Horizon problem+json body means
  `getFailureCategory` returns `transport`.
- Soroban/token transfer: `signedXDR` still holds the *unsigned* prepared XDR,
  which submits for real and comes back `tx_bad_auth`.

Either way `payment.failed`/`swap.failed` carried `amount`, the `amount_usd`
family and `asset_type`, inflating attempted-volume totals with transactions
that were never attempted. The `transport` bucket is the worse half: the
catalog reads it as "unresolved — may have settled", and a signing failure
definitively never reached the network.

The branch's own comment already asserted this could not happen ("A
pre-submission failure (signing, simulation) never reaches here"), so this
restores the documented intent rather than changing it.

Fix:

- Track whether signing actually succeeded rather than inferring it from
  `signedXDR` being empty, which the Soroban path defeats.
- On a pre-submission failure, emit the terminal event with only its
  pre-existing failure properties (asset codes, `payment_type`, a bounded
  `reason_code`) and no volume data, then return without submitting.
- Start the confirmation price snapshot after signing succeeds rather than
  before it, so no snapshot exists to attach on this path and prices sit
  closer to actual execution. Matches freighter-mobile.

The UI is unchanged: `signFreighterTransaction.rejected` already sets
ActionStatus.ERROR, so SubmitFail renders as before — now showing the real
signing error instead of one manufactured by submitting a bad XDR.

Adds five tests, all of which fail against the previous behavior.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9wRNPPgt137euKzquN5Tg
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

PR Preview build is ready: https://github.com/stellar/freighter/releases/tag/untagged-2cfef3933c62e06d8327
Backend: V1 prod + V2 beta (no sandbox configured for @JakeUrban). SDF collaborators only — install instructions in the release description.

…ed action

The pre-submission guard relied on `signFreighterTransaction.rejected` having
already put the flow into ActionStatus.ERROR. That holds for the ordinary case
— a signature that threw — but two paths reach the guard without dispatching
any rejected action:

- a sign that resolves *fulfilled* with an empty `signedTransaction` (the
  branch the existing `&& res.payload.signedTransaction` check exists for), and
- a hardware flow arriving with no signed XDR, which skips the signing dispatch
  entirely.

`signFreighterTransaction` has `pending` and `rejected` reducers but no
`fulfilled` one, so on those paths the status stayed PENDING (or IDLE for
hardware) and TransactionConfirm kept rendering SendingTransaction — stranding
the user on the sending spinner. Previously both fell through to
`submitFreighterTransaction`, whose rejection set the status; the guard removed
that side effect without replacing it.

Adds a `setSubmitError` action and dispatches it from the guard. It is
idempotent with the rejected reducer (same status, same error) on the path
where both run.

Adds three tests asserting the status reaches ERROR on all three paths; the two
covering the newly-handled paths fail without this change (PENDING and IDLE
respectively).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9wRNPPgt137euKzquN5Tg
@JakeUrban
JakeUrban marked this pull request as ready for review September 1, 2026 22:30
Copilot AI balanced review requested due to automatic review settings September 1, 2026 22:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents USD volume fields from being attached to signing failures.

Changes:

  • Detects signing failures before submission.
  • Emits failure-only telemetry and transitions the UI to an error state.
  • Adds regression coverage for software and hardware signing failures.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
transactionSubmission.ts Adds an explicit submission-error action.
useSubmitTxData.tsx Separates pre-submission failures from submitted outcomes.
useSubmitTxData.telemetry.test.tsx Tests failure telemetry and submission guards.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@CassioMG

CassioMG commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Two doc comments invalidated by the snapshot-timing change

Docs only — no behavior impact; not a merge blocker.

TL;DR: Moving the price snapshot to after signing leaves two load-bearing comments describing the old flow. The snapshot helper's own doc still says the fetch is "started at confirmation", while this PR's call site now correctly says the opposite ("after signing succeeded and immediately before submission") — a reader of the helper alone gets the wrong contract. And the failure screen's comment explaining why it emits no telemetry still claims the central emit site always has the price snapshot and transaction result in scope, which is no longer true for the new pre-submission failure path that emits with neither. Both are two-line comment fixes; these docs were relied on heavily during the parent PR's review, so keeping them truthful is worth the diff.


Detailed explanation (for agents)

(a) confirmationPriceSnapshot.ts module doc still reads "Issues ONE price fetch covering every leg's canonical id, started at confirmation and never blocking signing/submission" — its sole caller now starts it post-signing. The cachedDisplayPrices sentence ("captured by the caller at this same moment... 'the price already shown to the user'") also loosened: post-signing, the display cache may have refreshed during a password prompt or hardware approval, so the fallback is no longer guaranteed to be confirm-click state:

/**
* Issues ONE price fetch covering every leg's canonical id, started at
* confirmation and never blocking signing/submission — callers do not await
* this. `cachedDisplayPrices` is the price map already held for the on-screen
* fiat estimate, captured by the caller at this same moment: it must reflect
* "the price already shown to the user for this transaction", not whatever
* the cache holds later when `resolve()` is called.
*
* Cancellation is a real network abort on the v1 endpoint (a direct fetch).
* The v2 endpoint runs in the background service worker across a message

The updated call-site comment it now contradicts:

// Everything the volume telemetry needs is snapshotted here — after
// signing succeeded and immediately before submission, so the prices sit
// as close as possible to the transaction's actual execution time.
// Amounts and prices are frozen together and carried to whichever
// terminal event fires. Skipped entirely for collectible sends (unpriced,
// out of scope) and for custom networks (not real economic activity,
// shouldn't pollute volume metrics).
const cachedDisplayPrices =
allTokenPricesCache[networkDetails.networkPassphrase]?.[publicKey] ??

(b) SubmitFail/index.tsx centralization comment — the conclusion (single emit site, no double-counting) still holds, but the supporting rationale "it already has the confirmation price snapshot and the transaction result in scope" is false for the new !isSigned branch, which emits paymentFailed/swapFailed/collectibleSendFailed before any snapshot is started and without a transaction result:

// Renders the user-facing failure screen only — title, explanatory
// notification, and any error-specific messaging/links, chosen by
// classifying `error` into a RESULT_CODES case below. It does not emit
// telemetry: paymentFailed / swapFailed / collectibleSendFailed are NOT
// emitted here. They used to be, from an effect keyed on `error`/`asset`/etc
// — but that re-fires on every remount (double-counting attempted volume).
// useSubmitTxData's fetchData is the single, centralized emit site for every
// terminal event (success and failure alike): it already has the
// confirmation price snapshot and the transaction result in scope, and it
// runs exactly once per submission attempt.

Suggested fix: reword (a) to "started once signing succeeds, immediately before submission, never blocking submission" and adjust the cachedDisplayPrices sentence accordingly; reword (b)'s rationale to note the pre-submission failure path deliberately emits without snapshot/result.

@CassioMG CassioMG left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM - other than the stale-docs comment

Moving the price snapshot to after signing left two comments describing the
old flow:

- `confirmationPriceSnapshot.ts` still said the fetch is "started at
  confirmation", contradicting its sole caller. Its `cachedDisplayPrices`
  sentence also over-promised: post-signing that map is the display cache as
  of the start of submission, not as of the confirm tap, since a password
  prompt or hardware approval in between can let it refresh.
- `SubmitFail/index.tsx` justified the single-emit-site design with "it
  already has the confirmation price snapshot and the transaction result in
  scope", which is not true of the new pre-submission failure path — that one
  emits with neither, by design.

Comments only; no behavior change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E9wRNPPgt137euKzquN5Tg
Copilot AI review requested due to automatic review settings September 2, 2026 01:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (1)

extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx:260

  • Starting the snapshot only after signing changes the meaning of successful-event USD data: rates can now be fetched after the confirmation-time value has moved. Both #2984 and the linked mobile implementation define this snapshot as frozen before signing so telemetry reflects what the user confirmed. Omitting volume from a signing failure does not require moving the snapshot; start it before signing, then cancel it on !isSigned and emit that failure without resolving or spreading the USD properties.
      // Everything the volume telemetry needs is snapshotted here — after
      // signing succeeded and immediately before submission, so the prices sit
      // as close as possible to the transaction's actual execution time.

@JakeUrban
JakeUrban merged commit 6c30f39 into master Sep 2, 2026
12 checks passed
@JakeUrban
JakeUrban deleted the claude/send-swap-volume-telemetry-20yl5j branch September 2, 2026 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants