feat(telemetry): opt-in product telemetry and crash reporting - #756
Open
WiktorStarczewski wants to merge 44 commits into
Open
feat(telemetry): opt-in product telemetry and crash reporting#756WiktorStarczewski wants to merge 44 commits into
WiktorStarczewski wants to merge 44 commits into
Conversation
The two telemetry members were the only PascalCase values in WalletMessageType. Nothing referenced the raw strings, so aligning them keeps the message protocol greppable.
One API for every flow, so no screen has to know how telemetry works. `beginFlow` emits `started` eagerly — a flow abandoned mid-way never gets a terminal event, so an unmatched `started` is what makes abandonment visible at all. The handle settles once, so a cancel handler and an unmount both firing cannot double-report. `classifyError` inspects a message but returns a closed union, so no caught text can reach the wire through it.
Instrument create, import, unlock, recover, open and return so an unmatched `started` identifies where users abandon getting started. Two instrumentation points differ from the original plan, because the points it named do not exist: - `recover` lives on the ForgotPassword / reset-wallet path, the real "regain access to an existing wallet" entry. Onboarding's `import-select-recovery-method` was rejected: it fires mid-import, so labelling it would mean cancelling an in-flight `import` and starting a new flow halfway through, corrupting the started/ended pairing that abandonment inference relies on. - `open` and `return` live in PageRouter, not App. App mounts MidenProvider, so a hook in its own body sits above the readiness context it needs; PageRouter is the first component that can read it. Beginning an `open` with no reachable completion would make every launch look like an abandonment. `return` is mobile-only by nature rather than by compromise: off mobile there is no foreground signal, and a reopened popup is a fresh mount and so already an `open`. It measures foreground until the wallet is usable again, so a resume onto an auto-locked wallet stays open across the unlock. Telemetry is kept out of useForegroundRefresh, whose job is syncing, and lives in its own useAppLifecycleTelemetry hook. Unlock reports one flow per attempt rather than per screen visit: a rejected password is retryable, so a visit-scoped flow settled as errored would make the eventual successful retry a no-op on the idempotent handle and drop the success from the funnel.
Instruments send, fund, receive_share, note_handle and activity_view. The two view flows settle on the user actually seeing what they came for — the activity list's first load, an address ready to share — and are cancelled on a leave before that, rather than inventing a terminal state out of a tap that ordinary successful use never produces. send and fund are entry-scoped: they begin where intent starts (the send form, the bridge surface, connect step included) so walking away mid-compose is visible, and end at the outcome of the submit. A failed attempt settles errored and the next tap begins a fresh flow, so a retry's outcome is never swallowed by the handle that already ended. note_handle is scoped per claim attempt instead, since browsing pending notes is not handling one. Nothing user-bearing reaches telemetry: no amounts, addresses, note ids or counts — errors are classified to a broad kind at the boundary.
… logger server path
The prompt shipped as an onboarding step that production could never reach. Creating the wallet flips the app to the wallet home, which unmounts Welcome and every step inside it, so a telemetry ask placed in that machine could only ever run before the user had a wallet — exactly where it does not belong. Give it its own route instead, following /finish-side-panel: registered ahead of the `!ready` catch-all so it survives the Ready transition either way. Welcome's two success paths detour through it once the wallet exists and only while `hasTelemetryChoice()` is false; the prompt then continues to `postOnboardingRoute()`, keeping Chrome's create -> consent -> side-panel chain with the "Open wallet" click's own live gesture. "Never re-ask" is enforced on the route rather than only on the flows leading to it, so a bookmark or hand-typed URL cannot re-open a settled question either. Abandoning the prompt — dismissal, back, or a killed app — writes nothing, which leaves consent off, since off is the default. Removes the now-dead OnboardingStep / OnboardingActionId / navigator plumbing, which would otherwise be a second, unreachable entry point to the same screen.
The plan still carried "Share anonymous data". The shipped string is "Share usage data", because the ingest endpoint sees an IP like any request and the stronger claim is not one we can support.
…ndary Every earlier task defended privacy where it instrumented. Those defences are per-site, and the next person to add a flow will not read any of them, so this asserts the invariant once at the two places data actually leaves — and covers call sites that do not exist yet without being touched again. Product events are driven through the real chain: the lifecycle hook, the fund and note-claim wrappers, the module-scoped send handle and `beginFlow`, on the success, cancellation AND error path, into the real consent gate, the real serializer and the real fetch transport. Crash reports go through a real Sentry client to its real transport, so what is asserted is the envelope on the wire rather than an argument to a mocked `captureException`. `fetch` is the only thing replaced, because `fetch` is the wire. The error path gets the same weight as the success path — a leak mutation in an earlier task survived precisely because the privacy assertion only exercised success — so every poisoned error is wrapped three deep through `cause`, which is where a caught object is most likely to be passed along whole. Asserted: the wire key set is EXACTLY `WIRE_KEYS`, so a field added later fails loudly instead of shipping; no value on the wire is anything but a closed-union member, the version, or the ephemeral flow id; nothing nests; nothing reaches a host beyond the configured endpoint; and with consent off the transport receives nothing at all, paired with a consent-on control so the silence cannot be a broken driver. Positive facts are asserted before every absence, since "no leak" is trivially satisfied by "nothing was sent". `encodingVariantsOf` covers the encodings a value could wear — case, base64, base64url, percent (both hex cases, UTF-8 correct), JSON escapes, \uXXXX, hex — mirroring the decoders in `redact.ts`, because a substring check that misses base64 passes while leaking. Dense secrets are also checked in sliding windows and phrases in four-word windows, so a scrubber that mangles a private key without destroying it cannot pass either. Two tests assert there are only two ways out at all: the consent-gated sender has one caller outside the telemetry module, and the crash SDK is confined to the module that scrubs before it. A third egress point would be invisible to everything else here. `scripts/telemetry-egress-mutations.mjs` is the evidence: 42 deliberate leaks, 40 caught, 2 that produce no egress to catch (a poisoned stack header Sentry's parser never serialises, and a phrase the second scrubbing layer still discards — the pair that removes both layers at once IS caught). It refuses to run against a red baseline, since that would report every mutation as killed for the wrong reason.
…king assertions The privacy promises live in the store listing and the consent copy, where nothing can check them. This asserts them against the repository itself, so the change that breaks one fails the build rather than shipping: the iOS and Android projects, `PrivacyInfo.xcprivacy`, `package.json`, `yarn.lock`, the HTML entry documents, and the service worker's import graph. No ATT and no advertising id is asserted three ways, because a token list alone only catches spellings it already knows. Sixteen tokens cover the named APIs and the four acronyms; a structural read of `Info.plist` and `AndroidManifest.xml` catches a permission prompt or a permission the token list has never heard of; and `PrivacyInfo.xcprivacy` — the machine-readable form of this promise, and the file Apple actually reads — must keep declaring `NSPrivacyTracking` false with no tracking domains and nothing collected for tracking. The acronyms use non-alphanumeric lookarounds rather than `\b`, since `\b` treats `_` as a word character and reads straight past `IDFA_FALLBACK_KEY`, while a bare `\b` word match would fire inside a base64 run. The dependency promise is made separately at each of the three levels a package can exist at, because they disagree. `@sentry/browser` resolves `@sentry/replay`, `@sentry/replay-canvas` and `@sentry/feedback` — three DOM recorders — into `yarn.lock`. Nothing imports them, so the bundler drops them and no replay code ships. So they are forbidden where it counts (declared, imported) and exempted where it does not (resolved), with the full seven-package set pinned so an upgrade that drags in an eighth is read before it is accepted. Matching is against extracted package names, never raw text: a `rg analytics` guard fires on the word in a comment and gets deleted within the month. No persistent identifier is asserted as the absence of the capability rather than the absence of a known key. The telemetry module may not reach any persistence API at all bar the `localStorage.removeItem` that clears the legacy `analytics` key; only `report-flow` may import an id generator; no wire key may read as durable identity, which `WIRE_KEYS` being pinned elsewhere does not cover, since adding `userId` to both lists at once passes that pin. The service worker's ban on the `lib/telemetry` barrel had no lint rule and held only because two people remembered it. It is now a real graph walk from `background-entry.ts` over runtime imports, pinning the telemetry modules the worker reaches to the four leaf paths. Off-by-default is asserted end to end against the real settings module and empty storage, not just as a constant: a read-miss that fails open leaves `DEFAULT_TELEMETRY` false and every mocked-consent test passing, and only this notices. Paired with an opt-in control so the silence cannot be a broken driver. The third-party-origin allowlist records a finding rather than hiding it. The entry documents load Inter and Nunito from Google's CDN, which discloses IP and User-Agent on every launch on mobile and desktop; it is listed with that reason and a note that serving them locally, as Geist already is, would remove it. Anything not on the list fails. Every absence assertion here is satisfiable by scanning nothing, so each scan carries a floor and each extractor a positive control — and two of the mutations below sabotage the guard itself to prove those fire. `scripts/telemetry-guarantee-mutations.mjs` is the evidence: 66 deliberate violations, 65 caught, 1 genuinely equivalent (a type-only barrel import, which TypeScript erases). It verifies a green baseline before it starts and fails if the tree is left dirty.
Six of the eight HTML entry documents carried Google Fonts <link> tags, so every launch of the popup, side panel, full page, confirm window, mobile WebView and desktop shell disclosed the user's IP address and User-Agent to a third party before the consent prompt had asked anything. That set no cookie and carried no identifier, so it was not the cross-site tracking the wallet promises against — but "No tracking across other apps or sites" is hard to defend while the wallet phones Google to draw its own headings. Both families now ship as woff2 in `fonts/`, declared once in src/main.css and emitted by Vite alongside the compiled stylesheet, so the same sources serve all three targets. The axes are clamped to what the tree actually renders — upright only, `font-weight: 400 800` — which is 469 KiB packed where the full 100..900 plus italic families would have been over a megabyte. The per-subset `unicode-range` split is kept so a Latin-only session never decodes the Cyrillic or Greek files. `ALLOWED_THIRD_PARTY_ORIGINS` in the standing guarantee test is now empty, which turns the exception it recorded into a prohibition: any third-party tag reintroduced into an entry document fails the build.
…surface The privacy policy opened with "Data we collect: None", which this feature makes false. It now describes what the code actually sends, field by field, against WIRE_KEYS rather than against the design: the eight wire fields, the eleven flows, the eight error categories, and the scrubbed crash report. Two things are stated plainly rather than dressed up. There is no persistent identifier, so a per-user deletion request cannot be honoured — no identifier exists by which to find one person's rows — and that is disclosed as the cost of the identifier design rather than omitted. And the crash scrubber is a filter, not a guarantee: the policy names the two shapes that could get past it, at the level of consequence a user needs to judge the risk. The word "anonymous" is avoided throughout, consistent with the earlier ruling on the consent copy: the ingest endpoint sees an IP like any request. Declarations, in the form each store actually requires: - iOS: PrivacyInfo.xcprivacy declares CrashData (app functionality) and ProductInteraction (analytics), both unlinked and both with tracking false. NSPrivacyTracking and NSPrivacyTrackingDomains are untouched. - Firefox: AMO rejects a submission without a machine-readable declaration, so data_collection_permissions goes in the manifest — required ["none"], with technicalAndInteraction optional, which is the only category Mozilla permits to be optional and an exact fit for an off-by-default setting. - Play: the paste-ready Data Safety answers are corrected in place, including the deletion question. - Chrome and the App Store questionnaire live outside the repo and are captured in docs/telemetry-store-declarations.md, with the vendor-configuration checklist every retention claim depends on. The engineering notes in docs/telemetry-limitations.md carry the mechanism behind the redaction limits and the retry-after-error measurement bias. The bias is deliberately not in the public policy: it says nothing about what is collected about a user and cannot inform their decision, so it belongs where the people reading the dashboards are. Two items flagged rather than decided unilaterally: the "no tracking or analytics" line in the store listing, which is Product's claim to keep or change, and the consent copy, which describes the product events well but never mentions crash reports that the same setting turns on. One entry covers the whole feature, as the earlier tasks on this branch deferred theirs for exactly that.
…ash reports in the prompt Two consent-integrity defects, both of which let the wallet collect data the user had not agreed to give. Firefox 140+ asks its own question about `technicalAndInteraction` data, at install and under about:addons. We declared the category but never read the answer, so declining at the Firefox prompt and then enabling "Share usage data" was collected from. `isTelemetryEnabledAsync` now ANDs the two: one gate, which both egress points already call, rather than the check scattered across them. The gate distinguishes "this browser has no such concept" from "this browser said no" by the presence of the `data_collection` key in `permissions.getAll()`, which is what Mozilla documents for feature-detecting this at runtime. Keying off a thrown error instead would have been the trap: Chrome rejects an unknown `data_collection` key passed to `permissions.contains()`, so a throw means both "this is Chrome" and "something broke", and reading it as a refusal disables telemetry everywhere. Absent key means our setting decides; present key is authoritative and an empty array is a refusal. A throw, a rejection, or a non-array value fails closed, since an error reading a permission must never read as permission granted. Off-extension abstains rather than failing closed, so iOS and Android are unaffected. The consent copy described product events but never mentioned crash reporting, which the same setting turns on — a user could accept without being told stack traces would be sent. It now says so plainly, including that reports are scrubbed before sending. The four strings live only in en.json and already fall back to English, so no locale is left stale. Also fixes two stale anchors in the guarantee harness, one of them dating from the privacy-manifest edit that populated `NSPrivacyCollectedDataTypes` and silently left the "flagged as used for tracking" guard unproven since then.
These were the only `return await` sites in either dispatcher.
The wallet timed its own prove step by wrapping the call, which measured the round trip rather than proving itself and went blind whenever the SDK took an internal fallback path. The SDK now reports each wrapped client method through a client-construction observer, so take the measurement from there instead. createWalletSdkObserver translates observations into the closed telemetry unions: only proveTransaction is kept, reduced to a duration and a boolean outcome. Every other op is dropped rather than carried as free text, and the observation's optional sensitive payload is never read — the wallet does not open that channel at construction, so it is absent, and nothing downstream could accept it in any case. prove-telemetry turns from a self-timing shim into a consumer. beginProveAttempt scopes an attempt around proveWithFallback so asynchronous observations attach to the attempt in flight; a report arriving while attempts are ambiguous is dropped rather than misattributed. Note that this compiles only against web-sdk#303, which carries the observer option. The pinned @miden-sdk versions stay as they are — the interface is unpublished, so a bump would not resolve. CI patches in the linked PR build via the Web SDK PR marker.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged five high-severity findings in the new telemetry tests.
Three are the same weakness in the guards themselves: deciding a request
went to Sentry with url.includes('sentry.io') also accepts
https://elsewhere.invalid/?ref=sentry.io, so an assertion meant to prove
a crash report was sent could be satisfied by a request to anywhere.
Compare the parsed hostname instead. Mutating the new helper to return
false fails exactly the two positive assertions, so neither passes
vacuously; the negative assertion was never at risk, since a looser
filter only makes "sent nothing" harder to satisfy.
The remaining two are cosmetic but worth not leaving behind: the
identifier-matching regex escaped only '.', and a percent-encoding
expectation was built with a single-occurrence replace instead of
written out.
…aths Opt-in telemetry put a screen inside onboarding. `postCreationRoute()` sends a wallet that has just been created or recovered to `/help-improve-wallet` whenever `hasTelemetryChoice()` is false, so the chain is confirmation -> consent -> `/finish-side-panel` (or `/`) rather than confirmation -> handoff. That is the intended behaviour; what was wrong is that the harness never learned about it. Every driver clicked `onboarding-confirmation-submit` and then waited for a post-onboarding surface one screen further away than it used to be, so runs parked on the consent prompt until the wait expired — the "Open wallet" button that popup-smoke and dapp-provider look for never appeared. One helper, `dismissTelemetryConsent`, now gets past it, and it DECLINES. Accepting would write consent=true into the test browser, arming the crash reporter and the product-event egress for the rest of that profile's life; an e2e suite must never be in a state where a wallet under test can ship anything to Sentry. Declining is also the shipped default, so it is the state the rest of the suite assumes. The prompt is treated as optional, because it is: PageRouter SKIPs the route when the app is locked or a choice already exists, so a profile that carries one never sees it and an unconditional wait would cost those runs the whole timeout. Absence is a normal outcome, reported as false; only a Playwright TimeoutError is swallowed, everything else propagates. Where the caller has not yet observed anything proving `register()` finished, the wait races the prompt against the surface the flow lands on instead — a short poll would otherwise run ahead of a prompt that was still coming, and a long one would hang when there was none. Applied at every driver that completes onboarding: the Chrome POM's bypass-create and its via-UI guardian recovery, the forgot-password recovery journey, the dApp seed-import driver, the create-from-scratch spec, both smoke tests, and the iOS and Android POMs (which take the CSS-selector form, having no locators). The recovery paths dismiss after the hot-key rotation gate, not before: that gate is a `fixed inset-0` scrim above whatever route is mounted, so the prompt is on screen and unclickable until it clears. `submitRecoveryFromSeed` is deliberately untouched — it returns with registration in flight, and its other caller drives a recovery that fails and must stay on the confirmation screen. Two supporting test hooks. The consent buttons had nothing but localized titles, and this feature's own tests pin that copy as changeable for legal reasons, so both now carry ids. And the handoff screen had no id at all while sharing both its title and its button title verbatim with `Confirmation.tsx` — which is why popup-smoke's `/your wallet is ready/` assertion passed on the screen the flow had not left while the button assertion failed one line later. `finish-side-panel` makes those assertions say what they mean; they are now scoped to it rather than matching either screen.
…ears A Guardian wallet recovered from a seed is flagged requiresHotKeyRotation on the same store update that ends the bypass driver's readiness wait, so HotKeyRotationGate's `fixed inset-0 z-[9999]` scrim covers the consent prompt from before it is routed to until the rotation lands on-chain. Declining it inside createWalletViaBypass therefore clicked into that scrim on the one caller that recovers rather than creates, and guardian-recovery-stress failed on an element that was found, enabled, and permanently unactionable. Move the dismissal to completeHotKeyRotation, whose resolution IS the gate detaching and thus the first moment the prompt can be answered. Every path that raises the gate already awaits it, so the three recovery paths now share one line instead of repeating an ordering constraint each: the two branches of recoverGuardianFromSeed and recoverViaForgotPassword all drop their own call. createWalletViaBypass keeps declining in place for creates and OffChain seed imports, neither of which can raise a gate. Nothing waits the rotation out: an on-chain replace_signer has no duration a timeout could be sized against, and the fault-injection specs need the gate still standing when their recovery call returns — they drive it themselves. A decline blocked by the scrim now fails naming the scrim and the owning call, rather than as an anonymous actionability timeout.
Collaborator
Author
Known red:
|
The privacy policy names Aptabase as the processor for usage data, but no integration existed: the sink POSTed our own eight-field JSON to a generic URL, which Aptabase would have rejected. The published policy is now true as written. Two places Aptabase's contract collides with this design, both resolved towards the design rather than the vendor: - `sessionId` is the event's existing `flowId`, so an Aptabase "session" is exactly one wallet flow. Their SDKs reuse one id for four hours, which would link every flow a person performs into a single trail. There is also nowhere to keep such an id: the module cannot reach a persistence API at all, and a guard asserts it. - `systemProps` carries `isDebug`, `osName` (our coarse platform), `appVersion` and our own `sdkVersion`. `osVersion`, `locale` and `deviceModel` are fingerprinting vectors, are not required, and are never sent — asserted both on the outgoing envelope and as the absence of any API that could compute one. `props` is built field by field from the allowlist, never spread: Aptabase leaves that object open, so the "no free-form field in the wire type" guarantee stops being structural at the crossing and is re-established in the mapper. `eventName` is `<flow>_<phase>`, both halves closed unions. One event per request rather than the 25-event batch, since an MV3 worker has no guaranteed lifetime and a batch buffer is a buffer that gets killed. The host comes from the app key's region; `A-SH-*` and `A-DEV-*` require an explicit one. A missing or malformed key disables sending without throwing. `TELEMETRY_INGEST_URL` gives way to `APTABASE_APP_KEY` and `APTABASE_HOST`, the latter an origin rather than a full URL. Everything stays inert unset. Retargets the egress mutation harness, whose leak corpus injected into the serializer — no longer the last thing before `fetch`, so those mutations would have reported survived for defects that produce no egress at all.
"Configured not to store it" reads as "nothing derived from it is kept", which is a stronger promise than we can make from this repo. Aptabase reports a country breakdown and no field the wallet sends carries one, so something is derived at ingest. The policy now says a coarse derivation may happen, and the pre-submission checklist asks for the answer in writing so the sentence can be tightened or widened on fact rather than assumption.
The sdkVersion constant read bread-wallet-aptabase@1.0.0, naming a different product. It is sent to the vendor on every event and shows up in their dashboard, so the wrong name would have been the label on all of our usage data.
APTABASE_APP_KEY and SENTRY_DSN are read at build time and baked in by vite's `define`, so they have to be present in the environment of the job that produces the artifact. Four such jobs, not three: build-mobile has separate Android and iOS jobs that each run `yarn build:mobile`. Neither is confidential once shipped -- both are write-only ingestion ids that anyone can read out of a released build -- so the secret only keeps them out of git and out of logs. Absent, as in a fork, each resolves to '' and turns its feature off outright, which is why nothing had to change for CI to keep working before now. vite.desktop.config.ts was missing the three defines. App.tsx is shared, so the desktop bundle reaches the telemetry modules like every other surface. This would not have failed a build or thrown at runtime: nodePolyfills shims `process`, so the reads would have resolved to undefined and both features would have quietly disabled themselves -- telemetry absent on desktop while the consent toggle still offered it, which is worse than a visible break. The extension and mobile configs already had them; desktop was simply missed. The content-script configs stay untouched: their two entries do not reach lib/telemetry, directly or transitively.
Every existing telemetry test was blind to the shipped egress path: the jest tests assert against a mocked transport with synthetic fixtures, and every e2e suite builds with no Aptabase key, which makes telemetry inert at compile time. Nothing had ever seen a request leave. This suite builds the extension with its endpoint pointed at a local recorder and drives a real import, opt-in and opt-out. It asserts silence before consent, the exact envelope contents, and -- against a wallet holding a real seed, password and address -- that none of those bytes reach the wire in any encoding. It cannot share another job's artifact: the key and host are baked in by vite, and this is the only build where accepting consent is safe, because the only reachable endpoint is local. Deliberately not real Aptabase: self-hosting needs Postgres and ClickHouse plus an authenticated session to mint a key. The vendor contract is transcribed instead, and that gap is documented. It then found what it was written to find. Withdrawing consent drops the queue and stops the reporter at once, and no flow starting afterwards is sent -- but a flow already open when the switch flipped could still report its cancellation, because the background learns of the change through an unordered mirror write. The settings handler now awaits that write, closing the window belonging to the switch itself; closing the rest means routing withdrawal through the event channel, so the residual limit is recorded rather than papered over. Each assertion was verified to fail when the guarantee it covers is removed: the consent gate, the field allowlist, and the leak scan (caught a base64'd password hidden in an allowlisted field).
The first build on a real device reported, for a session containing exactly one swap: a `send_started` with no end, and a complete `receive_share` pair. Neither corresponded to anything the user did, and the swap reported nothing. Two causes, both making the data worse than absent. `TabLayout` renders Overview / Send / Receive / Earn / Swap as one carousel and mounts all five at once for the whole session, so a flow begun in a mount effect began on every app launch for a screen nobody had looked at — and was never settled, because swiping away does not unmount. `receive_share`, which completes when the address renders, therefore completed on every launch too. Those screens now gate on `pathname`, the carousel's own source of truth for which page is showing. A step effect has to depend on that gate as well as the step, since the flow now begins after the mount. And the flows that carry most of the wallet's value were not instrumented at all: swap, earn deposits, dApp connect and transaction approvals, and guardian rotation. All are now wired. dApp approvals report from the confirmation store, the one point all three approval surfaces pass through, which cannot import telemetry itself — it sits in the service worker's graph — so it declares the hole and the UI fills it. Adds `step` to the wire payload: the furthest screen a flow reached, from a closed union of screen names, carried on `ended` so progress costs no extra events. Without it a multi-step flow is unanalysable — an abandoned send and an abandoned swap both arrived as one bare `flow_started` — and "where do people get stuck", the question this telemetry exists to answer, could not be asked of the data at all. The distribution of `step` across cancelled flows is the drop-off funnel; onboarding's screens are named exhaustively, being the most actionable of them. `instrumentation-coverage.test.ts` fails if a flow or step is declared without a call site, which is how `swap` could typecheck, pass every test and report nothing. It cannot catch the carousel class of bug — a flow begun in the wrong place still has a call site — so the egress E2E now drives an abandoned send and asserts the step reaching the sink from a real service worker.
Four real defects, one of them worse than what it replaced. The route gate broke `receive_share` completion outright. Its completion effect was keyed on the address alone, which is resolved before the page is reachable and then never changes — so with the flow now beginning on the LATER navigation to /receive, the effect ran once at mount against no flow and never again. Every share would have reported cancelled: the previous phantom-completion bug exactly inverted. Keyed on the route gate as well, with a test for the sequence production actually takes (mount at `/`, then navigate) rather than the direct mount every existing test used. An earn deposit that failed and was retried reported nothing for the retry, so a deposit that failed once and then succeeded was recorded only as the failure. The swap gate assigned its handle where the other three adopt, which would abandon an open flow without settling it. dApp approvals reported nothing at all on the extension. `lib/miden/back/dapp.ts` guards the confirmation-store path with `!isExtension()` and uses an intercom and a popup window instead, so the store — where this was instrumented, and which claimed in a comment to cover the extension — is unreachable there. `ConfirmPage` now reports for itself. Mount is a fair trigger in a window that exists only to ask. Three declared onboarding steps could never be emitted: the seed-phrase screens belong to a different flow, and the wallet-type screen is where the flow is begun, so there is no flow open while the user is on it. Removed, and `Welcome.test.tsx` now fails on a step-table entry naming a screen no `setStep` call can produce — the coverage test could not catch this, since a dead table entry still counts as a call site. Documented rather than changed: route-gating means passing through a home pane opens and closes a flow, which would swamp the first bucket of the funnel. Every ended event carries `durationMs`, so a transit is separable from a real visit by reading it. A dwell timer would have to suppress the `started` event too, losing the information irrecoverably and freezing one guess at the threshold into shipped builds.
A second review pass found the extension half of the dApp reporting emitting the phantom events this work exists to remove. A `connect` from an already-permitted dApp is auto-approved during render: the user is shown nothing and asked nothing, and that path never reaches the settle callback. The flow begun on mount was therefore never settled, arriving as an unmatched `dapp_connect_started` — which the design reads as an abandoned approval, i.e. a refusal that never happened, recurring and correlated with nothing the user did. Such a prompt now begins no flow at all. Dismissing the popup reported nothing either. The cleanup meant to catch it was an unmount effect, and destroying a browser window does not unmount a React tree — so neither the X button nor the request timing out ran it, and the test asserting the behaviour used a jsdom `unmount`, modelling a route change rather than the teardown that actually happens. Reported from `pagehide` instead, which fires on both paths, and the event survives the window because it is posted to the background over the intercom. The old test is kept for the route change and a new one covers the case they were being conflated. Also from the review: the settle callback is memoized, so the three `ConfirmPage` callbacks depending on it stay memoized; the step-table reachability regex is anchored; and two claims are corrected rather than left overstated. The new test catches two of the three dead step entries, not three — the wallet-type screen is reachable, and dead for an ordering reason no static test can see, which is now recorded where the table is defined. The duration filter the docs prescribe is now on duration alone and not on `result`, because `receive_share` completes as soon as the address renders, so a transit and a real visit both arrive completed with a near-zero duration and `result` cannot separate them.
Aptabase's sessionId was carrying the per-flow id, so every started/ended pair arrived as its own session with a 0s duration and no ordering between them. A real swap therefore read as two unrelated 0s sessions rather than one visit that moved through the app. Mint an ephemeral runId per app run instead — in memory only, never stored, rotating after 30 minutes idle — and send that as sessionId, moving flowId into props where it still pairs the two halves of a flow. Sessions now have a duration and an ordered event sequence. The carousel mounts every pane at launch, so swiping past /send reported a flow the user never intended to start. Gate the route-driven flows on a 600ms dwell so only a screen the user actually settled on reports anything.
Settling a flow wrote the idle timestamp directly instead of going through the rotation check, so a flow held open past the window re-armed the clock rather than retiring the run. A send left open on a full-page tab overnight and dismissed in the morning linked the next morning's activity to the previous day's, and every subsequent long flow extended it again — the id could live as long as the page did. Route every write through one function that checks before it touches, and treat a backwards clock jump as a rotation trigger, since a negative elapsed time is never over the threshold and would suppress rotation indefinitely. The ended event still goes out under the id captured at the start, so this cannot split a pair. Both cases were uncovered; the existing idle test only exercised flows that began and ended at the same instant.
The 30-minute rule governs which flows JOIN a run, not how long one can span: a flow already open when the silence elapses still ends under the id it started with, because the alternative is a pair that no longer pairs. So a run's span is bounded by the lifetime of one straddling flow rather than by the clock, and a backwards clock step can split one run in two. Both were true of the code and absent from the docs. Also correct two more stale allowlist counts the first pass missed, pin the property that separates the idle check from an over-eager one that would fragment an active visit, reset the run between every test rather than only inside the block that asserts on it, and drive the E2E transit through pushState — the router has no hashchange listener, so assigning the hash worked only via the fragment change also firing popstate, which lands as a back navigation.
7 tasks
Two real conflicts, both import lines whose two sides are both still used in the merged body: History.tsx needed the union of the telemetry branch's `useEffect` and main's `useLayoutEffect`/`useRef`, and EarnDepositAmount.tsx needed both the route-flow reporters and main's `hasKnownScale`. Everything else was locale files and the changelog.
Adds an error/outcome axis to wallet telemetry: `<operation>_settled` events for work the wallet does on the user's behalf, where the flow events only ever covered work the user drove. A prover outage that failed a transaction produced no telemetry at all before this; now it produces a transaction outcome with a classified error kind and the stage it died in, a prove-health event, and a service-outage event with a duration. Covers every terminal writer, including the two row types that are `Completed` in the database from birth and carry their real outcome in `extraInputs.phase` — a failed earn withdrawal and a failed inbound bridge were both invisible. Privacy is unchanged: consent-gated, allowlist-serialized field by field, every value a closed union or a number, no `flowId` on a settled event so nothing links an operation back to a user's flow. The anti-leak boundary test now sweeps the settled axis as the full cross-product of its optional fields, and the instrumentation guards no longer accept an inert mapping table as evidence that anything reports.
Three declared conflicts and one silent mismerge. `RotateGuardianReview.tsx`: main dropped the `DetailCard` import along with the markup that used it, and added a guard against navigating someone who has already backed out. Kept main's guard and settled the telemetry flow above it, since one arm of that branch unmounts the page — a no-op when they had already left, which is correct, because the unmount cleanup settled it as cancelled then. `LanguageSettings.tsx` and its test: main developed both well past what this branch had, but its version calls `useAnalytics` from `lib/analytics`, which this branch deletes — that scaffold's payload type was an unbounded `properties?: object` bag and it had seeded every install with a permanent random user id. Took main's version without those calls. `PendingNotes.test.tsx` merged clean and failed: main replaced the `window.history.length` mock with a `historyPosition` from `useLocation`, so the `setHistoryLength(3)` this branch added to the telemetry block's `beforeEach` survived while its definition did not. Uses main's mechanism now.
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Web SDK PR: #303
Summary
Adds opt-in telemetry so we can see where people get stuck in the Wallet without learning anything about their money or their activity. It is off by default and does nothing until the user actively opts in, either at the prompt shown once after onboarding or from Settings at any time.
Eleven flows are instrumented across getting started (
open,unlock,create,import,recover,return) and everyday use (send,fund,receive_share,note_handle,activity_view). Each reports a start and an end, so an unmatched start is the abandonment signal — which is the thing we actually want to learn and the thing a single terminal event would lose whenever someone force-quits mid-flow.What can and cannot leave
The wire payload is exactly eight fields:
phase,flow,flowId,result,errorKind,durationMs,appVersion,platform. Every one is a closed literal union or a number.There is deliberately no free-form
stringfield, noobjectfield, and no index signature anywhere in the payload type. That is the primary guarantee rather than a convention: an address, an amount, a note id, or anerror.messagehas no field it could occupy, so a leak failsyarn tsbefore any test runs. The serializer builds the payload field by field and never spreads, so a future field cannot reach the wire without appearing there.appVersionandplatformare derived in the background, so a caller cannot supply them.Errors are reduced to one of eight broad categories.
classifyErrorinspects the message but its return type is a closed union, so no caught text can pass through it.Consent
Off by default, and absence of a stored choice also sends nothing. Turning it off stops new events, drops the queue, and stops crash reporting — off means off, not "queue and flush later". On Firefox the browser's own
technicalAndInteractionanswer is ANDed with our setting and fails closed, so someone who declines at the browser prompt is not collected from even if they later enable our toggle. Chrome, which has no such concept, is distinguished from a refusal by the presence of thedata_collectionkey inpermissions.getAll()rather than by whether a call throws.Crash reporting
Same consent gate. The client is a hand-built
BrowserClient+Scopethat never touches Sentry's global hub, so events cannot cross into a host page's project.Sentry.init()and namespace imports are both avoided — they have caused real Chrome Web Store rejections for MV3 extensions.Integrations are an allowlist, not a denylist, which was decisive: v10.70.0 ships a
ConversationIdintegration that attaches a persistent identifier — exactly the durable-id class this design exists to avoid — and a denylist would have admitted it silently.Breadcrumbsis excluded as the largest default leak vector (it auto-captures console output, every fetch/XHR URL, and DOM click targets). Constructing the three kept integrations directly rather than filteringgetDefaultIntegrations()also keeps the excluded code out of the shipped artifact, which matters because a store reviewer greps the artifact, not the source.The redactor's BIP-39 threshold is a run of 4, chosen from measurement rather than taste: across the 937 real strings in
en.json, 98.2% peak at three or fewer, while a 12-word phrase clears the bar three times over. A threshold of 1 was rejected outright because 63% of real strings contain at least one wordlist word — "about", "account", "amount", "note" and "key" are all BIP-39 entries.Identifiers, retention, deletion
Identifiers are ephemeral per-flow only. There is no persistent user or device identifier, no advertising identifier, and no ATT prompt. The honest consequence, stated plainly in the privacy policy: we cannot honour a per-user deletion request, because there is no identifier by which to find one person's data. That is a deliberate trade, not an evasion.
Standing guarantees
src/lib/telemetry/guarantees.test.tsencodes the promises as build-breaking assertions so they survive future changes. Dependencies are checked at three levels that genuinely disagree — declared, imported, and resolved — always against extracted package names rather than raw file text, because arg analyticsguard fires on a comment and gets deleted within the month. "No persistent identifier" is asserted as absence of the capability: the telemetry module may not reach any persistence API at all. Off-by-default is asserted end-to-end against empty storage, which catches the failure a constant check misses entirely — a read-miss that fails open leaves the constantfalsewhile every mocked-consent test still passes.src/lib/telemetry/egress-boundary.test.tsasserts at the boundary rather than per call site, driving real flows through the real consent gate, real serializer and real transport with onlyfetchreplaced, on success, cancellation and error paths. Crash reports go through a real Sentry client to its real fetch transport, so the assertion is against the actual envelope. The corpus covers Miden bech32 and composite addresses, private keys, note ids, amounts and credentialed URLs in base64, base64url, URI-encoded, fully percent-encoded, JSON-escaped and\uXXXXforms, plus values split across a wrapped-errorcausechain.Things removed or fixed along the way
@segment/analytics-nodeand thelocalStorage['analytics']key that seeded ananoid()userId persisting for the life of the install. Deleting code does not delete data, so existing installs are cleaned up at startup.!analyticsJson.enabled === truereturns early exactly when analytics is disabled, which is correct. The real defect was theanalytics &&prefix — with noanalyticskey at all the gate was skipped entirely and the code fell through to send, so it failed open on absent consent. Nothing ever leaked, because the sink was an empty stub.fonts.googleapis.comon every launch, disclosing IP and User-Agent before the user consented to anything. Not tracking and not telemetry, so it broke no promise as written — but shipping a prompt that promises "no tracking across other apps or sites" over it would have undercut the whole feature. Side effect: desktop typography is fixed, because the Tauri CSP had been silently blocking these fonts and rendering system fallbacks.Disclosed limitations
Stated in the privacy policy rather than left for someone to discover:
erroredas "hit an error at least once", not "did not succeed".Requires web-sdk#303
The final commit binds
ClientOptions.observerfrom 0xMiden/web-sdk#303 to replaceprove-telemetry.ts's wrapper, which measured the round trip and went blind whenever the SDK took an internal fallback path.yarn tsfails against the published 0.15.9 SDK with a single error —'observer' does not exist in type 'ClientOptions'— because that interface only exists in #303. TheWeb SDK PR: #303marker at the top of this description makes.github/actions/inject-linked-web-sdk-prpatch the deps before install and typecheck, which resolves it. The alternative, casting the option away, would have typechecked while silently no-opping against a published SDK that ignores it. The@miden-sdk/*pins are deliberately left at their published versions; the release, not this PR, bumps them.Note for reviewers reading the observations: SDK
opnames the wrapped client method, not the high-level call, so oneclient.transactions.send(...)reports four observations. The observer usesopto decide and never to carry — it keepsproveTransaction, reduces it to a duration and a boolean, and drops everything else at the boundary.Test plan
yarn lint --max-warnings 0andyarn lint:i18ncleanyarn tsclean against the web-sdk#303 build; fails against published 0.15.9 by design, as explained aboveProduct events go to Aptabase, in Aptabase's envelope
The privacy policy names Aptabase as the processor for usage data, so the transport now actually speaks Aptabase rather than POSTing our own shape at a generic URL.
src/lib/telemetry/aptabase.tsmaps the eight allowlisted fields onto their envelope and does nothing else — no queueing, no batching, no network call.sink.tsremains the single egress point.Two places this deliberately departs from how Aptabase's own SDKs fill that envelope, both because following them would have broken a promise in the policy:
sessionIdis the per-flow id, not a session. Their SDKs reuse one id across events with a four-hour timeout, which would link every flow a person performs into a single trail — exactly the linkability this design promises not to create, and there is nowhere to keep such an id anyway, since the telemetry module is asserted to reach no persistence API. So an Aptabase "session" here means one wallet flow. Thestarted/endedpair links, as it already did; nothing links across flows. Pinned on the wire rather than by name: the boundary test asserts no session spans two flows or carries more than two events, so a cached-first-id mutation (their actual behaviour) trips it.systemPropsis sent nearly empty. Their example carriesosVersion,localeanddeviceModel; all three are new data, all three are fingerprinting vectors, and none is mandatory. What goes out isappVersion,osName(our existing coarseplatform),isDebug, and a constant naming this transport. "Do not add code that could compute them" is enforceable rather than aspirational: the guarantee test now fails if the telemetry module reaches anynavigator.*,Intl,screen.*or Capacitor Device API.propsis where the type system stops helping — theirpropsis an open object, so the allowlist moved into the mapper, which copies three named fields with threeifs and never spreads. Spread,Object.assignand key-iteration are each a separately killed mutation.eventNameis<flow>_<phase>as a template-literal type over the two closed unions, so all twenty-two names are fixed at compile time and no free text can reach it. One event per request, deliberately not the 25-event batch endpoint: an MV3 service worker has no guaranteed lifetime, so a batch buffer is a buffer that gets killed with the worker.Not shippable until configured
APTABASE_APP_KEYandSENTRY_DSNare unset everywhere, so the feature is inert as merged — deliberate. A missing or malformed app key disables sending silently rather than throwing, because this path must never be able to fail a wallet operation.APTABASE_HOSTis only needed forA-SH-*andA-DEV-*keys; the hosted regions derive their host from the key, which means the policy's "European Union" row is enforced by the key itself rather than by a comment.The pre-existing release blocker is narrower but not gone: 90-day retention, the no-IP-storage claim and no-onward-forwarding remain vendor-console settings no code here can enforce. A policy claiming 90 days against a vendor defaulting to longer is worse than no claim, so
docs/telemetry-store-declarations.mdcarries a pre-submission checklist that has to be ticked first. One item on it is new and worth calling out: Aptabase's dashboard reports a country breakdown, and nothing we send carries a country, so something is derived from the connection at ingest. "Does not store your IP" and "stores nothing derived from it" are different promises, and the policy no longer implies the stronger one.mock-web-client.spec.ts— the linked-PR harness gap, now closedThe
Testjob was red onMiden napi module not found, and it was a property of linked-PR runs rather than of this branch: the inject action built the linked SDK WASM-only, so the napi binary the Node-side loader needs was never produced. The root cause turned out to be narrower than "WASM-only" suggests — the published@miden-sdk/miden-sdkreaches its.nodebinary through an optionalDependency on a per-platform package, and web-sdk's publish workflow writes those entries at publish time. They are absent fromcrates/web-client/package.jsonin source, so afile:dep built from a branch has no route to a binary at all, and the loader's last-resort repo-root walk (Cargo.toml +crates/) lands in the wallet, which has neither..github/actions/inject-linked-web-sdk-prnow builds it, mirroring web-sdk's owntest-web-client-nodejsjob:cargo build -p miden-client-web --no-default-features --features nodejson therelease-fastprofile — the same LTO-off profile the WASM build already uses, for the same reason, since CI verifies behaviour and never ships this artifact — then stages the cdylib under the.nodename and exportsMIDEN_MODULE_PATH. That env var is the only hook that survives thefile:rewrite, which is why the binary is pointed at rather than placed.It is behind a
build-napiinput, default off, set by theTestjob alone. That job is the only one that loads the SDK under Node; every other job reads the WASMdist/and shouldn't pay for a native build. Verified against #303's head: the binary loads under Node with 125 exports, the spec passes withMIDEN_MODULE_PATHset, and with the platform package moved aside and the variable unset it fails at the two frames CI reported.linked-web-sdk-pr-readyis pending by design: it gates merge on #303 being merged and released to npm at a version satisfying the wallet's pin.guardian-recovery-stress.spec.tsfailed twice on a migratingwas not bound in the connection/ closed-target error and then passed on a re-run with no code change. That signature is already recorded onmainfor this same spec atplaywright.e2e.config.ts:50-53(run 32175200493).