From 8211f0ede19fdfe31b0ac1efaf5b9529c689376a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 16:33:19 +0000 Subject: [PATCH 1/8] feat(analytics): emit signing metrics on the hardware wallet branch Hardware signers produced no signing.* event. `useSetupSigningFlow` diverts them to the HardwareSign overlay and never dispatches the sign thunk. The handlers in popup/metrics/access.ts key on that thunk, so every hardware approval was lost. Rejections still fired, because `rejectAndClose` dispatches the reject thunk for both key types. The funnel therefore showed hardware rejections with no approvals. This change adds the missing events for all three signing views: transaction, message, and auth entry. Add popup/metrics/signing.ts. This module owns the signing event schema. It selects the event name, adds the constant `message_type`, derives `origin`, and scrubs `reason_code`. Both key types emit through it, so the two paths cannot drift apart. Move the software-key handlers in access.ts onto the same module. The emitted payloads do not change. Emit from the HardwareSign overlay: - Success fires after `handleSignedHwPayload` resolves. The software event fires when the background resolves the dApp request, and that call is the equivalent point. A device signature that never reaches the dApp is not an approval. - Failure fires on a rejected sign thunk and on any throw. This covers no device attached, the mismatched-account refusal, and a payload delivery failure. - Internal send, swap, and trustline flows emit nothing. They report their outcome as payment.completed, swap.completed, or asset.added. Thread the dApp url from the three views into the overlay so hardware events carry the same `origin` as software events. A transaction runtime failure emits nothing, on both key types. The shared catalog has no such event, and access.ts registers no `signTransaction.rejected` handler. FAILED_EVENT records the gap explicitly. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- .../__tests__/HardwareSign.test.tsx | 155 +++++++++++++++++- .../hardwareConnect/HardwareSign/index.tsx | 50 ++++++ .../src/popup/helpers/useSetupSigningFlow.ts | 14 +- .../popup/metrics/__tests__/signing.test.ts | 151 +++++++++++++++++ extension/src/popup/metrics/access.ts | 65 +++----- extension/src/popup/metrics/signing.ts | 109 ++++++++++++ .../src/popup/views/SignAuthEntry/index.tsx | 1 + .../src/popup/views/SignMessage/index.tsx | 1 + .../src/popup/views/SignTransaction/index.tsx | 2 +- 9 files changed, 499 insertions(+), 49 deletions(-) create mode 100644 extension/src/popup/metrics/__tests__/signing.test.ts create mode 100644 extension/src/popup/metrics/signing.ts 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..b6101cf9ad 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx @@ -42,10 +42,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 +211,111 @@ 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", + 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", + 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", + reason_code: "No device selected", + 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", + 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", + }); + }); + }); + + it("emits no signing event for an internal flow", async () => { + // Internal send/swap/trustline steps report their outcome as + // payment.completed / swap.completed / asset.added instead. + mockGetWalletPublicKey.mockResolvedValue(TEST_PUBLIC_KEY); + + renderOverlay({ isInternal: true, uuid: undefined, url: undefined }); + + await waitFor(() => { + expect(mockHardwareSignMessage).toHaveBeenCalled(); + }); + expect(mockEmitMetric).not.toHaveBeenCalled(); + }); +}); diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx index 1d3e523f95..15e7f348f2 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -28,6 +28,11 @@ import { parseWalletError, MISMATCHED_HARDWARE_ACCOUNT_ERROR, } from "popup/helpers/hardwareConnect"; +import { + emitSigningApproved, + emitSigningFailed, + SigningKind, +} from "popup/metrics/signing"; import LedgerSigning from "popup/assets/ledger-signing.png"; import Ledger from "popup/assets/ledger.png"; @@ -41,6 +46,7 @@ export const HardwareSign = ({ isInternal = false, onCancel, uuid, + url, }: { walletType: ConfigurableWalletType; isSignSorobanAuthorization?: boolean; @@ -49,6 +55,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 +80,25 @@ 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. Only the dApp prompts have a signing.* event to + // emit: internal flows report their outcome as payment.completed / + // swap.completed / asset.added from useSubmitTxData instead. 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 signingKind: SigningKind = isSignMessage + ? "message" + : isSignSorobanAuthorization + ? "authEntry" + : "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 => + e instanceof Error ? e.message : JSON.stringify(e); + const closeOverlay = () => { if (hardwareConnectRef.current) { hardwareConnectRef.current.style.bottom = `-${POPUP_HEIGHT}px`; @@ -148,6 +179,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, url); + } } closeOverlay(); if (onSubmit) { @@ -155,6 +195,9 @@ export const HardwareSign = ({ } } else { setHardwareConnectSuccessful(false); + if (isDappSigningRequest) { + emitSigningFailed(signingKind, res.payload?.errorMessage, url); + } setConnectError( parseWalletError[walletType](res.payload?.errorMessage || ""), ); @@ -162,6 +205,13 @@ export const HardwareSign = ({ setHardwareWalletIsSigning(false); } catch (e) { setHardwareWalletIsSigning(false); + // Covers every throw in the block above: no device attached, the + // mismatched-account refusal, and a handleSignedHwPayload failure after a + // good signature. All three are runtime failures on the software path + // too, so they land on the same `*_failed` event. + if (isDappSigningRequest) { + emitSigningFailed(signingKind, errorMessage(e), url); + } setConnectError(parseWalletError[walletType](e)); } setIsDetecting(false); 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..956c91f150 --- /dev/null +++ b/extension/src/popup/metrics/__tests__/signing.test.ts @@ -0,0 +1,151 @@ +import { emitMetric } from "helpers/metrics"; +import { METRIC_NAMES } from "popup/constants/metricsNames"; + +import { + emitSigningApproved, + emitSigningFailed, + emitSigningRejected, + originProps, +} from "../signing"; + +jest.mock("helpers/metrics", () => ({ + emitMetric: jest.fn(), +})); + +const mockEmitMetric = emitMetric as jest.MockedFunction; + +const DAPP_URL = "https://example.com/app?foo=bar"; + +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("emits the transaction event with origin only", () => { + emitSigningApproved("transaction", DAPP_URL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingTransactionApproved, + { origin: "example.com" }, + ); + }); + + it("emits the message event with the blob message_type", () => { + emitSigningApproved("message", DAPP_URL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingMessageApproved, + { message_type: "blob", origin: "example.com" }, + ); + }); + + it("emits the auth entry event with origin only", () => { + emitSigningApproved("authEntry", DAPP_URL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingAuthEntryApproved, + { origin: "example.com" }, + ); + }); + + it("omits origin when no url is threaded through", () => { + emitSigningApproved("transaction"); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingTransactionApproved, + {}, + ); + }); +}); + +describe("emitSigningRejected", () => { + beforeEach(() => jest.clearAllMocks()); + + it.each([ + ["transaction", METRIC_NAMES.signingTransactionRejected, {}], + ["message", METRIC_NAMES.signingMessageRejected, { message_type: "blob" }], + ["authEntry", METRIC_NAMES.signingAuthEntryRejected, {}], + ] as const)("emits the %s rejection", (kind, name, extraProps) => { + emitSigningRejected(kind, DAPP_URL); + + // A rejection is a user decision, so it never carries a reason_code. + expect(mockEmitMetric).toHaveBeenCalledWith(name, { + ...extraProps, + origin: "example.com", + }); + }); +}); + +describe("emitSigningFailed", () => { + beforeEach(() => jest.clearAllMocks()); + + it("emits the message failure with a scrubbed reason_code", () => { + emitSigningFailed("message", "Device error", DAPP_URL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingMessageFailed, + { + message_type: "blob", + reason_code: "Device error", + origin: "example.com", + }, + ); + }); + + it("emits the auth entry failure with a scrubbed reason_code", () => { + emitSigningFailed("authEntry", "Device error", DAPP_URL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingAuthEntryFailed, + { reason_code: "Device error", origin: "example.com" }, + ); + }); + + it("scrubs Stellar StrKeys out of the reason_code", () => { + // Amplitude is a third-party sink not covered by Sentry's beforeSend, and a + // hardware signing error can echo the account it tried to sign as. + emitSigningFailed( + "message", + "cannot sign as GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + ); + + 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("message", undefined, DAPP_URL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingMessageFailed, + { + message_type: "blob", + reason_code: "unknown", + origin: "example.com", + }, + ); + }); + + it("emits nothing for a transaction failure", () => { + // The shared catalog has no transaction runtime-failure event, so the + // software path emits nothing here. The hardware path must match it. + emitSigningFailed("transaction", "Device error", DAPP_URL); + + expect(mockEmitMetric).not.toHaveBeenCalled(); + }); +}); diff --git a/extension/src/popup/metrics/access.ts b/extension/src/popup/metrics/access.ts index 15ab40a9c7..edb992ce15 100644 --- a/extension/src/popup/metrics/access.ts +++ b/extension/src/popup/metrics/access.ts @@ -13,30 +13,28 @@ 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, +} 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. @@ -63,51 +61,40 @@ registerHandler(rejectToken.fulfilled, (_state, action) => { ...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("transaction", argUrl(action)); }); registerHandler(rejectTransaction.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingTransactionRejected, originProps(action)); + emitSigningRejected("transaction", argUrl(action)); }); registerHandler(signBlob.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingMessageApproved, { - message_type: "blob", - ...originProps(action), - }); + emitSigningApproved("message", argUrl(action)); }); registerHandler(rejectBlob.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingMessageRejected, { - message_type: "blob", - ...originProps(action), - }); + emitSigningRejected("message", argUrl(action)); }); registerHandler(signEntry.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingAuthEntryApproved, originProps(action)); + emitSigningApproved("authEntry", argUrl(action)); }); registerHandler(rejectAuthEntry.fulfilled, (_state, action) => { - emitMetric(METRIC_NAMES.signingAuthEntryRejected, originProps(action)); + emitSigningRejected("authEntry", 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("message", rejectedError(action), argUrl(action)); }); registerHandler(signEntry.rejected, (_state, action) => { - emitMetric(METRIC_NAMES.signingAuthEntryFailed, { - reason_code: rejectedReasonCode(action), - ...originProps(action), - }); + emitSigningFailed("authEntry", rejectedError(action), argUrl(action)); }); diff --git a/extension/src/popup/metrics/signing.ts b/extension/src/popup/metrics/signing.ts new file mode 100644 index 0000000000..f2a1d1e212 --- /dev/null +++ b/extension/src/popup/metrics/signing.ts @@ -0,0 +1,109 @@ +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. + * + * Software keys sign inside the `popup/ducks/access` thunks, so their events + * are emitted by the redux handlers in `popup/metrics/access.ts`. Hardware keys + * never reach those thunks — `useSetupSigningFlow` diverts them to the + * HardwareSign overlay — so that component emits its own events. Both paths + * emit through this module so one signing request produces the same event name + * and the same property set whichever key type signed it. + */ +export type SigningKind = "transaction" | "message" | "authEntry"; + +/** + * 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> = { + transaction: {}, + message: { message_type: "blob" }, + authEntry: {}, +}; + +const APPROVED_EVENT: Record = { + transaction: METRIC_NAMES.signingTransactionApproved, + message: METRIC_NAMES.signingMessageApproved, + authEntry: METRIC_NAMES.signingAuthEntryApproved, +}; + +const REJECTED_EVENT: Record = { + transaction: METRIC_NAMES.signingTransactionRejected, + message: METRIC_NAMES.signingMessageRejected, + authEntry: METRIC_NAMES.signingAuthEntryRejected, +}; + +/** + * Runtime-failure events, kept distinct from the user-cancel `*_rejected` + * events above. + * + * `transaction` is deliberately `null`. The shared cross-platform catalog has + * no transaction runtime-failure member — METRIC_NAMES stops at + * `signingTransactionBlocked`, and `access.ts` registers no + * `signTransaction.rejected` handler — so a software transaction failure emits + * nothing today. The hardware path must match that exactly, so it emits + * nothing too. The key is spelled out rather than omitted to make the gap + * explicit: adding `signing.transaction_failed` is a catalog change that has to + * land on both key types at once. + */ +const FAILED_EVENT: Record = { + transaction: null, + message: METRIC_NAMES.signingMessageFailed, + authEntry: METRIC_NAMES.signingAuthEntryFailed, +}; + +/** The user approved the request and signing completed. */ +export const emitSigningApproved = (kind: SigningKind, url?: string): void => { + emitMetric(APPROVED_EVENT[kind], { + ...KIND_PROPS[kind], + ...originProps(url), + }); +}; + +/** + * The user cancelled the request. A user decision, never a runtime error — see + * emitSigningFailed for that. A rejection carries no `reason_code`. + */ +export const emitSigningRejected = (kind: SigningKind, url?: string): void => { + emitMetric(REJECTED_EVENT[kind], { + ...KIND_PROPS[kind], + ...originProps(url), + }); +}; + +/** + * Signing threw. `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. + * No-ops for `transaction` (see FAILED_EVENT). + */ +export const emitSigningFailed = ( + kind: SigningKind, + error?: string, + url?: string, +): void => { + const event = FAILED_EVENT[kind]; + if (!event) { + return; + } + + emitMetric(event, { + ...KIND_PROPS[kind], + reason_code: scrubStrKeys(error) || "unknown", + ...originProps(url), + }); +}; 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..8b34681a1a 100644 --- a/extension/src/popup/views/SignMessage/index.tsx +++ b/extension/src/popup/views/SignMessage/index.tsx @@ -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 ? ( From 8563afd5639f1caa363e38bd745a05cd567beea8 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 22:40:25 +0000 Subject: [PATCH 2/8] fix(analytics): report a device decline as a rejection, not a failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hardware decline is a user decision. The overlay reported every hardware error as a signing failure, so a decline landed on `signing.*_failed` next to real faults. Add `isDeviceRefusalError` beside `parseWalletError`, which already matches the same two messages. hw-app-str raises StellarUserRefusedError("User refused the request") for the deny status word on every sign call. Older apps and transports worded the same decision differently, so match both. Route a decline to `signing.*_rejected`, the event that already carries a popup reject. A rejection carries no `reason_code`: nothing went wrong. Every other hardware error stays on `signing.*_failed` — no device attached, a transport fault, the wrong device, an app too old. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- .../__tests__/HardwareSign.test.tsx | 41 +++++++++++++++++++ .../hardwareConnect/HardwareSign/index.tsx | 37 ++++++++++++----- .../helpers/__tests__/hardwareConnect.test.ts | 36 ++++++++++++++++ .../src/popup/helpers/hardwareConnect.ts | 25 +++++++++++ extension/src/popup/metrics/signing.ts | 16 +++++--- 5 files changed, 140 insertions(+), 15 deletions(-) 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 b6101cf9ad..68d7b9f9b4 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/__tests__/HardwareSign.test.tsx @@ -278,6 +278,47 @@ describe("HardwareSign message signing telemetry", () => { }); }); + 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", + 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", + origin: "example.com", + }); + }); + }); + it("emits signing.message_failed when the device derives a different account", async () => { mockGetWalletPublicKey.mockResolvedValue(OTHER_PUBLIC_KEY); diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx index 15e7f348f2..18dd9fbc83 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -26,11 +26,13 @@ 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, } from "popup/metrics/signing"; import LedgerSigning from "popup/assets/ledger-signing.png"; @@ -99,6 +101,25 @@ export const HardwareSign = ({ const errorMessage = (e: unknown): string => e instanceof Error ? e.message : 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 (!isDappSigningRequest) { + return; + } + if (isDeviceRefusalError(e)) { + emitSigningRejected(signingKind, url); + return; + } + emitSigningFailed(signingKind, errorMessage(e), url); + }; + const closeOverlay = () => { if (hardwareConnectRef.current) { hardwareConnectRef.current.style.bottom = `-${POPUP_HEIGHT}px`; @@ -195,9 +216,7 @@ export const HardwareSign = ({ } } else { setHardwareConnectSuccessful(false); - if (isDappSigningRequest) { - emitSigningFailed(signingKind, res.payload?.errorMessage, url); - } + emitSigningError(res.payload?.errorMessage); setConnectError( parseWalletError[walletType](res.payload?.errorMessage || ""), ); @@ -205,13 +224,11 @@ export const HardwareSign = ({ setHardwareWalletIsSigning(false); } catch (e) { setHardwareWalletIsSigning(false); - // Covers every throw in the block above: no device attached, the - // mismatched-account refusal, and a handleSignedHwPayload failure after a - // good signature. All three are runtime failures on the software path - // too, so they land on the same `*_failed` event. - if (isDappSigningRequest) { - emitSigningFailed(signingKind, errorMessage(e), url); - } + // 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/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/metrics/signing.ts b/extension/src/popup/metrics/signing.ts index f2a1d1e212..72a48e09e7 100644 --- a/extension/src/popup/metrics/signing.ts +++ b/extension/src/popup/metrics/signing.ts @@ -75,8 +75,9 @@ export const emitSigningApproved = (kind: SigningKind, url?: string): void => { }; /** - * The user cancelled the request. A user decision, never a runtime error — see - * emitSigningFailed for that. A rejection carries no `reason_code`. + * 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, url?: string): void => { emitMetric(REJECTED_EVENT[kind], { @@ -86,9 +87,14 @@ export const emitSigningRejected = (kind: SigningKind, url?: string): void => { }; /** - * Signing threw. `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. + * 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. * No-ops for `transaction` (see FAILED_EVENT). */ export const emitSigningFailed = ( From 2e8ca500273d04b98fd22b00dffdcddde2859200 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 22:50:45 +0000 Subject: [PATCH 3/8] feat(analytics): give each flow stage one meaning and add swap parity The `confirm` stage marked the submitting screen, which the user only reaches after approving. Mobile marks the review sheet, before the user decides. The two clients therefore counted different things. Move `confirm` to the review modal in the send and swap amount screens. Emit it from an effect on the modal's open state, so every entry point counts once and a reopen counts again. Drop the submitting screen from both step maps. Its stages are already reported by the submission-status effect (processing, then success), so a third event would double-count and reuse a name that now belongs to the review modal. Add the processing and success stages to the swap flow. Swap reported neither, so a swap could not be followed past the review screen. Reset the stage guards 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. Bump the schema version to 4. The `confirm` stage changes meaning, so consumers must tell an old client from a new one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- extension/src/helpers/metrics.test.ts | 10 +- extension/src/helpers/metrics.ts | 9 +- .../components/send/SendAmount/index.tsx | 15 +- .../__tests__/SwapAmount.telemetry.test.tsx | 4 + .../components/swap/SwapAmount/index.tsx | 10 +- extension/src/popup/views/Send/index.tsx | 21 +- extension/src/popup/views/Swap/index.tsx | 44 +++- .../src/popup/views/__tests__/Send.test.tsx | 83 ++++++- .../views/__tests__/Swap.telemetry.test.tsx | 211 ++++++++++++++++++ 9 files changed, 375 insertions(+), 32 deletions(-) create mode 100644 extension/src/popup/views/__tests__/Swap.telemetry.test.tsx 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/send/SendAmount/index.tsx b/extension/src/popup/components/send/SendAmount/index.tsx index e60cce40d0..b9a8c6e338 100644 --- a/extension/src/popup/components/send/SendAmount/index.tsx +++ b/extension/src/popup/components/send/SendAmount/index.tsx @@ -17,7 +17,7 @@ import { isMuxedAccount, } from "helpers/stellar"; import { NetworkCongestion } from "popup/helpers/useNetworkFees"; -import { emitMetric } from "helpers/metrics"; +import { emitMetric, emitScreenViewed } from "helpers/metrics"; import { trackSendFeeBreakdownOpened } from "popup/metrics/send"; import { getAssetDecimals, @@ -188,6 +188,19 @@ export const SendAmount = ({ const [isEditingSettings, setIsEditingSettings] = React.useState(false); const [isShowingFeesPane, setIsShowingFeesPane] = React.useState(false); const [isReviewingTx, setIsReviewingTx] = React.useState(false); + + // The review modal is the `confirm` stage: the user can see the transaction + // and has not decided yet. Emitted from an effect rather than the four + // handlers that open it, so every entry point counts once. Reopening the + // modal is a new view and emits again, matching mobile's review sheet. + useEffect(() => { + if (isReviewingTx) { + emitScreenViewed("send_payment_confirm", { + flow: "send", + step: "confirm", + }); + } + }, [isReviewingTx]); const [contractSupportsMuxed, setContractSupportsMuxed] = React.useState< boolean | null >(null); 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..08dcce54ec 100644 --- a/extension/src/popup/components/swap/SwapAmount/index.tsx +++ b/extension/src/popup/components/swap/SwapAmount/index.tsx @@ -41,7 +41,7 @@ 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 { InputType } from "helpers/transaction"; import { METRIC_NAMES } from "popup/constants/metricsNames"; import { XLM_RESERVE_HELP_URL } from "popup/constants/externalLinks"; @@ -187,6 +187,14 @@ export const SwapAmount = ({ const [isEditingSlippage, setIsEditingSlippage] = useState(false); const [isEditingSettings, setIsEditingSettings] = useState(false); const [isReviewingTx, setIsReviewingTx] = React.useState(false); + + // The review modal is the `confirm` stage — see the equivalent effect in + // SendAmount. + useEffect(() => { + if (isReviewingTx) { + emitScreenViewed("swap_confirm", { flow: "swap", step: "confirm" }); + } + }, [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, diff --git a/extension/src/popup/views/Send/index.tsx b/extension/src/popup/views/Send/index.tsx index 222e8d82fe..100df2765a 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" }, }; @@ -245,7 +243,14 @@ 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. hasEmittedProcessing.current = false; hasEmittedSuccess.current = false; } diff --git a/extension/src/popup/views/Swap/index.tsx b/extension/src/popup/views/Swap/index.tsx index 672d4d5a8d..48e96306cc 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,8 @@ export const Swap = () => { const location = useLocation(); const [activeStep, setActiveStep] = useState(STEPS.AMOUNT); const lastEmittedStep = useRef(null); + const hasEmittedProcessing = useRef(false); + const hasEmittedSuccess = useRef(false); // Emit a screen-view metric only once per step transition. useEffect(() => { @@ -75,6 +75,36 @@ export const Swap = () => { }, [activeStep]); const submission = useSelector(transactionSubmissionSelector); + + // 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 (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 + ) { + hasEmittedProcessing.current = false; + hasEmittedSuccess.current = false; + } + }, [submission.submitStatus]); + const { transactionSimulation, transactionData } = submission; const networkDetails = useSelector(settingsNetworkDetailsSelector); diff --git a/extension/src/popup/views/__tests__/Send.test.tsx b/extension/src/popup/views/__tests__/Send.test.tsx index 78124d62b5..e6d3c347fe 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,7 +29,10 @@ 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"; @@ -299,10 +309,13 @@ describe("Send", () => { ); 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 +325,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("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); + }); +}); From 77aa63b97daf25811ba21f83d760f1a8b142e542 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 22:59:27 +0000 Subject: [PATCH 4/8] feat(analytics): report signing for internal transactions Internal transactions reported nothing for the signing action. Only a dApp request did, so approvals and signing outcomes were unmeasurable for the transactions the wallet composes itself. Add `signing.transaction_failed`. The message and auth-entry families each carry approved, rejected and failed. The transaction family had no failure event, so a signing fault went unreported on every path. Register it for the dApp thunk as well. Add a `source` property to every signing event: `dapp_api` for a website request, `internal` for a wallet-composed one. Both origins now emit the same events with the same properties, so one query counts all signing and `source` splits it. An internal transaction has no origin, so it omits that property. Emit from every place internal signing ends: - the submission hook, for software keys; - the hardware overlay, for a device. A device signs before the submission hook runs, and a decline keeps the user on the overlay, so the flow never reaches the hook. This is the case that reported nothing at all; - the review screen's cancel, which is the internal rejection; - the trustline flow, which runs its own signing step. Split the ownership by key type so an internal hardware signing is reported once: the overlay owns the device attempt, the submission hook owns the software attempt. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- .../useSubmitTxData.telemetry.test.tsx | 73 ++++++++++ .../hooks/useSubmitTxData.tsx | 23 ++++ .../__tests__/HardwareSign.test.tsx | 126 +++++++++++++++++- .../hardwareConnect/HardwareSign/index.tsx | 31 +++-- .../hooks/useChangeTrust.tsx | 9 ++ .../components/send/SendAmount/index.tsx | 9 +- .../components/swap/SwapAmount/index.tsx | 9 +- .../constants/__tests__/metricsNames.test.ts | 15 ++- extension/src/popup/constants/metricsNames.ts | 8 +- .../popup/metrics/__tests__/signing.test.ts | 117 ++++++++-------- extension/src/popup/metrics/access.ts | 36 +++-- extension/src/popup/metrics/signing.ts | 74 +++++----- 12 files changed, 414 insertions(+), 116 deletions(-) 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..9cb893bda0 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,7 @@ 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 } from "popup/metrics/signing"; import { useSubmitTxData } from "../useSubmitTxData"; // The emit site is the unit under test — emitMetric itself is mocked so no @@ -30,6 +31,15 @@ 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", () => ({ + 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 +215,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 +556,67 @@ 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("transaction", { + source: "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( + "transaction", + expect.anything(), + { source: "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..7e31b9d8c1 100644 --- a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx +++ b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx @@ -17,6 +17,7 @@ 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 } from "popup/metrics/signing"; import { getAssetFromCanonical, getCanonicalFromAsset, @@ -208,6 +209,20 @@ 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("transaction", signingError?.errorMessage, { + source: "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 +270,14 @@ 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("transaction", { source: "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 68d7b9f9b4..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), + }, }; }); @@ -236,6 +246,7 @@ describe("HardwareSign message signing telemetry", () => { await waitFor(() => { expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_approved", { message_type: "blob", + source: "dapp_api", origin: "example.com", }); }); @@ -254,6 +265,7 @@ describe("HardwareSign message signing telemetry", () => { await waitFor(() => { expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_failed", { message_type: "blob", + source: "dapp_api", reason_code: "Request expired", origin: "example.com", }); @@ -272,6 +284,7 @@ describe("HardwareSign message signing telemetry", () => { await waitFor(() => { expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_failed", { message_type: "blob", + source: "dapp_api", reason_code: "No device selected", origin: "example.com", }); @@ -293,6 +306,7 @@ describe("HardwareSign message signing telemetry", () => { await waitFor(() => { expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_rejected", { message_type: "blob", + source: "dapp_api", origin: "example.com", }); }); @@ -314,6 +328,7 @@ describe("HardwareSign message signing telemetry", () => { await waitFor(() => { expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_rejected", { message_type: "blob", + source: "dapp_api", origin: "example.com", }); }); @@ -329,6 +344,7 @@ describe("HardwareSign message signing telemetry", () => { "signing.message_failed", expect.objectContaining({ message_type: "blob", + source: "dapp_api", origin: "example.com", }), ); @@ -343,13 +359,15 @@ describe("HardwareSign message signing telemetry", () => { await waitFor(() => { expect(mockEmitMetric).toHaveBeenCalledWith("signing.message_approved", { message_type: "blob", + source: "dapp_api", }); }); }); - it("emits no signing event for an internal flow", async () => { - // Internal send/swap/trustline steps report their outcome as - // payment.completed / swap.completed / asset.added instead. + 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 }); @@ -360,3 +378,103 @@ describe("HardwareSign message signing telemetry", () => { 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 18dd9fbc83..842d0a4df1 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -34,6 +34,7 @@ import { emitSigningFailed, emitSigningRejected, SigningKind, + SigningSource, } from "popup/metrics/signing"; import LedgerSigning from "popup/assets/ledger-signing.png"; import Ledger from "popup/assets/ledger.png"; @@ -83,12 +84,18 @@ export const HardwareSign = ({ const [connectError, setConnectError] = useState(""); // The overlay serves both the dApp signing prompts and the internal - // send/swap/trustline flows. Only the dApp prompts have a signing.* event to - // emit: internal flows report their outcome as payment.completed / - // swap.completed / asset.added from useSubmitTxData instead. A dApp request - // is the one that carries a `uuid` (the pending-request id) and is not - // rendered inline as an internal step. + // 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 + ? "dapp_api" + : "internal"; + // An internal flow has no dApp, so it carries no origin. + const signingProps = { + source: signingSource, + ...(isDappSigningRequest ? { url } : {}), + }; const signingKind: SigningKind = isSignMessage ? "message" : isSignSorobanAuthorization @@ -110,14 +117,11 @@ export const HardwareSign = ({ * runtime failure and keeps its scrubbed reason. */ const emitSigningError = (e: unknown): void => { - if (!isDappSigningRequest) { - return; - } if (isDeviceRefusalError(e)) { - emitSigningRejected(signingKind, url); + emitSigningRejected(signingKind, signingProps); return; } - emitSigningFailed(signingKind, errorMessage(e), url); + emitSigningFailed(signingKind, errorMessage(e), signingProps); }; const closeOverlay = () => { @@ -180,6 +184,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, @@ -207,7 +216,7 @@ export const HardwareSign = ({ // handleSignedHwPayload just did. Emitting when the device returned a // signature would count an approval that never reached the dApp. if (isDappSigningRequest) { - emitSigningApproved(signingKind, url); + emitSigningApproved(signingKind, signingProps); } } closeOverlay(); 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..b6b43e1e9b 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,7 @@ import { useTranslation } from "react-i18next"; import { initialState, reducer } from "helpers/request"; import { AppDispatch } from "popup/App"; +import { emitSigningApproved, emitSigningFailed } from "popup/metrics/signing"; import { signFreighterTransaction, submitFreighterTransaction, @@ -97,10 +98,18 @@ 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("transaction", res.payload?.errorMessage, { + source: "internal", + }); throw new Error(t("failed to sign transaction")); } if (signFreighterTransaction.fulfilled.match(res)) { + emitSigningApproved("transaction", { source: "internal" }); + const submitResp = await reduxDispatch( submitFreighterTransaction({ publicKey, diff --git a/extension/src/popup/components/send/SendAmount/index.tsx b/extension/src/popup/components/send/SendAmount/index.tsx index b9a8c6e338..cb4e9c2dda 100644 --- a/extension/src/popup/components/send/SendAmount/index.tsx +++ b/extension/src/popup/components/send/SendAmount/index.tsx @@ -18,6 +18,7 @@ import { } from "helpers/stellar"; import { NetworkCongestion } from "popup/helpers/useNetworkFees"; import { emitMetric, emitScreenViewed } from "helpers/metrics"; +import { emitSigningRejected } from "popup/metrics/signing"; import { trackSendFeeBreakdownOpened } from "popup/metrics/send"; import { getAssetDecimals, @@ -974,7 +975,13 @@ export const SendAmount = ({ assetIcon={assetIcon} fee={fee} networkDetails={data.networkDetails} - onCancel={() => setIsReviewingTx(false)} + onCancel={() => { + // Backing out of the review is the internal equivalent of + // pressing reject on a dApp prompt, so it reports the same + // event. A rejection carries no reason_code. + emitSigningRejected("transaction", { source: "internal" }); + setIsReviewingTx(false); + }} onConfirm={goToNext} onAddMemo={() => { setIsReviewingTx(false); diff --git a/extension/src/popup/components/swap/SwapAmount/index.tsx b/extension/src/popup/components/swap/SwapAmount/index.tsx index 08dcce54ec..c19c58858b 100644 --- a/extension/src/popup/components/swap/SwapAmount/index.tsx +++ b/extension/src/popup/components/swap/SwapAmount/index.tsx @@ -42,6 +42,7 @@ import { getBalanceCanonicalKey } from "popup/helpers/balance"; import { useBlockaidOverrideState } from "popup/helpers/blockaid"; import { AppDispatch } from "popup/App"; import { emitMetric, emitScreenViewed } from "helpers/metrics"; +import { emitSigningRejected } 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"; @@ -865,7 +866,13 @@ export const SwapAmount = ({ assetIcon={assetIcon} fee={fee} networkDetails={networkDetails} - onCancel={() => setIsReviewingTx(false)} + onCancel={() => { + // Backing out of the review is the internal equivalent of + // pressing reject on a dApp prompt, so it reports the same + // event. A rejection carries no reason_code. + emitSigningRejected("transaction", { source: "internal" }); + setIsReviewingTx(false); + }} // The trustline-added + swap-success metrics fire post-confirmation // (in useSubmitTxData), once the swap actually settles — not here at // review time. 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/metrics/__tests__/signing.test.ts b/extension/src/popup/metrics/__tests__/signing.test.ts index 956c91f150..52e0fa0e75 100644 --- a/extension/src/popup/metrics/__tests__/signing.test.ts +++ b/extension/src/popup/metrics/__tests__/signing.test.ts @@ -15,6 +15,8 @@ jest.mock("helpers/metrics", () => ({ const mockEmitMetric = emitMetric as jest.MockedFunction; const DAPP_URL = "https://example.com/app?foo=bar"; +const DAPP = { source: "dapp_api" as const, url: DAPP_URL }; +const INTERNAL = { source: "internal" as const }; describe("originProps", () => { beforeEach(() => jest.clearAllMocks()); @@ -36,39 +38,31 @@ describe("originProps", () => { describe("emitSigningApproved", () => { beforeEach(() => jest.clearAllMocks()); - it("emits the transaction event with origin only", () => { - emitSigningApproved("transaction", DAPP_URL); - - expect(mockEmitMetric).toHaveBeenCalledWith( - METRIC_NAMES.signingTransactionApproved, - { origin: "example.com" }, - ); - }); - - it("emits the message event with the blob message_type", () => { - emitSigningApproved("message", DAPP_URL); - - expect(mockEmitMetric).toHaveBeenCalledWith( - METRIC_NAMES.signingMessageApproved, - { message_type: "blob", origin: "example.com" }, - ); - }); - - it("emits the auth entry event with origin only", () => { - emitSigningApproved("authEntry", DAPP_URL); - - expect(mockEmitMetric).toHaveBeenCalledWith( - METRIC_NAMES.signingAuthEntryApproved, - { origin: "example.com" }, - ); - }); + it.each([ + ["transaction", METRIC_NAMES.signingTransactionApproved, {}], + ["message", METRIC_NAMES.signingMessageApproved, { message_type: "blob" }], + ["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: "dapp_api", + origin: "example.com", + }); + }, + ); - it("omits origin when no url is threaded through", () => { - emitSigningApproved("transaction"); + 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("transaction", INTERNAL); expect(mockEmitMetric).toHaveBeenCalledWith( METRIC_NAMES.signingTransactionApproved, - {}, + { source: "internal" }, ); }); }); @@ -80,48 +74,64 @@ describe("emitSigningRejected", () => { ["transaction", METRIC_NAMES.signingTransactionRejected, {}], ["message", METRIC_NAMES.signingMessageRejected, { message_type: "blob" }], ["authEntry", METRIC_NAMES.signingAuthEntryRejected, {}], - ] as const)("emits the %s rejection", (kind, name, extraProps) => { - emitSigningRejected(kind, DAPP_URL); + ] 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, { - ...extraProps, + ...extra, + source: "dapp_api", origin: "example.com", }); }); + + it("emits an internal rejection with no origin", () => { + emitSigningRejected("transaction", INTERNAL); + + expect(mockEmitMetric).toHaveBeenCalledWith( + METRIC_NAMES.signingTransactionRejected, + { source: "internal" }, + ); + }); }); describe("emitSigningFailed", () => { beforeEach(() => jest.clearAllMocks()); - it("emits the message failure with a scrubbed reason_code", () => { - emitSigningFailed("message", "Device error", DAPP_URL); - - expect(mockEmitMetric).toHaveBeenCalledWith( - METRIC_NAMES.signingMessageFailed, - { - message_type: "blob", + it.each([ + ["transaction", METRIC_NAMES.signingTransactionFailed, {}], + ["message", METRIC_NAMES.signingMessageFailed, { message_type: "blob" }], + ["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: "dapp_api", reason_code: "Device error", origin: "example.com", - }, - ); - }); + }); + }, + ); - it("emits the auth entry failure with a scrubbed reason_code", () => { - emitSigningFailed("authEntry", "Device error", DAPP_URL); + it("emits an internal failure with no origin", () => { + emitSigningFailed("transaction", "op_underfunded", INTERNAL); expect(mockEmitMetric).toHaveBeenCalledWith( - METRIC_NAMES.signingAuthEntryFailed, - { reason_code: "Device error", origin: "example.com" }, + METRIC_NAMES.signingTransactionFailed, + { source: "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 - // hardware signing error can echo the account it tried to sign as. + // 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( "message", "cannot sign as GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + DAPP, ); const [, props] = mockEmitMetric.mock.calls[0]; @@ -129,23 +139,16 @@ describe("emitSigningFailed", () => { }); it("falls back to unknown when there is no message", () => { - emitSigningFailed("message", undefined, DAPP_URL); + emitSigningFailed("message", undefined, DAPP); expect(mockEmitMetric).toHaveBeenCalledWith( METRIC_NAMES.signingMessageFailed, { message_type: "blob", + source: "dapp_api", reason_code: "unknown", origin: "example.com", }, ); }); - - it("emits nothing for a transaction failure", () => { - // The shared catalog has no transaction runtime-failure event, so the - // software path emits nothing here. The hardware path must match it. - emitSigningFailed("transaction", "Device error", DAPP_URL); - - expect(mockEmitMetric).not.toHaveBeenCalled(); - }); }); diff --git a/extension/src/popup/metrics/access.ts b/extension/src/popup/metrics/access.ts index edb992ce15..ff1c260919 100644 --- a/extension/src/popup/metrics/access.ts +++ b/extension/src/popup/metrics/access.ts @@ -65,22 +65,28 @@ registerHandler(rejectToken.fulfilled, (_state, action) => { // 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) => { - emitSigningApproved("transaction", argUrl(action)); + emitSigningApproved("transaction", { + source: "dapp_api", + url: argUrl(action), + }); }); registerHandler(rejectTransaction.fulfilled, (_state, action) => { - emitSigningRejected("transaction", argUrl(action)); + emitSigningRejected("transaction", { + source: "dapp_api", + url: argUrl(action), + }); }); registerHandler(signBlob.fulfilled, (_state, action) => { - emitSigningApproved("message", argUrl(action)); + emitSigningApproved("message", { source: "dapp_api", url: argUrl(action) }); }); registerHandler(rejectBlob.fulfilled, (_state, action) => { - emitSigningRejected("message", argUrl(action)); + emitSigningRejected("message", { source: "dapp_api", url: argUrl(action) }); }); registerHandler(signEntry.fulfilled, (_state, action) => { - emitSigningApproved("authEntry", argUrl(action)); + emitSigningApproved("authEntry", { source: "dapp_api", url: argUrl(action) }); }); registerHandler(rejectAuthEntry.fulfilled, (_state, action) => { - emitSigningRejected("authEntry", argUrl(action)); + emitSigningRejected("authEntry", { source: "dapp_api", url: argUrl(action) }); }); // Runtime signing FAILURE paths — distinct from the user-cancel @@ -93,8 +99,22 @@ const rejectedError = (action: { }): string | undefined => action.error?.message || action.payload?.errorMessage; registerHandler(signBlob.rejected, (_state, action) => { - emitSigningFailed("message", rejectedError(action), argUrl(action)); + emitSigningFailed("message", rejectedError(action), { + source: "dapp_api", + url: argUrl(action), + }); }); registerHandler(signEntry.rejected, (_state, action) => { - emitSigningFailed("authEntry", rejectedError(action), argUrl(action)); + emitSigningFailed("authEntry", rejectedError(action), { + source: "dapp_api", + 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("transaction", rejectedError(action), { + source: "dapp_api", + url: argUrl(action), + }); }); diff --git a/extension/src/popup/metrics/signing.ts b/extension/src/popup/metrics/signing.ts index 72a48e09e7..38c31f6e82 100644 --- a/extension/src/popup/metrics/signing.ts +++ b/extension/src/popup/metrics/signing.ts @@ -6,15 +6,33 @@ import { getUrlHostname } from "helpers/urls"; /** * The three signing requests a dApp can make of the extension. * - * Software keys sign inside the `popup/ducks/access` thunks, so their events - * are emitted by the redux handlers in `popup/metrics/access.ts`. Hardware keys - * never reach those thunks — `useSetupSigningFlow` diverts them to the - * HardwareSign overlay — so that component emits its own events. Both paths - * emit through this module so one signing request produces the same event name - * and the same property set whichever key type signed it. + * 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 type SigningKind = "transaction" | "message" | "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 type SigningSource = "dapp_api" | "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. @@ -48,28 +66,24 @@ const REJECTED_EVENT: Record = { }; /** - * Runtime-failure events, kept distinct from the user-cancel `*_rejected` - * events above. - * - * `transaction` is deliberately `null`. The shared cross-platform catalog has - * no transaction runtime-failure member — METRIC_NAMES stops at - * `signingTransactionBlocked`, and `access.ts` registers no - * `signTransaction.rejected` handler — so a software transaction failure emits - * nothing today. The hardware path must match that exactly, so it emits - * nothing too. The key is spelled out rather than omitted to make the gap - * explicit: adding `signing.transaction_failed` is a catalog change that has to - * land on both key types at once. + * 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 = { - transaction: null, +const FAILED_EVENT: Record = { + transaction: METRIC_NAMES.signingTransactionFailed, message: METRIC_NAMES.signingMessageFailed, authEntry: METRIC_NAMES.signingAuthEntryFailed, }; -/** The user approved the request and signing completed. */ -export const emitSigningApproved = (kind: SigningKind, url?: string): void => { +/** 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), }); }; @@ -79,9 +93,13 @@ export const emitSigningApproved = (kind: SigningKind, url?: string): void => { * 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, url?: string): void => { +export const emitSigningRejected = ( + kind: SigningKind, + { source, url }: SigningEventOptions, +): void => { emitMetric(REJECTED_EVENT[kind], { ...KIND_PROPS[kind], + source, ...originProps(url), }); }; @@ -99,16 +117,12 @@ export const emitSigningRejected = (kind: SigningKind, url?: string): void => { */ export const emitSigningFailed = ( kind: SigningKind, - error?: string, - url?: string, + error: string | undefined, + { source, url }: SigningEventOptions, ): void => { - const event = FAILED_EVENT[kind]; - if (!event) { - return; - } - - emitMetric(event, { + emitMetric(FAILED_EVENT[kind], { ...KIND_PROPS[kind], + source, reason_code: scrubStrKeys(error) || "unknown", ...originProps(url), }); From ff396199d53cc186140c0c5f68868041f400438f Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 03:05:51 +0000 Subject: [PATCH 5/8] fix(analytics): make signing outcomes reachable and correctly paired The signing wrappers discarded every failure. The background answers with an error object rather than throwing, and the wrappers ignored both that answer and any transport exception. A failed signing therefore resolved like a success: the approval event fired, and the failure event was unreachable. Surface both kinds of failure, matching the pattern the token-add wrapper already uses. This applies to the hardware payload handover as well. That path reported an approval even when the request never reached the website. Report a message rejection when the user declines a message prompt. The message view dispatched the transaction reject request, so declining a message reported a transaction rejection and omitted the message type. Report a rejection when the user cancels a trustline review. A dedicated handler keeps the success and close paths out of the count, because they reuse the same cancel callback. Correct the failure helper's description, which still described the transaction outcome as disabled. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- @shared/api/__tests__/internal.test.ts | 50 ++++++++++++++++ @shared/api/internal.ts | 60 +++++++++++++++++-- .../ChangeTrustInternal/index.tsx | 17 +++++- extension/src/popup/metrics/signing.ts | 1 - .../src/popup/views/SignMessage/index.tsx | 4 +- 5 files changed, 123 insertions(+), 9 deletions(-) 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..0a06da7ecd 100644 --- a/@shared/api/internal.ts +++ b/@shared/api/internal.ts @@ -1556,15 +1556,28 @@ 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( + typeof res.error === "string" ? res.error : JSON.stringify(res.error), + ); + } } catch (e) { console.error(e); + throw e; } }; @@ -1597,13 +1610,26 @@ 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( + typeof res.error === "string" ? res.error : JSON.stringify(res.error), + ); + } } catch (e) { console.error(e); + throw e; } }; @@ -1617,14 +1643,27 @@ 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( + typeof res.error === "string" ? res.error : JSON.stringify(res.error), + ); + } } catch (e) { console.error(e); + throw e; } }; @@ -1636,13 +1675,26 @@ 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( + typeof res.error === "string" ? res.error : JSON.stringify(res.error), + ); + } } catch (e) { console.error(e); + throw e; } }; diff --git a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx index 2fd735bbf8..c12844036d 100644 --- a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx +++ b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx @@ -26,6 +26,8 @@ import { useBlockaidOverrideState, getAssetSecurityLevel, } from "popup/helpers/blockaid"; +import { emitSigningRejected } from "popup/metrics/signing"; + import { useGetChangeTrustData } from "./hooks/useChangeTrustData"; import { Fee } from "./Settings/Fee"; import { Timeout } from "./Settings/Timeout"; @@ -373,6 +375,17 @@ export const ChangeTrustInternal = ({ ); + /** + * Reports a rejection, then leaves the review. + * + * Wired only to the review's Cancel buttons. `onCancel` also serves as the + * success and close fallback further down, and those are not rejections. + */ + const onCancelReview = () => { + emitSigningRejected("transaction", { source: "internal" }); + onCancel(); + }; + const renderBlockaidWarningButtons = () => ( <> @@ -415,7 +428,7 @@ export const ChangeTrustInternal = ({ isRounded size="lg" variant="tertiary" - onClick={onCancel} + onClick={onCancelReview} > {t("Cancel")} diff --git a/extension/src/popup/metrics/signing.ts b/extension/src/popup/metrics/signing.ts index 38c31f6e82..ebc283e57b 100644 --- a/extension/src/popup/metrics/signing.ts +++ b/extension/src/popup/metrics/signing.ts @@ -113,7 +113,6 @@ export const 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. - * No-ops for `transaction` (see FAILED_EVENT). */ export const emitSigningFailed = ( kind: SigningKind, diff --git a/extension/src/popup/views/SignMessage/index.tsx b/extension/src/popup/views/SignMessage/index.tsx index 8b34681a1a..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, From 1bd9d8e0c954764a006fc32937f689bed1f543ec Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 20:12:53 +0000 Subject: [PATCH 6/8] fix(analytics): cover every way a user leaves a review, and read real errors Report a rejection when the user leaves a review by any route. The handlers covered the Cancel buttons only, so dismissing through the modal backdrop reported a review stage with no outcome. Each flow now keys the rejection on the review closing, and marks an approval first so it is not counted. Leaving the send review to edit the memo is not a decision, so it is not counted either. Report a trustline rejection on the same basis. That review lives in a modal whose backdrop no button handler sees. Ignore a submission status left behind by an earlier submission. The status lives in the store, so a view that mounted and found a terminal status reported a stage the user never reached. Both flows now wait until the status has been seen idle, which the reset on mount guarantees. The swap stage effect moves below that reset, matching the send flow. Read a real message out of a background error. The error is an object on some paths, which stringified to "{}" and reached the reason code with no information. Keep a string error unquoted. The hardware overlay passes the message directly on one branch, and stringifying it wrapped the reason code in quotes. Expect a signing failure in the integration helper. It drives the signing wrappers with a placeholder request id, so they now reject, and an unhandled rejection would stop the remaining checks. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- @shared/api/internal.ts | 43 ++++++++---- .../hardwareConnect/HardwareSign/index.tsx | 10 ++- .../ChangeTrustInternal/index.tsx | 35 ++++++---- .../components/send/SendAmount/index.tsx | 41 +++++++++--- .../components/swap/SwapAmount/index.tsx | 34 +++++++--- extension/src/popup/views/IntegrationTest.tsx | 45 +++++++++---- extension/src/popup/views/Send/index.tsx | 9 +++ extension/src/popup/views/Swap/index.tsx | 67 +++++++++++-------- .../src/popup/views/__tests__/Send.test.tsx | 23 ++++++- .../views/__tests__/Swap.telemetry.test.tsx | 14 ++++ 10 files changed, 227 insertions(+), 94 deletions(-) diff --git a/@shared/api/internal.ts b/@shared/api/internal.ts index 0a06da7ecd..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, @@ -1571,9 +1598,7 @@ export const handleSignedHwPayload = async ({ // 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( - typeof res.error === "string" ? res.error : JSON.stringify(res.error), - ); + throw new Error(backgroundErrorMessage(res.error)); } } catch (e) { console.error(e); @@ -1623,9 +1648,7 @@ export const signTransaction = async ({ // 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( - typeof res.error === "string" ? res.error : JSON.stringify(res.error), - ); + throw new Error(backgroundErrorMessage(res.error)); } } catch (e) { console.error(e); @@ -1657,9 +1680,7 @@ export const signBlob = async ({ // 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( - typeof res.error === "string" ? res.error : JSON.stringify(res.error), - ); + throw new Error(backgroundErrorMessage(res.error)); } } catch (e) { console.error(e); @@ -1688,9 +1709,7 @@ export const signAuthEntry = async ({ // 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( - typeof res.error === "string" ? res.error : JSON.stringify(res.error), - ); + throw new Error(backgroundErrorMessage(res.error)); } } catch (e) { console.error(e); diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx index 842d0a4df1..2a37231628 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -105,8 +105,14 @@ export const HardwareSign = ({ // 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 => - e instanceof Error ? e.message : JSON.stringify(e); + 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; + } + return e instanceof Error ? e.message : JSON.stringify(e); + }; /** * Reports a hardware signing error as either a rejection or a failure. diff --git a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx index c12844036d..c3ae8debce 100644 --- a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx +++ b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx @@ -98,6 +98,21 @@ 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("transaction", { source: "internal" }); + } + }, + [], + ); + const { t } = useTranslation(); // Check override state (takes precedence, dev mode only) @@ -375,17 +390,6 @@ export const ChangeTrustInternal = ({ ); - /** - * Reports a rejection, then leaves the review. - * - * Wired only to the review's Cancel buttons. `onCancel` also serves as the - * success and close fallback further down, and those are not rejections. - */ - const onCancelReview = () => { - emitSigningRejected("transaction", { source: "internal" }); - onCancel(); - }; - const renderBlockaidWarningButtons = () => ( <> @@ -428,7 +432,7 @@ export const ChangeTrustInternal = ({ isRounded size="lg" variant="tertiary" - onClick={onCancelReview} + onClick={onCancel} > {t("Cancel")} @@ -437,7 +441,10 @@ export const ChangeTrustInternal = ({ isFullWidth isRounded size="lg" - onClick={() => setActiveBodyContent(ActiveBodyContent.submitTx)} + onClick={() => { + hasApprovedRef.current = true; + setActiveBodyContent(ActiveBodyContent.submitTx); + }} > {t("Confirm")} diff --git a/extension/src/popup/components/send/SendAmount/index.tsx b/extension/src/popup/components/send/SendAmount/index.tsx index cb4e9c2dda..ae9c1eb0f6 100644 --- a/extension/src/popup/components/send/SendAmount/index.tsx +++ b/extension/src/popup/components/send/SendAmount/index.tsx @@ -189,18 +189,38 @@ 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); - // The review modal is the `confirm` stage: the user can see the transaction - // and has not decided yet. Emitted from an effect rather than the four - // handlers that open it, so every entry point counts once. Reopening the - // modal is a new view and emits again, matching mobile's review sheet. + /** + * 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("transaction", { source: "internal" }); }, [isReviewingTx]); const [contractSupportsMuxed, setContractSupportsMuxed] = React.useState< boolean | null @@ -975,15 +995,14 @@ export const SendAmount = ({ assetIcon={assetIcon} fee={fee} networkDetails={data.networkDetails} - onCancel={() => { - // Backing out of the review is the internal equivalent of - // pressing reject on a dApp prompt, so it reports the same - // event. A rejection carries no reason_code. - emitSigningRejected("transaction", { source: "internal" }); - setIsReviewingTx(false); + onCancel={() => setIsReviewingTx(false)} + onConfirm={() => { + skipRejectionRef.current = true; + goToNext(); }} - onConfirm={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/index.tsx b/extension/src/popup/components/swap/SwapAmount/index.tsx index c19c58858b..43345f9791 100644 --- a/extension/src/popup/components/swap/SwapAmount/index.tsx +++ b/extension/src/popup/components/swap/SwapAmount/index.tsx @@ -189,12 +189,29 @@ export const SwapAmount = ({ const [isEditingSettings, setIsEditingSettings] = useState(false); const [isReviewingTx, setIsReviewingTx] = React.useState(false); - // The review modal is the `confirm` stage — see the equivalent effect in - // SendAmount. + // 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("transaction", { source: "internal" }); }, [isReviewingTx]); const [isXlmReserveOpen, setIsXlmReserveOpen] = useState(false); // Tracks focus on the sell input so the "Enter an amount" CTA can disable @@ -866,17 +883,14 @@ export const SwapAmount = ({ assetIcon={assetIcon} fee={fee} networkDetails={networkDetails} - onCancel={() => { - // Backing out of the review is the internal equivalent of - // pressing reject on a dApp prompt, so it reports the same - // event. A rejection carries no reason_code. - emitSigningRejected("transaction", { source: "internal" }); - setIsReviewingTx(false); - }} + onCancel={() => setIsReviewingTx(false)} // 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/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 100df2765a..4c989ee638 100644 --- a/extension/src/popup/views/Send/index.tsx +++ b/extension/src/popup/views/Send/index.tsx @@ -189,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, @@ -227,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; @@ -251,6 +259,7 @@ export const Send = () => { // 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/Swap/index.tsx b/extension/src/popup/views/Swap/index.tsx index 48e96306cc..3d3d61cc74 100644 --- a/extension/src/popup/views/Swap/index.tsx +++ b/extension/src/popup/views/Swap/index.tsx @@ -61,6 +61,11 @@ export const Swap = () => { 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(() => { @@ -76,35 +81,6 @@ export const Swap = () => { const submission = useSelector(transactionSubmissionSelector); - // 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 (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 - ) { - hasEmittedProcessing.current = false; - hasEmittedSuccess.current = false; - } - }, [submission.submitStatus]); - const { transactionSimulation, transactionData } = submission; const networkDetails = useSelector(settingsNetworkDetailsSelector); @@ -189,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 e6d3c347fe..0e137972f3 100644 --- a/extension/src/popup/views/__tests__/Send.test.tsx +++ b/extension/src/popup/views/__tests__/Send.test.tsx @@ -39,7 +39,6 @@ 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)); @@ -254,7 +253,6 @@ describe("Send", () => { transactionSubmission: { ...transactionSubmissionInitialState, accountBalances: mockBalances, - submitStatus: ActionStatus.PENDING, }, tokenPaymentSimulation: tokenPaymentActions.initialState, }} @@ -263,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", @@ -299,7 +306,6 @@ describe("Send", () => { transactionSubmission: { ...transactionSubmissionInitialState, accountBalances: mockBalances, - submitStatus: ActionStatus.SUCCESS, }, tokenPaymentSimulation: tokenPaymentActions.initialState, }} @@ -308,6 +314,17 @@ 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", diff --git a/extension/src/popup/views/__tests__/Swap.telemetry.test.tsx b/extension/src/popup/views/__tests__/Swap.telemetry.test.tsx index c0e4f1bf37..fbf1c8adfe 100644 --- a/extension/src/popup/views/__tests__/Swap.telemetry.test.tsx +++ b/extension/src/popup/views/__tests__/Swap.telemetry.test.tsx @@ -195,6 +195,20 @@ describe("Swap flow stage telemetry", () => { }); }); + 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. From 751cd0e61cd3193268836b884e2e388389852606 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 21:31:20 +0000 Subject: [PATCH 7/8] fix(analytics): latch every trustline approval, and model the signing sets as enums Route every approval of the trustline review through one handler. The path behind the security warning skipped the approval mark, so a transaction approved that way reported an approval and then a rejection. Model the signing kind and the signing origin as enums. The repository requires a finite named set of string values to be an enum, not a union type. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- .../useSubmitTxData.telemetry.test.tsx | 19 ++++-- .../hooks/useSubmitTxData.tsx | 21 +++++-- .../hardwareConnect/HardwareSign/index.tsx | 10 ++-- .../hooks/useChangeTrust.tsx | 15 +++-- .../ChangeTrustInternal/index.tsx | 29 ++++++--- .../components/send/SendAmount/index.tsx | 10 +++- .../components/swap/SwapAmount/index.tsx | 10 +++- .../popup/metrics/__tests__/signing.test.ts | 60 ++++++++++++------- extension/src/popup/metrics/access.ts | 46 +++++++++----- extension/src/popup/metrics/signing.ts | 35 ++++++----- 10 files changed, 171 insertions(+), 84 deletions(-) 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 9cb893bda0..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,7 +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 } from "popup/metrics/signing"; +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 @@ -36,6 +41,7 @@ jest.mock("helpers/metrics", () => ({ // 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(), })); @@ -573,9 +579,10 @@ describe("useSubmitTxData terminal-event telemetry", () => { await result.current.fetchData({ isSwap: false }); }); - expect(emitSigningApproved).toHaveBeenCalledWith("transaction", { - source: "internal", - }); + expect(emitSigningApproved).toHaveBeenCalledWith( + SigningKind.Transaction, + { source: SigningSource.Internal }, + ); expect(emitSigningFailed).not.toHaveBeenCalled(); }); @@ -590,9 +597,9 @@ describe("useSubmitTxData terminal-event telemetry", () => { }); expect(emitSigningFailed).toHaveBeenCalledWith( - "transaction", + SigningKind.Transaction, expect.anything(), - { source: "internal" }, + { source: SigningSource.Internal }, ); expect(emitSigningApproved).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 7e31b9d8c1..102afe4284 100644 --- a/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx +++ b/extension/src/popup/components/InternalTransaction/SubmitTransaction/hooks/useSubmitTxData.tsx @@ -17,7 +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 } from "popup/metrics/signing"; +import { + emitSigningApproved, + emitSigningFailed, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { getAssetFromCanonical, getCanonicalFromAsset, @@ -218,9 +223,13 @@ function useSubmitTxData({ // HardwareSign overlay, which reports that attempt itself; this hook // only receives the result, so emitting here would double-count. if (!isHardwareWallet) { - emitSigningFailed("transaction", signingError?.errorMessage, { - source: "internal", - }); + emitSigningFailed( + SigningKind.Transaction, + signingError?.errorMessage, + { + source: SigningSource.Internal, + }, + ); } // Pre-submission failure: signing rejected, or a hardware flow arrived @@ -275,7 +284,9 @@ function useSubmitTxData({ // Software keys only — the overlay owns the hardware attempt (see the // unsigned branch above). if (!isHardwareWallet) { - emitSigningApproved("transaction", { source: "internal" }); + emitSigningApproved(SigningKind.Transaction, { + source: SigningSource.Internal, + }); } // Everything the volume telemetry needs is snapshotted here — after diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx index 2a37231628..2d084f0ef8 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -89,18 +89,18 @@ export const HardwareSign = ({ // id) and is not rendered inline as an internal step. const isDappSigningRequest = !isInternal && !!uuid; const signingSource: SigningSource = isDappSigningRequest - ? "dapp_api" - : "internal"; + ? 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 - ? "message" + ? SigningKind.Message : isSignSorobanAuthorization - ? "authEntry" - : "transaction"; + ? SigningKind.AuthEntry + : SigningKind.Transaction; // Mirrors the software path's error extraction (`action.error.message`). // Scrubbing and the "unknown" fallback belong to emitSigningFailed, so both 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 b6b43e1e9b..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,7 +4,12 @@ import { useTranslation } from "react-i18next"; import { initialState, reducer } from "helpers/request"; import { AppDispatch } from "popup/App"; -import { emitSigningApproved, emitSigningFailed } from "popup/metrics/signing"; +import { + emitSigningApproved, + emitSigningFailed, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { signFreighterTransaction, submitFreighterTransaction, @@ -101,14 +106,16 @@ function useGetChangeTrust() { // 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("transaction", res.payload?.errorMessage, { - source: "internal", + emitSigningFailed(SigningKind.Transaction, res.payload?.errorMessage, { + source: SigningSource.Internal, }); throw new Error(t("failed to sign transaction")); } if (signFreighterTransaction.fulfilled.match(res)) { - emitSigningApproved("transaction", { source: "internal" }); + emitSigningApproved(SigningKind.Transaction, { + source: SigningSource.Internal, + }); const submitResp = await reduxDispatch( submitFreighterTransaction({ diff --git a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx index c3ae8debce..c1d6d51199 100644 --- a/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx +++ b/extension/src/popup/components/manageAssets/ManageAssetRows/ChangeTrustInternal/index.tsx @@ -26,7 +26,11 @@ import { useBlockaidOverrideState, getAssetSecurityLevel, } from "popup/helpers/blockaid"; -import { emitSigningRejected } from "popup/metrics/signing"; +import { + emitSigningRejected, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { useGetChangeTrustData } from "./hooks/useChangeTrustData"; import { Fee } from "./Settings/Fee"; @@ -107,12 +111,26 @@ export const ChangeTrustInternal = ({ useEffect( () => () => { if (!hasApprovedRef.current) { - emitSigningRejected("transaction", { source: "internal" }); + 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) @@ -408,7 +426,7 @@ export const ChangeTrustInternal = ({ }`} onClick={(e) => { e.preventDefault(); - setActiveBodyContent(ActiveBodyContent.submitTx); + onApproveReview(); }} > {t("Confirm anyway")} @@ -441,10 +459,7 @@ export const ChangeTrustInternal = ({ isFullWidth isRounded size="lg" - onClick={() => { - hasApprovedRef.current = true; - 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 ae9c1eb0f6..974477efee 100644 --- a/extension/src/popup/components/send/SendAmount/index.tsx +++ b/extension/src/popup/components/send/SendAmount/index.tsx @@ -18,7 +18,11 @@ import { } from "helpers/stellar"; import { NetworkCongestion } from "popup/helpers/useNetworkFees"; import { emitMetric, emitScreenViewed } from "helpers/metrics"; -import { emitSigningRejected } from "popup/metrics/signing"; +import { + emitSigningRejected, + SigningKind, + SigningSource, +} from "popup/metrics/signing"; import { trackSendFeeBreakdownOpened } from "popup/metrics/send"; import { getAssetDecimals, @@ -220,7 +224,9 @@ export const SendAmount = ({ if (skipRejectionRef.current) { return; } - emitSigningRejected("transaction", { source: "internal" }); + emitSigningRejected(SigningKind.Transaction, { + source: SigningSource.Internal, + }); }, [isReviewingTx]); const [contractSupportsMuxed, setContractSupportsMuxed] = React.useState< boolean | null diff --git a/extension/src/popup/components/swap/SwapAmount/index.tsx b/extension/src/popup/components/swap/SwapAmount/index.tsx index 43345f9791..42a64afd24 100644 --- a/extension/src/popup/components/swap/SwapAmount/index.tsx +++ b/extension/src/popup/components/swap/SwapAmount/index.tsx @@ -42,7 +42,11 @@ import { getBalanceCanonicalKey } from "popup/helpers/balance"; import { useBlockaidOverrideState } from "popup/helpers/blockaid"; import { AppDispatch } from "popup/App"; import { emitMetric, emitScreenViewed } from "helpers/metrics"; -import { emitSigningRejected } from "popup/metrics/signing"; +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"; @@ -211,7 +215,9 @@ export const SwapAmount = ({ if (skipRejectionRef.current) { return; } - emitSigningRejected("transaction", { source: "internal" }); + 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 diff --git a/extension/src/popup/metrics/__tests__/signing.test.ts b/extension/src/popup/metrics/__tests__/signing.test.ts index 52e0fa0e75..84f9950122 100644 --- a/extension/src/popup/metrics/__tests__/signing.test.ts +++ b/extension/src/popup/metrics/__tests__/signing.test.ts @@ -6,6 +6,8 @@ import { emitSigningFailed, emitSigningRejected, originProps, + SigningKind, + SigningSource, } from "../signing"; jest.mock("helpers/metrics", () => ({ @@ -15,8 +17,8 @@ jest.mock("helpers/metrics", () => ({ const mockEmitMetric = emitMetric as jest.MockedFunction; const DAPP_URL = "https://example.com/app?foo=bar"; -const DAPP = { source: "dapp_api" as const, url: DAPP_URL }; -const INTERNAL = { source: "internal" as const }; +const DAPP = { source: SigningSource.DappApi, url: DAPP_URL }; +const INTERNAL = { source: SigningSource.Internal }; describe("originProps", () => { beforeEach(() => jest.clearAllMocks()); @@ -39,9 +41,13 @@ describe("emitSigningApproved", () => { beforeEach(() => jest.clearAllMocks()); it.each([ - ["transaction", METRIC_NAMES.signingTransactionApproved, {}], - ["message", METRIC_NAMES.signingMessageApproved, { message_type: "blob" }], - ["authEntry", METRIC_NAMES.signingAuthEntryApproved, {}], + [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) => { @@ -49,7 +55,7 @@ describe("emitSigningApproved", () => { expect(mockEmitMetric).toHaveBeenCalledWith(name, { ...extra, - source: "dapp_api", + source: SigningSource.DappApi, origin: "example.com", }); }, @@ -58,11 +64,11 @@ describe("emitSigningApproved", () => { 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("transaction", INTERNAL); + emitSigningApproved(SigningKind.Transaction, INTERNAL); expect(mockEmitMetric).toHaveBeenCalledWith( METRIC_NAMES.signingTransactionApproved, - { source: "internal" }, + { source: SigningSource.Internal }, ); }); }); @@ -71,26 +77,30 @@ describe("emitSigningRejected", () => { beforeEach(() => jest.clearAllMocks()); it.each([ - ["transaction", METRIC_NAMES.signingTransactionRejected, {}], - ["message", METRIC_NAMES.signingMessageRejected, { message_type: "blob" }], - ["authEntry", METRIC_NAMES.signingAuthEntryRejected, {}], + [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: "dapp_api", + source: SigningSource.DappApi, origin: "example.com", }); }); it("emits an internal rejection with no origin", () => { - emitSigningRejected("transaction", INTERNAL); + emitSigningRejected(SigningKind.Transaction, INTERNAL); expect(mockEmitMetric).toHaveBeenCalledWith( METRIC_NAMES.signingTransactionRejected, - { source: "internal" }, + { source: SigningSource.Internal }, ); }); }); @@ -99,9 +109,13 @@ describe("emitSigningFailed", () => { beforeEach(() => jest.clearAllMocks()); it.each([ - ["transaction", METRIC_NAMES.signingTransactionFailed, {}], - ["message", METRIC_NAMES.signingMessageFailed, { message_type: "blob" }], - ["authEntry", METRIC_NAMES.signingAuthEntryFailed, {}], + [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) => { @@ -109,7 +123,7 @@ describe("emitSigningFailed", () => { expect(mockEmitMetric).toHaveBeenCalledWith(name, { ...extra, - source: "dapp_api", + source: SigningSource.DappApi, reason_code: "Device error", origin: "example.com", }); @@ -117,11 +131,11 @@ describe("emitSigningFailed", () => { ); it("emits an internal failure with no origin", () => { - emitSigningFailed("transaction", "op_underfunded", INTERNAL); + emitSigningFailed(SigningKind.Transaction, "op_underfunded", INTERNAL); expect(mockEmitMetric).toHaveBeenCalledWith( METRIC_NAMES.signingTransactionFailed, - { source: "internal", reason_code: "op_underfunded" }, + { source: SigningSource.Internal, reason_code: "op_underfunded" }, ); }); @@ -129,7 +143,7 @@ describe("emitSigningFailed", () => { // 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( - "message", + SigningKind.Message, "cannot sign as GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", DAPP, ); @@ -139,13 +153,13 @@ describe("emitSigningFailed", () => { }); it("falls back to unknown when there is no message", () => { - emitSigningFailed("message", undefined, DAPP); + emitSigningFailed(SigningKind.Message, undefined, DAPP); expect(mockEmitMetric).toHaveBeenCalledWith( METRIC_NAMES.signingMessageFailed, { message_type: "blob", - source: "dapp_api", + 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 ff1c260919..57d99696c8 100644 --- a/extension/src/popup/metrics/access.ts +++ b/extension/src/popup/metrics/access.ts @@ -18,6 +18,8 @@ import { emitSigningFailed, emitSigningRejected, originProps, + SigningKind, + SigningSource, } from "popup/metrics/signing"; import { AppState } from "popup/App"; @@ -50,14 +52,14 @@ 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), }); }); @@ -65,28 +67,40 @@ registerHandler(rejectToken.fulfilled, (_state, action) => { // 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) => { - emitSigningApproved("transaction", { - source: "dapp_api", + emitSigningApproved(SigningKind.Transaction, { + source: SigningSource.DappApi, url: argUrl(action), }); }); registerHandler(rejectTransaction.fulfilled, (_state, action) => { - emitSigningRejected("transaction", { - source: "dapp_api", + emitSigningRejected(SigningKind.Transaction, { + source: SigningSource.DappApi, url: argUrl(action), }); }); registerHandler(signBlob.fulfilled, (_state, action) => { - emitSigningApproved("message", { source: "dapp_api", url: argUrl(action) }); + emitSigningApproved(SigningKind.Message, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); registerHandler(rejectBlob.fulfilled, (_state, action) => { - emitSigningRejected("message", { source: "dapp_api", url: argUrl(action) }); + emitSigningRejected(SigningKind.Message, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); registerHandler(signEntry.fulfilled, (_state, action) => { - emitSigningApproved("authEntry", { source: "dapp_api", url: argUrl(action) }); + emitSigningApproved(SigningKind.AuthEntry, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); registerHandler(rejectAuthEntry.fulfilled, (_state, action) => { - emitSigningRejected("authEntry", { source: "dapp_api", url: argUrl(action) }); + emitSigningRejected(SigningKind.AuthEntry, { + source: SigningSource.DappApi, + url: argUrl(action), + }); }); // Runtime signing FAILURE paths — distinct from the user-cancel @@ -99,22 +113,22 @@ const rejectedError = (action: { }): string | undefined => action.error?.message || action.payload?.errorMessage; registerHandler(signBlob.rejected, (_state, action) => { - emitSigningFailed("message", rejectedError(action), { - source: "dapp_api", + emitSigningFailed(SigningKind.Message, rejectedError(action), { + source: SigningSource.DappApi, url: argUrl(action), }); }); registerHandler(signEntry.rejected, (_state, action) => { - emitSigningFailed("authEntry", rejectedError(action), { - source: "dapp_api", + 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("transaction", rejectedError(action), { - source: "dapp_api", + 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 index ebc283e57b..b6cf653bc7 100644 --- a/extension/src/popup/metrics/signing.ts +++ b/extension/src/popup/metrics/signing.ts @@ -11,7 +11,11 @@ import { getUrlHostname } from "helpers/urls"; * the dApp thunks in `popup/metrics/access.ts`, the HardwareSign overlay, the * internal submission hook, and the trustline flow. */ -export type SigningKind = "transaction" | "message" | "authEntry"; +export enum SigningKind { + Transaction = "transaction", + Message = "message", + AuthEntry = "authEntry", +} /** * Where a signing request came from. @@ -22,7 +26,10 @@ export type SigningKind = "transaction" | "message" | "authEntry"; * properties, so one query counts all signing and `source` splits it. The * token add and remove events already use `dapp_api` this way. */ -export type SigningSource = "dapp_api" | "internal"; +export enum SigningSource { + DappApi = "dapp_api", + Internal = "internal", +} interface SigningEventOptions { source: SigningSource; @@ -48,21 +55,21 @@ export const originProps = (url?: string): { origin?: string } => { * is the constant "blob" (mobile emits the same constant). */ const KIND_PROPS: Record> = { - transaction: {}, - message: { message_type: "blob" }, - authEntry: {}, + [SigningKind.Transaction]: {}, + [SigningKind.Message]: { message_type: "blob" }, + [SigningKind.AuthEntry]: {}, }; const APPROVED_EVENT: Record = { - transaction: METRIC_NAMES.signingTransactionApproved, - message: METRIC_NAMES.signingMessageApproved, - authEntry: METRIC_NAMES.signingAuthEntryApproved, + [SigningKind.Transaction]: METRIC_NAMES.signingTransactionApproved, + [SigningKind.Message]: METRIC_NAMES.signingMessageApproved, + [SigningKind.AuthEntry]: METRIC_NAMES.signingAuthEntryApproved, }; const REJECTED_EVENT: Record = { - transaction: METRIC_NAMES.signingTransactionRejected, - message: METRIC_NAMES.signingMessageRejected, - authEntry: METRIC_NAMES.signingAuthEntryRejected, + [SigningKind.Transaction]: METRIC_NAMES.signingTransactionRejected, + [SigningKind.Message]: METRIC_NAMES.signingMessageRejected, + [SigningKind.AuthEntry]: METRIC_NAMES.signingAuthEntryRejected, }; /** @@ -71,9 +78,9 @@ const REJECTED_EVENT: Record = { * outcome: approved, rejected, or failed. */ const FAILED_EVENT: Record = { - transaction: METRIC_NAMES.signingTransactionFailed, - message: METRIC_NAMES.signingMessageFailed, - authEntry: METRIC_NAMES.signingAuthEntryFailed, + [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. */ From 86ee12d81e684c71e0aaac04a1ad1da6f8433112 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 22:15:30 +0000 Subject: [PATCH 8/8] fix(metrics): keep the hardware error message a string JSON.stringify returns undefined for a value it cannot represent. Fall back to the empty string so the helper keeps its declared return type. emitSigningFailed reports that as "unknown". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Qkq7j6uvUMU1bBQdcuGX8Z --- .../components/hardwareConnect/HardwareSign/index.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx index 2d084f0ef8..3985211faf 100644 --- a/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx +++ b/extension/src/popup/components/hardwareConnect/HardwareSign/index.tsx @@ -111,7 +111,13 @@ export const HardwareSign = ({ // would wrap the reason code in quotes. return e; } - return e instanceof Error ? e.message : JSON.stringify(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) ?? ""; }; /**