diff --git a/@shared/api/__tests__/internal.test.ts b/@shared/api/__tests__/internal.test.ts index 0b0c5ea11c..3240dfb7ad 100644 --- a/@shared/api/__tests__/internal.test.ts +++ b/@shared/api/__tests__/internal.test.ts @@ -17,6 +17,56 @@ describe("internalApi", () => { jest.clearAllMocks(); jest.restoreAllMocks(); }); + describe("signing wrappers surface failure", () => { + // The background answers with `{ error }` rather than throwing. These + // wrappers used to discard both that answer and any transport exception, + // so a failed signing resolved like a success and telemetry recorded an + // approval that never happened. + const SIGNERS = [ + [ + "signTransaction", + () => internalApi.signTransaction({ activePublicKey: "G1", uuid: "u" }), + ], + [ + "signBlob", + () => internalApi.signBlob({ activePublicKey: "G1", uuid: "u" }), + ], + [ + "signAuthEntry", + () => internalApi.signAuthEntry({ activePublicKey: "G1", uuid: "u" }), + ], + [ + "handleSignedHwPayload", + () => + internalApi.handleSignedHwPayload({ signedPayload: "x", uuid: "u" }), + ], + ] as const; + + it.each(SIGNERS)( + "%s rejects when the background reports an error", + async (_name, call) => { + mockedSend.mockResolvedValue({ error: "Transaction not found" }); + + await expect(call()).rejects.toThrow("Transaction not found"); + }, + ); + + it.each(SIGNERS)( + "%s rejects when the message transport throws", + async (_name, call) => { + mockedSend.mockRejectedValue(new Error("Receiving end does not exist")); + + await expect(call()).rejects.toThrow("Receiving end does not exist"); + }, + ); + + it.each(SIGNERS)("%s resolves on success", async (_name, call) => { + mockedSend.mockResolvedValue({}); + + await expect(call()).resolves.toBeUndefined(); + }); + }); + describe("getAssetDomains", () => { it("should return a list of domains from a list of issuers", async () => { jest diff --git a/@shared/api/internal.ts b/@shared/api/internal.ts index 07f660d08a..d84f8b89ef 100644 --- a/@shared/api/internal.ts +++ b/@shared/api/internal.ts @@ -1546,6 +1546,33 @@ export const grantAccess = async ({ } }; +/** + * Reads a reportable message out of a background `{ error }` payload. + * + * The background returns whatever it caught, so the value is a string on some + * paths and an Error on others. `JSON.stringify` renders an Error as "{}", + * which reaches telemetry as a reason code with no information, so read the + * usual message fields first. + */ +const backgroundErrorMessage = (error: unknown): string => { + if (typeof error === "string") { + return error; + } + if (error && typeof error === "object") { + const { message, errorMessage } = error as { + message?: unknown; + errorMessage?: unknown; + }; + if (typeof message === "string" && message) { + return message; + } + if (typeof errorMessage === "string" && errorMessage) { + return errorMessage; + } + } + return "Unknown error"; +}; + export const handleSignedHwPayload = async ({ signedPayload, signerAddress, @@ -1556,15 +1583,26 @@ export const handleSignedHwPayload = async ({ uuid: string; }): Promise => { try { - await sendMessageToBackground({ + const res = await sendMessageToBackground<{ + error?: unknown; + }>({ activePublicKey: null, signedPayload, signerAddress, uuid, type: SERVICE_TYPES.HANDLE_SIGNED_HW_PAYLOAD, }); + + // The background answers with `{ error }` rather than throwing, so a + // signing failure previously looked identical to success: the caller + // resolved, and telemetry recorded an approval that never happened. + // Surface both kinds of failure so the caller can report the real outcome. + if (res && res.error) { + throw new Error(backgroundErrorMessage(res.error)); + } } catch (e) { console.error(e); + throw e; } }; @@ -1597,13 +1635,24 @@ export const signTransaction = async ({ uuid: string; }): Promise => { try { - await sendMessageToBackground({ + const res = await sendMessageToBackground<{ + error?: unknown; + }>({ activePublicKey, uuid, type: SERVICE_TYPES.SIGN_TRANSACTION, }); + + // The background answers with `{ error }` rather than throwing, so a + // signing failure previously looked identical to success: the caller + // resolved, and telemetry recorded an approval that never happened. + // Surface both kinds of failure so the caller can report the real outcome. + if (res && res.error) { + throw new Error(backgroundErrorMessage(res.error)); + } } catch (e) { console.error(e); + throw e; } }; @@ -1617,14 +1666,25 @@ export const signBlob = async ({ uuid: string; }): Promise => { try { - await sendMessageToBackground({ + const res = await sendMessageToBackground<{ + error?: unknown; + }>({ apiVersion, activePublicKey, uuid, type: SERVICE_TYPES.SIGN_BLOB, }); + + // The background answers with `{ error }` rather than throwing, so a + // signing failure previously looked identical to success: the caller + // resolved, and telemetry recorded an approval that never happened. + // Surface both kinds of failure so the caller can report the real outcome. + if (res && res.error) { + throw new Error(backgroundErrorMessage(res.error)); + } } catch (e) { console.error(e); + throw e; } }; @@ -1636,13 +1696,24 @@ export const signAuthEntry = async ({ uuid: string; }): Promise => { try { - await sendMessageToBackground({ + const res = await sendMessageToBackground<{ + error?: unknown; + }>({ activePublicKey, uuid, type: SERVICE_TYPES.SIGN_AUTH_ENTRY, }); + + // The background answers with `{ error }` rather than throwing, so a + // signing failure previously looked identical to success: the caller + // resolved, and telemetry recorded an approval that never happened. + // Surface both kinds of failure so the caller can report the real outcome. + if (res && res.error) { + throw new Error(backgroundErrorMessage(res.error)); + } } catch (e) { console.error(e); + throw e; } }; diff --git a/extension/src/helpers/metrics.test.ts b/extension/src/helpers/metrics.test.ts index 460d700ea3..abbc16e001 100644 --- a/extension/src/helpers/metrics.test.ts +++ b/extension/src/helpers/metrics.test.ts @@ -145,8 +145,8 @@ describe("buildCommonContext (four-bucket property model)", () => { ); }); - it("stamps schema_version '3'", () => { - expect(buildCommonContext({} as never).schema_version).toBe("3"); + it("stamps schema_version '4'", () => { + expect(buildCommonContext({} as never).schema_version).toBe("4"); }); it("emits the reshaped event-level bucket", () => { @@ -212,7 +212,7 @@ describe("buildCommonContext (four-bucket property model)", () => { expect(ctx).not.toHaveProperty("account_funded"); expect(ctx).not.toHaveProperty("is_hardware_account"); // non-account context is still present - expect(ctx).toMatchObject({ schema_version: "3", network: "TESTNET" }); + expect(ctx).toMatchObject({ schema_version: "4", network: "TESTNET" }); expect(ctx.surface).toBeDefined(); }); @@ -524,7 +524,7 @@ describe("emitScreenViewed (screen.viewed consolidation)", () => { expect(body).toMatchObject({ screen_name: "send_payment_amount", flow: "send", - schema_version: "3", + schema_version: "4", }); // surface comes from the Slice-A common context (getSurface()). expect(body.surface).toBeDefined(); @@ -610,7 +610,7 @@ describe("app.opened", () => { expect(call![1]).toMatchObject({ connection_type: "wifi", effective_type: "4g", - schema_version: "3", + schema_version: "4", }); expect(call![1].surface).toBeDefined(); }); diff --git a/extension/src/helpers/metrics.ts b/extension/src/helpers/metrics.ts index cf43ac9c1c..cd7f245cbb 100644 --- a/extension/src/helpers/metrics.ts +++ b/extension/src/helpers/metrics.ts @@ -123,11 +123,12 @@ const AMPLITUDE_FLUSH_INTERVAL_MS = 500; /** * Schema generation marker for the cross-platform property model. Bumped to - * "3" for the swap/send USD volume telemetry: without a bump, an event with - * no `amount_usd` is ambiguous between a pre-change client and a post-change - * client that genuinely had no price. + * "4" for the signing/step alignment: `step: "confirm"` now marks the review + * screen (before the user decides) rather than the submitting screen, so + * without a bump a `confirm` event is ambiguous between a pre-change and a + * post-change client. */ -export const SCHEMA_VERSION = "3"; +export const SCHEMA_VERSION = "4"; /** Maps the internal account type to the RFC's wire value for `account_type`. */ const ACCOUNT_TYPE_WIRE: Record = { 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 2b7a8aa713..8dffcf2a9f 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 @@ -21,6 +21,12 @@ import { makeDummyStore } from "popup/__testHelpers__"; import { initialState as txSubmissionInitialState } from "popup/ducks/transactionSubmission"; import { METRIC_NAMES } from "popup/constants/metricsNames"; import { emitMetric } from "helpers/metrics"; +import { + emitSigningApproved, + emitSigningFailed, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { useSubmitTxData } from "../useSubmitTxData"; // The emit site is the unit under test — emitMetric itself is mocked so no @@ -30,6 +36,16 @@ jest.mock("helpers/metrics", () => ({ emitMetric: jest.fn(), })); +// The signing events are a separate contract with their own suite +// (popup/metrics/__tests__/signing.test.ts). Mock the helpers so they do not +// reach emitMetric — this suite asserts on the terminal event and counts +// calls, and a signing event landing in the same mock would break that. +jest.mock("popup/metrics/signing", () => ({ + ...jest.requireActual("popup/metrics/signing"), + emitSigningApproved: jest.fn(), + emitSigningFailed: jest.fn(), +})); + // Post-success refetches are outside the telemetry contract; stub them so the // test never touches the balance/collectible backends. jest.mock("helpers/hooks/useGetBalances", () => ({ @@ -205,6 +221,8 @@ describe("useSubmitTxData terminal-event telemetry", () => { afterEach(() => { jest.restoreAllMocks(); (emitMetric as jest.Mock).mockClear(); + (emitSigningApproved as jest.Mock).mockClear(); + (emitSigningFailed as jest.Mock).mockClear(); }); it("payment.completed carries identity, token amount, and the source-leg USD family (confirmation_fetch)", async () => { @@ -544,6 +562,68 @@ describe("useSubmitTxData terminal-event telemetry", () => { expect(emitMetric).not.toHaveBeenCalled(); }); + describe("internal signing events", () => { + // Internal transactions report signing with the same events a dApp + // request uses; `source` separates the two. Without these, a wallet- + // composed transaction reported nothing for the signing action. + const mockSigningFailure = () => + jest + .spyOn(ApiInternal, "signFreighterTransaction") + .mockRejectedValue(new Error("Incorrect password")); + + it("reports an approval once a software key produces a signature", async () => { + mockSubmitOk(buildResultXdr("880000000")); + + const { result } = renderSubmitHook(makeState({ asset: "native" })); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(emitSigningApproved).toHaveBeenCalledWith( + SigningKind.Transaction, + { source: SigningSource.Internal }, + ); + expect(emitSigningFailed).not.toHaveBeenCalled(); + }); + + it("reports a failure when signing throws", async () => { + // The user already approved at the review screen, so a signing error is + // a fault, never a decision. + mockSigningFailure(); + + const { result } = renderSubmitHook(makeState({ asset: "native" })); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(emitSigningFailed).toHaveBeenCalledWith( + SigningKind.Transaction, + expect.anything(), + { source: SigningSource.Internal }, + ); + expect(emitSigningApproved).not.toHaveBeenCalled(); + }); + + it("reports nothing for a hardware flow, which the overlay owns", async () => { + // A hardware device signs in the HardwareSign overlay, which reports + // that attempt itself. This hook only receives the result, so emitting + // here would double-count. + mockSubmitOk(buildResultXdr("880000000")); + + const { result } = renderSubmitHook( + makeState({ asset: "native" }), + MAINNET_NETWORK_DETAILS, + { isHardwareWallet: true }, + ); + await act(async () => { + await result.current.fetchData({ isSwap: false }); + }); + + expect(emitSigningApproved).not.toHaveBeenCalled(); + expect(emitSigningFailed).not.toHaveBeenCalled(); + }); + }); + describe("pre-submission (signing) failure", () => { const mockSigningFailure = () => jest diff --git a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx index bd28218462..102afe4284 100644 --- a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx +++ b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx @@ -17,6 +17,12 @@ import { useGetCollectibles } from "helpers/hooks/useGetCollectibles"; import { NetworkDetails } from "@shared/constants/stellar"; import { emitMetric } from "helpers/metrics"; import { METRIC_NAMES } from "popup/constants/metricsNames"; +import { + emitSigningApproved, + emitSigningFailed, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { getAssetFromCanonical, getCanonicalFromAsset, @@ -208,6 +214,24 @@ function useSubmitTxData({ } if (!isSigned) { + // Signing did not produce a signature. This is always a fault, never + // a user decision: the user already approved at the review screen, + // and a hardware decline never reaches here — the overlay keeps the + // user on it, so the flow does not advance. + // + // Reported only for software keys. A hardware device signs in the + // HardwareSign overlay, which reports that attempt itself; this hook + // only receives the result, so emitting here would double-count. + if (!isHardwareWallet) { + emitSigningFailed( + SigningKind.Transaction, + signingError?.errorMessage, + { + source: SigningSource.Internal, + }, + ); + } + // 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 @@ -255,6 +279,16 @@ function useSubmitTxData({ return error; } + // A signature exists, so the signing action succeeded. Reported with + // the same event the dApp path uses; `source` separates the two. + // Software keys only — the overlay owns the hardware attempt (see the + // unsigned branch above). + if (!isHardwareWallet) { + emitSigningApproved(SigningKind.Transaction, { + source: SigningSource.Internal, + }); + } + // 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. diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx index ad15b83535..b11277f8e4 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx @@ -1,7 +1,13 @@ import React from "react"; import { render, screen, waitFor } from "@testing-library/react"; -import { Keypair } from "stellar-sdk"; +import { + Account, + Asset, + Keypair, + Operation, + TransactionBuilder, +} from "stellar-sdk"; import { HardwareSign } from "popup/components/hardwareConnect/HardwareSign"; import { Wrapper } from "popup/__testHelpers__"; @@ -20,6 +26,7 @@ const OTHER_PUBLIC_KEY = Keypair.fromRawEd25519Seed( const mockGetWalletPublicKey = jest.fn(); const mockHardwareSignMessage = jest.fn(); +const mockHardwareSign = jest.fn(); jest.mock("popup/helpers/hardwareConnect", () => { const actual = jest.requireActual("popup/helpers/hardwareConnect"); @@ -31,6 +38,9 @@ jest.mock("popup/helpers/hardwareConnect", () => { hardwareSignMessage: { Ledger: (...args: unknown[]) => mockHardwareSignMessage(...args), }, + hardwareSign: { + Ledger: (...args: unknown[]) => mockHardwareSign(...args), + }, }; }); @@ -42,10 +52,44 @@ const { handleSignedHwPayload } = jest.requireMock< typeof import("@shared/api/internal") >("@shared/api/internal"); -const renderOverlay = ({ - message = "Hello, Stellar!", -}: { message?: string } = {}) => - render( +const mockHandleSignedHwPayload = handleSignedHwPayload as jest.MockedFunction< + typeof handleSignedHwPayload +>; + +// Assert against the real schema builders rather than a stubbed emitter: the +// point of the hardware telemetry is that it produces the SAME event name and +// properties as the software-key path, and popup/metrics/signing is where that +// equivalence lives. +jest.mock("helpers/metrics", () => { + const actual = jest.requireActual("helpers/metrics"); + return { ...actual, emitMetric: jest.fn() }; +}); + +const { emitMetric } = + jest.requireMock("helpers/metrics"); +const mockEmitMetric = emitMetric as jest.MockedFunction; + +const DAPP_URL = "https://example.com/sign"; + +const renderOverlay = ( + opts: { + message?: string; + uuid?: string; + url?: string; + isInternal?: boolean; + } = {}, +) => { + const { + message = "Hello, Stellar!", + uuid = "test-uuid", + isInternal = false, + } = opts; + // A default parameter cannot tell "not specified" (use the default dApp url) + // from an explicit `undefined` (the view parsed no url out of the request), + // since both trigger the default. Check for the key instead. + const url = "url" in opts ? opts.url : DAPP_URL; + + return render( , ); +}; describe("HardwareSign message signing", () => { beforeEach(() => { @@ -174,3 +221,260 @@ describe("HardwareSign message signing", () => { expect(screen.queryByText("Review transaction on device")).toBeNull(); }); }); + +describe("HardwareSign message signing telemetry", () => { + // Regression cover for the hole this suite's flows used to have: a hardware + // signer produced no signing.* event at all, because useSetupSigningFlow + // diverts them to this overlay and never dispatches the signBlob thunk the + // popup/metrics/access.ts handlers are keyed on. The funnel therefore showed + // hardware rejections with no matching approvals. + beforeEach(() => { + jest.clearAllMocks(); + mockHardwareSignMessage.mockImplementation(({ message }) => + Promise.resolve(deviceKeypair.sign(encodeSep53Message(message))), + ); + // clearAllMocks does not drain a queued *Once implementation, so restore + // the resolving default explicitly for the rejection case below. + mockHandleSignedHwPayload.mockResolvedValue(undefined); + }); + + it("emits signing.message_approved once the payload reaches the background", async () => { + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + + renderOverlay(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_approved", { + message_type: "blob", + source: "dapp_api", + origin: "example.com", + }); + }); + }); + + it("does not emit the approval when the background rejects the payload", async () => { + // The signature was good, but the dApp's request was never resolved. The + // software path emits nothing on approval here either. + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + mockHandleSignedHwPayload.mockRejectedValueOnce( + new Error("Request expired"), + ); + + renderOverlay(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_failed", { + message_type: "blob", + source: "dapp_api", + reason_code: "Request expired", + origin: "example.com", + }); + }); + expect(mockEmitMetric).not.toHaveBeenCalledWith( + "signing.message_approved", + expect.anything(), + ); + }); + + it("emits signing.message_failed when no device is attached", async () => { + mockGetWalletPublicKey.mockRejectedValue(new Error("No device selected")); + + renderOverlay(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_failed", { + message_type: "blob", + source: "dapp_api", + reason_code: "No device selected", + origin: "example.com", + }); + }); + }); + + it("emits signing.message_rejected when the user declines on the device", async () => { + // A decline is a user decision, not a fault. hw-app-str raises + // StellarUserRefusedError for the deny status word, so this must land on + // the same event as pressing reject in the popup — and carry no + // reason_code, since there is nothing to report. + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + mockHardwareSignMessage.mockRejectedValue( + new Error("User refused the request"), + ); + + renderOverlay(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_rejected", { + message_type: "blob", + source: "dapp_api", + origin: "example.com", + }); + }); + expect(mockEmitMetric).not.toHaveBeenCalledWith( + "signing.message_failed", + expect.anything(), + ); + }); + + it("still reports a decline as a rejection on the legacy message", async () => { + // Older apps and transports worded the same decision differently. + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + mockHardwareSignMessage.mockRejectedValue( + new Error("Transaction approval request was rejected"), + ); + + renderOverlay(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_rejected", { + message_type: "blob", + source: "dapp_api", + origin: "example.com", + }); + }); + }); + + it("emits signing.message_failed when the device derives a different account", async () => { + mockGetWalletPublicKey.mockResolvedValue(OTHER_PUBLIC_KEY); + + renderOverlay(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith( + "signing.message_failed", + expect.objectContaining({ + message_type: "blob", + source: "dapp_api", + origin: "example.com", + }), + ); + }); + }); + + it("omits origin when the flow has no dApp url", async () => { + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + + renderOverlay({ url: undefined }); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_approved", { + message_type: "blob", + source: "dapp_api", + }); + }); + }); + + it("emits no signing event for an internal message flow", async () => { + // Internal flows never sign a message — they sign transactions, and that + // branch reports `source: "internal"` (covered below). A message flow + // with no uuid reaches neither branch, so it stays silent. + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + + renderOverlay({ isInternal: true, uuid: undefined, url: undefined }); + + await waitFor(() => { + expect(mockHardwareSignMessage).toHaveBeenCalled(); + }); + expect(mockEmitMetric).not.toHaveBeenCalled(); + }); +}); + +describe("HardwareSign internal transaction telemetry", () => { + // An internal send, swap or trustline change signs here, not in + // useSubmitTxData — that hook only receives the signed XDR. So this is the + // only place an internal hardware signing outcome can be reported. + const buildTxXdr = () => + new TransactionBuilder(new Account(TEST_PUBLIC_KEY, "0"), { + fee: "100", + networkPassphrase: TESTNET_NETWORK_DETAILS.networkPassphrase, + }) + .addOperation( + Operation.payment({ + destination: OTHER_PUBLIC_KEY, + asset: Asset.native(), + amount: "1", + }), + ) + .setTimeout(30) + .build() + .toXDR(); + + const renderInternalTransaction = () => + render( + + + , + ); + + beforeEach(() => { + jest.clearAllMocks(); + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + mockHardwareSign.mockResolvedValue(Buffer.alloc(64, 7)); + }); + + it("reports an internal approval with no origin once the device signs", async () => { + renderInternalTransaction(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith( + "signing.transaction_approved", + { source: "internal" }, + ); + }); + }); + + it("reports an internal rejection when the user declines on the device", async () => { + // This is the case that was invisible: a decline during an internal send + // produced no event at all, because the flow never advances to the + // submission hook. + mockHardwareSign.mockRejectedValue(new Error("User refused the request")); + + renderInternalTransaction(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith( + "signing.transaction_rejected", + { source: "internal" }, + ); + }); + expect(mockEmitMetric).not.toHaveBeenCalledWith( + "signing.transaction_approved", + expect.anything(), + ); + }); + + it("reports an internal failure when the device is missing", async () => { + mockGetWalletPublicKey.mockRejectedValue(new Error("No device selected")); + + renderInternalTransaction(); + + await waitFor(() => { + expect(mockEmitMetric).toHaveBeenCalledWith( + "signing.transaction_failed", + { source: "internal", reason_code: "No device selected" }, + ); + }); + }); +}); diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx index 1d3e523f95..3985211faf 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -26,8 +26,16 @@ import { WalletErrorBlock } from "popup/views/AddAccount/connect/DeviceConnect"; import { getWalletPublicKey, parseWalletError, + isDeviceRefusalError, MISMATCHED_HARDWARE_ACCOUNT_ERROR, } from "popup/helpers/hardwareConnect"; +import { + emitSigningApproved, + emitSigningFailed, + emitSigningRejected, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import LedgerSigning from "popup/assets/ledger-signing.png"; import Ledger from "popup/assets/ledger.png"; @@ -41,6 +49,7 @@ export const HardwareSign = ({ isInternal = false, onCancel, uuid, + url, }: { walletType: ConfigurableWalletType; isSignSorobanAuthorization?: boolean; @@ -49,6 +58,12 @@ export const HardwareSign = ({ isInternal?: boolean; onCancel?: () => void; uuid?: string; + /** + * The requesting dApp's URL, threaded from the signing views so the signing + * events carry the same `origin` the software-key path emits. Absent on the + * internal (send/swap/trustline) flows, which have no dApp. + */ + url?: string; }) => { const dispatch = useDispatch(); const { t } = useTranslation(); @@ -68,6 +83,59 @@ export const HardwareSign = ({ // leaves the overlay sitting on "Connect device to computer" with no reason. const [connectError, setConnectError] = useState(""); + // The overlay serves both the dApp signing prompts and the internal + // send/swap/trustline flows. Both report signing, and `source` separates + // them. A dApp request is the one that carries a `uuid` (the pending-request + // id) and is not rendered inline as an internal step. + const isDappSigningRequest = !isInternal && !!uuid; + const signingSource: SigningSource = isDappSigningRequest + ? SigningSource.DappApi + : SigningSource.Internal; + // An internal flow has no dApp, so it carries no origin. + const signingProps = { + source: signingSource, + ...(isDappSigningRequest ? { url } : {}), + }; + const signingKind: SigningKind = isSignMessage + ? SigningKind.Message + : isSignSorobanAuthorization + ? SigningKind.AuthEntry + : SigningKind.Transaction; + + // Mirrors the software path's error extraction (`action.error.message`). + // Scrubbing and the "unknown" fallback belong to emitSigningFailed, so both + // key types derive `reason_code` identically. + const errorMessage = (e: unknown): string => { + if (typeof e === "string") { + // The rejected-thunk branch passes the message directly. Stringifying it + // would wrap the reason code in quotes. + return e; + } + if (e instanceof Error) { + return e.message; + } + // `JSON.stringify` returns undefined for a value it cannot represent, so + // fall back to the empty string. `emitSigningFailed` turns that into + // "unknown". + return JSON.stringify(e) ?? ""; + }; + + /** + * Reports a hardware signing error as either a rejection or a failure. + * + * Declining on the device is a user decision, so it lands on the same + * `*_rejected` event as pressing reject in the popup — a rejection carries no + * `reason_code`, because there is no fault to report. Everything else is a + * runtime failure and keeps its scrubbed reason. + */ + const emitSigningError = (e: unknown): void => { + if (isDeviceRefusalError(e)) { + emitSigningRejected(signingKind, signingProps); + return; + } + emitSigningFailed(signingKind, errorMessage(e), signingProps); + }; + const closeOverlay = () => { if (hardwareConnectRef.current) { hardwareConnectRef.current.style.bottom = `-${POPUP_HEIGHT}px`; @@ -128,6 +196,11 @@ export const HardwareSign = ({ // should support saving signed xdr for SubmitTransaction to submit if (signWithHardwareWallet.fulfilled.match(res)) { if (shouldSubmit && !isSignSorobanAuthorization && !isSignMessage) { + // The internal branch: the device produced a signature and the flow + // carries it to submission. This is where an internal hardware + // signing succeeds — useSubmitTxData only receives the result, so it + // cannot report it. + emitSigningApproved(signingKind, signingProps); dispatch( saveSimulation({ preparedTransaction: res.payload, @@ -148,6 +221,15 @@ export const HardwareSign = ({ signerAddress: isSignMessage ? publicKey : undefined, uuid, }); + + // Emitted here, not on signWithHardwareWallet.fulfilled: the software + // path's approval event fires once the background has accepted the + // signed payload and resolved the dApp's request, and that is what + // handleSignedHwPayload just did. Emitting when the device returned a + // signature would count an approval that never reached the dApp. + if (isDappSigningRequest) { + emitSigningApproved(signingKind, signingProps); + } } closeOverlay(); if (onSubmit) { @@ -155,6 +237,7 @@ export const HardwareSign = ({ } } else { setHardwareConnectSuccessful(false); + emitSigningError(res.payload?.errorMessage); setConnectError( parseWalletError[walletType](res.payload?.errorMessage || ""), ); @@ -162,6 +245,11 @@ export const HardwareSign = ({ setHardwareWalletIsSigning(false); } catch (e) { setHardwareWalletIsSigning(false); + // Covers every throw in the block above: the user declining on the + // device, no device attached, the mismatched-account refusal, and a + // handleSignedHwPayload failure after a good signature. emitSigningError + // splits the decline (a user decision) from the rest (runtime failures). + emitSigningError(e); setConnectError(parseWalletError[walletType](e)); } setIsDetecting(false); diff --git a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/hooks/useChangeTrust.tsx b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/hooks/useChangeTrust.tsx index 4a01c3b26b..b6aedf6378 100644 --- a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/hooks/useChangeTrust.tsx +++ b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/hooks/useChangeTrust.tsx @@ -4,6 +4,12 @@ import { useTranslation } from "react-i18next"; import { initialState, reducer } from "helpers/request"; import { AppDispatch } from "popup/App"; +import { + emitSigningApproved, + emitSigningFailed, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { signFreighterTransaction, submitFreighterTransaction, @@ -97,10 +103,20 @@ function useGetChangeTrust() { ); if (signFreighterTransaction.rejected.match(res)) { + // Signing threw. The user already approved, so this is a fault, not a + // decision. Reported with the same event every other signing path + // uses; `source` marks it as wallet-composed. + emitSigningFailed(SigningKind.Transaction, res.payload?.errorMessage, { + source: SigningSource.Internal, + }); throw new Error(t("failed to sign transaction")); } if (signFreighterTransaction.fulfilled.match(res)) { + emitSigningApproved(SigningKind.Transaction, { + source: SigningSource.Internal, + }); + const submitResp = await reduxDispatch( submitFreighterTransaction({ publicKey, diff --git a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx index 2fd735bbf8..c1d6d51199 100644 --- a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx +++ b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx @@ -26,6 +26,12 @@ import { useBlockaidOverrideState, getAssetSecurityLevel, } from "popup/helpers/blockaid"; +import { + emitSigningRejected, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; + import { useGetChangeTrustData } from "./hooks/useChangeTrustData"; import { Fee } from "./Settings/Fee"; import { Timeout } from "./Settings/Timeout"; @@ -96,6 +102,35 @@ export const ChangeTrustInternal = ({ const [activeBodyContent, setActiveBodyContent] = useState( ActiveBodyContent.details, ); + // True once the user approves. This component renders only while the + // trustline review is open, so anything else that unmounts it is the user + // leaving without deciding — including the enclosing modal's backdrop, + // which no button handler sees. + const hasApprovedRef = useRef(false); + + useEffect( + () => () => { + if (!hasApprovedRef.current) { + emitSigningRejected(SigningKind.Transaction, { + source: SigningSource.Internal, + }); + } + }, + [], + ); + + /** + * Approves the review and moves to the submit step. + * + * Every route to that step goes through here, including the one behind the + * Blockaid warning. A route that skipped the latch would report an approval + * and then a rejection for the same transaction. + */ + const onApproveReview = () => { + hasApprovedRef.current = true; + setActiveBodyContent(ActiveBodyContent.submitTx); + }; + const { t } = useTranslation(); // Check override state (takes precedence, dev mode only) @@ -391,7 +426,7 @@ export const ChangeTrustInternal = ({ }`} onClick={(e) => { e.preventDefault(); - setActiveBodyContent(ActiveBodyContent.submitTx); + onApproveReview(); }} > {t("Confirm anyway")} @@ -424,7 +459,7 @@ export const ChangeTrustInternal = ({ isFullWidth isRounded size="lg" - onClick={() => setActiveBodyContent(ActiveBodyContent.submitTx)} + onClick={onApproveReview} > {t("Confirm")} diff --git a/extension/src/popup/components/send/SendAmount/index.tsx b/extension/src/popup/components/send/SendAmount/index.tsx index e60cce40d0..974477efee 100644 --- a/extension/src/popup/components/send/SendAmount/index.tsx +++ b/extension/src/popup/components/send/SendAmount/index.tsx @@ -17,7 +17,12 @@ import { isMuxedAccount, } from "helpers/stellar"; import { NetworkCongestion } from "popup/helpers/useNetworkFees"; -import { emitMetric } from "helpers/metrics"; +import { emitMetric, emitScreenViewed } from "helpers/metrics"; +import { + emitSigningRejected, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { trackSendFeeBreakdownOpened } from "popup/metrics/send"; import { getAssetDecimals, @@ -188,6 +193,41 @@ export const SendAmount = ({ const [isEditingSettings, setIsEditingSettings] = React.useState(false); const [isShowingFeesPane, setIsShowingFeesPane] = React.useState(false); const [isReviewingTx, setIsReviewingTx] = React.useState(false); + // True while the review is open, so closing it can be told apart from the + // first render. True once the user approves, or leaves to edit the memo, + // so neither is reported as a rejection. + const wasReviewingRef = useRef(false); + const skipRejectionRef = useRef(false); + + /** + * Reports the review stages: the `confirm` stage when the review opens, and + * a rejection when the user leaves it without approving. + * + * Keyed on the review's open state rather than the Cancel button, because + * the user can also leave through the modal's backdrop, and that never + * reaches a button handler. + */ + useEffect(() => { + if (isReviewingTx) { + wasReviewingRef.current = true; + skipRejectionRef.current = false; + emitScreenViewed("send_payment_confirm", { + flow: "send", + step: "confirm", + }); + return; + } + if (!wasReviewingRef.current) { + return; + } + wasReviewingRef.current = false; + if (skipRejectionRef.current) { + return; + } + emitSigningRejected(SigningKind.Transaction, { + source: SigningSource.Internal, + }); + }, [isReviewingTx]); const [contractSupportsMuxed, setContractSupportsMuxed] = React.useState< boolean | null >(null); @@ -962,8 +1002,13 @@ export const SendAmount = ({ fee={fee} networkDetails={data.networkDetails} onCancel={() => setIsReviewingTx(false)} - onConfirm={goToNext} + onConfirm={() => { + skipRejectionRef.current = true; + goToNext(); + }} onAddMemo={() => { + // Leaving to edit the memo is not a decision on the transaction. + skipRejectionRef.current = true; setIsReviewingTx(false); setMemoEditingContext(MemoEditingContext.Review); setIsEditingMemo(true); diff --git a/extension/src/popup/components/swap/SwapAmount/__tests__/SwapAmount.telemetry.test.tsx b/extension/src/popup/components/swap/SwapAmount/__tests__/SwapAmount.telemetry.test.tsx index b3f507bed9..635fb54b79 100644 --- a/extension/src/popup/components/swap/SwapAmount/__tests__/SwapAmount.telemetry.test.tsx +++ b/extension/src/popup/components/swap/SwapAmount/__tests__/SwapAmount.telemetry.test.tsx @@ -23,6 +23,10 @@ import * as XlmReserve from "popup/helpers/xlmReserve"; jest.mock("helpers/metrics", () => ({ ...jest.requireActual("helpers/metrics"), emitMetric: jest.fn(), + // Opening the review modal emits the `confirm` screen view. The real + // emitScreenViewed runs buildCommonContext, which reads Redux slices this + // suite's minimal store does not provide, so stub it too. + emitScreenViewed: jest.fn(), })); // The quote-expired notice is a sonner toast; assert it fires rather than diff --git a/extension/src/popup/components/swap/SwapAmount/index.tsx b/extension/src/popup/components/swap/SwapAmount/index.tsx index 81fceb4c6f..42a64afd24 100644 --- a/extension/src/popup/components/swap/SwapAmount/index.tsx +++ b/extension/src/popup/components/swap/SwapAmount/index.tsx @@ -41,7 +41,12 @@ import { getAvailableBalance } from "popup/helpers/soroban"; import { getBalanceCanonicalKey } from "popup/helpers/balance"; import { useBlockaidOverrideState } from "popup/helpers/blockaid"; import { AppDispatch } from "popup/App"; -import { emitMetric } from "helpers/metrics"; +import { emitMetric, emitScreenViewed } from "helpers/metrics"; +import { + emitSigningRejected, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { InputType } from "helpers/transaction"; import { METRIC_NAMES } from "popup/constants/metricsNames"; import { XLM_RESERVE_HELP_URL } from "popup/constants/externalLinks"; @@ -187,6 +192,33 @@ export const SwapAmount = ({ const [isEditingSlippage, setIsEditingSlippage] = useState(false); const [isEditingSettings, setIsEditingSettings] = useState(false); const [isReviewingTx, setIsReviewingTx] = React.useState(false); + + // True while the review is open, and true once the user approves. See the + // equivalent refs in SendAmount. + const wasReviewingRef = useRef(false); + const skipRejectionRef = useRef(false); + + // Reports the `confirm` stage when the review opens, and a rejection when + // the user leaves it without approving — see the equivalent effect in + // SendAmount for why this is keyed on the open state. + useEffect(() => { + if (isReviewingTx) { + wasReviewingRef.current = true; + skipRejectionRef.current = false; + emitScreenViewed("swap_confirm", { flow: "swap", step: "confirm" }); + return; + } + if (!wasReviewingRef.current) { + return; + } + wasReviewingRef.current = false; + if (skipRejectionRef.current) { + return; + } + emitSigningRejected(SigningKind.Transaction, { + source: SigningSource.Internal, + }); + }, [isReviewingTx]); const [isXlmReserveOpen, setIsXlmReserveOpen] = useState(false); // Tracks focus on the sell input so the "Enter an amount" CTA can disable // itself while the input is focused. The extension has no virtual keyboard, @@ -861,7 +893,10 @@ export const SwapAmount = ({ // The trustline-added + swap-success metrics fire post-confirmation // (in useSubmitTxData), once the swap actually settles — not here at // review time. - onConfirm={goToNext} + onConfirm={() => { + skipRejectionRef.current = true; + goToNext(); + }} sendAmount={amount} // Show the same fiat figure the amount screen displayed: the // entered dollars in fiat mode, the computed USD of the crypto diff --git a/extension/src/popup/constants/__tests__/metricsNames.test.ts b/extension/src/popup/constants/__tests__/metricsNames.test.ts index 65f067493e..855a8e530b 100644 --- a/extension/src/popup/constants/__tests__/metricsNames.test.ts +++ b/extension/src/popup/constants/__tests__/metricsNames.test.ts @@ -30,7 +30,9 @@ describe("METRIC_NAMES domain-event catalog", () => { it("names swap events (routed/path-payment outcomes settle here too)", () => { expect(METRIC_NAMES.swapPickerOpened).toBe("swap.picker_opened"); expect(METRIC_NAMES.swapSourceSelected).toBe("swap.source_selected"); - expect(METRIC_NAMES.swapDestinationSelected).toBe("swap.destination_selected"); + expect(METRIC_NAMES.swapDestinationSelected).toBe( + "swap.destination_selected", + ); expect(METRIC_NAMES.swapDirectionToggled).toBe("swap.direction_toggled"); expect(METRIC_NAMES.swapTrustlineAdded).toBe("swap.trustline_added"); expect(METRIC_NAMES.swapXlmReserveInsufficientShown).toBe( @@ -94,6 +96,9 @@ describe("METRIC_NAMES domain-event catalog", () => { expect(METRIC_NAMES.signingTransactionRejected).toBe( "signing.transaction_rejected", ); + expect(METRIC_NAMES.signingTransactionFailed).toBe( + "signing.transaction_failed", + ); expect(METRIC_NAMES.signingTransactionBlocked).toBe( "signing.transaction_blocked", ); @@ -106,8 +111,12 @@ describe("METRIC_NAMES domain-event catalog", () => { expect(METRIC_NAMES.signingAuthEntryFailed).toBe( "signing.auth_entry_failed", ); - expect(METRIC_NAMES.signingMessageApproved).toBe("signing.message_approved"); - expect(METRIC_NAMES.signingMessageRejected).toBe("signing.message_rejected"); + expect(METRIC_NAMES.signingMessageApproved).toBe( + "signing.message_approved", + ); + expect(METRIC_NAMES.signingMessageRejected).toBe( + "signing.message_rejected", + ); expect(METRIC_NAMES.signingMessageFailed).toBe("signing.message_failed"); }); diff --git a/extension/src/popup/constants/metricsNames.ts b/extension/src/popup/constants/metricsNames.ts index 0758f417a5..0cbcf45a23 100644 --- a/extension/src/popup/constants/metricsNames.ts +++ b/extension/src/popup/constants/metricsNames.ts @@ -81,7 +81,8 @@ export const METRIC_NAMES = { "onboarding.recovery_phrase_confirm_failed", // Not emitted on extension: the create-account recovery-phrase screens have // no Back affordance to instrument. Mobile emits it; kept for a shared catalog. - onboardingRecoveryPhraseBackClicked: "onboarding.recovery_phrase_back_clicked", + onboardingRecoveryPhraseBackClicked: + "onboarding.recovery_phrase_back_clicked", onboardingCompleted: "onboarding.completed", // -- Account recovery / management -------------------------------------- @@ -124,6 +125,11 @@ export const METRIC_NAMES = { // -- Signing ------------------------------------------------------------- signingTransactionApproved: "signing.transaction_approved", signingTransactionRejected: "signing.transaction_rejected", + // Signing threw for a reason the user did not choose. Kept distinct from + // `rejected`, which is the user declining — in the popup or on a hardware + // device. Mirrors the message and auth-entry families, which have carried + // both halves of this split from the start. + signingTransactionFailed: "signing.transaction_failed", signingTransactionBlocked: "signing.transaction_blocked", signingAuthEntryApproved: "signing.auth_entry_approved", signingAuthEntryRejected: "signing.auth_entry_rejected", diff --git a/extension/src/popup/helpers/__tests__/hardwareConnect.test.ts b/extension/src/popup/helpers/__tests__/hardwareConnect.test.ts index d11c205c85..4709dd2fba 100644 --- a/extension/src/popup/helpers/__tests__/hardwareConnect.test.ts +++ b/extension/src/popup/helpers/__tests__/hardwareConnect.test.ts @@ -7,6 +7,7 @@ import { hardwareSign, hardwareSignAuth, hardwareSignMessage, + isDeviceRefusalError, parseWalletError, MIN_SIGN_MESSAGE_APP_VERSION, UNSUPPORTED_SIGN_MESSAGE_APP_ERROR, @@ -304,3 +305,38 @@ describe("parseWalletError", () => { expect(message).toBe("Some other device failure"); }); }); + +describe("isDeviceRefusalError", () => { + // Telemetry uses this to tell a user decision from a fault: a decline is + // reported as a rejection, everything else as a failure. + it("recognises the refusal hw-app-str raises for the deny status word", () => { + expect(isDeviceRefusalError(new Error("User refused the request"))).toBe( + true, + ); + }); + + it("recognises the wording older apps and transports produced", () => { + expect( + isDeviceRefusalError( + new Error("Transaction approval request was rejected"), + ), + ).toBe(true); + }); + + it("does not treat a missing device as a refusal", () => { + expect(isDeviceRefusalError(new Error("No device selected"))).toBe(false); + }); + + it("does not treat the mismatched-account sentinel as a refusal", () => { + // The wallet refuses here, not the user. + expect( + isDeviceRefusalError(new Error(MISMATCHED_HARDWARE_ACCOUNT_ERROR)), + ).toBe(false); + }); + + it("handles a non-Error value without throwing", () => { + expect(isDeviceRefusalError("User refused the request")).toBe(true); + expect(isDeviceRefusalError(undefined)).toBe(false); + expect(isDeviceRefusalError(null)).toBe(false); + }); +}); diff --git a/extension/src/popup/helpers/hardwareConnect.ts b/extension/src/popup/helpers/hardwareConnect.ts index 8f0089c96a..b668a80de0 100644 --- a/extension/src/popup/helpers/hardwareConnect.ts +++ b/extension/src/popup/helpers/hardwareConnect.ts @@ -42,6 +42,31 @@ export const UNVERIFIED_SIGN_MESSAGE_ERROR = // getAppConfiguration reports alongside the version. export const OVERSIZED_SIGN_MESSAGE_ERROR = "SIGN_MESSAGE_TOO_LARGE"; +// Messages that mean the user declined on the device rather than something +// going wrong. hw-app-str raises StellarUserRefusedError("User refused the +// request") for the deny status word (0x6985) on every sign call; the second +// string is what older apps and transports produced for the same decision. +// Kept next to parseWalletError, which matches the same two strings, so the +// two never drift apart. +const DEVICE_REFUSAL_MESSAGES = [ + "User refused the request", + "Transaction approval request was rejected", +]; + +/** + * True when a hardware error is the user declining on the device. + * + * A decline is a user decision, so telemetry reports it as a rejection, the + * same as pressing reject in the popup. Every other hardware error — no device + * attached, a transport fault, the wrong device, an app too old — is a failure + * the user did not choose. Matching on the message is the only signal + * available: the deny status word reaches us already wrapped in an Error. + */ +export const isDeviceRefusalError = (error: unknown): boolean => { + const message = error instanceof Error ? error.message : String(error ?? ""); + return DEVICE_REFUSAL_MESSAGES.some((refusal) => message.includes(refusal)); +}; + /* ** HELPER METHODS */ diff --git a/extension/src/popup/helpers/useSetupSigningFlow.ts b/extension/src/popup/helpers/useSetupSigningFlow.ts index 5a2b0dc9b2..f601474b27 100644 --- a/extension/src/popup/helpers/useSetupSigningFlow.ts +++ b/extension/src/popup/helpers/useSetupSigningFlow.ts @@ -47,10 +47,16 @@ export function useSetupSigningFlow( hardwareWalletData: { status: hwStatus }, } = useSelector(transactionSubmissionSelector); - // Approval/rejection telemetry is emitted per signing type by the redux - // handlers in popup/metrics/access.ts (signing.transaction_*, - // signing.message_*, signing.auth_entry_*), keyed off the specific - // sign/reject thunk this flow dispatches — so no generic event fires here. + // Approval/rejection telemetry is emitted per signing type (signing.transaction_*, + // signing.message_*, signing.auth_entry_*) — so no generic event fires here. + // Which component emits depends on the branch signAndClose() takes below: + // - software keys: the redux handlers in popup/metrics/access.ts, keyed off + // the specific sign/reject thunk this flow dispatches. + // - hardware keys: the HardwareSign overlay, since startHwSign bypasses the + // sign thunk entirely and those handlers would never fire. + // Both paths emit through popup/metrics/signing so the schemas stay identical. + // Rejection is shared: rejectAndClose dispatches the reject thunk for both + // key types, so the *_rejected events come from access.ts either way. const rejectAndClose = () => { dispatch(reject({ uuid, url })); window.close(); diff --git a/extension/src/popup/metrics/__tests__/signing.test.ts b/extension/src/popup/metrics/__tests__/signing.test.ts new file mode 100644 index 0000000000..84f9950122 --- /dev/null +++ b/extension/src/popup/metrics/__tests__/signing.test.ts @@ -0,0 +1,168 @@ +import { emitMetric } from "helpers/metrics"; +import { METRIC_NAMES } from "popup/constants/metricsNames"; + +import { + emitSigningApproved, + emitSigningFailed, + emitSigningRejected, + originProps, + SigningKind, + SigningSource, +} from "../signing"; + +jest.mock("helpers/metrics", () => ({ + emitMetric: jest.fn(), +})); + +const mockEmitMetric = emitMetric as jest.MockedFunction; + +const DAPP_URL = "https://example.com/app?foo=bar"; +const DAPP = { source: SigningSource.DappApi, url: DAPP_URL }; +const INTERNAL = { source: SigningSource.Internal }; + +describe("originProps", () => { + beforeEach(() => jest.clearAllMocks()); + + it("reduces a full URL to the bare hostname", () => { + expect(originProps(DAPP_URL)).toEqual({ origin: "example.com" }); + }); + + it("omits origin when there is no url", () => { + expect(originProps(undefined)).toEqual({}); + expect(originProps("")).toEqual({}); + }); + + it("omits origin when the url does not parse", () => { + expect(originProps("not-a-url")).toEqual({}); + }); +}); + +describe("emitSigningApproved", () => { + beforeEach(() => jest.clearAllMocks()); + + it.each([ + [SigningKind.Transaction, METRIC_NAMES.signingTransactionApproved, {}], + [ + SigningKind.Message, + METRIC_NAMES.signingMessageApproved, + { message_type: "blob" }, + ], + [SigningKind.AuthEntry, METRIC_NAMES.signingAuthEntryApproved, {}], + ] as const)( + "emits the %s approval for a dApp request", + (kind, name, extra) => { + emitSigningApproved(kind, DAPP); + + expect(mockEmitMetric).toHaveBeenCalledWith(name, { + ...extra, + source: SigningSource.DappApi, + origin: "example.com", + }); + }, + ); + + it("emits an internal approval with no origin", () => { + // An internal transaction has no dApp, so `origin` stays off the payload + // and `source` is what separates it from a website request. + emitSigningApproved(SigningKind.Transaction, INTERNAL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingTransactionApproved, + { source: SigningSource.Internal }, + ); + }); +}); + +describe("emitSigningRejected", () => { + beforeEach(() => jest.clearAllMocks()); + + it.each([ + [SigningKind.Transaction, METRIC_NAMES.signingTransactionRejected, {}], + [ + SigningKind.Message, + METRIC_NAMES.signingMessageRejected, + { message_type: "blob" }, + ], + [SigningKind.AuthEntry, METRIC_NAMES.signingAuthEntryRejected, {}], + ] as const)("emits the %s rejection", (kind, name, extra) => { + emitSigningRejected(kind, DAPP); + + // A rejection is a user decision, so it never carries a reason_code. + expect(mockEmitMetric).toHaveBeenCalledWith(name, { + ...extra, + source: SigningSource.DappApi, + origin: "example.com", + }); + }); + + it("emits an internal rejection with no origin", () => { + emitSigningRejected(SigningKind.Transaction, INTERNAL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingTransactionRejected, + { source: SigningSource.Internal }, + ); + }); +}); + +describe("emitSigningFailed", () => { + beforeEach(() => jest.clearAllMocks()); + + it.each([ + [SigningKind.Transaction, METRIC_NAMES.signingTransactionFailed, {}], + [ + SigningKind.Message, + METRIC_NAMES.signingMessageFailed, + { message_type: "blob" }, + ], + [SigningKind.AuthEntry, METRIC_NAMES.signingAuthEntryFailed, {}], + ] as const)( + "emits the %s failure with a reason_code", + (kind, name, extra) => { + emitSigningFailed(kind, "Device error", DAPP); + + expect(mockEmitMetric).toHaveBeenCalledWith(name, { + ...extra, + source: SigningSource.DappApi, + reason_code: "Device error", + origin: "example.com", + }); + }, + ); + + it("emits an internal failure with no origin", () => { + emitSigningFailed(SigningKind.Transaction, "op_underfunded", INTERNAL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingTransactionFailed, + { source: SigningSource.Internal, reason_code: "op_underfunded" }, + ); + }); + + it("scrubs Stellar StrKeys out of the reason_code", () => { + // Amplitude is a third-party sink not covered by Sentry's beforeSend, and + // a signing error can echo the account it tried to sign as. + emitSigningFailed( + SigningKind.Message, + "cannot sign as GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + DAPP, + ); + + const [, props] = mockEmitMetric.mock.calls[0]; + expect(props!.reason_code).toBe("cannot sign as G***"); + }); + + it("falls back to unknown when there is no message", () => { + emitSigningFailed(SigningKind.Message, undefined, DAPP); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingMessageFailed, + { + message_type: "blob", + source: SigningSource.DappApi, + reason_code: "unknown", + origin: "example.com", + }, + ); + }); +}); diff --git a/extension/src/popup/metrics/access.ts b/extension/src/popup/metrics/access.ts index 15ab40a9c7..57d99696c8 100644 --- a/extension/src/popup/metrics/access.ts +++ b/extension/src/popup/metrics/access.ts @@ -13,30 +13,30 @@ import { rejectAuthEntry, } from "popup/ducks/access"; import { registerHandler, emitMetric } from "helpers/metrics"; -import { scrubStrKeys } from "helpers/stellarStrKey"; -import { getUrlHostname } from "helpers/urls"; +import { + emitSigningApproved, + emitSigningFailed, + emitSigningRejected, + originProps, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { AppState } from "popup/App"; // account_type / is_hardware_account now ride on every event via // buildCommonContext, so the per-handler metricsData reads are gone. // The dApp origin rides in the thunk arg (`action.meta.arg.url`), threaded from -// the signing/grant views (useSetupSigningFlow / grantAccess). Attach it as -// `origin`, normalized to the bare hostname so it matches mobile's -// dappDomain-based `origin` (never a full URL). Omit when no url is present. -const originProps = (action: { - meta?: { arg?: { url?: string } }; -}): { origin?: string } => { - const url = action.meta?.arg?.url; - const origin = url ? getUrlHostname(url) : ""; - return origin ? { origin } : {}; -}; +// the signing/grant views (useSetupSigningFlow / grantAccess). `originProps` +// turns it into the `origin` property (bare hostname, never a full URL). +const argUrl = (action: { meta?: { arg?: { url?: string } } }) => + action.meta?.arg?.url; registerHandler(grantAccess.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.dappAccessGranted, originProps(action)); + emitMetric(METRIC_NAMES.dappAccessGranted, originProps(argUrl(action))); }); registerHandler(rejectAccess.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.dappAccessRejected, originProps(action)); + emitMetric(METRIC_NAMES.dappAccessRejected, originProps(argUrl(action))); }); // asset_code (when the flow knew the token's code) mirrors mobile's // asset_add.responded { asset_code }; undefined stays off the payload. @@ -52,62 +52,83 @@ registerHandler(addToken.fulfilled, (_state, action) => { // source is fixed. Distinguishes it from mobile's manual add (source:manage_assets). emitMetric(METRIC_NAMES.assetAddResponded, { decision: "confirm", - source: "dapp_api", + source: SigningSource.DappApi, ...assetCodeProps(action), }); }); registerHandler(rejectToken.fulfilled, (_state, action) => { emitMetric(METRIC_NAMES.assetAddResponded, { decision: "reject", - source: "dapp_api", + source: SigningSource.DappApi, ...assetCodeProps(action), }); }); +// Software-key signing outcomes. Hardware keys never reach these thunks (see +// popup/metrics/signing and the HardwareSign overlay); both key types emit +// through the same helpers so the two paths cannot drift apart. registerHandler(signTransaction.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingTransactionApproved, originProps(action)); + emitSigningApproved(SigningKind.Transaction, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); registerHandler(rejectTransaction.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingTransactionRejected, originProps(action)); + emitSigningRejected(SigningKind.Transaction, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); registerHandler(signBlob.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingMessageApproved, { - message_type: "blob", - ...originProps(action), + emitSigningApproved(SigningKind.Message, { + source: SigningSource.DappApi, + url: argUrl(action), }); }); registerHandler(rejectBlob.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingMessageRejected, { - message_type: "blob", - ...originProps(action), + emitSigningRejected(SigningKind.Message, { + source: SigningSource.DappApi, + url: argUrl(action), }); }); registerHandler(signEntry.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingAuthEntryApproved, originProps(action)); + emitSigningApproved(SigningKind.AuthEntry, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); registerHandler(rejectAuthEntry.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingAuthEntryRejected, originProps(action)); + emitSigningRejected(SigningKind.AuthEntry, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); // Runtime signing FAILURE paths — distinct from the user-cancel // (`reject*.fulfilled`) events above. The sign thunks don't catch, so a runtime // error surfaces as `.rejected` with the message on `action.error`. -const rejectedReasonCode = (action: { +// emitSigningFailed scrubs it and applies the "unknown" fallback. +const rejectedError = (action: { error?: { message?: string }; payload?: { errorMessage?: string }; -}): string => - scrubStrKeys(action.error?.message || action.payload?.errorMessage) || - "unknown"; +}): string | undefined => action.error?.message || action.payload?.errorMessage; registerHandler(signBlob.rejected, (_state, action) => { - emitMetric(METRIC_NAMES.signingMessageFailed, { - message_type: "blob", - reason_code: rejectedReasonCode(action), - ...originProps(action), + emitSigningFailed(SigningKind.Message, rejectedError(action), { + source: SigningSource.DappApi, + url: argUrl(action), }); }); registerHandler(signEntry.rejected, (_state, action) => { - emitMetric(METRIC_NAMES.signingAuthEntryFailed, { - reason_code: rejectedReasonCode(action), - ...originProps(action), + emitSigningFailed(SigningKind.AuthEntry, rejectedError(action), { + source: SigningSource.DappApi, + url: argUrl(action), + }); +}); +// The transaction family now has a failure event too, so a dApp transaction +// that throws while signing reports an outcome instead of going silent. +registerHandler(signTransaction.rejected, (_state, action) => { + emitSigningFailed(SigningKind.Transaction, rejectedError(action), { + source: SigningSource.DappApi, + url: argUrl(action), }); }); diff --git a/extension/src/popup/metrics/signing.ts b/extension/src/popup/metrics/signing.ts new file mode 100644 index 0000000000..b6cf653bc7 --- /dev/null +++ b/extension/src/popup/metrics/signing.ts @@ -0,0 +1,135 @@ +import { METRIC_NAMES } from "popup/constants/metricsNames"; +import { emitMetric } from "helpers/metrics"; +import { scrubStrKeys } from "helpers/stellarStrKey"; +import { getUrlHostname } from "helpers/urls"; + +/** + * The three signing requests a dApp can make of the extension. + * + * Every signing path emits through this module, so one signing request + * produces the same event name and the same property set wherever it ran: + * the dApp thunks in `popup/metrics/access.ts`, the HardwareSign overlay, the + * internal submission hook, and the trustline flow. + */ +export enum SigningKind { + Transaction = "transaction", + Message = "message", + AuthEntry = "authEntry", +} + +/** + * Where a signing request came from. + * + * `dapp_api` is a website asking through the injected API. `internal` is a + * transaction the wallet composed itself — a send, a swap, a collectible send, + * or a trustline change. Both origins emit the same events with the same + * properties, so one query counts all signing and `source` splits it. The + * token add and remove events already use `dapp_api` this way. + */ +export enum SigningSource { + DappApi = "dapp_api", + Internal = "internal", +} + +interface SigningEventOptions { + source: SigningSource; + /** + * The requesting dApp's URL. Internal transactions have no origin, so they + * omit it and the `origin` property stays off the payload. + */ + url?: string; +} + +/** + * The dApp origin, normalized to the bare hostname so it matches mobile's + * dappDomain-based `origin` (never a full URL). Omitted when no url is present. + */ +export const originProps = (url?: string): { origin?: string } => { + const origin = url ? getUrlHostname(url) : ""; + return origin ? { origin } : {}; +}; + +/** + * Payload-identifying properties that every event of a kind carries. Only the + * message events have one: the extension signs SEP-53 blobs, so `message_type` + * is the constant "blob" (mobile emits the same constant). + */ +const KIND_PROPS: Record> = { + [SigningKind.Transaction]: {}, + [SigningKind.Message]: { message_type: "blob" }, + [SigningKind.AuthEntry]: {}, +}; + +const APPROVED_EVENT: Record = { + [SigningKind.Transaction]: METRIC_NAMES.signingTransactionApproved, + [SigningKind.Message]: METRIC_NAMES.signingMessageApproved, + [SigningKind.AuthEntry]: METRIC_NAMES.signingAuthEntryApproved, +}; + +const REJECTED_EVENT: Record = { + [SigningKind.Transaction]: METRIC_NAMES.signingTransactionRejected, + [SigningKind.Message]: METRIC_NAMES.signingMessageRejected, + [SigningKind.AuthEntry]: METRIC_NAMES.signingAuthEntryRejected, +}; + +/** + * Runtime-failure events, kept distinct from the user-decline `*_rejected` + * events above. Every kind has one, so a signing attempt always reports an + * outcome: approved, rejected, or failed. + */ +const FAILED_EVENT: Record = { + [SigningKind.Transaction]: METRIC_NAMES.signingTransactionFailed, + [SigningKind.Message]: METRIC_NAMES.signingMessageFailed, + [SigningKind.AuthEntry]: METRIC_NAMES.signingAuthEntryFailed, +}; + +/** The user approved the request and signing produced a signature. */ +export const emitSigningApproved = ( + kind: SigningKind, + { source, url }: SigningEventOptions, +): void => { + emitMetric(APPROVED_EVENT[kind], { + ...KIND_PROPS[kind], + source, + ...originProps(url), + }); +}; + +/** + * The user declined the request — by pressing reject in the popup, or by + * declining on a hardware device. Both are the same decision, so both land + * here. A rejection carries no `reason_code`: nothing went wrong. + */ +export const emitSigningRejected = ( + kind: SigningKind, + { source, url }: SigningEventOptions, +): void => { + emitMetric(REJECTED_EVENT[kind], { + ...KIND_PROPS[kind], + source, + ...originProps(url), + }); +}; + +/** + * Signing threw for a reason the user did not choose: a locked wallet, a key + * that does not decrypt, a malformed payload, a missing or wrong hardware + * device, a transport fault. A user declining is NOT a failure — that is + * emitSigningRejected. + * + * `reason_code` carries the scrubbed message: a signing error can embed a + * G…/S… key and Amplitude is a third-party sink not covered by Sentry's + * beforeSend. Falls back to "unknown" so the property is never absent. + */ +export const emitSigningFailed = ( + kind: SigningKind, + error: string | undefined, + { source, url }: SigningEventOptions, +): void => { + emitMetric(FAILED_EVENT[kind], { + ...KIND_PROPS[kind], + source, + reason_code: scrubStrKeys(error) || "unknown", + ...originProps(url), + }); +}; diff --git a/extension/src/popup/views/IntegrationTest.tsx b/extension/src/popup/views/IntegrationTest.tsx index 28e19a7f2f..50fa5a3e66 100644 --- a/extension/src/popup/views/IntegrationTest.tsx +++ b/extension/src/popup/views/IntegrationTest.tsx @@ -293,19 +293,38 @@ export const IntegrationTest = () => { runAsserts("grantAccess", () => {}); - await handleSignedHwPayload({ - signedPayload: "", - uuid: "integration-test", - }); - - runAsserts("handleSignedHwPayload", () => {}); - - await signTransaction({ - activePublicKey: testPublicKey, - uuid: "integration-test", - }); - - runAsserts("signTransaction", () => {}); + // The signing wrappers report a background failure by rejecting. This + // run uses a placeholder request id, which no queue entry matches, so + // each call rejects by design. Swallow it here: the check is that the + // call completes its round trip, not that the signing succeeds. + const expectSigningFailure = async ( + name: string, + call: () => Promise, + ) => { + let rejected = false; + try { + await call(); + } catch { + rejected = true; + } + runAsserts(name, () => { + assertEq(rejected, true); + }); + }; + + await expectSigningFailure("handleSignedHwPayload", () => + handleSignedHwPayload({ + signedPayload: "", + uuid: "integration-test", + }), + ); + + await expectSigningFailure("signTransaction", () => + signTransaction({ + activePublicKey: testPublicKey, + uuid: "integration-test", + }), + ); res = await signFreighterTransaction({ activePublicKey: testPublicKey, diff --git a/extension/src/popup/views/Send/index.tsx b/extension/src/popup/views/Send/index.tsx index 222e8d82fe..4c989ee638 100644 --- a/extension/src/popup/views/Send/index.tsx +++ b/extension/src/popup/views/Send/index.tsx @@ -43,13 +43,11 @@ const SEND_SCREEN_BY_STEP: Partial< flow: "send", }, [STEPS.AMOUNT]: { screen_name: "send_payment_amount", flow: "send" }, - [STEPS.PAYMENT_CONFIRM]: { - screen_name: "send_payment_confirm", - flow: "send", - // Canonical cross-platform stage (RFC #2883): mobile tags this screen - // step:"confirm"; keep them in sync so `step` is funnel-able across both. - step: "confirm", - }, + // STEPS.PAYMENT_CONFIRM is deliberately absent. It renders the submitting + // screen, which the user only reaches after approving, so it is not the + // `confirm` stage — SendAmount's review modal is, and it emits + // `send_payment_confirm` itself. This screen's own stages are already + // covered by the submitStatus effect below (processing, then success). [STEPS.DESTINATION]: { screen_name: "send_payment_to", flow: "send" }, }; @@ -191,6 +189,11 @@ export const Send = () => { const lastEmittedStep = useRef(null); const hasEmittedProcessing = useRef(false); const hasEmittedSuccess = useRef(false); + // The submission status lives in the store, so it outlives this component. + // A mount that finds a stale terminal status would report a stage the user + // never reached, so wait until the status has been seen idle. The reset + // this component dispatches on mount guarantees that happens. + const hasSeenIdle = useRef(false); const goToStep = ( next: STEPS, @@ -229,6 +232,9 @@ export const Send = () => { // emits once per submission; reset when the status clears so a subsequent // send re-emits. useEffect(() => { + if (!hasSeenIdle.current && submission.submitStatus !== ActionStatus.IDLE) { + return; + } if (submission.submitStatus === ActionStatus.PENDING) { if (!hasEmittedProcessing.current) { hasEmittedProcessing.current = true; @@ -245,7 +251,15 @@ export const Send = () => { step: "success", }); } - } else if (submission.submitStatus === ActionStatus.IDLE) { + } else if ( + submission.submitStatus === ActionStatus.IDLE || + submission.submitStatus === ActionStatus.ERROR + ) { + // Reset on ERROR as well as IDLE. A retry goes ERROR -> PENDING without + // passing through IDLE (the user returns via goBack, which does not + // reset the submission), so guarding on IDLE alone silently dropped + // every retried attempt's `processing` stage. + hasSeenIdle.current = true; hasEmittedProcessing.current = false; hasEmittedSuccess.current = false; } diff --git a/extension/src/popup/views/SignAuthEntry/index.tsx b/extension/src/popup/views/SignAuthEntry/index.tsx index 7dc35bac26..34bc6c93f5 100644 --- a/extension/src/popup/views/SignAuthEntry/index.tsx +++ b/extension/src/popup/views/SignAuthEntry/index.tsx @@ -250,6 +250,7 @@ export const SignAuthEntry = () => { walletType={hardwareWalletType} isSignSorobanAuthorization uuid={params.uuid} + url={params.url} /> )} diff --git a/extension/src/popup/views/SignMessage/index.tsx b/extension/src/popup/views/SignMessage/index.tsx index a3fe3a7d9b..74c57bfeee 100644 --- a/extension/src/popup/views/SignMessage/index.tsx +++ b/extension/src/popup/views/SignMessage/index.tsx @@ -37,7 +37,7 @@ import { Loading } from "popup/components/Loading"; import { AppDataType } from "helpers/hooks/useGetAppData"; import { openTab } from "popup/helpers/navigate"; import { useSetupSigningFlow } from "popup/helpers/useSetupSigningFlow"; -import { rejectTransaction, signBlob } from "popup/ducks/access"; +import { rejectBlob, signBlob } from "popup/ducks/access"; import { publicKeySelector } from "popup/ducks/accountServices"; import { reRouteOnboarding } from "popup/helpers/route"; import { getSiteFavicon } from "popup/helpers/getSiteFavicon"; @@ -94,7 +94,7 @@ export const SignMessage = () => { verifyPasswordThenSign, hardwareWalletType, } = useSetupSigningFlow( - rejectTransaction, + rejectBlob, signBlob, message.message, message.uuid, @@ -215,6 +215,7 @@ export const SignMessage = () => { walletType={hardwareWalletType} isSignMessage uuid={message.uuid} + url={url} /> )} diff --git a/extension/src/popup/views/SignTransaction/index.tsx b/extension/src/popup/views/SignTransaction/index.tsx index 61a06310e6..e395ad5e32 100644 --- a/extension/src/popup/views/SignTransaction/index.tsx +++ b/extension/src/popup/views/SignTransaction/index.tsx @@ -458,7 +458,7 @@ export const SignTransaction = () => { ) : ( <> {hwStatus === ShowOverlayStatus.IN_PROGRESS && hardwareWalletType && ( - + )}
{isOnBlockaidSheet ? ( diff --git a/extension/src/popup/views/Swap/index.tsx b/extension/src/popup/views/Swap/index.tsx index 672d4d5a8d..3d3d61cc74 100644 --- a/extension/src/popup/views/Swap/index.tsx +++ b/extension/src/popup/views/Swap/index.tsx @@ -42,13 +42,11 @@ import { const SWAP_SCREEN_BY_STEP: Partial< Record > = { - [STEPS.SWAP_CONFIRM]: { - screen_name: "swap_confirm", - flow: "swap", - // Canonical cross-platform stage (RFC #2883): mobile tags this screen - // step:"confirm"; keep them in sync so `step` is funnel-able across both. - step: "confirm", - }, + // STEPS.SWAP_CONFIRM is deliberately absent. It renders the submitting + // screen, which the user only reaches after approving, so it is not the + // `confirm` stage — SwapAmount's review modal is, and it emits + // `swap_confirm` itself. This screen's own stages are covered by the + // submitStatus effect below (processing, then success). [STEPS.SET_DST_ASSET]: { screen_name: "swap_to_asset", flow: "swap" }, [STEPS.AMOUNT]: { screen_name: "swap_amount", flow: "swap" }, [STEPS.CONFIRM_AMOUNT]: { screen_name: "swap_amount_review", flow: "swap" }, @@ -61,6 +59,13 @@ export const Swap = () => { const location = useLocation(); const [activeStep, setActiveStep] = useState(STEPS.AMOUNT); const lastEmittedStep = useRef(null); + const hasEmittedProcessing = useRef(false); + const hasEmittedSuccess = useRef(false); + // The submission status lives in the store, so it outlives this component. + // A mount that finds a stale terminal status would report a stage the user + // never reached, so wait until the status has been seen idle. The reset + // this component dispatches on mount guarantees that happens. + const hasSeenIdle = useRef(false); // Emit a screen-view metric only once per step transition. useEffect(() => { @@ -75,6 +80,7 @@ export const Swap = () => { }, [activeStep]); const submission = useSelector(transactionSubmissionSelector); + const { transactionSimulation, transactionData } = submission; const networkDetails = useSelector(settingsNetworkDetailsSelector); @@ -159,6 +165,39 @@ export const Swap = () => { setAreDefaultsApplied(true); }, [dispatch, location.search, networkDetails.network]); + // The in-flight submission and its terminal success are internal states of + // the submitting screen rather than distinct steps/routes, so emit their + // `screen.viewed` here as the submission status advances. Mirrors the send + // flow's effect so both internal flows report the same stages. Each emits + // once per submission; the guards reset on IDLE and on ERROR, so a retry + // after a failure re-emits. + useEffect(() => { + if (!hasSeenIdle.current && submission.submitStatus !== ActionStatus.IDLE) { + return; + } + if (submission.submitStatus === ActionStatus.PENDING) { + if (!hasEmittedProcessing.current) { + hasEmittedProcessing.current = true; + emitScreenViewed("swap_processing", { + flow: "swap", + step: "processing", + }); + } + } else if (submission.submitStatus === ActionStatus.SUCCESS) { + if (!hasEmittedSuccess.current) { + hasEmittedSuccess.current = true; + emitScreenViewed("swap_success", { flow: "swap", step: "success" }); + } + } else if ( + submission.submitStatus === ActionStatus.IDLE || + submission.submitStatus === ActionStatus.ERROR + ) { + hasSeenIdle.current = true; + hasEmittedProcessing.current = false; + hasEmittedSuccess.current = false; + } + }, [submission.submitStatus]); + const renderStep = (step: STEPS) => { switch (step) { case STEPS.SWAP_CONFIRM: { diff --git a/extension/src/popup/views/__tests__/Send.test.tsx b/extension/src/popup/views/__tests__/Send.test.tsx index 78124d62b5..0e137972f3 100644 --- a/extension/src/popup/views/__tests__/Send.test.tsx +++ b/extension/src/popup/views/__tests__/Send.test.tsx @@ -1,11 +1,18 @@ import React from "react"; -import { render, waitFor, fireEvent, screen } from "@testing-library/react"; +import { + render, + waitFor, + fireEvent, + screen, + act, +} from "@testing-library/react"; import { Wrapper, mockBalances, mockTestnetBalances, mockAccounts, + getTestStore, } from "../../__testHelpers__"; import * as ApiInternal from "@shared/api/internal"; import * as UseNetworkFees from "popup/helpers/useNetworkFees"; @@ -22,14 +29,16 @@ import { import { APPLICATION_STATE as ApplicationState } from "@shared/constants/applicationState"; import { ROUTES } from "popup/constants/routes"; import { Send } from "popup/views/Send"; -import { initialState as transactionSubmissionInitialState } from "popup/ducks/transactionSubmission"; +import { + initialState as transactionSubmissionInitialState, + submitFreighterTransaction, +} from "popup/ducks/transactionSubmission"; import * as AccountServices from "popup/ducks/accountServices"; import * as CheckSuspiciousAsset from "popup/helpers/checkForSuspiciousAsset"; import * as RouteHelpers from "popup/helpers/route"; import * as tokenPaymentActions from "popup/ducks/token-payment"; import * as GetIconHelper from "@shared/api/helpers/getIconUrlFromIssuer"; import { WalletType } from "@shared/constants/hardwareWallet"; -import { ActionStatus } from "@shared/api/types"; import { emitScreenViewed } from "helpers/metrics"; jest.mock("lodash/debounce", () => jest.fn((fn) => fn)); @@ -244,7 +253,6 @@ describe("Send", () => { transactionSubmission: { ...transactionSubmissionInitialState, accountBalances: mockBalances, - submitStatus: ActionStatus.PENDING, }, tokenPaymentSimulation: tokenPaymentActions.initialState, }} @@ -253,6 +261,15 @@ describe("Send", () => { , ); + // Drive a real transition. A status seeded at mount is the stale-store + // case the emit now ignores on purpose. + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + act(() => { + getTestStore()!.dispatch({ + type: submitFreighterTransaction.pending.type, + } as never); + }); + await waitFor(() => { expect(emitScreenViewedMock).toHaveBeenCalledWith( "send_payment_processing", @@ -289,7 +306,6 @@ describe("Send", () => { transactionSubmission: { ...transactionSubmissionInitialState, accountBalances: mockBalances, - submitStatus: ActionStatus.SUCCESS, }, tokenPaymentSimulation: tokenPaymentActions.initialState, }} @@ -298,11 +314,25 @@ describe("Send", () => { , ); + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + act(() => { + const store = getTestStore()!; + store.dispatch({ + type: submitFreighterTransaction.pending.type, + } as never); + store.dispatch({ + type: submitFreighterTransaction.fulfilled.type, + } as never); + }); + await waitFor(() => { - expect(emitScreenViewedMock).toHaveBeenCalledWith("send_payment_success", { - flow: "send", - step: "success", - }); + expect(emitScreenViewedMock).toHaveBeenCalledWith( + "send_payment_success", + { + flow: "send", + step: "success", + }, + ); }); // Emitted exactly once for the successful submission, never as a duplicate. @@ -312,6 +342,64 @@ describe("Send", () => { expect(successCalls).toHaveLength(1); }); + it("emits send_payment_processing again when the user retries after a failure", async () => { + // The guards reset on ERROR as well as IDLE. A retry goes ERROR -> + // PENDING without passing through IDLE, because returning from the failure + // screen does not reset the submission. Guarding on IDLE alone dropped + // every retried attempt. + // + // This suite does not clear the emit mock between tests, and an earlier + // test already emits this screen name, so start from a clean count. + emitScreenViewedMock.mockClear(); + + render( + + + , + ); + + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + + const store = getTestStore()!; + const dispatchStatus = (type: string, payload?: unknown) => + act(() => { + store.dispatch({ type, payload } as never); + }); + + dispatchStatus(submitFreighterTransaction.pending.type); + dispatchStatus(submitFreighterTransaction.rejected.type, { + errorMessage: "op_underfunded", + }); + dispatchStatus(submitFreighterTransaction.pending.type); + + await waitFor(() => { + const processingCalls = emitScreenViewedMock.mock.calls.filter( + (c) => c[0] === "send_payment_processing", + ); + expect(processingCalls).toHaveLength(2); + }); + }); + it("starts on the token picker step when no asset is pre-selected", async () => { render( ({ + ...jest.requireActual("helpers/metrics"), + emitMetric: jest.fn(), + emitScreenViewed: jest.fn(), +})); + +const emitScreenViewedMock = emitScreenViewed as jest.Mock; + +const nativeBalance = { + token: { type: "native", code: "XLM" }, + total: new BigNumber("100"), + available: new BigNumber("100"), + blockaidData: {}, +}; + +const swapData = { + type: AppDataType.RESOLVED, + applicationState: "MNEMONIC_PHRASE_CONFIRMED", + networkDetails: { network: "TESTNET" }, + icons: {}, + userBalances: { balances: [nativeBalance] }, + tokenPrices: {}, +}; + +const resolvedFromState = { + state: RequestState.SUCCESS, + data: { + type: AppDataType.RESOLVED, + publicKey: "G123", + balances: { balances: [], icons: {} }, + filteredBalances: [], + networkDetails: { network: "PUBLIC", networkUrl: "" }, + applicationState: "MNEMONIC_PHRASE_CONFIRMED", + tokenPrices: {}, + }, + error: null, +}; + +const emptyLookupResult = { + sections: { yourTokens: [], popular: [], verified: [], unverified: [] }, + isSearch: false, + hadSorobanMatches: false, + isFallback: false, +}; + +const renderSwap = () => + render( + + + , + ); + +/** Drives a real submission-status transition through the root reducer. */ +const dispatchStatus = (type: string, payload?: unknown) => + act(() => { + getTestStore()!.dispatch({ type, payload } as never); + }); + +const callsFor = (screenName: string) => + emitScreenViewedMock.mock.calls.filter((c) => c[0] === screenName); + +describe("Swap flow stage telemetry", () => { + beforeEach(() => { + jest.spyOn(UseNetworkFees, "useNetworkFees").mockReturnValue({ + networkCongestion: "LOW", + recommendedFee: "0.00001", + } as any); + jest.spyOn(UseSimulateSwapData, "useSimulateTxData").mockReturnValue({ + state: { + state: RequestState.SUCCESS, + data: { transactionXdr: "AAAA", scanResult: null }, + error: null, + }, + isQuoteExpired: false, + fetchData: jest.fn().mockResolvedValue(undefined), + } as any); + jest.spyOn(UseGetSwapAmountData, "useGetSwapAmountData").mockReturnValue({ + state: { state: RequestState.SUCCESS, data: swapData, error: null }, + fetchData: jest.fn().mockResolvedValue(undefined), + } as any); + jest + .spyOn(XlmReserve, "shouldShowXlmReservePreflight") + .mockReturnValue(false); + jest.spyOn(UseSwapFromData, "useGetSwapFromData").mockReturnValue({ + state: resolvedFromState, + fetchData: jest.fn().mockResolvedValue(undefined), + filterBalances: jest.fn(), + } as any); + jest.spyOn(UseSwapTokenLookup, "useSwapTokenLookup").mockReturnValue({ + fetchData: jest.fn().mockResolvedValue(undefined), + state: { + state: RequestState.SUCCESS, + data: emptyLookupResult, + error: null, + }, + } as any); + }); + + afterEach(() => { + jest.restoreAllMocks(); + emitScreenViewedMock.mockClear(); + }); + + it("emits swap_processing once while a submission is in flight", async () => { + // The swap flow had no processing stage at all, so a swap could not be + // followed past the review screen. + renderSwap(); + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + + dispatchStatus(submitFreighterTransaction.pending.type); + + await waitFor(() => { + expect(emitScreenViewedMock).toHaveBeenCalledWith("swap_processing", { + flow: "swap", + step: "processing", + }); + }); + expect(callsFor("swap_processing")).toHaveLength(1); + }); + + it("emits swap_success once when a submission succeeds", async () => { + renderSwap(); + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + + dispatchStatus(submitFreighterTransaction.pending.type); + dispatchStatus(submitFreighterTransaction.fulfilled.type); + + await waitFor(() => { + expect(emitScreenViewedMock).toHaveBeenCalledWith("swap_success", { + flow: "swap", + step: "success", + }); + }); + expect(callsFor("swap_success")).toHaveLength(1); + }); + + it("emits swap_processing again when the user retries after a failure", async () => { + // The guards reset on ERROR as well as IDLE. A retry goes ERROR -> + // PENDING without passing through IDLE, so guarding on IDLE alone would + // drop the retried attempt. + renderSwap(); + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + + dispatchStatus(submitFreighterTransaction.pending.type); + dispatchStatus(submitFreighterTransaction.rejected.type, { + errorMessage: "op_underfunded", + }); + dispatchStatus(submitFreighterTransaction.pending.type); + + await waitFor(() => { + expect(callsFor("swap_processing")).toHaveLength(2); + }); + }); + + it("ignores a terminal status left in the store by an earlier submission", async () => { + // The submission status outlives the view, so a mount that finds a stale + // terminal status would report a stage the user never reached. + getTestStore()?.dispatch({ + type: submitFreighterTransaction.fulfilled.type, + } as never); + + renderSwap(); + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + + expect(callsFor("swap_success")).toHaveLength(0); + expect(callsFor("swap_processing")).toHaveLength(0); + }); + + it("does not emit the submitting screen as the confirm stage", async () => { + // `confirm` belongs to the review modal, which the user sees before + // deciding. The submitting screen is reached only after approval. + renderSwap(); + await waitFor(() => expect(emitScreenViewedMock).toHaveBeenCalled()); + + dispatchStatus(submitFreighterTransaction.pending.type); + + await waitFor(() => { + expect(callsFor("swap_processing")).toHaveLength(1); + }); + expect(callsFor("swap_confirm")).toHaveLength(0); + }); +});