feat(analytics): property-model foundation for cross-platform schema alignment (#2883)#2903
Open
piyalbasu wants to merge 13 commits into
Open
feat(analytics): property-model foundation for cross-platform schema alignment (#2883)#2903piyalbasu wants to merge 13 commits into
piyalbasu wants to merge 13 commits into
Conversation
First slice of the cross-platform analytics schema-alignment epic: global property-model cleanup (schema_version, account_id_hash, Identify traits, drop SDK-duplicated fields, app.opened snapshot). Evolves the existing emitMetric path in place; hard cutover, no dual-write. Event renames deferred to follow-on slices B/C. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds deriveIdentifyTraits (pure wallet_count/has_hardware_wallet/ has_imported_account) and syncIdentifyTraits (dirty-checked Amplitude Identify call), wired into storeAccountMetricsData. Also fixes a privacy violation found in review: storeBalanceMetricData emitted a truncated public key in the freighterAccountFunded event body. Replaced with account_id_hash of the full public key, matching the schema's "never emit a raw or truncated public key" constraint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
syncIdentifyTraits cached the dirty-check fingerprint before the AMPLITUDE_KEY/hasInitialized guard, so a pre-init call would poison the cache without ever sending an Identify, causing a later identical call to silently short-circuit and never sync the user's traits. Move the cache write to after the guard so it's only recorded once an Identify actually fires.
…duplicate) Final review found accountServices.ts uses a private duplicate storeAccountMetricsData that doesn't call syncIdentifyTraits; the effective sync sites are useGetAppData + the recover hook. Non-functional gap (traits self-heal via useGetAppData). Note added + dedupe follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces the analytics property-model foundation without renaming existing events.
Changes:
- Adds schema versioning, hashed account IDs, surface metadata, and
app.opened. - Moves durable wallet traits to Amplitude Identify properties.
- Adds analytics tests and design documentation.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
extension/src/helpers/metrics.ts |
Implements analytics context, identity traits, hashing, and initialization. |
extension/src/helpers/metrics.test.ts |
Tests the new analytics behavior. |
extension/src/popup/constants/metricsNames.ts |
Adds the app.opened event name. |
extension/specs/ANALYTICS_SCHEMA_FOUNDATION.md |
Documents the schema foundation. |
extension/src/popup/locales/en/translation.json |
Adds generated auto-lock labels. |
extension/src/popup/locales/pt/translation.json |
Adds Portuguese locale entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The canonical analytics schema lives in the cross-platform RFC (stellar/wallet-eng-monorepo#10); this repo doc was a brainstorming artifact (slice refs, PR numbers, migration-decision narrative) that would rot as a second source of truth. Design context lives in the PR description; cross-platform contracts go to the RFC. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lock omission - app.opened: defer emit until the data-sharing preference resolves to allowed (it defaults false and hydrates async, so emitting at init was suppressed and lost). Emit once, when consent becomes allowed. - syncIdentifyTraits: gate on consent and don't cache the fingerprint while opted out, so traits re-sync after consent hydrates. - buildCommonContext: omit account_type/account_funded/is_hardware_account pre-unlock (no active key), per the spec's omission rule. - pt locale: real Portuguese for the auto-generated Auto-lock timer keys. - Tests: 22/22; adds consent-hydration regressions for app.opened + Identify. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
TL;DR
First slice of the cross-platform analytics refactor (#2883): brings the extension's Amplitude property model in line with the shared cross-platform schema, without renaming any events yet. Every event now carries a
schema_version; a privacy-safe hashed account identifier replaces the truncated public key that events previously carried; device/app metadata is no longer hand-duplicated onto each event (the Amplitude SDK already supplies it); durable wallet traits (wallet count, has-hardware-wallet, has-imported-account) move to user-level properties; and a newapp.openedevent captures a one-time per-open snapshot. No new data is collected — behavioral autocapture stays off, and this net removes a truncated key from event payloads.Event names are intentionally unchanged in this PR; the
screen.viewedcollapse and domain-event renames are follow-on slices. The canonical cross-platform schema this follows lives in the RFC (stellar/wallet-eng-monorepo#10).Implementation details (for agents/reviewers)
All emission logic stays centralized in
extension/src/helpers/metrics.ts— this PR changes its contents, not its shape (no typed-catalog rewrite; the RFC is a property/naming cleanup, and the spec guards drift).What changed:
schema_version("2") on every event — stamped centrally inbuildCommonContextvia theSCHEMA_VERSIONconstant. It marks the new property model even while event names remain legacy.account_id_hashreplaces the truncatedpublicKey—getAccountIdHashis a memoized, synchronous SHA-256 hex of the full G-address (viastellar-sdk'shash). This is a cross-platform contract — mobile must hash the same G-address string with SHA-256 to produce a matching value (committed test vector). The one other emit that carried a truncated key (freighterAccountFundedinstoreBalanceMetricData) was also switched toaccount_id_hash.platform/platformVersion/appVersion) are no longer hand-sent —appVersionis passed once toamplitude.initand the SDK's context plugin supplies the rest; behavioralautocapturestaysfalse. Volatile context (network,surface,account_type,account_funded,is_hardware_account,account_id_hash) stays event-level. Connectivity fields (connection_type/effective_type) drop toapp.openedonly.syncIdentifyTraitssendswallet_count(=allAccounts.length),has_hardware_wallet,has_imported_accountvia Amplitude Identify, dirty-checked so it only fires when a trait changes. The fingerprint is recorded only after the init/consent guard (so a pre-init call can't permanently suppress the first Identify).app.opened— new event (emit) fired once per popup/fullpage/sidebar mount with the one-time snapshot.initAmplitudebecameasyncto resolve the surface first;hasInitializedis still set synchronously before the firstawait, so ordering is race-free and theApp.tsx:48fire-and-forget call is unaffected.getSurface()/resolveSurface()classify popup vs sidebar vs fullpage (async resolved once, cached, read synchronously on the hot path). The only new event name added toMETRIC_NAMESisappOpened: "app.opened".Verification: built via TDD, one commit per task with an independent spec-compliance + quality review per task and a whole-branch review at the end.
yarn jest extension/src/helpers/metrics.test.ts→ 20/20 (new suite); existingAccount/SignTransaction/popup/metricssuites unaffected (they mockhelpers/metrics);tsc --noEmitclean. A regression test asserts no event payload contains a raw/truncated public key.Follow-ups / out of scope:
screen.viewedcollapse) and C (domain-event renames/consolidations/removals) — event renaming.accountServices.tshas a private duplicatestoreAccountMetricsDatathat doesn't callsyncIdentifyTraits; non-functional gap (traits self-heal viauseGetAppData), but the two copies should be deduped.wallet_count = allAccounts.lengthmatches (confirmed — mobile labels accounts "wallets" in UI but the count is identical). The one remaining cross-platform check is that mobile reproduces theaccount_id_hashSHA-256 vector when it implements.translation.json(en/pt) changes are auto-generated by the repo's huskyi18next-scannerpre-commit hook filling a pre-existing gap onmaster; unrelated to this feature.