diff --git a/packages/profile-metrics-controller/CHANGELOG.md b/packages/profile-metrics-controller/CHANGELOG.md index 5e625908749..221d4af8a48 100644 --- a/packages/profile-metrics-controller/CHANGELOG.md +++ b/packages/profile-metrics-controller/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- **BREAKING:** Add batch proof-of-ownership signing for wallet snap accounts during profile metrics sync. ([#10142](https://github.com/MetaMask/core/pull/10142)) + ## [5.1.0] ### Added diff --git a/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts b/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts index 28dcdbb97e0..e56a259cc6f 100644 --- a/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts +++ b/packages/profile-metrics-controller/src/ProfileMetricsController.test.ts @@ -19,7 +19,11 @@ import type { ProfileMetricsFetchNoncesRequest, ProfileMetricsSubmitMetricsRequest, } from './ProfileMetricsService.js'; -import type { ProofOfOwnershipSignRequest } from './ProofOfOwnershipService.js'; +import type { + ProofOfOwnershipSignBatchRequest, + ProofOfOwnershipSignBatchResponse, + ProofOfOwnershipSignRequest, +} from './ProofOfOwnershipService.js'; import { ProofUnsupportedNamespaceError } from './utils/canonicalize.js'; /** @@ -1089,16 +1093,22 @@ describe('ProfileMetricsController', () => { getMetaMetricsId, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([createMockAccount(lowercased)]); mockFetchNonces.mockResolvedValueOnce({ [checksummedAddress]: 'nonce-1', }); - mockSignProof.mockResolvedValueOnce({ - nonce: 'nonce-1', - signature: '0xdeadbeef', + mockSignProofBatch.mockResolvedValueOnce({ + results: [ + { + proof: { + nonce: 'nonce-1', + signature: '0xdeadbeef', + }, + }, + ], }); await controller._executePoll(); @@ -1107,9 +1117,13 @@ describe('ProfileMetricsController', () => { identifiers: [checksummedAddress], entropySourceId: 'id1', }); - expect(mockSignProof).toHaveBeenCalledWith({ - account: expect.objectContaining({ address: lowercased }), - nonce: 'nonce-1', + expect(mockSignProofBatch).toHaveBeenCalledWith({ + items: [ + { + account: expect.objectContaining({ address: lowercased }), + nonce: 'nonce-1', + }, + ], }); expect(mockSubmitMetrics).toHaveBeenCalledWith({ metametricsId: getMetaMetricsId(), @@ -1157,7 +1171,7 @@ describe('ProfileMetricsController', () => { getMetaMetricsId, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([ @@ -1167,7 +1181,7 @@ describe('ProfileMetricsController', () => { await controller._executePoll(); expect(mockFetchNonces).not.toHaveBeenCalled(); - expect(mockSignProof).not.toHaveBeenCalled(); + expect(mockSignProofBatch).not.toHaveBeenCalled(); expect(mockSubmitMetrics).toHaveBeenCalledWith({ metametricsId: getMetaMetricsId(), entropySourceId: null, @@ -1228,7 +1242,7 @@ describe('ProfileMetricsController', () => { controller, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { const btcAccount: InternalAccount = { @@ -1237,9 +1251,15 @@ describe('ProfileMetricsController', () => { }; registerAccounts([btcAccount]); mockFetchNonces.mockResolvedValueOnce({ [canonical]: 'n-btc' }); - mockSignProof.mockResolvedValueOnce({ - nonce: 'n-btc', - signature: '0xbtcsig', + mockSignProofBatch.mockResolvedValueOnce({ + results: [ + { + proof: { + nonce: 'n-btc', + signature: '0xbtcsig', + }, + }, + ], }); await controller._executePoll(); @@ -1277,14 +1297,16 @@ describe('ProfileMetricsController', () => { async ({ controller, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([createMockAccount(lowercased)]); mockFetchNonces.mockResolvedValueOnce({ [address]: 'n' }); - mockSignProof.mockResolvedValue({ - nonce: 'n', - signature: '0xsig', + mockSignProofBatch.mockResolvedValue({ + results: [ + { proof: { nonce: 'n', signature: '0xsig' } }, + { proof: { nonce: 'n', signature: '0xsig' } }, + ], }); await controller._executePoll(); @@ -1313,7 +1335,7 @@ describe('ProfileMetricsController', () => { getMetaMetricsId, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([ @@ -1326,7 +1348,7 @@ describe('ProfileMetricsController', () => { await controller._executePoll(); expect(mockFetchNonces).not.toHaveBeenCalled(); - expect(mockSignProof).not.toHaveBeenCalled(); + expect(mockSignProofBatch).not.toHaveBeenCalled(); expect(mockSubmitMetrics).toHaveBeenCalledWith({ metametricsId: getMetaMetricsId(), entropySourceId: 'id1', @@ -1414,7 +1436,7 @@ describe('ProfileMetricsController', () => { getMetaMetricsId, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, }) => { // AccountsController is intentionally empty: the account // was removed between enqueue and poll. @@ -1422,7 +1444,7 @@ describe('ProfileMetricsController', () => { await controller._executePoll(); expect(mockFetchNonces).not.toHaveBeenCalled(); - expect(mockSignProof).not.toHaveBeenCalled(); + expect(mockSignProofBatch).not.toHaveBeenCalled(); expect(mockSubmitMetrics).toHaveBeenCalledWith({ metametricsId: getMetaMetricsId(), entropySourceId: 'id1', @@ -1447,7 +1469,7 @@ describe('ProfileMetricsController', () => { controller, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { const consoleErrorSpy = jest @@ -1460,7 +1482,7 @@ describe('ProfileMetricsController', () => { await controller._executePoll(); - expect(mockSignProof).not.toHaveBeenCalled(); + expect(mockSignProofBatch).not.toHaveBeenCalled(); expect(mockSubmitMetrics).toHaveBeenCalledWith( expect.objectContaining({ entropySourceId: 'id1', @@ -1492,7 +1514,7 @@ describe('ProfileMetricsController', () => { controller, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([createMockAccount(address.toLowerCase())]); @@ -1500,7 +1522,7 @@ describe('ProfileMetricsController', () => { await controller._executePoll(); - expect(mockSignProof).not.toHaveBeenCalled(); + expect(mockSignProofBatch).not.toHaveBeenCalled(); expect(mockSubmitMetrics).toHaveBeenCalledWith( expect.objectContaining({ accounts: [{ address, scopes: ['eip155:1'] }], @@ -1510,7 +1532,7 @@ describe('ProfileMetricsController', () => { ); }); - it('attaches proofs for the successful accounts and submits the rejected one without a proof when sign throws', async () => { + it('attaches proofs for successful accounts and submits rejected items without a proof', async () => { const goodAddress = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const badAddress = '0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359'; const goodLower = goodAddress.toLowerCase(); @@ -1531,7 +1553,7 @@ describe('ProfileMetricsController', () => { controller, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { const consoleErrorSpy = jest @@ -1545,11 +1567,11 @@ describe('ProfileMetricsController', () => { [goodAddress]: 'n-good', [badAddress]: 'n-bad', }); - mockSignProof.mockImplementation(async ({ account }) => { - if (account.address === goodLower) { - return { nonce: 'n-good', signature: '0xgood' }; - } - throw new Error('Method not found: signProofOfOwnership'); + mockSignProofBatch.mockResolvedValueOnce({ + results: [ + { proof: { nonce: 'n-good', signature: '0xgood' } }, + { error: 'Method not found: signProofOfOwnershipBatch' }, + ], }); await controller._executePoll(); @@ -1574,6 +1596,82 @@ describe('ProfileMetricsController', () => { ); }); + it('keeps the batch in the queue when signBatch rejects', async () => { + const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; + const accounts: Record = { + id1: [{ address, scopes: ['eip155:1'] }], + }; + await withController( + { + options: { + state: { syncQueue: accounts, initialDelayEndTimestamp: 0 }, + }, + }, + async ({ + controller, + mockSubmitMetrics, + mockFetchNonces, + mockSignProofBatch, + registerAccounts, + }) => { + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(); + registerAccounts([createMockAccount(address.toLowerCase())]); + mockFetchNonces.mockResolvedValueOnce({ [address]: 'n' }); + mockSignProofBatch.mockRejectedValueOnce( + new Error('batch signing failed'), + ); + + await controller._executePoll(); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Failed to submit profile metrics for sync queue key id1:', + expect.any(Error), + ); + expect(mockSubmitMetrics).not.toHaveBeenCalled(); + expect(controller.state.syncQueue).toStrictEqual(accounts); + }, + ); + }); + + it('keeps the batch in the queue when signBatch returns the wrong number of results', async () => { + const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; + const accounts: Record = { + id1: [{ address, scopes: ['eip155:1'] }], + }; + await withController( + { + options: { + state: { syncQueue: accounts, initialDelayEndTimestamp: 0 }, + }, + }, + async ({ + controller, + mockSubmitMetrics, + mockFetchNonces, + mockSignProofBatch, + registerAccounts, + }) => { + const consoleErrorSpy = jest + .spyOn(console, 'error') + .mockImplementation(); + registerAccounts([createMockAccount(address.toLowerCase())]); + mockFetchNonces.mockResolvedValueOnce({ [address]: 'n' }); + mockSignProofBatch.mockResolvedValueOnce({ results: [] }); + + await controller._executePoll(); + + expect(consoleErrorSpy).toHaveBeenCalledWith( + 'Failed to submit profile metrics for sync queue key id1:', + expect.any(Error), + ); + expect(mockSubmitMetrics).not.toHaveBeenCalled(); + expect(controller.state.syncQueue).toStrictEqual(accounts); + }, + ); + }); + it('keeps the batch in the queue when submitMetrics fails after proofs have been signed', async () => { const address = '0x71C7656EC7ab88b098defB751B7401B5f6d8976F'; const accounts: Record = { @@ -1589,15 +1687,14 @@ describe('ProfileMetricsController', () => { controller, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { jest.spyOn(console, 'error').mockImplementation(); registerAccounts([createMockAccount(address.toLowerCase())]); mockFetchNonces.mockResolvedValueOnce({ [address]: 'n' }); - mockSignProof.mockResolvedValueOnce({ - nonce: 'n', - signature: '0xsig', + mockSignProofBatch.mockResolvedValueOnce({ + results: [{ proof: { nonce: 'n', signature: '0xsig' } }], }); mockSubmitMetrics.mockRejectedValueOnce(new Error('500')); @@ -1624,7 +1721,7 @@ describe('ProfileMetricsController', () => { async ({ controller, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([ @@ -1634,9 +1731,10 @@ describe('ProfileMetricsController', () => { mockFetchNonces.mockImplementation(async ({ identifiers }) => Object.fromEntries(identifiers.map((id) => [id, `n-${id}`])), ); - mockSignProof.mockImplementation(async ({ nonce }) => ({ - nonce, - signature: '0xsig', + mockSignProofBatch.mockImplementation(async ({ items }) => ({ + results: items.map(({ nonce }) => ({ + proof: { nonce, signature: '0xsig' }, + })), })); await controller._executePoll(); @@ -1673,7 +1771,7 @@ describe('ProfileMetricsController', () => { controller, mockSubmitMetrics, mockFetchNonces, - mockSignProof, + mockSignProofBatch, registerAccounts, }) => { registerAccounts([ @@ -1684,9 +1782,8 @@ describe('ProfileMetricsController', () => { }, ]); mockFetchNonces.mockResolvedValueOnce({ [evmAddress]: 'n' }); - mockSignProof.mockResolvedValueOnce({ - nonce: 'n', - signature: '0xsig', + mockSignProofBatch.mockResolvedValueOnce({ + results: [{ proof: { nonce: 'n', signature: '0xsig' } }], }); await controller._executePoll(); @@ -1695,7 +1792,7 @@ describe('ProfileMetricsController', () => { identifiers: [evmAddress], entropySourceId: 'id1', }); - expect(mockSignProof).toHaveBeenCalledTimes(1); + expect(mockSignProofBatch).toHaveBeenCalledTimes(1); expect(mockSubmitMetrics).toHaveBeenCalledWith( expect.objectContaining({ accounts: [ @@ -1920,6 +2017,10 @@ type WithControllerCallback = (payload: { Promise, [ProofOfOwnershipSignRequest] >; + mockSignProofBatch: jest.Mock< + Promise, + [ProofOfOwnershipSignBatchRequest] + >; registerAccounts: (accounts: InternalAccount[]) => void; }) => Promise | ReturnValue; @@ -1961,6 +2062,7 @@ function getMessenger( 'ProfileMetricsService:submitMetrics', 'ProfileMetricsService:fetchNonces', 'ProofOfOwnershipService:sign', + 'ProofOfOwnershipService:signBatch', ], events: [ 'KeyringController:unlock', @@ -1996,6 +2098,11 @@ async function withController( const mockSignProof = jest .fn() .mockRejectedValue(new Error('mockSignProof not configured for this test')); + const mockSignProofBatch = jest + .fn() + .mockRejectedValue( + new Error('mockSignProofBatch not configured for this test'), + ); const mockAssertUserOptedIn = jest.fn().mockReturnValue(true); const mockGetMetaMetricsId = jest.fn().mockReturnValue('test-metrics-id'); @@ -2024,6 +2131,10 @@ async function withController( 'ProofOfOwnershipService:sign', mockSignProof, ); + rootMessenger.registerActionHandler( + 'ProofOfOwnershipService:signBatch', + mockSignProofBatch, + ); rootMessenger.registerActionHandler('AccountsController:getState', () => ({ internalAccounts: { accounts: Object.fromEntries(accountsById), @@ -2053,6 +2164,7 @@ async function withController( mockSubmitMetrics, mockFetchNonces, mockSignProof, + mockSignProofBatch, registerAccounts, }); } diff --git a/packages/profile-metrics-controller/src/ProfileMetricsController.ts b/packages/profile-metrics-controller/src/ProfileMetricsController.ts index 31507db5d3a..066bfc3d205 100644 --- a/packages/profile-metrics-controller/src/ProfileMetricsController.ts +++ b/packages/profile-metrics-controller/src/ProfileMetricsController.ts @@ -23,6 +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'; @@ -438,28 +439,68 @@ export class ProfileMetricsController extends StaticIntervalPollingController()< ); } - return await Promise.all( - accounts.map(async (queued): Promise => { - const account = proofCandidates.get(queued.address); - const nonce = nonces[queued.address]; - if (!account || !nonce) { - return queued; - } - try { - 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 ${account.id}:`, - error, - ); - return queued; - } - }), + const accountsWithProofs = [...accounts]; + const signingRequests: { + index: number; + account: InternalAccount; + nonce: string; + }[] = []; + + accounts.forEach((queued, index) => { + const account = proofCandidates.get(queued.address); + const nonce = nonces[queued.address]; + if (!account || !nonce) { + return; + } + signingRequests.push({ + index, + account, + nonce, + }); + }); + + if (signingRequests.length === 0) { + return accountsWithProofs; + } + + const { results } = await this.messenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: signingRequests.map(({ account, nonce }) => ({ + account, + nonce, + })), + }, ); + + if (results.length !== signingRequests.length) { + throw new Error( + `ProofOfOwnershipService:signBatch returned ${results.length} results for ${signingRequests.length} requests.`, + ); + } + + results.forEach((result, resultIndex) => { + const { index, account } = signingRequests[resultIndex] as { + index: number; + account: InternalAccount; + }; + const { proof } = result as { proof?: AccountOwnershipProof }; + if (proof) { + accountsWithProofs[index] = { + ...accountsWithProofs[index], + proof, + } as AccountWithScopes; + return; + } + + const { error } = result as { error: string }; + console.error( + `Failed to sign proof of ownership for account ${account.id}:`, + new Error(error), + ); + }); + + return accountsWithProofs; } /** diff --git a/packages/profile-metrics-controller/src/ProofOfOwnershipService-method-action-types.ts b/packages/profile-metrics-controller/src/ProofOfOwnershipService-method-action-types.ts index 650ecfe26e0..72d40e0c38b 100644 --- a/packages/profile-metrics-controller/src/ProofOfOwnershipService-method-action-types.ts +++ b/packages/profile-metrics-controller/src/ProofOfOwnershipService-method-action-types.ts @@ -25,8 +25,26 @@ export type ProofOfOwnershipServiceSignAction = { handler: ProofOfOwnershipService['sign']; }; +/** + * Sign proofs of ownership for multiple accounts. + * + * EVM accounts continue to sign through the keyring one account at a time. + * Snap-backed accounts are grouped by snap ID and sent through the + * `signProofOfOwnershipBatch` snap method once per snap. + * + * @param data - The account/nonce pairs to prove ownership of. + * @returns Per-item proof or error results in input order. + * @throws if a snap batch request rejects, returns a malformed response, or + * returns a result count/account ordering that does not match the request. + */ +export type ProofOfOwnershipServiceSignBatchAction = { + type: `ProofOfOwnershipService:signBatch`; + handler: ProofOfOwnershipService['signBatch']; +}; + /** * Union of all ProofOfOwnershipService action types. */ export type ProofOfOwnershipServiceMethodActions = - ProofOfOwnershipServiceSignAction; + | ProofOfOwnershipServiceSignAction + | ProofOfOwnershipServiceSignBatchAction; diff --git a/packages/profile-metrics-controller/src/ProofOfOwnershipService.test.ts b/packages/profile-metrics-controller/src/ProofOfOwnershipService.test.ts index a2ed340939b..868261491dc 100644 --- a/packages/profile-metrics-controller/src/ProofOfOwnershipService.test.ts +++ b/packages/profile-metrics-controller/src/ProofOfOwnershipService.test.ts @@ -8,7 +8,10 @@ import type { import { ProofOfOwnershipService } from './index.js'; import type { ProofOfOwnershipServiceMessenger } from './index.js'; -import { SNAP_SIGN_PROOF_OF_OWNERSHIP_METHOD } from './ProofOfOwnershipService.js'; +import { + SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD, + SNAP_SIGN_PROOF_OF_OWNERSHIP_METHOD, +} from './ProofOfOwnershipService.js'; import { ProofUnsupportedNamespaceError } from './utils/canonicalize.js'; /** @@ -76,6 +79,21 @@ describe('ProofOfOwnershipService', () => { signature: '0xdefaultsig', }); }); + + it('registers the signBatch messenger action on construction', async () => { + const { rootMessenger } = getService(); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [{ account: createMockAccount(), nonce: 'n0' }], + }, + ); + + expect(response).toStrictEqual({ + results: [{ proof: { nonce: 'n0', signature: '0xdefaultsig' } }], + }); + }); }); describe('eip155 dispatch', () => { @@ -293,6 +311,343 @@ describe('ProofOfOwnershipService', () => { }); }); + describe('batch dispatch', () => { + it('routes EVM accounts through KeyringController:signPersonalMessage in input order', async () => { + const signPersonalMessage = jest + .fn, [{ data: string; from: string }]>() + .mockResolvedValueOnce('0xsig1') + .mockResolvedValueOnce('0xsig2'); + const { rootMessenger } = getService({ signPersonalMessage }); + const account1 = createMockAccount({ + id: 'evm-1', + address: '0x5aaeb6053f3e94c9b9a09f33669435e7ef1beaed', + }); + const account2 = createMockAccount({ + id: 'evm-2', + address: '0xfb6916095ca1df60bb79ce92ce3ea74c37c5d359', + }); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [ + { account: account1, nonce: 'n1' }, + { account: account2, nonce: 'n2' }, + ], + }, + ); + + expect(signPersonalMessage).toHaveBeenCalledTimes(2); + expect(signPersonalMessage).toHaveBeenNthCalledWith(1, { + data: 'metamask:proof-of-ownership:n1:0x5aAeb6053F3E94C9b9A09f33669435E7Ef1BeAed', + from: account1.address, + }); + expect(signPersonalMessage).toHaveBeenNthCalledWith(2, { + data: 'metamask:proof-of-ownership:n2:0xfB6916095ca1df60bB79Ce92cE3Ea74c37c5d359', + from: account2.address, + }); + expect(response).toStrictEqual({ + results: [ + { proof: { nonce: 'n1', signature: '0xsig1' } }, + { proof: { nonce: 'n2', signature: '0xsig2' } }, + ], + }); + }); + + it('returns item-level errors for EVM signing failures', async () => { + const signPersonalMessage = jest + .fn, [{ data: string; from: string }]>() + .mockRejectedValue(new Error('keyring locked')); + const { rootMessenger } = getService({ signPersonalMessage }); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [{ account: createMockAccount(), nonce: 'n' }], + }, + ); + + expect(response).toStrictEqual({ + results: [{ error: 'keyring locked' }], + }); + }); + + it('stringifies non-Error values thrown while signing batch requests', async () => { + const signPersonalMessage = jest + .fn, [{ data: string; from: string }]>() + .mockRejectedValue('keyring locked'); + const { rootMessenger } = getService({ signPersonalMessage }); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [{ account: createMockAccount(), nonce: 'n' }], + }, + ); + + expect(response).toStrictEqual({ + results: [{ error: 'keyring locked' }], + }); + }); + + it('returns an item-level error when a snap account has no snap metadata', async () => { + const snapHandle = jest.fn, [unknown]>(); + const { rootMessenger } = getService({ snapHandle }); + const account = createMockAccount({ + id: 'orphan', + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + address: '9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + metadata: { + keyring: { type: 'Test Keyring' }, + name: 'Orphan', + importTime: 0, + }, + }); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [{ account, nonce: 'n' }], + }, + ); + + expect(response).toStrictEqual({ + results: [ + { + error: + "ProofOfOwnershipService: account 'orphan' has no snap to sign a proof of ownership.", + }, + ], + }); + expect(snapHandle).not.toHaveBeenCalled(); + }); + + it('returns an item-level error when an account namespace is unsupported', async () => { + const snapHandle = jest.fn, [unknown]>(); + const { rootMessenger } = getService({ snapHandle }); + const account = createMockAccount({ + scopes: ['cosmos:cosmoshub-4'], + address: 'cosmos1abc', + }); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [{ account, nonce: 'n' }], + }, + ); + + expect(response).toStrictEqual({ + results: [ + { + error: + "Proof of ownership is not supported for namespace 'cosmos'.", + }, + ], + }); + expect(snapHandle).not.toHaveBeenCalled(); + }); + + it('groups snap accounts by snap ID and preserves per-item results', async () => { + const snapHandle = jest + .fn, [unknown]>() + .mockImplementation(async ({ snapId }: { snapId: string }) => { + if (snapId === 'npm:@metamask/solana-wallet-snap') { + return { + results: [ + { accountId: 'solana-1', signature: '0xsolanasig' }, + { accountId: 'solana-2', error: 'account not found' }, + ], + }; + } + return { + results: [{ accountId: 'tron-1', signature: '0xtronsig' }], + }; + }); + const { rootMessenger } = getService({ snapHandle }); + const solanaMetadata = { + keyring: { type: 'Snap Keyring' }, + name: 'Solana', + importTime: 0, + snap: { + id: 'npm:@metamask/solana-wallet-snap', + name: 'Solana Wallet Snap', + enabled: true, + }, + }; + const tronMetadata = { + keyring: { type: 'Snap Keyring' }, + name: 'Tron', + importTime: 0, + snap: { + id: 'npm:@metamask/tron-wallet-snap', + name: 'Tron Wallet Snap', + enabled: true, + }, + }; + const solana1 = createMockAccount({ + id: 'solana-1', + address: '9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + metadata: solanaMetadata, + }); + const tron = createMockAccount({ + id: 'tron-1', + address: 'TRX9Yg4yFqyKBcXBSc1nKMpHsfYVgKvN3p', + scopes: ['tron:0x2b6653dc'], + metadata: tronMetadata, + }); + const solana2 = createMockAccount({ + id: 'solana-2', + address: 'ANotherSolanaAddress', + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + metadata: solanaMetadata, + }); + + const response = await rootMessenger.call( + 'ProofOfOwnershipService:signBatch', + { + items: [ + { account: solana1, nonce: 'n-sol-1' }, + { account: tron, nonce: 'n-tron' }, + { account: solana2, nonce: 'n-sol-2' }, + ], + }, + ); + + expect(snapHandle).toHaveBeenCalledTimes(2); + expect(snapHandle).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + snapId: 'npm:@metamask/solana-wallet-snap', + origin: 'metamask', + handler: 'onClientRequest', + request: expect.objectContaining({ + method: SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD, + params: { + items: [ + { + accountId: 'solana-1', + message: + 'metamask:proof-of-ownership:n-sol-1:9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + }, + { + accountId: 'solana-2', + message: + 'metamask:proof-of-ownership:n-sol-2:ANotherSolanaAddress', + }, + ], + }, + }), + }), + ); + expect(snapHandle).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + snapId: 'npm:@metamask/tron-wallet-snap', + request: expect.objectContaining({ + method: SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD, + params: { + items: [ + { + accountId: 'tron-1', + message: + 'metamask:proof-of-ownership:n-tron:TRX9Yg4yFqyKBcXBSc1nKMpHsfYVgKvN3p', + }, + ], + }, + }), + }), + ); + expect(response).toStrictEqual({ + results: [ + { proof: { nonce: 'n-sol-1', signature: '0xsolanasig' } }, + { proof: { nonce: 'n-tron', signature: '0xtronsig' } }, + { error: 'account not found' }, + ], + }); + }); + + it('throws when a snap batch response is malformed', async () => { + const snapHandle = jest + .fn, [unknown]>() + .mockResolvedValue({ results: [{ accountId: 'snap-1' }] }); + const { rootMessenger } = getService({ snapHandle }); + const account = createMockAccount({ + id: 'snap-1', + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + address: '9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + }); + + await expect( + rootMessenger.call('ProofOfOwnershipService:signBatch', { + items: [{ account, nonce: 'n' }], + }), + ).rejects.toThrow( + `ProofOfOwnershipService: snap 'npm:@metamask/test-wallet-snap' returned a malformed response to '${SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD}'.`, + ); + }); + + it('throws when a snap batch response result count does not match the request count', async () => { + const snapHandle = jest + .fn, [unknown]>() + .mockResolvedValue({ results: [] }); + const { rootMessenger } = getService({ snapHandle }); + const account = createMockAccount({ + id: 'snap-1', + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + address: '9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + }); + + await expect( + rootMessenger.call('ProofOfOwnershipService:signBatch', { + items: [{ account, nonce: 'n' }], + }), + ).rejects.toThrow( + `ProofOfOwnershipService: snap 'npm:@metamask/test-wallet-snap' returned 0 results for 1 '${SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD}' requests.`, + ); + }); + + it('throws when a snap batch response does not preserve account order', async () => { + const snapHandle = jest + .fn, [unknown]>() + .mockResolvedValue({ + results: [{ accountId: 'wrong-account', signature: '0xsig' }], + }); + const { rootMessenger } = getService({ snapHandle }); + const account = createMockAccount({ + id: 'snap-1', + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + address: '9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + }); + + await expect( + rootMessenger.call('ProofOfOwnershipService:signBatch', { + items: [{ account, nonce: 'n' }], + }), + ).rejects.toThrow( + "ProofOfOwnershipService: snap 'npm:@metamask/test-wallet-snap' returned a result for account 'wrong-account' at index 0, expected 'snap-1'.", + ); + }); + + it('surfaces snap batch request errors without singleton fallback', async () => { + const snapHandle = jest + .fn, [unknown]>() + .mockRejectedValue(new Error('batch unavailable')); + const { rootMessenger } = getService({ snapHandle }); + const account = createMockAccount({ + scopes: ['solana:5eykt4UsFv8P8NJdTREpY1vzqKqZKvdp'], + address: '9WzDXwBbmkg8ZTbNMqUxvQRAyrZzDsGYdLVL9zYtAWWM', + }); + + await expect( + rootMessenger.call('ProofOfOwnershipService:signBatch', { + items: [{ account, nonce: 'n' }], + }), + ).rejects.toThrow('batch unavailable'); + }); + }); + describe('namespace handling', () => { it('throws ProofUnsupportedNamespaceError for unrecognized namespaces', async () => { const { rootMessenger } = getService(); diff --git a/packages/profile-metrics-controller/src/ProofOfOwnershipService.ts b/packages/profile-metrics-controller/src/ProofOfOwnershipService.ts index 1dd1b7e910c..6c034c124e0 100644 --- a/packages/profile-metrics-controller/src/ProofOfOwnershipService.ts +++ b/packages/profile-metrics-controller/src/ProofOfOwnershipService.ts @@ -4,7 +4,12 @@ import type { Messenger } from '@metamask/messenger'; import type { SnapControllerHandleRequestAction } from '@metamask/snaps-controllers'; import type { SnapId } from '@metamask/snaps-sdk'; import { HandlerType } from '@metamask/snaps-utils'; -import { string, type as structType } from '@metamask/superstruct'; +import { + array, + string, + type as structType, + union, +} from '@metamask/superstruct'; import { KnownCaipNamespace, parseCaipChainId } from '@metamask/utils'; import { v4 as uuid } from 'uuid'; @@ -42,6 +47,48 @@ export type ProofOfOwnershipSignRequest = { nonce: string; }; +/** + * The shape of the request object for signing proofs of ownership for multiple + * accounts. + */ +export type ProofOfOwnershipSignBatchRequest = { + /** + * The account/nonce pairs to sign proofs for. Results preserve this order. + */ + items: ProofOfOwnershipSignRequest[]; +}; + +/** + * Successful proof-of-ownership batch item. + */ +export type ProofOfOwnershipSignBatchSuccess = { + proof: AccountOwnershipProof; +}; + +/** + * Failed proof-of-ownership batch item. + */ +export type ProofOfOwnershipSignBatchError = { + error: string; +}; + +/** + * Result for one proof-of-ownership batch signing item. + */ +export type ProofOfOwnershipSignBatchResult = + | ProofOfOwnershipSignBatchSuccess + | ProofOfOwnershipSignBatchError; + +/** + * Batch proof-of-ownership signing response. + */ +export type ProofOfOwnershipSignBatchResponse = { + /** + * Per-item results in the same order as the input items. + */ + results: ProofOfOwnershipSignBatchResult[]; +}; + /** * The JSON-RPC method name exposed by non-EVM wallet snaps for silent * proof-of-ownership signing. Each supported snap (Bitcoin, Solana, Tron) @@ -51,6 +98,13 @@ export type ProofOfOwnershipSignRequest = { */ export const SNAP_SIGN_PROOF_OF_OWNERSHIP_METHOD = 'signProofOfOwnership'; +/** + * The JSON-RPC method name exposed by non-EVM wallet snaps for silent batch + * proof-of-ownership signing. + */ +export const SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD = + 'signProofOfOwnershipBatch'; + /** * The shape of a successful response from a non-EVM wallet snap's * {@link SNAP_SIGN_PROOF_OF_OWNERSHIP_METHOD} handler. Validated at runtime; @@ -61,6 +115,39 @@ const SnapSignProofResponseStruct = structType({ signature: string(), }); +/** + * The shape of a response from a non-EVM wallet snap's + * {@link SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD} handler. Validated at + * runtime; declared with `type()` (not `object()`) so additive snap-side schema + * changes do not break the client. + */ +const SnapSignProofBatchResponseStruct = structType({ + results: array( + union([ + structType({ + accountId: string(), + signature: string(), + }), + structType({ + accountId: string(), + error: string(), + }), + ]), + ), +}); + +type SnapSignProofBatchResponse = { + results: ( + | { accountId: string; signature: string } + | { accountId: string; error: string } + )[]; +}; + +type PreparedProofOfOwnershipRequest = ProofOfOwnershipSignRequest & { + index: number; + message: string; +}; + /** * Builds the canonical message string that all chains sign for a proof of * ownership: `metamask:proof-of-ownership::`. @@ -76,6 +163,16 @@ function buildProofMessage(nonce: string, canonicalAddress: string): string { return `metamask:proof-of-ownership:${nonce}:${canonicalAddress}`; } +/** + * Converts an unknown thrown value into an error message. + * + * @param error - The thrown value. + * @returns A string error message. + */ +function getErrorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + /** * Extracts the CAIP-2 namespace from the first scope of an account. All * scopes on a single account are expected to share a namespace (e.g. an @@ -104,7 +201,7 @@ function getAccountNamespace(account: InternalAccount): string { // === MESSENGER === -const MESSENGER_EXPOSED_METHODS = ['sign'] as const; +const MESSENGER_EXPOSED_METHODS = ['sign', 'signBatch'] as const; /** * Actions that {@link ProofOfOwnershipService} exposes to other consumers. @@ -223,6 +320,85 @@ export class ProofOfOwnershipService { return { nonce, signature }; } + /** + * Sign proofs of ownership for multiple accounts. + * + * EVM accounts continue to sign through the keyring one account at a time. + * Snap-backed accounts are grouped by snap ID and sent through the + * `signProofOfOwnershipBatch` snap method once per snap. + * + * @param data - The account/nonce pairs to prove ownership of. + * @returns Per-item proof or error results in input order. + * @throws if a snap batch request rejects, returns a malformed response, or + * returns a result count/account ordering that does not match the request. + */ + async signBatch( + data: ProofOfOwnershipSignBatchRequest, + ): Promise { + const results: ProofOfOwnershipSignBatchResult[] = new Array( + data.items.length, + ); + const evmRequests: PreparedProofOfOwnershipRequest[] = []; + const snapRequestsBySnapId = new Map< + SnapId, + PreparedProofOfOwnershipRequest[] + >(); + + data.items.forEach((item, index) => { + try { + const namespace = getAccountNamespace(item.account); + const canonicalAddress = canonicalizeAddress( + item.account.address, + namespace, + ); + const message = buildProofMessage(item.nonce, canonicalAddress); + const request = { ...item, index, message }; + + if (namespace === KnownCaipNamespace.Eip155) { + evmRequests.push(request); + return; + } + + const snapId = item.account.metadata.snap?.id; + if (!snapId) { + results[index] = { + error: `ProofOfOwnershipService: account '${item.account.id}' has no snap to sign a proof of ownership.`, + }; + return; + } + + const snapRequests = snapRequestsBySnapId.get(snapId as SnapId) ?? []; + snapRequests.push(request); + snapRequestsBySnapId.set(snapId as SnapId, snapRequests); + } catch (error) { + results[index] = { error: getErrorMessage(error) }; + } + }); + + await Promise.all([ + ...evmRequests.map(async (request) => { + try { + results[request.index] = { + proof: { + nonce: request.nonce, + signature: await this.#signEvm( + request.account.address, + request.message, + ), + }, + }; + } catch (error) { + results[request.index] = { error: getErrorMessage(error) }; + } + }), + ...[...snapRequestsBySnapId.entries()].map(async ([snapId, requests]) => { + await this.#signViaSnapBatch(snapId, requests, results); + }), + ]); + + return { results }; + } + /** * Sign an EIP-191 personal message via the keyring controller. * @@ -291,4 +467,78 @@ export class ProofOfOwnershipService { return response.signature; } + + /** + * Sign a group of proof messages through a single snap batch request. + * + * @param snapId - Snap ID shared by every request in the group. + * @param requests - Prepared requests for this snap. + * @param results - Mutable output array indexed to match the original input. + * @throws if the snap response is malformed or does not preserve request + * order/account IDs. + */ + async #signViaSnapBatch( + snapId: SnapId, + requests: PreparedProofOfOwnershipRequest[], + results: ProofOfOwnershipSignBatchResult[], + ): Promise { + const response: unknown = await this.#messenger.call( + 'SnapController:handleRequest', + { + snapId, + origin: 'metamask', + handler: HandlerType.OnClientRequest, + request: { + id: uuid(), + jsonrpc: '2.0', + method: SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD, + params: { + items: requests.map(({ account, message }) => ({ + accountId: account.id, + message, + })), + }, + }, + }, + ); + + if (!SnapSignProofBatchResponseStruct.is(response)) { + // Intentionally generic — a malformed snap response may still carry + // partial signatures, and we don't want fragments of secret material + // landing in error logs. + throw new Error( + `ProofOfOwnershipService: snap '${snapId}' returned a malformed response to '${SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD}'.`, + ); + } + + const { results: snapResults } = response as SnapSignProofBatchResponse; + if (snapResults.length !== requests.length) { + throw new Error( + `ProofOfOwnershipService: snap '${snapId}' returned ${snapResults.length} results for ${requests.length} '${SNAP_SIGN_PROOF_OF_OWNERSHIP_BATCH_METHOD}' requests.`, + ); + } + + snapResults.forEach((snapResult, position) => { + const request = requests[position]; + if (snapResult.accountId !== request.account.id) { + throw new Error( + `ProofOfOwnershipService: snap '${snapId}' returned a result for account '${snapResult.accountId}' at index ${position}, expected '${request.account.id}'.`, + ); + } + + const { signature } = snapResult as { signature?: string }; + if (signature !== undefined) { + results[request.index] = { + proof: { + nonce: request.nonce, + signature, + }, + }; + return; + } + + const { error } = snapResult as { error: string }; + results[request.index] = { error }; + }); + } }