diff --git a/extension/src/helpers/confirmationPriceSnapshot.ts b/extension/src/helpers/confirmationPriceSnapshot.ts index 239f87d474..c733b93657 100644 --- a/extension/src/helpers/confirmationPriceSnapshot.ts +++ b/extension/src/helpers/confirmationPriceSnapshot.ts @@ -39,12 +39,17 @@ export interface ConfirmationSnapshotHandle { } /** - * 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. + * Issues ONE price fetch covering every leg's canonical id, started once + * signing has succeeded and immediately before submission — as close to the + * transaction's execution as the flow allows — and never blocking submission, + * since 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, and is the fallback `resolve()` closes on when the fetch + * has not landed by terminal status. Because the snapshot starts after + * signing, that map is the display cache as of the start of submission rather + * than as of the confirm tap — a password prompt or hardware approval in + * between can have let it refresh. Either way it must be captured here, not + * read 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 diff --git a/extension/src/popup/components/InternalTransaction/SubmitFail/index.tsx b/extension/src/popup/components/InternalTransaction/SubmitFail/index.tsx index 60fe161557..629b3c63ff 100644 --- a/extension/src/popup/components/InternalTransaction/SubmitFail/index.tsx +++ b/extension/src/popup/components/InternalTransaction/SubmitFail/index.tsx @@ -35,9 +35,11 @@ interface ErrorDetails { // 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. +// terminal event (success and failure alike), and it runs exactly once per +// confirmation attempt. For a submitted transaction it has the confirmation +// price snapshot and the transaction result in scope, so its events carry +// volume; its pre-submission failure path deliberately emits without either, +// since nothing reached the network to have volume or a result. export const SubmitFail = () => { const { error } = useSelector(transactionSubmissionSelector); const isSwap = useIsSwap(); diff --git a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/__tests__/useSubmitTxData.telemetry.test.tsx b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/__tests__/useSubmitTxData.telemetry.test.tsx index c2d185f62e..2b7a8aa713 100644 --- a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/__tests__/useSubmitTxData.telemetry.test.tsx +++ b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/__tests__/useSubmitTxData.telemetry.test.tsx @@ -15,6 +15,7 @@ import { NetworkDetails, } from "@shared/constants/stellar"; import { CUSTOM_NETWORK } from "@shared/helpers/stellar"; +import { ActionStatus } from "@shared/api/types"; import * as ApiInternal from "@shared/api/internal"; import { makeDummyStore } from "popup/__testHelpers__"; import { initialState as txSubmissionInitialState } from "popup/ducks/transactionSubmission"; @@ -147,21 +148,25 @@ const makeState = ({ const renderSubmitHook = ( state: ReturnType, networkDetails: NetworkDetails = MAINNET_NETWORK_DETAILS, + { isHardwareWallet = false }: { isHardwareWallet?: boolean } = {}, ) => { const store = makeDummyStore(state); const wrapper = ({ children }: { children: React.ReactNode }) => ( {children} ); - return renderHook( + const rendered = renderHook( () => useSubmitTxData({ - isHardwareWallet: false, + isHardwareWallet, networkDetails, publicKey: PUBLIC_KEY, xdr: buildSwapXdr(), }), { wrapper }, ); + // The store is a real one over rootReducer, so submitStatus is observable — + // it is what TransactionConfirm switches on to render SubmitFail. + return { ...rendered, store }; }; const mockSubmitOk = (resultXdr: string) => { @@ -539,4 +544,179 @@ describe("useSubmitTxData terminal-event telemetry", () => { expect(emitMetric).not.toHaveBeenCalled(); }); + describe("pre-submission (signing) failure", () => { + const mockSigningFailure = () => + jest + .spyOn(ApiInternal, "signFreighterTransaction") + .mockRejectedValue(new Error("Incorrect password")); + + it("emits payment.failed with its failure properties and no volume data", async () => { + mockSigningFailure(); + jest + .spyOn(ApiInternal, "getTokenPrices") + .mockResolvedValue({ native: { currentPrice: "0.5" } }); + + const { result } = renderSubmitHook(makeState({ asset: "native" })); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + const props = emitted(METRIC_NAMES.paymentFailed); + expect(props).toEqual({ + payment_type: "payment", + asset_code: "XLM", + reason_code: "unknown", + }); + // The transaction never left the device, so it has no attempted volume. + expect(props).not.toHaveProperty("amount"); + expect(props).not.toHaveProperty("amount_usd"); + expect(props).not.toHaveProperty("amount_usd_status"); + // ...and it is emphatically not a transport failure, which per the + // catalog reads as "unresolved — may have settled". + expect(props).not.toHaveProperty("failure_category"); + }); + + it("emits swap.failed with both asset codes and no volume data", async () => { + mockSigningFailure(); + jest.spyOn(ApiInternal, "getTokenPrices").mockResolvedValue({ + native: { currentPrice: "0.5" }, + [USDC_CANONICAL]: { currentPrice: "1.0" }, + }); + + const { result } = renderSubmitHook( + makeState({ + asset: "native", + destinationAsset: USDC_CANONICAL, + destinationAmount: "90", + }), + ); + await act(async () => { + await result.current.fetchData({ isSwap: true }); + }); + + expect(emitted(METRIC_NAMES.swapFailed)).toEqual({ + from_asset_code: "XLM", + to_asset_code: "USDC", + reason_code: "unknown", + }); + }); + + it("does not submit a classic payment whose signature failed", async () => { + mockSigningFailure(); + const fetchSpy = jest.fn(); + global.fetch = fetchSpy as unknown as typeof fetch; + + const { result } = renderSubmitHook( + // preparedTransaction: null is the classic-payment shape, where a + // failed signature used to leave signedXDR as "" and submit it. + makeState({ asset: "native", preparedTransaction: null }), + ); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("does not submit the UNSIGNED prepared XDR when a token transfer's signature failed", async () => { + mockSigningFailure(); + const fetchSpy = jest.fn(); + global.fetch = fetchSpy as unknown as typeof fetch; + + const { result } = renderSubmitHook( + // A Soroban/token transfer carries a prepared XDR, so `signedXDR` is + // truthy even when signing failed — the guard has to key off whether + // signing actually succeeded, not off the XDR being empty. + makeState({ asset: "native", preparedTransaction: buildSwapXdr() }), + ); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("puts the flow into ActionStatus.ERROR so SubmitFail renders (signature threw)", async () => { + mockSigningFailure(); + + const { result, store } = renderSubmitHook( + makeState({ asset: "native" }), + ); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + // TransactionConfirm switches on submitStatus to decide between + // SendingTransaction and SubmitFail. + expect(store.getState().transactionSubmission.submitStatus).toBe( + ActionStatus.ERROR, + ); + }); + + it("reaches ERROR even when signing resolves fulfilled with an empty payload", async () => { + // No rejected action is dispatched on this path, so the reducer never + // sets the status — without setSubmitError the view would sit on + // PENDING and strand the user on the sending spinner. + jest + .spyOn(ApiInternal, "signFreighterTransaction") + .mockResolvedValue({ signedTransaction: "" }); + const fetchSpy = jest.fn(); + global.fetch = fetchSpy as unknown as typeof fetch; + + const { result, store } = renderSubmitHook( + makeState({ asset: "native" }), + ); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(store.getState().transactionSubmission.submitStatus).toBe( + ActionStatus.ERROR, + ); + expect(fetchSpy).not.toHaveBeenCalled(); + expect(emitted(METRIC_NAMES.paymentFailed)).toEqual({ + payment_type: "payment", + asset_code: "XLM", + reason_code: "unknown", + }); + }); + + it("reaches ERROR for a hardware flow that arrives with no signed XDR", async () => { + // Hardware skips the signing dispatch entirely (HardwareSign stores the + // signed XDR in preparedTransaction), so nothing sets the status here + // either. + const fetchSpy = jest.fn(); + global.fetch = fetchSpy as unknown as typeof fetch; + + const { result, store } = renderSubmitHook( + makeState({ asset: "native", preparedTransaction: null }), + MAINNET_NETWORK_DETAILS, + { isHardwareWallet: true }, + ); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(store.getState().transactionSubmission.submitStatus).toBe( + ActionStatus.ERROR, + ); + expect(fetchSpy).not.toHaveBeenCalled(); + }); + + it("issues no confirmation price fetch when signing fails", async () => { + mockSigningFailure(); + const pricesSpy = jest + .spyOn(ApiInternal, "getTokenPrices") + .mockResolvedValue({ native: { currentPrice: "0.5" } }); + + const { result } = renderSubmitHook(makeState({ asset: "native" })); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + // The snapshot starts only once signing has succeeded, so a signing + // failure never issues a price request it would just have to abort. + expect(pricesSpy).not.toHaveBeenCalled(); + }); + }); }); diff --git a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx index 4bd2cecf36..bd28218462 100644 --- a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx +++ b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx @@ -7,6 +7,7 @@ import { initialState, reducer, isError } from "helpers/request"; import { AppDispatch } from "popup/App"; import { addRecentAddress, + setSubmitError, signFreighterTransaction, submitFreighterTransaction, transactionSubmissionSelector, @@ -22,7 +23,7 @@ import { isMainnet, } from "helpers/stellar"; import { getSdk, isCustomNetwork } from "@shared/helpers/stellar"; -import { AssetIcons } from "@shared/api/types"; +import { AssetIcons, ErrorMessage } from "@shared/api/types"; import { allAccountsSelector } from "popup/ducks/accountServices"; import { balancesSelector, tokenPricesSelector } from "popup/ducks/cache"; import { tokenPricesV2Selector } from "popup/ducks/remoteConfig"; @@ -52,6 +53,14 @@ interface SubmitTxData { error?: string; } +/** + * `reason_code` for a terminal event that never reached the network, so there + * is no Horizon result code to report. Deliberately the same bounded literal + * the post-submission path falls back to rather than the raw signing-error + * text, which would give `reason_code` unbounded cardinality. + */ +const PRE_SUBMISSION_REASON_CODE = "unknown"; + /** * The `amount_usd`-family properties are named identically on all four * terminal events so `SUM(amount_usd)` works across event names. @@ -134,12 +143,11 @@ function useSubmitTxData({ status: "success", } as SubmitTxData; - // Everything the volume telemetry needs is snapshotted here, at - // confirmation, before signing/submission — 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). + // Asset identities for the volume telemetry. Classified up front (the + // price snapshot they feed starts after signing, below). Skipped + // entirely for collectible sends (unpriced, out of scope) and for custom + // networks (not real economic activity, shouldn't pollute volume + // metrics). const isCustom = isCustomNetwork(networkDetails); const accountBalances = allBalancesCache[networkDetails.network]?.[publicKey]?.balances ?? null; @@ -165,28 +173,6 @@ function useSubmitTxData({ ) : null; - const cachedDisplayPrices = - allTokenPricesCache[networkDetails.networkPassphrase]?.[publicKey] ?? - null; - snapshotHandle = sourceIdentity - ? startConfirmationPriceSnapshot({ - canonicalIds: [ - getCanonicalFromAsset(sourceIdentity.code, sourceIdentity.issuer), - ...(destIdentity - ? [ - getCanonicalFromAsset( - destIdentity.code, - destIdentity.issuer, - ), - ] - : []), - ], - networkDetails, - useV2: useTokenPricesV2, - cachedDisplayPrices, - }) - : null; - // Not a non-null assertion and not a guard: `preparedTransaction` is // legitimately null for a classic payment (simulateTx's "classic" arm // returns a fee and no payload at all — the built XDR arrives via the @@ -196,6 +182,11 @@ function useSubmitTxData({ // replaces it, so `?? ""` — the same fallback Send/index.tsx uses on // this field — is the honest starting value. let signedXDR = transactionSimulation.preparedTransaction ?? ""; + // Tracked explicitly rather than inferred from `signedXDR` being empty: + // on the Soroban/token path a failed signature leaves `signedXDR` + // holding the *unsigned* prepared XDR, which is truthy. + let isSigned = isHardwareWallet && !!signedXDR; + let signingError: ErrorMessage | undefined; if (!isHardwareWallet) { const res = await reduxDispatch( signFreighterTransaction({ @@ -208,9 +199,91 @@ function useSubmitTxData({ res.payload.signedTransaction ) { signedXDR = res.payload.signedTransaction; + isSigned = true; + } else { + signingError = signFreighterTransaction.rejected.match(res) + ? res.payload + : undefined; } } + if (!isSigned) { + // Pre-submission failure: signing rejected, or a hardware flow arrived + // without a signed XDR. Submitting anyway is what this guard exists to + // prevent — the transaction never left the device, so it has no + // attempted volume and no meaningful Horizon result code. The terminal + // event still fires (this is the flow's outcome and the funnel counts + // on it), but carries only its pre-existing failure properties. + // + // The ordinary case — a signature that threw — has already been put + // into ActionStatus.ERROR by `signFreighterTransaction.rejected`, so + // TransactionConfirm renders SubmitFail as before, now showing that + // real error instead of one manufactured by submitting a bad XDR. + // + // The two paths that dispatch no rejected action — a sign that + // resolves fulfilled with an empty payload, and a hardware flow with + // no signed XDR — would otherwise leave Redux on PENDING and strand + // the user on the sending spinner, since nothing downstream sets the + // status any more. `setSubmitError` closes that: it is idempotent + // with the reducer above (same status, same error) on the path where + // both run. + if (!isCustom) { + if (isCollectible) { + emitMetric(METRIC_NAMES.collectibleSendFailed, { + reason_code: PRE_SUBMISSION_REASON_CODE, + }); + } else if (isSwap) { + emitMetric(METRIC_NAMES.swapFailed, { + from_asset_code: getAssetFromCanonical(asset).code, + to_asset_code: getAssetFromCanonical(destinationAsset).code, + reason_code: PRE_SUBMISSION_REASON_CODE, + }); + } else { + emitMetric(METRIC_NAMES.paymentFailed, { + payment_type: "payment", + asset_code: sourceAsset.code, + reason_code: PRE_SUBMISSION_REASON_CODE, + }); + } + } + + const error = + signingError ?? + ({ errorMessage: "Failed to sign transaction" } as ErrorMessage); + reduxDispatch(setSubmitError(error)); + dispatch({ type: "FETCH_DATA_ERROR", payload: error }); + return error; + } + + // 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] ?? + null; + snapshotHandle = sourceIdentity + ? startConfirmationPriceSnapshot({ + canonicalIds: [ + getCanonicalFromAsset(sourceIdentity.code, sourceIdentity.issuer), + ...(destIdentity + ? [ + getCanonicalFromAsset( + destIdentity.code, + destIdentity.issuer, + ), + ] + : []), + ], + networkDetails, + useV2: useTokenPricesV2, + cachedDisplayPrices, + }) + : null; + const submitResp = await reduxDispatch( submitFreighterTransaction({ publicKey, diff --git a/extension/src/popup/ducks/transactionSubmission.ts b/extension/src/popup/ducks/transactionSubmission.ts index bcf8c26ad1..5924ac348b 100644 --- a/extension/src/popup/ducks/transactionSubmission.ts +++ b/extension/src/popup/ducks/transactionSubmission.ts @@ -629,6 +629,19 @@ const transactionSubmissionSlice = createSlice({ resetSubmitStatus: (state) => { state.submitStatus = initialState.submitStatus; }, + /** + * Puts the flow into its failed state without a submission having been + * attempted. `submitFreighterTransaction.rejected` covers the ordinary + * case, and `signFreighterTransaction.rejected` covers a signature that + * threw — but a sign that resolves *fulfilled* with an empty payload, and + * a hardware flow that arrives with no signed XDR, dispatch neither, so + * without this the view stays on ActionStatus.PENDING and the user is + * stranded on the sending spinner instead of reaching SubmitFail. + */ + setSubmitError: (state, action: { payload: ErrorMessage | undefined }) => { + state.submitStatus = ActionStatus.ERROR; + state.error = action.payload; + }, clearSwapQuoteExpired: (state) => { state.isSwapQuoteExpired = false; }, @@ -863,6 +876,7 @@ const transactionSubmissionSlice = createSlice({ export const { resetSubmission, resetSubmitStatus, + setSubmitError, clearSwapQuoteExpired, saveDestination, saveRecipientName,