From f311866a14335282a141bbefdff955c3eda7ba14 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 4 Sep 2026 11:26:21 +0200 Subject: [PATCH 1/2] fix(profile-metrics-controller): tag, canonicalize, and dedupe hardware, imported, and non-mnemonic snap accounts --- .../profile-metrics-controller/CHANGELOG.md | 8 + .../src/ProfileMetricsController.test.ts | 443 ++++++++++++++++-- .../src/ProfileMetricsController.ts | 378 +++++++++------ .../src/ProfileMetricsService.test.ts | 15 +- .../src/ProfileMetricsService.ts | 11 +- 5 files changed, 667 insertions(+), 188 deletions(-) diff --git a/packages/profile-metrics-controller/CHANGELOG.md b/packages/profile-metrics-controller/CHANGELOG.md index 4b71836b9b9..5a3f0288204 100644 --- a/packages/profile-metrics-controller/CHANGELOG.md +++ b/packages/profile-metrics-controller/CHANGELOG.md @@ -7,8 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Tag hardware, imported-private-key, and non-mnemonic Snap accounts with a `source` on `AccountWithScopes` when submitting profile metrics + - The value (`'hardware' | 'imported' | 'snap'`) is derived from `metadata.keyring.type`; mnemonic-backed accounts carry no source. + - Adds a `reportedAccounts` state property recording the canonical addresses that have been submitted, so enqueuing them again is a no-op. + - Re-enqueues all known accounts on the first unlock after upgrading so previously-synced records get a source and canonical address, gated by a new `accountSourceBackfillEnqueued` state flag (fresh installs flip the flag on their initial sync). + ### Changed +- Canonicalize account addresses when they are enqueued rather than only when a proof of ownership is signed, so accounts without a proof (hardware, imported, non-mnemonic Snap) are also submitted in canonical form - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.8.0` ([#9780](https://github.com/MetaMask/core/pull/9780), [#10080](https://github.com/MetaMask/core/pull/10080), [#9798](https://github.com/MetaMask/core/pull/9798), [#9823](https://github.com/MetaMask/core/pull/9823), [#9960](https://github.com/MetaMask/core/pull/9960), [#9969](https://github.com/MetaMask/core/pull/9969), [#10046](https://github.com/MetaMask/core/pull/10046)) - Bump `@metamask/superstruct` from `^3.1.0` to `^3.4.1` ([#9754](https://github.com/MetaMask/core/pull/9754)) - Bump `@metamask/accounts-controller` from `^39.0.6` to `^39.1.1` ([#9791](https://github.com/MetaMask/core/pull/9791), [#9807](https://github.com/MetaMask/core/pull/9807), [#9969](https://github.com/MetaMask/core/pull/9969)) diff --git a/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts b/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts index daac6019019..778bcaaf7ef 100644 --- a/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts +++ b/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts @@ -1,4 +1,5 @@ import { deriveStateFromMetadata } from '@metamask/base-controller'; +import { KeyringTypes } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import type { @@ -26,11 +27,13 @@ import { ProofUnsupportedNamespaceError } from './utils/canonicalize.js'; * * @param address - The address of the mock account. * @param withEntropy - Whether to include entropy information in the account options. Defaults to true. + * @param keyringType - The keyring type for the account. * @returns A mock InternalAccount object. */ function createMockAccount( address: string, withEntropy = true, + keyringType: string = 'Test Keyring', ): InternalAccount { return { id: `id-${address}`, @@ -50,7 +53,7 @@ function createMockAccount( type: 'any:account', metadata: { keyring: { - type: 'Test Keyring', + type: keyringType, }, name: `Account ${address}`, importTime: 1713153716, @@ -97,7 +100,7 @@ describe('ProfileMetricsController', () => { async ({ controller, rootMessenger, registerAccounts }) => { registerAccounts([ createMockAccount('0xAccount1'), - createMockAccount('0xAccount2', false), + createMockAccount('0xAccount2', false, KeyringTypes.simple), ]); rootMessenger.publish('KeyringController:unlock'); @@ -109,11 +112,39 @@ describe('ProfileMetricsController', () => { // enqueue — accounts are queued with proofs in mind from // the very first poll. expect(controller.state.proofBackfillEnqueued).toBe(true); + expect(controller.state.accountSourceBackfillEnqueued).toBe(true); expect(controller.state.syncQueue).toStrictEqual({ 'entropy-0xAccount1': [ { address: '0xAccount1', scopes: ['eip155:1'] }, ], - null: [{ address: '0xAccount2', scopes: ['eip155:1'] }], + null: [ + { + address: '0xAccount2', + scopes: ['eip155:1'], + source: 'imported', + }, + ], + }); + }, + ); + }); + + it('enqueues canonical addresses', async () => { + const checksummedAddress = + '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; + await withController( + async ({ controller, rootMessenger, registerAccounts }) => { + registerAccounts([ + createMockAccount(checksummedAddress.toLowerCase()), + ]); + + rootMessenger.publish('KeyringController:unlock'); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + [`entropy-${checksummedAddress.toLowerCase()}`]: [ + { address: checksummedAddress, scopes: ['eip155:1'] }, + ], }); }, ); @@ -134,6 +165,64 @@ describe('ProfileMetricsController', () => { }, ); }); + + it('does not re-enqueue accounts that were already reported', async () => { + await withController( + { + options: { + state: { reportedAccounts: ['0xAccount1'] }, + }, + }, + async ({ controller, rootMessenger, registerAccounts }) => { + registerAccounts([ + createMockAccount('0xAccount1'), + createMockAccount('0xAccount2'), + ]); + + rootMessenger.publish('KeyringController:unlock'); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + 'entropy-0xAccount2': [ + { address: '0xAccount2', scopes: ['eip155:1'] }, + ], + }); + }, + ); + }); + + it('groups all non-mnemonic accounts into a single batch, each tagged with its source', async () => { + await withController( + async ({ controller, rootMessenger, registerAccounts }) => { + registerAccounts([ + createMockAccount('0xHardware', false, KeyringTypes.ledger), + createMockAccount('0xImported', false, KeyringTypes.simple), + createMockAccount('0xSnap', false, KeyringTypes.snap), + createMockAccount('0xUnknown', false, 'Custom Keyring'), + ]); + + rootMessenger.publish('KeyringController:unlock'); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + null: [ + { + address: '0xHardware', + scopes: ['eip155:1'], + source: 'hardware', + }, + { + address: '0xImported', + scopes: ['eip155:1'], + source: 'imported', + }, + { address: '0xSnap', scopes: ['eip155:1'], source: 'snap' }, + { address: '0xUnknown', scopes: ['eip155:1'] }, + ], + }); + }, + ); + }); }); describe('when `initialEnqueueCompleted` is true', () => { @@ -147,6 +236,8 @@ describe('ProfileMetricsController', () => { state: { initialEnqueueCompleted: true, proofBackfillEnqueued: true, + accountSourceBackfillEnqueued: true, + reportedAccounts: ['0xAccount1', '0xAccount2'], }, }, }, @@ -260,7 +351,7 @@ describe('ProfileMetricsController', () => { }); }); - describe('when `proofBackfillEnqueued` is true', () => { + describe('when all backfills are complete', () => { it('does not re-enqueue accounts on subsequent unlocks', async () => { await withController( { @@ -269,6 +360,8 @@ describe('ProfileMetricsController', () => { state: { initialEnqueueCompleted: true, proofBackfillEnqueued: true, + accountSourceBackfillEnqueued: true, + reportedAccounts: ['0xAccount1'], }, }, }, @@ -371,6 +464,224 @@ describe('ProfileMetricsController', () => { }, ); }); + + it.each([ + { keyringType: KeyringTypes.qr, source: 'hardware' }, + { keyringType: KeyringTypes.trezor, source: 'hardware' }, + { keyringType: KeyringTypes.oneKey, source: 'hardware' }, + { keyringType: KeyringTypes.ledger, source: 'hardware' }, + { keyringType: KeyringTypes.lattice, source: 'hardware' }, + { keyringType: KeyringTypes.simple, source: 'imported' }, + { keyringType: KeyringTypes.snap, source: 'snap' }, + ] as const)( + 'adds the new `$keyringType` account to the sync queue under `null` tagged with the `$source` source', + async ({ keyringType, source }) => { + await withController( + { options: { assertUserOptedIn: () => assertUserOptedIn } }, + async ({ controller, rootMessenger }) => { + const newAccount = createMockAccount( + '0xNewAccount', + false, + keyringType, + ); + + rootMessenger.publish( + 'AccountsController:accountAdded', + newAccount, + ); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + null: [ + { + address: '0xNewAccount', + scopes: ['eip155:1'], + source, + }, + ], + }); + }, + ); + }, + ); + + it('does not tag a mnemonic-backed Snap account with a source', async () => { + await withController( + { options: { assertUserOptedIn: () => assertUserOptedIn } }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish( + 'AccountsController:accountAdded', + createMockAccount('0xNewAccount', true, KeyringTypes.snap), + ); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + 'entropy-0xNewAccount': [ + { address: '0xNewAccount', scopes: ['eip155:1'] }, + ], + }); + }, + ); + }); + + it('adds the new account to the sync queue with its canonical address', async () => { + const checksummedAddress = + '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; + await withController( + { options: { assertUserOptedIn: () => assertUserOptedIn } }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish( + 'AccountsController:accountAdded', + createMockAccount(checksummedAddress.toLowerCase(), false), + ); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + null: [{ address: checksummedAddress, scopes: ['eip155:1'] }], + }); + }, + ); + }); + + it('adds the new account to the sync queue as-is and logs when its address cannot be canonicalized', async () => { + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(); + await withController( + { options: { assertUserOptedIn: () => assertUserOptedIn } }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish('AccountsController:accountAdded', { + ...createMockAccount('0xNewAccount'), + scopes: [], + }); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + 'entropy-0xNewAccount': [ + { address: '0xNewAccount', scopes: [] }, + ], + }); + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Failed to canonicalize address for account id-0xNewAccount:', + new Error('Scope not found for account id-0xNewAccount'), + ); + }, + ); + }); + + it('adds the new account to the sync queue as-is without logging when its namespace is unsupported', async () => { + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(); + await withController( + { options: { assertUserOptedIn: () => assertUserOptedIn } }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish('AccountsController:accountAdded', { + ...createMockAccount('cosmos1abc'), + scopes: ['cosmos:cosmoshub-4'], + }); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + 'entropy-cosmos1abc': [ + { address: 'cosmos1abc', scopes: ['cosmos:cosmoshub-4'] }, + ], + }); + expect(consoleErrorSpy).not.toHaveBeenCalled(); + }, + ); + }); + + it('does not enqueue an account whose canonical address has already been reported', async () => { + const checksummedAddress = + '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; + await withController( + { + options: { + assertUserOptedIn: () => assertUserOptedIn, + state: { reportedAccounts: [checksummedAddress] }, + }, + }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish( + 'AccountsController:accountAdded', + createMockAccount(checksummedAddress.toLowerCase()), + ); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({}); + }, + ); + }); + + it('does not enqueue an account that is already queued', async () => { + await withController( + { + options: { + assertUserOptedIn: () => assertUserOptedIn, + state: { + syncQueue: { + existing: [ + { + address: '0xNewAccount', + scopes: ['eip155:1'], + }, + ], + }, + }, + }, + }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish( + 'AccountsController:accountAdded', + createMockAccount('0xNewAccount'), + ); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + existing: [ + { + address: '0xNewAccount', + scopes: ['eip155:1'], + }, + ], + }); + }, + ); + }); + + it('appends distinct accounts to an existing batch', async () => { + await withController( + { + options: { + assertUserOptedIn: () => assertUserOptedIn, + state: { + syncQueue: { + null: [{ address: '0xAccount1', scopes: ['eip155:1'] }], + }, + }, + }, + }, + async ({ controller, rootMessenger }) => { + rootMessenger.publish( + 'AccountsController:accountAdded', + createMockAccount('0xAccount2', false, KeyringTypes.simple), + ); + await Promise.resolve(); + + expect(controller.state.syncQueue).toStrictEqual({ + null: [ + { address: '0xAccount1', scopes: ['eip155:1'] }, + { + address: '0xAccount2', + scopes: ['eip155:1'], + source: 'imported', + }, + ], + }); + }, + ); + }); }, ); }); @@ -561,6 +872,24 @@ describe('ProfileMetricsController', () => { }); describe('when the initial delay period has ended', () => { + it('does not read accounts or submit anything when the sync queue is empty', async () => { + await withController( + { + options: { + state: { initialDelayEndTimestamp: 0 }, + }, + }, + async ({ controller, messenger, mockSubmitMetrics }) => { + const callSpy = jest.spyOn(messenger, 'call'); + + await controller._executePoll(); + + expect(callSpy).not.toHaveBeenCalled(); + expect(mockSubmitMetrics).not.toHaveBeenCalled(); + }, + ); + }); + it('processes the sync queue on each poll', async () => { const accounts: Record = { id1: [{ address: '0xAccount1', scopes: ['eip155:1'] }], @@ -660,8 +989,11 @@ describe('ProfileMetricsController', () => { expect(controller.state.syncQueue).toStrictEqual({ id1: [{ address: '0xAccount1', scopes: ['eip155:1'] }], }); + expect(controller.state.reportedAccounts).toStrictEqual([ + '0xAccount2', + ]); expect(consoleErrorSpy).toHaveBeenCalledWith( - 'Failed to submit profile metrics for entropy source ID id1:', + 'Failed to submit profile metrics for sync queue key id1:', expect.any(Error), ); }, @@ -669,12 +1001,12 @@ describe('ProfileMetricsController', () => { }); describe('proof of ownership wiring', () => { - it('fetches nonces and signs proofs for queued accounts, submitting canonical addresses', async () => { + it('fetches nonces and signs proofs for queued accounts, matching them against live accounts by canonical address', async () => { const checksummedAddress = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const lowercased = checksummedAddress.toLowerCase(); const accounts: Record = { - id1: [{ address: lowercased, scopes: ['eip155:1'] }], + id1: [{ address: checksummedAddress, scopes: ['eip155:1'] }], }; await withController( { @@ -725,10 +1057,20 @@ describe('ProfileMetricsController', () => { ); }); - it('skips proof-of-ownership entirely for accounts with no entropy source', async () => { + it('submits the non-mnemonic batch without an entropy source ID or proofs and records its accounts as reported', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const accounts: Record = { - null: [{ address: address.toLowerCase(), scopes: ['eip155:1'] }], + null: [ + { address, scopes: ['eip155:1'], source: 'hardware' }, + { + address: '0xImported', + scopes: ['eip155:1'], + source: 'imported', + }, + { address: '0xSnap', scopes: ['eip155:1'], source: 'snap' }, + // Persisted by a version that did not tag sources. + { address: '0xLegacy', scopes: ['eip155:1'] }, + ], }; await withController( { @@ -745,7 +1087,7 @@ describe('ProfileMetricsController', () => { registerAccounts, }) => { registerAccounts([ - createMockAccount(address.toLowerCase(), false), + createMockAccount(address, false, KeyringTypes.ledger), ]); await controller._executePoll(); @@ -755,22 +1097,49 @@ describe('ProfileMetricsController', () => { expect(mockSubmitMetrics).toHaveBeenCalledWith({ metametricsId: getMetaMetricsId(), entropySourceId: null, - accounts: [ - { address: address.toLowerCase(), scopes: ['eip155:1'] }, - ], + accounts: accounts.null, }); expect(controller.state.syncQueue).toStrictEqual({}); + expect(controller.state.reportedAccounts).toStrictEqual([ + address, + '0xImported', + '0xSnap', + '0xLegacy', + ]); + }, + ); + }); + + it('does not record the same reported address twice', async () => { + await withController( + { + options: { + state: { + syncQueue: { + id1: [{ address: '0xAccount1', scopes: ['eip155:1'] }], + }, + reportedAccounts: ['0xAccount1'], + initialDelayEndTimestamp: 0, + }, + }, + }, + async ({ controller }) => { + await controller._executePoll(); + + expect(controller.state.reportedAccounts).toStrictEqual([ + '0xAccount1', + ]); }, ); }); - it('canonicalizes mixed-case bech32 Bitcoin addresses to lowercase before fetching the nonce', async () => { + it('matches queued bech32 Bitcoin addresses against the lowercased live account address before fetching the nonce', async () => { const mixedCase = 'BC1QAR0SRRR7XFKVY5L643LYDNW9RE59GTZZWF5MDQ'; const canonical = mixedCase.toLowerCase(); const accounts: Record = { id1: [ { - address: mixedCase, + address: canonical, scopes: ['bip122:000000000019d6689c085ae165831e93'], }, ], @@ -816,13 +1185,13 @@ describe('ProfileMetricsController', () => { ); }); - it('de-duplicates identifiers when the same canonical address is queued twice', async () => { + it('de-duplicates identifiers when the same address is queued twice', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const lowercased = address.toLowerCase(); const accounts: Record = { id1: [ - { address: lowercased, scopes: ['eip155:1'] }, - { address: lowercased, scopes: ['eip155:1'] }, + { address, scopes: ['eip155:1'] }, + { address, scopes: ['eip155:1'] }, ], }; await withController( @@ -944,7 +1313,7 @@ describe('ProfileMetricsController', () => { }), ); expect(consoleErrorSpy).toHaveBeenCalledWith( - `Skipping proof for account id-${address}:`, + `Failed to canonicalize address for account id-${address}:`, expect.any(Error), ); }, @@ -989,10 +1358,10 @@ describe('ProfileMetricsController', () => { ); }); - it('submits canonicalized accounts without proofs and logs when fetchNonces rejects', async () => { + it('submits accounts without proofs and logs when fetchNonces rejects', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const accounts: Record = { - id1: [{ address: address.toLowerCase(), scopes: ['eip155:1'] }], + id1: [{ address, scopes: ['eip155:1'] }], }; await withController( { @@ -1037,7 +1406,7 @@ describe('ProfileMetricsController', () => { it('submits the account without proof when the nonce response omits its identifier', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const accounts: Record = { - id1: [{ address: address.toLowerCase(), scopes: ['eip155:1'] }], + id1: [{ address, scopes: ['eip155:1'] }], }; await withController( { @@ -1074,8 +1443,8 @@ describe('ProfileMetricsController', () => { const badLower = badAddress.toLowerCase(); const accounts: Record = { id1: [ - { address: goodLower, scopes: ['eip155:1'] }, - { address: badLower, scopes: ['eip155:1'] }, + { address: goodAddress, scopes: ['eip155:1'] }, + { address: badAddress, scopes: ['eip155:1'] }, ], }; await withController( @@ -1134,7 +1503,7 @@ describe('ProfileMetricsController', () => { it('keeps the batch in the queue when submitMetrics fails after proofs have been signed', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const accounts: Record = { - id1: [{ address: address.toLowerCase(), scopes: ['eip155:1'] }], + id1: [{ address, scopes: ['eip155:1'] }], }; await withController( { @@ -1169,8 +1538,8 @@ describe('ProfileMetricsController', () => { const address1 = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const address2 = '0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359'; const accounts: Record = { - id1: [{ address: address1.toLowerCase(), scopes: ['eip155:1'] }], - id2: [{ address: address2.toLowerCase(), scopes: ['eip155:1'] }], + id1: [{ address: address1, scopes: ['eip155:1'] }], + id2: [{ address: address2, scopes: ['eip155:1'] }], }; await withController( { @@ -1216,7 +1585,7 @@ describe('ProfileMetricsController', () => { const cosmosAddress = 'cosmos1abc'; const accounts: Record = { id1: [ - { address: evmAddress.toLowerCase(), scopes: ['eip155:1'] }, + { address: evmAddress, scopes: ['eip155:1'] }, { address: cosmosAddress, scopes: ['cosmos:cosmoshub-4'] }, ], }; @@ -1275,7 +1644,7 @@ describe('ProfileMetricsController', () => { it('submits the account as-is when its live scopes list is empty', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const accounts: Record = { - id1: [{ address: address.toLowerCase(), scopes: ['eip155:1'] }], + id1: [{ address, scopes: ['eip155:1'] }], }; await withController( { @@ -1296,7 +1665,7 @@ describe('ProfileMetricsController', () => { .mockImplementation(); registerAccounts([ { - ...createMockAccount(address.toLowerCase()), + ...createMockAccount(address), scopes: [], }, ]); @@ -1308,17 +1677,15 @@ describe('ProfileMetricsController', () => { expect.objectContaining({ accounts: [ { - address: address.toLowerCase(), + address, scopes: ['eip155:1'], }, ], }), ); expect(consoleErrorSpy).toHaveBeenCalledWith( - `Skipping proof for account id-${address.toLowerCase()}:`, - new Error( - `Scope not found for account id-${address.toLowerCase()}`, - ), + `Failed to canonicalize address for account id-${address}:`, + new Error(`Scope not found for account id-${address}`), ); }, ); @@ -1376,6 +1743,7 @@ describe('ProfileMetricsController', () => { ), ).toMatchInlineSnapshot(` { + "accountSourceBackfillEnqueued": false, "initialDelayEndTimestamp": 10, "initialEnqueueCompleted": false, "proofBackfillEnqueued": false, @@ -1397,9 +1765,11 @@ describe('ProfileMetricsController', () => { ), ).toMatchInlineSnapshot(` { + "accountSourceBackfillEnqueued": false, "initialDelayEndTimestamp": 10, "initialEnqueueCompleted": false, "proofBackfillEnqueued": false, + "reportedAccounts": [], "syncQueue": {}, } `); @@ -1419,9 +1789,11 @@ describe('ProfileMetricsController', () => { ), ).toMatchInlineSnapshot(` { + "accountSourceBackfillEnqueued": false, "initialDelayEndTimestamp": 10, "initialEnqueueCompleted": false, "proofBackfillEnqueued": false, + "reportedAccounts": [], "syncQueue": {}, } `); @@ -1590,7 +1962,6 @@ async function withController( ]), ), })); - const messenger = getMessenger(rootMessenger); const controller = new ProfileMetricsController({ messenger, diff --git a/packages/profile-metrics-controller/src/ProfileMetricsController.ts b/packages/profile-metrics-controller/src/ProfileMetricsController.ts index d459af4169e..546c767a418 100644 --- a/packages/profile-metrics-controller/src/ProfileMetricsController.ts +++ b/packages/profile-metrics-controller/src/ProfileMetricsController.ts @@ -12,6 +12,7 @@ import type { KeyringControllerLockEvent, KeyringControllerUnlockEvent, } from '@metamask/keyring-controller'; +import { KeyringTypes } from '@metamask/keyring-controller'; import type { InternalAccount } from '@metamask/keyring-internal-api'; import type { Messenger } from '@metamask/messenger'; import { StaticIntervalPollingController } from '@metamask/polling-controller'; @@ -22,7 +23,7 @@ import { Mutex } from 'async-mutex'; import type { ProfileMetricsControllerMethodActions } from './ProfileMetricsController-method-action-types.js'; import type { ProfileMetricsServiceMethodActions } from './ProfileMetricsService-method-action-types.js'; import type { - AccountOwnershipProof, + AccountSource, AccountWithScopes, } from './ProfileMetricsService.js'; import type { ProofOfOwnershipServiceMethodActions } from './ProofOfOwnershipService-method-action-types.js'; @@ -46,6 +47,15 @@ export const DEFAULT_INITIAL_DELAY_DURATION = inMilliseconds( Duration.Minute, ); +/** + * The sync queue key for accounts that are not backed by a mnemonic (hardware, + * imported private key, and non-mnemonic Snap accounts). These are submitted + * without an entropy source ID, which the AuthenticationController resolves + * to the primary SRP. The value is `String(null)` so that queues persisted by + * earlier versions keep draining. + */ +const NON_MNEMONIC_QUEUE_KEY = 'null'; + /** * Describes the shape of the state object for {@link ProfileMetricsController}. */ @@ -56,10 +66,10 @@ export type ProfileMetricsControllerState = { */ initialEnqueueCompleted: boolean; /** - * The queue of accounts to be synced. + * The queue of accounts to be synced, with canonical addresses. * Each key is an entropy source ID, and each value is an array of account * addresses associated with that entropy source. Accounts with no entropy - * source ID are grouped under the key "null". + * source ID are grouped under {@link NON_MNEMONIC_QUEUE_KEY}. */ syncQueue: Record; /** @@ -74,6 +84,18 @@ export type ProfileMetricsControllerState = { * first poll already attaches proofs. */ proofBackfillEnqueued: boolean; + /** + * Whether known accounts have been re-enqueued once so that they are + * reported with canonical addresses and, for non-mnemonic accounts, an + * account source. Set on the first unlock after upgrading; fresh installs + * flip this on their initial sync. + */ + accountSourceBackfillEnqueued: boolean; + /** + * Canonical addresses that have already been reported. Enqueuing one of + * these again is a no-op. + */ + reportedAccounts: string[]; }; /** @@ -104,6 +126,18 @@ const profileMetricsControllerMetadata = { includeInStateLogs: true, usedInUi: false, }, + accountSourceBackfillEnqueued: { + persist: true, + includeInDebugSnapshot: true, + includeInStateLogs: true, + usedInUi: false, + }, + reportedAccounts: { + persist: true, + includeInDebugSnapshot: false, + includeInStateLogs: true, + usedInUi: false, + }, } satisfies StateMetadata; /** @@ -119,6 +153,8 @@ export function getDefaultProfileMetricsControllerState(): ProfileMetricsControl initialEnqueueCompleted: false, syncQueue: {}, proofBackfillEnqueued: false, + accountSourceBackfillEnqueued: false, + reportedAccounts: [], }; } @@ -297,10 +333,14 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< /** * Execute a single poll to sync user profile data. * - * The queued accounts are sent to the ProfileMetricsService, each with - * a proof of ownership when one can be produced (see {@link #attachProofs}), - * and the sync queue is cleared. This operation is mutexed to prevent - * concurrent executions. + * Each batch of queued accounts is sent to the ProfileMetricsService and + * then dropped from the sync queue, with its addresses recorded as + * reported. Mnemonic batches are attributed to their own entropy source + * and carry proofs of ownership when one can be produced (see + * {@link #attachProofs}); the non-mnemonic batch is submitted without an + * entropy source ID (resolved to the primary SRP downstream) and never + * carries proofs. This operation is mutexed to prevent concurrent + * executions. * * @returns A promise that resolves when the poll is complete. */ @@ -313,34 +353,40 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< if (!this.#isInitialDelayComplete()) { return; } - const fullAccountsByAddress = this.#getFullAccountsByAddress(); - for (const [entropySourceId, accounts] of Object.entries( - this.state.syncQueue, - )) { - const normalizedEntropySourceId = - entropySourceId === 'null' ? null : entropySourceId; - // Skip proof-of-ownership for accounts without an entropy source - const accountsWithProofs = - normalizedEntropySourceId === null - ? accounts - : await this.#attachProofs( - accounts, - fullAccountsByAddress, - normalizedEntropySourceId, - ); + const batches = Object.entries(this.state.syncQueue); + if (batches.length === 0) { + return; + } + const proofCandidates = this.#getProofCandidatesByAddress(); + for (const [queueKey, accounts] of batches) { + const entropySourceId = + queueKey === NON_MNEMONIC_QUEUE_KEY ? null : queueKey; try { + const accountsToSubmit = + entropySourceId === null + ? accounts + : await this.#attachProofs( + accounts, + proofCandidates, + entropySourceId, + ); await this.messenger.call('ProfileMetricsService:submitMetrics', { metametricsId: this.#getMetaMetricsId(), - entropySourceId: normalizedEntropySourceId, - accounts: accountsWithProofs, + entropySourceId, + accounts: accountsToSubmit, }); this.update((state) => { - delete state.syncQueue[entropySourceId]; + for (const { address } of accountsToSubmit) { + if (!state.reportedAccounts.includes(address)) { + state.reportedAccounts.push(address); + } + } + delete state.syncQueue[queueKey]; }); } catch (error) { // We want to log the error but continue processing other batches. console.error( - `Failed to submit profile metrics for entropy source ID ${entropySourceId}:`, + `Failed to submit profile metrics for sync queue key ${queueKey}:`, error, ); } @@ -350,60 +396,30 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< /** * Attach a proof of ownership to each account in a single entropy-source - * batch when possible, canonicalizing the address along the way. + * batch when possible. * - * Per-account failures (unknown namespace, snap missing the - * `signProofOfOwnership` method, snap rejection) and whole-batch nonce - * failures are caught and downgraded to "submit without a proof" so the - * batch still goes through and the proof is retried on the next poll. + * Per-account failures (snap missing the `signProofOfOwnership` method, + * snap rejection) and whole-batch nonce failures are caught and + * downgraded to "submit without a proof" so the batch still goes through + * and the proof is retried on the next poll. * * @param accounts - The queued accounts for a single batch. - * @param fullAccountsByAddress - Live `InternalAccount` lookup keyed by address. - * @param entropySourceId - The entropy source ID for this batch. Callers - * are expected to short-circuit before invoking this method when the - * batch has no entropy source; see `_executePoll` for why. + * @param proofCandidates - Live accounts that can produce a proof, keyed by + * canonical address (see {@link #getProofCandidatesByAddress}). + * @param entropySourceId - The entropy source ID for this batch. * @returns The accounts with `proof` populated where signing succeeded. */ async #attachProofs( accounts: AccountWithScopes[], - fullAccountsByAddress: Map, + proofCandidates: Map, entropySourceId: string, ): Promise { - const candidates = new Map< - string, - { account: InternalAccount; canonicalAddress: string } - >(); - const identifiers = new Set(); - for (const queued of accounts) { - const fullAccount = fullAccountsByAddress.get(queued.address); - if (!fullAccount) { - continue; - } - try { - const [scope] = fullAccount.scopes; - if (!scope) { - throw new Error(`Scope not found for account ${fullAccount.id}`); - } - const { namespace } = parseCaipChainId(scope); - const canonicalAddress = canonicalizeAddress( - fullAccount.address, - namespace, - ); - candidates.set(queued.address, { - account: fullAccount, - canonicalAddress, - }); - identifiers.add(canonicalAddress); - } catch (error) { - // Unsupported namespaces are an expected pass-through; anything - // else is logged so a new namespace doesn't go unnoticed. - if (!(error instanceof ProofUnsupportedNamespaceError)) { - console.error(`Skipping proof for account ${fullAccount.id}:`, error); - } - } - } - - if (candidates.size === 0) { + const identifiers = new Set( + accounts + .map(({ address }) => address) + .filter((address) => proofCandidates.has(address)), + ); + if (identifiers.size === 0) { return accounts; } @@ -422,61 +438,59 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< return await Promise.all( accounts.map(async (queued): Promise => { - const candidate = candidates.get(queued.address); - if (!candidate) { + const account = proofCandidates.get(queued.address); + const nonce = nonces[queued.address]; + if (!account || !nonce) { return queued; } - const nonce = nonces[candidate.canonicalAddress]; - if (!nonce) { - return { ...queued, address: candidate.canonicalAddress }; - } - let proof: AccountOwnershipProof; try { - proof = await this.messenger.call('ProofOfOwnershipService:sign', { - account: candidate.account, - nonce, - }); + const proof = await this.messenger.call( + 'ProofOfOwnershipService:sign', + { account, nonce }, + ); + return { ...queued, proof }; } catch (error) { console.error( - `Failed to sign proof of ownership for account ${candidate.account.id}:`, + `Failed to sign proof of ownership for account ${account.id}:`, error, ); - return { ...queued, address: candidate.canonicalAddress }; + return queued; } - return { - address: candidate.canonicalAddress, - scopes: queued.scopes, - proof, - }; }), ); } /** - * Snapshot the live `InternalAccount` map keyed by address for the - * current poll. + * Snapshot the live accounts keyed by canonical address, so they can be + * matched against queued accounts when signing proofs of ownership. + * Accounts whose namespace has no canonical form cannot produce a proof + * and are left out. * - * @returns A map of address → `InternalAccount`. + * @returns A map of canonical address → `InternalAccount`. */ - #getFullAccountsByAddress(): Map { - const byAddress = new Map(); - const accountsState = this.messenger.call('AccountsController:getState'); - for (const account of Object.values( - accountsState.internalAccounts.accounts, - )) { - byAddress.set(account.address, account); + #getProofCandidatesByAddress(): Map { + const candidates = new Map(); + const { accounts } = this.messenger.call( + 'AccountsController:getState', + ).internalAccounts; + for (const account of Object.values(accounts)) { + const canonicalAddress = getCanonicalAddress(account); + if (canonicalAddress) { + candidates.set(canonicalAddress, account); + } } - return byAddress; + return candidates; } /** * Enqueue all currently-known accounts onto the sync queue if needed. - * Single entry point covering both the fresh-install first sync and - * the one-time proof-of-ownership backfill for users upgrading. + * Single entry point covering the fresh-install first sync and the + * one-time backfills for users upgrading (proofs of ownership, then + * canonical addresses and account sources). * * Bails for opted-out users (the poll wouldn't drain the queue - * anyway), and bails once both bootstrap steps have already run. - * Otherwise enqueues all known accounts and flips both flags so this + * anyway), and bails once every bootstrap step has already run. + * Otherwise enqueues all known accounts and flips every flag so this * becomes a permanent no-op for the lifetime of the install. */ async #enqueueAccountsIfNeeded(): Promise { @@ -486,28 +500,26 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< } if ( this.state.initialEnqueueCompleted && - this.state.proofBackfillEnqueued + this.state.proofBackfillEnqueued && + this.state.accountSourceBackfillEnqueued ) { return; } - const groupedAccounts = groupAccountsByEntropySourceId( - Object.values( - this.messenger.call('AccountsController:getState').internalAccounts - .accounts, - ), - ); + const { accounts } = this.messenger.call( + 'AccountsController:getState', + ).internalAccounts; this.update((state) => { // Replace the queue rather than append. `AccountsController` is - // the source of truth and the queue is otherwise kept in sync - // with it via the `accountAdded` / `accountRemoved` subscriptions, - // so assigning here avoids duplicating entries that survived from - // a prior session or were pushed earlier in this same unlock - // cycle. Duplicates would matter because nonces are single-use: - // letting one through causes `#attachProofs` to sign and submit - // twice with the same nonce. - state.syncQueue = groupedAccounts; + // the source of truth, so this drops stale entries that survived + // from a prior session and re-derives each account's canonical + // address and source. + state.syncQueue = {}; + for (const account of Object.values(accounts)) { + enqueueAccount(state, account); + } state.initialEnqueueCompleted = true; state.proofBackfillEnqueued = true; + state.accountSourceBackfillEnqueued = true; }); }); } @@ -535,21 +547,15 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< } /** - * Queue the given account to be synced at the next poll. + * Queue the given account to be synced at the next poll, unless it has + * already been reported or queued. * * @param account - The account to sync. */ async #addAccountToQueue(account: InternalAccount): Promise { await this.#mutex.runExclusive(async () => { this.update((state) => { - const entropySourceId = getAccountEntropySourceId(account) ?? 'null'; - if (!state.syncQueue[entropySourceId]) { - state.syncQueue[entropySourceId] = []; - } - state.syncQueue[entropySourceId].push({ - address: account.address, - scopes: account.scopes, - }); + enqueueAccount(state, account); }); }); } @@ -596,25 +602,103 @@ function getAccountEntropySourceId(account: InternalAccount): string | null { } /** - * Groups accounts by their entropy source ID. + * Derive the reporting source of an account that is not backed by a mnemonic, + * from its keyring type. * - * @param accounts - The accounts to group. - * @returns An object where each key is an entropy source ID and each value is - * an array of account addresses associated with that entropy source ID. + * @param account - The account to classify. + * @returns The account source, or undefined for mnemonic-backed accounts + * (which are attributed to their entropy source instead) and for + * unrecognized keyring types. */ -function groupAccountsByEntropySourceId( - accounts: InternalAccount[], -): Record { - return accounts.reduce( - (result: Record, account) => { - const entropySourceId = getAccountEntropySourceId(account); - const key = entropySourceId ?? 'null'; - if (!result[key]) { - result[key] = []; - } - result[key].push({ address: account.address, scopes: account.scopes }); - return result; - }, - {}, - ); +function getAccountSource(account: InternalAccount): AccountSource | undefined { + if (getAccountEntropySourceId(account) !== null) { + return undefined; + } + switch (account.metadata.keyring.type) { + case KeyringTypes.simple: + return 'imported'; + case KeyringTypes.snap: + return 'snap'; + case KeyringTypes.qr: + case KeyringTypes.trezor: + case KeyringTypes.oneKey: + case KeyringTypes.ledger: + case KeyringTypes.lattice: + return 'hardware'; + default: + return undefined; + } +} + +/** + * Retrieves the canonical address of the given account, as expected by the + * auth API (see {@link canonicalizeAddress}). + * + * @param account - The account whose address to canonicalize. + * @returns The canonical address, or undefined when the account's namespace + * has no canonical form (in which case it cannot produce a proof of + * ownership either). + */ +function getCanonicalAddress(account: InternalAccount): string | undefined { + try { + const [scope] = account.scopes; + if (!scope) { + throw new Error(`Scope not found for account ${account.id}`); + } + return canonicalizeAddress( + account.address, + parseCaipChainId(scope).namespace, + ); + } catch (error) { + // Unsupported namespaces are an expected pass-through; anything + // else is logged so a new namespace doesn't go unnoticed. + if (!(error instanceof ProofUnsupportedNamespaceError)) { + console.error( + `Failed to canonicalize address for account ${account.id}:`, + error, + ); + } + return undefined; + } +} + +/** + * Convert an internal account to the payload stored in the sync queue and + * submitted to the ProfileMetricsService. + * + * @param account - The internal account. + * @returns The queued account, with a canonical address and, for + * non-mnemonic accounts, a source. + */ +function toQueuedAccount(account: InternalAccount): AccountWithScopes { + const source = getAccountSource(account); + return { + address: getCanonicalAddress(account) ?? account.address, + scopes: account.scopes, + ...(source ? { source } : {}), + }; +} + +/** + * Push the given account onto the sync queue held in `state`, unless its + * canonical address has already been reported or is already queued. + * + * @param state - The controller state to mutate. + * @param account - The account to enqueue. + */ +function enqueueAccount( + state: ProfileMetricsControllerState, + account: InternalAccount, +): void { + const queuedAccount = toQueuedAccount(account); + const isKnown = + state.reportedAccounts.includes(queuedAccount.address) || + Object.values(state.syncQueue).some((batch) => + batch.some(({ address }) => address === queuedAccount.address), + ); + if (isKnown) { + return; + } + const queueKey = getAccountEntropySourceId(account) ?? NON_MNEMONIC_QUEUE_KEY; + (state.syncQueue[queueKey] ??= []).push(queuedAccount); } diff --git a/packages/profile-metrics-controller/src/ProfileMetricsService.test.ts b/packages/profile-metrics-controller/src/ProfileMetricsService.test.ts index 26fa1b6d566..41bfd79868e 100644 --- a/packages/profile-metrics-controller/src/ProfileMetricsService.test.ts +++ b/packages/profile-metrics-controller/src/ProfileMetricsService.test.ts @@ -369,7 +369,7 @@ describe('ProfileMetricsService', () => { expect(submitMetricsResponse).toBeUndefined(); }); - it('serializes the optional proof field for each account that has one and omits it for those that do not', async () => { + it('serializes the optional proof and source fields for each account that has them and omits them for those that do not', async () => { const mockFetch = jest.fn().mockResolvedValue( // eslint-disable-next-line no-restricted-globals new Response(JSON.stringify({ data: { success: true } }), { @@ -389,7 +389,11 @@ describe('ProfileMetricsService', () => { createMockRequest({ accounts: [ { address: '0xAccountWithProof', scopes: ['eip155:1'], proof }, - { address: '0xAccountWithoutProof', scopes: ['eip155:1'] }, + { + address: '0xImportedAccount', + scopes: ['eip155:1'], + source: 'imported', + }, ], }), ); @@ -397,8 +401,13 @@ describe('ProfileMetricsService', () => { const body = JSON.parse(mockFetch.mock.calls[0][1].body); expect(body.accounts).toStrictEqual([ { address: '0xAccountWithProof', scopes: ['eip155:1'], proof }, - { address: '0xAccountWithoutProof', scopes: ['eip155:1'] }, + { + address: '0xImportedAccount', + scopes: ['eip155:1'], + source: 'imported', + }, ]); + expect(body.accounts[0]).not.toHaveProperty('source'); expect(body.accounts[1]).not.toHaveProperty('proof'); }); }); diff --git a/packages/profile-metrics-controller/src/ProfileMetricsService.ts b/packages/profile-metrics-controller/src/ProfileMetricsService.ts index 6e8169d2ccf..4ec47da5b63 100644 --- a/packages/profile-metrics-controller/src/ProfileMetricsService.ts +++ b/packages/profile-metrics-controller/src/ProfileMetricsService.ts @@ -63,12 +63,19 @@ export type AccountOwnershipProof = { }; /** - * An account address along with its associated scopes and an optional - * ownership proof. + * The source of an account that is not derived from an SRP. + */ +export type AccountSource = 'hardware' | 'imported' | 'snap'; + +/** + * An account address along with its associated scopes, an optional source + * for accounts that are not derived from an SRP, and an optional ownership + * proof. */ export type AccountWithScopes = { address: string; scopes: `${string}:${string}`[]; + source?: AccountSource; proof?: AccountOwnershipProof; }; From 13f68d51a4eab158b7817cffcbd7074ddf0b1886 Mon Sep 17 00:00:00 2001 From: Mathieu Artu Date: Fri, 4 Sep 2026 11:34:27 +0200 Subject: [PATCH 2/2] fix: update CHANGELOG --- packages/profile-metrics-controller/CHANGELOG.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/profile-metrics-controller/CHANGELOG.md b/packages/profile-metrics-controller/CHANGELOG.md index 5a3f0288204..4aab67632f1 100644 --- a/packages/profile-metrics-controller/CHANGELOG.md +++ b/packages/profile-metrics-controller/CHANGELOG.md @@ -9,14 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added -- Tag hardware, imported-private-key, and non-mnemonic Snap accounts with a `source` on `AccountWithScopes` when submitting profile metrics +- Tag hardware, imported-private-key, and non-mnemonic Snap accounts with a `source` on `AccountWithScopes` when submitting profile metrics ([#10107](https://github.com/MetaMask/core/pull/10107)) - The value (`'hardware' | 'imported' | 'snap'`) is derived from `metadata.keyring.type`; mnemonic-backed accounts carry no source. - Adds a `reportedAccounts` state property recording the canonical addresses that have been submitted, so enqueuing them again is a no-op. - Re-enqueues all known accounts on the first unlock after upgrading so previously-synced records get a source and canonical address, gated by a new `accountSourceBackfillEnqueued` state flag (fresh installs flip the flag on their initial sync). ### Changed -- Canonicalize account addresses when they are enqueued rather than only when a proof of ownership is signed, so accounts without a proof (hardware, imported, non-mnemonic Snap) are also submitted in canonical form +- Canonicalize account addresses when they are enqueued rather than only when a proof of ownership is signed, so accounts without a proof (hardware, imported, non-mnemonic Snap) are also submitted in canonical form ([#10107](https://github.com/MetaMask/core/pull/10107)) - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.8.0` ([#9780](https://github.com/MetaMask/core/pull/9780), [#10080](https://github.com/MetaMask/core/pull/10080), [#9798](https://github.com/MetaMask/core/pull/9798), [#9823](https://github.com/MetaMask/core/pull/9823), [#9960](https://github.com/MetaMask/core/pull/9960), [#9969](https://github.com/MetaMask/core/pull/9969), [#10046](https://github.com/MetaMask/core/pull/10046)) - Bump `@metamask/superstruct` from `^3.1.0` to `^3.4.1` ([#9754](https://github.com/MetaMask/core/pull/9754)) - Bump `@metamask/accounts-controller` from `^39.0.6` to `^39.1.1` ([#9791](https://github.com/MetaMask/core/pull/9791), [#9807](https://github.com/MetaMask/core/pull/9807), [#9969](https://github.com/MetaMask/core/pull/9969))