diff --git a/packages/bitcoin-wallet-snap/CHANGELOG.md b/packages/bitcoin-wallet-snap/CHANGELOG.md index 8b1adc9c..a5b7a425 100644 --- a/packages/bitcoin-wallet-snap/CHANGELOG.md +++ b/packages/bitcoin-wallet-snap/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- Populate the optional `transaction_type` property on the transaction lifecycle tracking events. ([#393](https://github.com/MetaMask/internal-snaps/pull/393)) + ## [3.1.0] ### Added diff --git a/packages/bitcoin-wallet-snap/integration-test/client-request.test.ts b/packages/bitcoin-wallet-snap/integration-test/client-request.test.ts index f984332a..ed2710e3 100644 --- a/packages/bitcoin-wallet-snap/integration-test/client-request.test.ts +++ b/packages/bitcoin-wallet-snap/integration-test/client-request.test.ts @@ -1,5 +1,10 @@ import type { KeyringAccount } from '@metamask/keyring-api'; -import { FeeType, BtcAccountType, BtcScope } from '@metamask/keyring-api'; +import { + FeeType, + BtcAccountType, + BtcScope, + TransactionType, +} from '@metamask/keyring-api'; import type { Snap } from '@metamask/snaps-jest'; import { installSnap } from '@metamask/snaps-jest'; @@ -97,6 +102,7 @@ describe('OnClientRequestHandler', () => { chain_id_caip: BtcScope.Regtest, message: 'Snap transaction submitted', origin: ORIGIN, + transaction_type: TransactionType.Send, tx_id: transactionId, }, }); @@ -117,6 +123,7 @@ describe('OnClientRequestHandler', () => { message: 'Snap transaction finalized', chain_id_caip: BtcScope.Regtest, account_type: BtcAccountType.P2wpkh, + transaction_type: TransactionType.Send, tx_id: transactionId, }, }); diff --git a/packages/bitcoin-wallet-snap/integration-test/cron-sync.test.ts b/packages/bitcoin-wallet-snap/integration-test/cron-sync.test.ts index e489c5ef..e96261af 100644 --- a/packages/bitcoin-wallet-snap/integration-test/cron-sync.test.ts +++ b/packages/bitcoin-wallet-snap/integration-test/cron-sync.test.ts @@ -1,5 +1,9 @@ import type { KeyringAccount } from '@metamask/keyring-api'; -import { BtcAccountType, BtcScope } from '@metamask/keyring-api'; +import { + BtcAccountType, + BtcScope, + TransactionType, +} from '@metamask/keyring-api'; import type { Snap } from '@metamask/snaps-jest'; import { installSnap } from '@metamask/snaps-jest'; @@ -93,6 +97,7 @@ describe('CronHandler', () => { message: 'Snap transaction received', chain_id_caip: BtcScope.Regtest, account_type: BtcAccountType.P2wpkh, + transaction_type: TransactionType.Receive, tx_id: txid, }, }); @@ -116,6 +121,7 @@ describe('CronHandler', () => { message: 'Snap transaction received', chain_id_caip: BtcScope.Regtest, account_type: BtcAccountType.P2wpkh, + transaction_type: TransactionType.Receive, tx_id: txid, }, }); diff --git a/packages/bitcoin-wallet-snap/jest.config.js b/packages/bitcoin-wallet-snap/jest.config.js index e40fa85c..5cf8ed05 100644 --- a/packages/bitcoin-wallet-snap/jest.config.js +++ b/packages/bitcoin-wallet-snap/jest.config.js @@ -16,9 +16,9 @@ module.exports = { coverageThreshold: { global: { branches: 76.02, - functions: 64.28, - lines: 83.34, - statements: 83.11, + functions: 64.39, + lines: 83.4, + statements: 83.18, }, }, }; diff --git a/packages/bitcoin-wallet-snap/snap.manifest.json b/packages/bitcoin-wallet-snap/snap.manifest.json index 595b1304..1cddca96 100644 --- a/packages/bitcoin-wallet-snap/snap.manifest.json +++ b/packages/bitcoin-wallet-snap/snap.manifest.json @@ -7,7 +7,7 @@ "url": "https://github.com/MetaMask/internal-snaps.git" }, "source": { - "shasum": "9NN98un9yylKPM/JFLrwaVhv6lKFDfBMXOBkPifOPik=", + "shasum": "TpgI53gTWDVp3HpUr/jO13/WzJJhOUuxJjjLOVNs15o=", "location": { "npm": { "filePath": "dist/bundle.js", diff --git a/packages/bitcoin-wallet-snap/src/entities/index.ts b/packages/bitcoin-wallet-snap/src/entities/index.ts index 952bd168..66f215b6 100644 --- a/packages/bitcoin-wallet-snap/src/entities/index.ts +++ b/packages/bitcoin-wallet-snap/src/entities/index.ts @@ -4,7 +4,8 @@ export type * from './config'; export * from './chain'; export * from './currency'; export * from './send-flow'; -export type * from './transaction'; +export { mapToTransactionType } from './transaction'; +export type { TransactionBuilder } from './transaction'; export * from './snap'; export type * from './meta-protocols'; export type * from './translator'; diff --git a/packages/bitcoin-wallet-snap/src/entities/snap.ts b/packages/bitcoin-wallet-snap/src/entities/snap.ts index f7a1cf1a..9505ca5d 100644 --- a/packages/bitcoin-wallet-snap/src/entities/snap.ts +++ b/packages/bitcoin-wallet-snap/src/entities/snap.ts @@ -1,5 +1,6 @@ import type { AddressType, Network, WalletTx } from '@metamask/bitcoindevkit'; import type { JsonSLIP10Node, SLIP10Node } from '@metamask/key-tree'; +import type { TransactionType } from '@metamask/keyring-api'; import type { ComponentOrElement, GetClientStatusResult, @@ -276,12 +277,14 @@ export type SnapClient = { * @param account The correlated bitcoin account * @param tx The transaction we want to capture metrics for * @param origin The origin/source that triggered this event + * @param transactionType The classification of the transaction. */ emitTrackingEvent( eventType: TransactionBroadcastEventType, account: BitcoinAccount, tx: WalletTx, origin: string, + transactionType: TransactionType, ): Promise; /** @@ -291,8 +294,13 @@ export type SnapClient = { * * @param account The account the transaction belongs to. * @param origin The origin/source that triggered this event. + * @param transactionType The classification of the transaction. */ - trackTransactionAdded(account: BitcoinAccount, origin: string): Promise; + trackTransactionAdded( + account: BitcoinAccount, + origin: string, + transactionType: TransactionType, + ): Promise; /** * Track a "Transaction Approved" event when the user approves a transaction. @@ -301,10 +309,12 @@ export type SnapClient = { * * @param account The account the transaction belongs to. * @param origin The origin/source that triggered this event. + * @param transactionType The classification of the transaction. */ trackTransactionApproved( account: BitcoinAccount, origin: string, + transactionType: TransactionType, ): Promise; /** @@ -314,10 +324,12 @@ export type SnapClient = { * * @param account The account the transaction belongs to. * @param origin The origin/source that triggered this event. + * @param transactionType The classification of the transaction. */ trackTransactionRejected( account: BitcoinAccount, origin: string, + transactionType: TransactionType, ): Promise; /** diff --git a/packages/bitcoin-wallet-snap/src/entities/transaction.test.ts b/packages/bitcoin-wallet-snap/src/entities/transaction.test.ts new file mode 100644 index 00000000..8e948a9d --- /dev/null +++ b/packages/bitcoin-wallet-snap/src/entities/transaction.test.ts @@ -0,0 +1,35 @@ +import type { Amount, Transaction } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; +import { mock } from 'jest-mock-extended'; + +import type { BitcoinAccount } from './account'; +import { mapToTransactionType } from './transaction'; + +describe('mapToTransactionType', () => { + const createAccount = (sentBtc: number): BitcoinAccount => { + const sentAmount = mock(); + jest.spyOn(sentAmount, 'to_btc').mockReturnValue(sentBtc); + + const account = mock(); + jest + .spyOn(account, 'sentAndReceived') + .mockReturnValue([sentAmount, mock()]); + return account; + }; + + it('classifies a transaction that spends funds as a send', () => { + const account = createAccount(1); + + expect(mapToTransactionType(account, mock())).toBe( + TransactionType.Send, + ); + }); + + it('classifies a transaction that spends nothing as a receive', () => { + const account = createAccount(0); + + expect(mapToTransactionType(account, mock())).toBe( + TransactionType.Receive, + ); + }); +}); diff --git a/packages/bitcoin-wallet-snap/src/entities/transaction.ts b/packages/bitcoin-wallet-snap/src/entities/transaction.ts index e647523b..470cf978 100644 --- a/packages/bitcoin-wallet-snap/src/entities/transaction.ts +++ b/packages/bitcoin-wallet-snap/src/entities/transaction.ts @@ -1,4 +1,32 @@ -import type { Amount, Psbt, ScriptBuf } from '@metamask/bitcoindevkit'; +import type { + Amount, + Psbt, + ScriptBuf, + Transaction, +} from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; + +import type { BitcoinAccount } from './account'; + +/** + * Resolves the transaction classification for a Bitcoin transaction. + * + * Bitcoin has no contract calls, so a transaction is either outgoing (`send`, + * including self-sends and consolidations) or incoming (`receive`). Used both + * for the keyring transaction payload and for the `transaction_type` analytics + * dimension, so the two can never disagree. + * + * @param account - The account the transaction belongs to. + * @param tx - The raw Bitcoin transaction. + * @returns The transaction type. + */ +export function mapToTransactionType( + account: BitcoinAccount, + tx: Transaction, +): TransactionType { + const [sent] = account.sentAndReceived(tx); + return sent.to_btc() > 0 ? TransactionType.Send : TransactionType.Receive; +} /** * A Bitcoin transaction builder. diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts index f961e7c8..2a0d16cb 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.test.ts @@ -1,4 +1,5 @@ -import type { WalletTx } from '@metamask/bitcoindevkit'; +import type { Amount, WalletTx } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; import { getSelectedAccounts } from '@metamask/keyring-snap-sdk'; import { SynchronizationError } from '@metamask/snap-networks-utils'; import type { SnapsProvider, JsonRpcRequest } from '@metamask/snaps-sdk'; @@ -51,6 +52,17 @@ describe('CronHandler', () => { const mockAccounts = [mockAccount1, mockAccount2]; const request = { method: 'synchronizeAccounts' } as JsonRpcRequest; + beforeEach(() => { + // Transactions built from these accounts are classified as receives. + const receivedAmount = mock(); + jest.spyOn(receivedAmount, 'to_btc').mockReturnValue(0); + for (const account of mockAccounts) { + jest + .mocked(account.sentAndReceived) + .mockReturnValue([receivedAmount, receivedAmount]); + } + }); + it('synchronizes all selected accounts and emits batched events', async () => { const mockResult1: SyncResult = { account: mockAccount1, @@ -306,6 +318,7 @@ describe('CronHandler', () => { mockAccount1, txNew, 'cron', + TransactionType.Receive, ); expect( mockSnapClient.emitAccountBalancesUpdatedEvent, diff --git a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts index 24bf4e9d..777e9d07 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/CronHandler.ts @@ -7,7 +7,11 @@ import { import type { JsonRpcRequest, SnapsProvider } from '@metamask/snaps-sdk'; import { array, assert, is, object, string } from 'superstruct'; -import { InexistentMethodError, TrackingSnapEvent } from '../entities'; +import { + InexistentMethodError, + mapToTransactionType, + TrackingSnapEvent, +} from '../entities'; import type { Logger, SnapClient, SyncResult } from '../entities'; import type { SendFlowUseCases, AccountUseCases } from '../use-cases'; @@ -301,6 +305,7 @@ export class CronHandler { account, tx, 'cron', + mapToTransactionType(account, tx.tx), ); } } diff --git a/packages/bitcoin-wallet-snap/src/handlers/mappings.ts b/packages/bitcoin-wallet-snap/src/handlers/mappings.ts index 42ae9d92..87d1b895 100644 --- a/packages/bitcoin-wallet-snap/src/handlers/mappings.ts +++ b/packages/bitcoin-wallet-snap/src/handlers/mappings.ts @@ -12,10 +12,15 @@ import type { KeyringAccount, Transaction as KeyringTransaction, } from '@metamask/keyring-api'; -import { FeeType, TransactionStatus } from '@metamask/keyring-api'; +import { + FeeType, + TransactionStatus, + TransactionType, +} from '@metamask/keyring-api'; import { canAccountTxidBeMalleated, networkToCurrencyUnit } from '../entities'; import type { BitcoinAccount } from '../entities'; +import { mapToTransactionType } from '../entities/transaction'; import type { Caip19Asset } from './caip'; import { addressTypeToCaip, networkToCaip19, networkToScope } from './caip'; @@ -169,11 +174,11 @@ export function mapToTransaction( const { network } = account; const [events, timestamp, status] = mapToEvents(chainPosition); - const [sent] = account.sentAndReceived(tx); - const isSend = sent.to_btc() > 0; + const type = mapToTransactionType(account, tx); + const isSend = type === TransactionType.Send; const transaction: KeyringTransaction = { - type: isSend ? 'send' : 'receive', + type, id: txid.toString(), account: account.id, chain: networkToScope[network], diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts index c414a208..1126e06b 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.test.ts @@ -1,4 +1,5 @@ import type { Amount, WalletTx } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; import { getJsonError } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; @@ -68,6 +69,7 @@ describe('SnapClientAdapter', () => { account, tx, 'metamask', + TransactionType.Receive, ), ).toBeUndefined(); @@ -81,6 +83,7 @@ describe('SnapClientAdapter', () => { message: 'Snap transaction received', chain_id_caip: 'bip122:000000000019d6689c085ae165831e93', account_type: 'bip122:p2wpkh', + transaction_type: 'receive', tx_id: 'txid-123', }, }, @@ -111,6 +114,7 @@ describe('SnapClientAdapter', () => { account, tx, 'metamask', + TransactionType.Send, ), ).toBeUndefined(); @@ -124,6 +128,7 @@ describe('SnapClientAdapter', () => { message: 'Snap discovered missed transaction', chain_id_caip: 'bip122:000000000019d6689c085ae165831e93', account_type: 'bip122:p2wpkh', + transaction_type: 'send', transaction_hash: 'txid-123', }, }, @@ -155,6 +160,7 @@ describe('SnapClientAdapter', () => { account, tx, 'metamask', + TransactionType.Receive, ), ).toBeUndefined(); @@ -184,6 +190,7 @@ describe('SnapClientAdapter', () => { account, tx, 'metamask', + TransactionType.Send, ), ).toBeUndefined(); }); @@ -200,7 +207,11 @@ describe('SnapClientAdapter', () => { mockRequest.mockResolvedValue(undefined); expect( - await snapClient.trackTransactionAdded(account, 'https://dapp.test'), + await snapClient.trackTransactionAdded( + account, + 'https://dapp.test', + TransactionType.Send, + ), ).toBeUndefined(); expect(mockRequest).toHaveBeenCalledWith({ @@ -213,6 +224,7 @@ describe('SnapClientAdapter', () => { message: 'Snap transaction added', chain_id_caip: 'bip122:000000000019d6689c085ae165831e93', account_type: 'bip122:p2wpkh', + transaction_type: 'send', }, }, }, @@ -231,7 +243,11 @@ describe('SnapClientAdapter', () => { mockRequest.mockRejectedValue(trackingError); expect( - await snapClient.trackTransactionAdded(account, 'metamask'), + await snapClient.trackTransactionAdded( + account, + 'metamask', + TransactionType.Send, + ), ).toBeUndefined(); expect(mockLogger.error).toHaveBeenCalledWith( @@ -252,7 +268,11 @@ describe('SnapClientAdapter', () => { mockRequest.mockResolvedValue(undefined); expect( - await snapClient.trackTransactionApproved(account, 'metamask'), + await snapClient.trackTransactionApproved( + account, + 'metamask', + TransactionType.Send, + ), ).toBeUndefined(); expect(mockRequest).toHaveBeenCalledWith({ @@ -265,6 +285,7 @@ describe('SnapClientAdapter', () => { message: 'Snap transaction approved', chain_id_caip: 'bip122:000000000933ea01ad0ee984209779ba', account_type: 'bip122:p2tr', + transaction_type: 'send', }, }, }, @@ -283,7 +304,11 @@ describe('SnapClientAdapter', () => { mockRequest.mockRejectedValue(trackingError); expect( - await snapClient.trackTransactionApproved(account, 'metamask'), + await snapClient.trackTransactionApproved( + account, + 'metamask', + TransactionType.Send, + ), ).toBeUndefined(); expect(mockLogger.error).toHaveBeenCalledWith( @@ -304,7 +329,11 @@ describe('SnapClientAdapter', () => { mockRequest.mockResolvedValue(undefined); expect( - await snapClient.trackTransactionRejected(account, 'metamask'), + await snapClient.trackTransactionRejected( + account, + 'metamask', + TransactionType.Send, + ), ).toBeUndefined(); expect(mockRequest).toHaveBeenCalledWith({ @@ -317,6 +346,7 @@ describe('SnapClientAdapter', () => { message: 'Snap transaction rejected', chain_id_caip: 'bip122:000000000019d6689c085ae165831e93', account_type: 'bip122:p2wpkh', + transaction_type: 'send', }, }, }, @@ -335,7 +365,11 @@ describe('SnapClientAdapter', () => { mockRequest.mockRejectedValue(trackingError); expect( - await snapClient.trackTransactionRejected(account, 'metamask'), + await snapClient.trackTransactionRejected( + account, + 'metamask', + TransactionType.Send, + ), ).toBeUndefined(); expect(mockLogger.error).toHaveBeenCalledWith( diff --git a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts index 56817e1d..84cad405 100644 --- a/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts +++ b/packages/bitcoin-wallet-snap/src/infra/SnapClientAdapter.ts @@ -2,7 +2,7 @@ import type { WalletTx } from '@metamask/bitcoindevkit'; import { Amount } from '@metamask/bitcoindevkit'; import type { JsonSLIP10Node } from '@metamask/key-tree'; import { SLIP10Node } from '@metamask/key-tree'; -import { KeyringEvent } from '@metamask/keyring-api'; +import { KeyringEvent, TransactionType } from '@metamask/keyring-api'; import { emitSnapKeyringEvent } from '@metamask/keyring-snap-sdk'; import type { GetClientStatusResult, @@ -259,6 +259,7 @@ export class SnapClientAdapter implements SnapClient { account: BitcoinAccount, tx: WalletTx, origin: string, + transactionType: TransactionType, ): Promise { const transactionKey = eventType === TrackingSnapEvent.MissedTransactionsDiscovered @@ -270,6 +271,7 @@ export class SnapClientAdapter implements SnapClient { message: this.#getTrackingMessage(eventType), chain_id_caip: networkToScope[account.network], account_type: addressTypeToCaip[account.addressType], + transaction_type: transactionType, [transactionKey]: tx.txid.toString(), })); } @@ -277,33 +279,39 @@ export class SnapClientAdapter implements SnapClient { async trackTransactionAdded( account: BitcoinAccount, origin: string, + transactionType: TransactionType, ): Promise { await this.#trackConfirmationEvent( TrackingSnapEvent.TransactionAdded, account, origin, + transactionType, ); } async trackTransactionApproved( account: BitcoinAccount, origin: string, + transactionType: TransactionType, ): Promise { await this.#trackConfirmationEvent( TrackingSnapEvent.TransactionApproved, account, origin, + transactionType, ); } async trackTransactionRejected( account: BitcoinAccount, origin: string, + transactionType: TransactionType, ): Promise { await this.#trackConfirmationEvent( TrackingSnapEvent.TransactionRejected, account, origin, + transactionType, ); } @@ -314,17 +322,20 @@ export class SnapClientAdapter implements SnapClient { * @param eventType - The confirmation event type. * @param account - The account the transaction belongs to. * @param origin - The origin/source that triggered this event. + * @param transactionType - The classification of the transaction. */ async #trackConfirmationEvent( eventType: TransactionConfirmationEventType, account: BitcoinAccount, origin: string, + transactionType: TransactionType, ): Promise { await this.#trackEvent(eventType, () => ({ origin, message: this.#getTrackingMessage(eventType), chain_id_caip: networkToScope[account.network], account_type: addressTypeToCaip[account.addressType], + transaction_type: transactionType, })); } diff --git a/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.test.tsx b/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.test.tsx index dfa854e8..287343ef 100644 --- a/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.test.tsx +++ b/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.test.tsx @@ -7,6 +7,7 @@ import type { TxOut, } from '@metamask/bitcoindevkit'; import { Address as BdkAddress } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; import type { GetPreferencesResult } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; @@ -236,6 +237,7 @@ describe('JSXConfirmationRepository', () => { expect(mockSnapClient.trackTransactionRejected).toHaveBeenCalledWith( mockAccount, origin, + TransactionType.Send, ); expect(mockSnapClient.trackTransactionApproved).not.toHaveBeenCalled(); }); @@ -246,10 +248,12 @@ describe('JSXConfirmationRepository', () => { expect(mockSnapClient.trackTransactionAdded).toHaveBeenCalledWith( mockAccount, origin, + TransactionType.Send, ); expect(mockSnapClient.trackTransactionApproved).toHaveBeenCalledWith( mockAccount, origin, + TransactionType.Send, ); expect(mockSnapClient.trackTransactionRejected).not.toHaveBeenCalled(); @@ -345,6 +349,10 @@ describe('JSXConfirmationRepository', () => { const origin = 'https://dapp.example.com'; beforeEach(() => { + mockAccount.sentAndReceived.mockReturnValue([ + mock({ to_sat: () => BigInt(0) }), + mock(), + ]); mockSnapClient.createInterface.mockResolvedValue('psbt-interface-id'); mockSnapClient.displayConfirmation.mockResolvedValue(true); mockTranslator.load.mockResolvedValue(mockMessages); @@ -394,6 +402,10 @@ describe('JSXConfirmationRepository', () => { network: 'bitcoin', publicAddress: mock
({ toString: () => 'myAddress' }), isMine: () => true, + sentAndReceived: () => [ + mock({ to_sat: () => BigInt(0) }), + mock(), + ], }); await repo.insertSignPsbt(changeAccount, mockSignPsbt, origin, options); @@ -455,6 +467,7 @@ describe('JSXConfirmationRepository', () => { expect(mockSnapClient.trackTransactionRejected).toHaveBeenCalledWith( mockAccount, origin, + TransactionType.Unknown, ); expect(mockSnapClient.trackTransactionApproved).not.toHaveBeenCalled(); }); @@ -465,10 +478,12 @@ describe('JSXConfirmationRepository', () => { expect(mockSnapClient.trackTransactionAdded).toHaveBeenCalledWith( mockAccount, origin, + TransactionType.Unknown, ); expect(mockSnapClient.trackTransactionApproved).toHaveBeenCalledWith( mockAccount, origin, + TransactionType.Unknown, ); expect(mockSnapClient.trackTransactionRejected).not.toHaveBeenCalled(); @@ -482,6 +497,44 @@ describe('JSXConfirmationRepository', () => { expect(displayOrder).toBeLessThan(approvedOrder as number); }); + it('classifies a PSBT that spends the account inputs as a send', async () => { + mockAccount.sentAndReceived.mockReturnValue([ + mock({ to_sat: () => BigInt(1500) }), + mock(), + ]); + + await repo.insertSignPsbt(mockAccount, mockSignPsbt, origin, options); + + expect(mockSnapClient.trackTransactionAdded).toHaveBeenCalledWith( + mockAccount, + origin, + TransactionType.Send, + ); + expect(mockSnapClient.trackTransactionApproved).toHaveBeenCalledWith( + mockAccount, + origin, + TransactionType.Send, + ); + }); + + it('classifies a rejected PSBT that spends the account inputs as a send', async () => { + mockAccount.sentAndReceived.mockReturnValue([ + mock({ to_sat: () => BigInt(1500) }), + mock(), + ]); + mockSnapClient.displayConfirmation.mockResolvedValue(false); + + await expect( + repo.insertSignPsbt(mockAccount, mockSignPsbt, origin, options), + ).rejects.toThrow('User canceled the confirmation'); + + expect(mockSnapClient.trackTransactionRejected).toHaveBeenCalledWith( + mockAccount, + origin, + TransactionType.Send, + ); + }); + it('handles PSBT without fee information gracefully', async () => { const psbtNoFee = mock({ toString: () => 'psbt-no-fee', @@ -526,6 +579,10 @@ describe('JSXConfirmationRepository', () => { network: 'testnet', publicAddress: mock
({ toString: () => 'myAddress' }), isMine: () => false, + sentAndReceived: () => [ + mock({ to_sat: () => BigInt(0) }), + mock(), + ], }); await repo.insertSignPsbt(testnetAccount, mockSignPsbt, origin, options); diff --git a/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.tsx b/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.tsx index 1efe4e48..53d0058a 100644 --- a/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.tsx +++ b/packages/bitcoin-wallet-snap/src/store/JSXConfirmationRepository.tsx @@ -1,5 +1,6 @@ import type { Psbt } from '@metamask/bitcoindevkit'; import { Address as BdkAddress } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; import { getCurrentUnixTimestamp } from '@metamask/keyring-snap-sdk'; import type { @@ -120,16 +121,28 @@ export class JSXConfirmationRepository implements ConfirmationRepository { context, ); - await this.#snapClient.trackTransactionAdded(account, origin); + await this.#snapClient.trackTransactionAdded( + account, + origin, + TransactionType.Send, + ); const confirmed = await this.#snapClient.displayConfirmation(interfaceId); if (!confirmed) { - await this.#snapClient.trackTransactionRejected(account, origin); + await this.#snapClient.trackTransactionRejected( + account, + origin, + TransactionType.Send, + ); throw new UserActionError('User canceled the confirmation'); } - await this.#snapClient.trackTransactionApproved(account, origin); + await this.#snapClient.trackTransactionApproved( + account, + origin, + TransactionType.Send, + ); } async insertSignPsbt( @@ -200,16 +213,35 @@ export class JSXConfirmationRepository implements ConfirmationRepository { context, ); - await this.#snapClient.trackTransactionAdded(account, origin); + // A PSBT that spends this account's inputs is a send. Anything else + // (a cosign, or a PSBT this account does not fund) stays unknown: + // signing it is not a receive. + const [sent] = account.sentAndReceived(psbt.unsigned_tx); + const transactionType = + sent.to_sat() > 0n ? TransactionType.Send : TransactionType.Unknown; + + await this.#snapClient.trackTransactionAdded( + account, + origin, + transactionType, + ); const confirmed = await this.#snapClient.displayConfirmation(interfaceId); if (!confirmed) { - await this.#snapClient.trackTransactionRejected(account, origin); + await this.#snapClient.trackTransactionRejected( + account, + origin, + transactionType, + ); throw new UserActionError('User canceled the confirmation'); } - await this.#snapClient.trackTransactionApproved(account, origin); + await this.#snapClient.trackTransactionApproved( + account, + origin, + transactionType, + ); } async #getExchangeRate( diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts index ed3985af..c485d9ce 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.test.ts @@ -16,6 +16,7 @@ import { mnemonicPhraseToBytes, SLIP10Node as RealSlip10Node, } from '@metamask/key-tree'; +import { TransactionType } from '@metamask/keyring-api'; import { Signer } from 'bip322-js'; import { mock } from 'jest-mock-extended'; @@ -67,6 +68,21 @@ describe('AccountUseCases', () => { mockMetaProtocols, ); + /** + * Stubs the sent amount on an account so that transactions built from it are + * classified as sends by `mapToTransactionType`. + * + * @param account - The account to stub. + * @param sentBtc - The sent amount in BTC. + */ + const stubSentAmount = (account: BitcoinAccount, sentBtc = 1): void => { + const sentAmount = mock(); + jest.spyOn(sentAmount, 'to_btc').mockReturnValue(sentBtc); + jest + .mocked(account.sentAndReceived) + .mockReturnValue([sentAmount, mock()]); + }; + describe('get', () => { it('returns account', async () => { const mockAccount = mock(); @@ -483,6 +499,7 @@ describe('AccountUseCases', () => { mockAccount, mockTransaction, 'test', + TransactionType.Receive, ); expect(mockSnapClient.emitTrackingEvent).toHaveBeenCalledTimes(1); expect(result).toStrictEqual({ @@ -540,6 +557,7 @@ describe('AccountUseCases', () => { mockAccount, mockTxConfirmed, 'test', + TransactionType.Receive, ); expect(result).toStrictEqual({ account: mockAccount, @@ -601,6 +619,7 @@ describe('AccountUseCases', () => { mockAccount, mockTxConfirmed, origin, + TransactionType.Receive, ); // Check for TransactionReceived event for new transaction @@ -609,6 +628,7 @@ describe('AccountUseCases', () => { mockAccount, mockTxNew, origin, + TransactionType.Receive, ); // Check for TransactionReorged event for reorged transaction @@ -617,6 +637,7 @@ describe('AccountUseCases', () => { mockAccount, mockTxReorged, origin, + TransactionType.Receive, ); expect(mockSnapClient.emitTrackingEvent).toHaveBeenCalledTimes(3); @@ -647,6 +668,7 @@ describe('AccountUseCases', () => { mockAccount, mockTxReorged, 'test', + TransactionType.Receive, ); expect(result).toStrictEqual({ account: mockAccount, @@ -735,6 +757,7 @@ describe('AccountUseCases', () => { mockAccount, mockTransaction, 'test', + TransactionType.Receive, ); // error should be logged @@ -1175,6 +1198,7 @@ describe('AccountUseCases', () => { mockTxBuilder.finish.mockReturnValue(mockFilledPsbt); mockTxBuilder.unspendable.mockReturnThis(); mockChain.getFeeEstimates.mockResolvedValue(mockFeeEstimates); + stubSentAmount(mockAccount); }); it('throws error if account is not found', async () => { @@ -1241,6 +1265,7 @@ describe('AccountUseCases', () => { mockAccount, mockWalletTx, 'metamask', + TransactionType.Send, ); expect(txid).toBe(mockTxid); expect(psbt).toBe('mockSignedPsbt'); @@ -1310,6 +1335,7 @@ describe('AccountUseCases', () => { mockAccount, mockWalletTx, 'metamask', + TransactionType.Send, ); expect(txid).toBe(mockTxid); expect(psbt).toBe('mockSignedPsbt'); @@ -1394,6 +1420,7 @@ describe('AccountUseCases', () => { mockAccount, mockWalletTx, 'metamask', + TransactionType.Send, ); // Error should be logged @@ -2186,6 +2213,7 @@ describe('AccountUseCases', () => { mockTxBuilder.unspendable.mockReturnThis(); mockChain.getFeeEstimates.mockResolvedValue(mockFeeEstimates); mockRepository.getFrozenUTXOs.mockResolvedValue([]); + stubSentAmount(mockAccount); }); it('throws error if there are multiple recipients', async () => { @@ -2249,6 +2277,7 @@ describe('AccountUseCases', () => { mockAccount, mockWalletTx, 'metamask', + TransactionType.Send, ); expect(result.txid).toBe(mockTxid); expect(result.canBeMalleable).toBe(false); @@ -2321,6 +2350,7 @@ describe('AccountUseCases', () => { mockTransaction.compute_txid.mockReturnValue(mockTxid); mockTransaction.clone.mockReturnThis(); mockAccount.getTransaction.mockReturnValue(mockWalletTx); + stubSentAmount(mockAccount); }); it('throws error if account is not found', async () => { diff --git a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts index d49f2b8b..416480dd 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/AccountUseCases.ts @@ -36,6 +36,7 @@ import { NotFoundError, PermissionError, TrackingSnapEvent, + mapToTransactionType, ValidationError, WalletError, } from '../entities'; @@ -510,6 +511,7 @@ export class AccountUseCases { account, tx, origin, + mapToTransactionType(account, tx.tx), ); continue; @@ -530,6 +532,7 @@ export class AccountUseCases { account, tx, origin, + mapToTransactionType(account, tx.tx), ); } else { // if the status was changed, and now it's NOT confirmed @@ -541,6 +544,7 @@ export class AccountUseCases { account, tx, origin, + mapToTransactionType(account, tx.tx), ); } } @@ -1096,6 +1100,9 @@ export class AccountUseCases { origin: string, ): Promise { const txid = tx.compute_txid(); + // Resolve the classification before `applyUnconfirmedTx` takes ownership of + // the underlying wasm transaction; reading `tx` afterwards panics. + const transactionType = mapToTransactionType(account, tx); await this.#chain.broadcast(account.network, tx.clone()); account.applyUnconfirmedTx(tx, getCurrentUnixTimestamp()); await this.#repository.update(account); @@ -1114,6 +1121,7 @@ export class AccountUseCases { account, walletTx, origin, + transactionType, ); } diff --git a/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.test.ts b/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.test.ts index a8697f84..29c86de8 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.test.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.test.ts @@ -4,6 +4,7 @@ import type { Transaction, } from '@metamask/bitcoindevkit'; import { Psbt, Address, Amount } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; import type { GetPreferencesResult } from '@metamask/snaps-sdk'; import { mock } from 'jest-mock-extended'; @@ -1059,10 +1060,12 @@ describe('SendFlowUseCases', () => { expect(mockSnapClient.trackTransactionAdded).toHaveBeenCalledWith( mockAccount, 'metamask', + TransactionType.Send, ); expect(mockSnapClient.trackTransactionApproved).toHaveBeenCalledWith( mockAccount, 'metamask', + TransactionType.Send, ); expect(mockSnapClient.trackTransactionRejected).not.toHaveBeenCalled(); }); @@ -1077,6 +1080,7 @@ describe('SendFlowUseCases', () => { expect(mockSnapClient.trackTransactionRejected).toHaveBeenCalledWith( mockAccount, 'metamask', + TransactionType.Send, ); expect(mockSnapClient.trackTransactionApproved).not.toHaveBeenCalled(); }); diff --git a/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.ts b/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.ts index 19d34cca..5f24d582 100644 --- a/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.ts +++ b/packages/bitcoin-wallet-snap/src/use-cases/SendFlowUseCases.ts @@ -1,5 +1,6 @@ import { Address, Amount, Psbt } from '@metamask/bitcoindevkit'; import type { Network, Transaction } from '@metamask/bitcoindevkit'; +import { TransactionType } from '@metamask/keyring-api'; import { getCurrentUnixTimestamp } from '@metamask/keyring-snap-sdk'; import type { InputChangeEvent } from '@metamask/snaps-sdk'; @@ -142,18 +143,30 @@ export class SendFlowUseCases { const interfaceId = await this.#sendFlowRepository.insertConfirmSendForm(context); - await this.#snapClient.trackTransactionAdded(account, METAMASK_ORIGIN); + await this.#snapClient.trackTransactionAdded( + account, + METAMASK_ORIGIN, + TransactionType.Send, + ); // Blocks and waits for user actions. const confirmed = await this.#snapClient.displayUserPrompt(interfaceId); if (!confirmed) { - await this.#snapClient.trackTransactionRejected(account, METAMASK_ORIGIN); + await this.#snapClient.trackTransactionRejected( + account, + METAMASK_ORIGIN, + TransactionType.Send, + ); throw new UserActionError('User canceled the confirmation'); } - await this.#snapClient.trackTransactionApproved(account, METAMASK_ORIGIN); + await this.#snapClient.trackTransactionApproved( + account, + METAMASK_ORIGIN, + TransactionType.Send, + ); // sign and broadcast const signedPsbt = ( diff --git a/packages/snap-networks-utils/CHANGELOG.md b/packages/snap-networks-utils/CHANGELOG.md index 675cf723..50f7b9e5 100644 --- a/packages/snap-networks-utils/CHANGELOG.md +++ b/packages/snap-networks-utils/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- Populate the optional `transaction_type` property on the `Transaction Added`, `Transaction Approved`, `Transaction Rejected`, and `Transaction Submitted` events tracked by `AnalyticsService`. ([#393](https://github.com/MetaMask/internal-snaps/pull/393)) - Add a shared `EstimatedChanges` Snaps JSX component for transaction confirmations, rendering send/receive asset rows with loading, not-available, and no-changes states ([#369](https://github.com/MetaMask/internal-snaps/pull/369)) - Takes translated `labels`, display-ready `assets` (`EstimatedChangesAsset`), a `scanFetchStatus` (`EstimatedChangesFetchStatus`), and a `scanError` - Add `SynchronizationError`, `formatAccountSyncFailures`, and the `AccountSyncFailure` type, for reporting account synchronization failures with per-account failure details embedded in the error message (details must live in the message because `snap_trackError` only serializes `name`, `message`, `stack`, and `cause`). ([#374](https://github.com/MetaMask/internal-snaps/pull/374)) diff --git a/packages/snap-networks-utils/src/services/analytics/AnalyticsService.test.ts b/packages/snap-networks-utils/src/services/analytics/AnalyticsService.test.ts index 005b3df4..c63967b8 100644 --- a/packages/snap-networks-utils/src/services/analytics/AnalyticsService.test.ts +++ b/packages/snap-networks-utils/src/services/analytics/AnalyticsService.test.ts @@ -1,3 +1,5 @@ +import { TransactionType } from '@metamask/keyring-api'; + import { mockLogger } from '../../utils/logger/__mocks__/Logger'; import { AnalyticsService, @@ -64,13 +66,62 @@ describe('AnalyticsService', () => { }); }); + it.each([ + [ + 'trackTransactionAdded', + TransactionEventType.TransactionAdded, + 'Snap transaction added', + ], + [ + 'trackTransactionRejected', + TransactionEventType.TransactionRejected, + 'Snap transaction rejected', + ], + [ + 'trackTransactionApproved', + TransactionEventType.TransactionApproved, + 'Snap transaction approved', + ], + [ + 'trackTransactionSubmitted', + TransactionEventType.TransactionSubmitted, + 'Snap transaction submitted', + ], + ] as const)( + 'tracks %s with a transaction type', + async (method, event, message) => { + await analytics[method]({ + origin: 'metamask', + accountType: 'bip122:p2wpkh', + chainIdCaip: 'bip122:000000000019d6689c085ae165831e93', + transactionType: TransactionType.Send, + }); + + expect(request).toHaveBeenCalledWith({ + method: 'snap_trackEvent', + params: { + event: { + event, + properties: { + message, + origin: 'metamask', + account_type: 'bip122:p2wpkh', + chain_id_caip: 'bip122:000000000019d6689c085ae165831e93', + transaction_type: 'send', + }, + }, + }, + }); + }, + ); + it('tracks finalized transactions with optional transaction details', async () => { await analytics.trackTransactionFinalized({ origin: 'https://example.com', accountType: 'eip155:eoa', chainIdCaip: 'eip155:1', transactionStatus: 'confirmed', - transactionType: 'send', + transactionType: TransactionType.Send, }); expect(request).toHaveBeenCalledWith({ @@ -159,6 +210,24 @@ describe('AnalyticsService', () => { }); }); + it('does not emit transaction_type on security events', async () => { + // `SecurityAlertDetectedEventProperties` and + // `SecurityScanCompletedEventProperties` must not advertise + // `transactionType`, otherwise a type-valid caller value is discarded. + await analytics.trackSecurityScanCompleted({ + origin: 'https://example.com', + accountType: 'eip155:eoa', + chainIdCaip: 'eip155:1', + scanStatus: 'success', + hasSecurityAlerts: false, + // @ts-expect-error - transactionType is not a security event property. + transactionType: TransactionType.Send, + }); + + const event = request.mock.calls[0]?.[0].params.event; + expect(event.properties).not.toHaveProperty('transaction_type'); + }); + it('tracks WebSocket connection failures', async () => { await analytics.trackWebSocketConnectionClosedNotCleanly({ origin: 'metamask', diff --git a/packages/snap-networks-utils/src/services/analytics/AnalyticsService.ts b/packages/snap-networks-utils/src/services/analytics/AnalyticsService.ts index e8d6d623..d8c5b6c6 100644 --- a/packages/snap-networks-utils/src/services/analytics/AnalyticsService.ts +++ b/packages/snap-networks-utils/src/services/analytics/AnalyticsService.ts @@ -1,3 +1,4 @@ +import type { TransactionType } from '@metamask/keyring-api'; import type { Json } from '@metamask/snaps-sdk'; import type { TrackErrorFn } from '../../utils/errors'; @@ -44,29 +45,42 @@ export type AnalyticsServiceOptions< trackError: TrackErrorFn; }; -export type TransactionEventProperties = { +/** + * Fields shared by every event tied to an account on a chain. + */ +export type AccountEventProperties = { origin: string; accountType: string; chainIdCaip: string; }; +/** + * Properties of a transaction lifecycle event. + * + * `transactionType` is the optional classification of the transaction. + * + * Security events intentionally do not extend this type: they are not tied to a + * transaction classification, so advertising `transactionType` there would let + * callers pass a value that is silently discarded. + */ +export type TransactionEventProperties = AccountEventProperties & { + transactionType?: `${TransactionType}`; +}; + export type TransactionFinalizedEventProperties = TransactionEventProperties & { transactionStatus?: string; - transactionType?: string; }; -export type SecurityAlertDetectedEventProperties = - TransactionEventProperties & { - securityAlertResponse: string; - securityAlertReason: string | null; - securityAlertDescription: string; - }; +export type SecurityAlertDetectedEventProperties = AccountEventProperties & { + securityAlertResponse: string; + securityAlertReason: string | null; + securityAlertDescription: string; +}; -export type SecurityScanCompletedEventProperties = - TransactionEventProperties & { - scanStatus: string; - hasSecurityAlerts: boolean; - }; +export type SecurityScanCompletedEventProperties = AccountEventProperties & { + scanStatus: string; + hasSecurityAlerts: boolean; +}; export type WebSocketConnectionClosedEventProperties = { origin: string; @@ -143,6 +157,7 @@ export class AnalyticsService< * @param properties.origin - The origin of the request. * @param properties.accountType - The type of account. * @param properties.chainIdCaip - The CAIP-2 chain ID. + * @param properties.transactionType - Optional transaction type. */ async trackTransactionAdded( properties: TransactionEventProperties, @@ -161,6 +176,7 @@ export class AnalyticsService< * @param properties.origin - The origin of the request. * @param properties.accountType - The type of account. * @param properties.chainIdCaip - The CAIP-2 chain ID. + * @param properties.transactionType - Optional transaction type. */ async trackTransactionRejected( properties: TransactionEventProperties, @@ -179,6 +195,7 @@ export class AnalyticsService< * @param properties.origin - The origin of the request. * @param properties.accountType - The type of account. * @param properties.chainIdCaip - The CAIP-2 chain ID. + * @param properties.transactionType - Optional transaction type. */ async trackTransactionApproved( properties: TransactionEventProperties, @@ -197,6 +214,7 @@ export class AnalyticsService< * @param properties.origin - The origin of the request. * @param properties.accountType - The type of account. * @param properties.chainIdCaip - The CAIP-2 chain ID. + * @param properties.transactionType - Optional transaction type. */ async trackTransactionSubmitted( properties: TransactionEventProperties, @@ -320,16 +338,39 @@ export class AnalyticsService< }); } + /** + * Track a transaction lifecycle event. + * + * Centralizes the shared payload so the common properties only have to change + * in one place. `transaction_type` is added only when the emitting Snap could + * classify the transaction. + * + * @param event - Event name. + * @param message - Human-readable event message. + * @param properties - Event properties. + * @param properties.origin - The origin of the request. + * @param properties.accountType - The type of account. + * @param properties.chainIdCaip - The CAIP-2 chain ID. + * @param properties.transactionType - Optional transaction type. + */ async #trackTransactionEvent( event: TransactionEventType, message: string, - { origin, accountType, chainIdCaip }: TransactionEventProperties, + { + origin, + accountType, + chainIdCaip, + transactionType, + }: TransactionEventProperties, ): Promise { await this.trackEvent(event, { message, origin, account_type: accountType, chain_id_caip: chainIdCaip, + ...(transactionType === undefined + ? {} + : { transaction_type: transactionType }), }); } }