From d5ab1e15916fdd27f5ede3b6fbc44d3302ef0527 Mon Sep 17 00:00:00 2001 From: ryanml Date: Thu, 24 Jun 2021 12:00:54 -0700 Subject: [PATCH] Adding metric events for Approved, Rejected, and Submitted to the TxController (#11358) --- app/scripts/controllers/transactions/index.js | 153 ++++++++--- .../controllers/transactions/index.test.js | 254 +++++++++++++----- 2 files changed, 311 insertions(+), 96 deletions(-) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index a544c222f..3f10c4ba3 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -40,6 +40,14 @@ const hstInterface = new ethers.utils.Interface(abi); const MAX_MEMSTORE_TX_LIST_SIZE = 100; // Number of transactions (by unique nonces) to keep in memory +export const TRANSACTION_EVENTS = { + ADDED: 'Transaction Added', + APPROVED: 'Transaction Approved', + FINALIZED: 'Transaction Finalized', + REJECTED: 'Transaction Rejected', + SUBMITTED: 'Transaction Submitted', +}; + /** Transaction Controller is an aggregate of sub-controllers and trackers composing them in a way to be exposed to the metamask controller @@ -202,32 +210,9 @@ export default class TransactionController extends EventEmitter { @emits ${txMeta.id}:unapproved */ addTransaction(txMeta) { - const { - type, - status, - chainId, - origin: referrer, - txParams: { gasPrice }, - metamaskNetworkId: network, - } = txMeta; - const source = referrer === 'metamask' ? 'user' : 'dapp'; - this.txStateManager.addTransaction(txMeta); this.emit(`${txMeta.id}:unapproved`, txMeta); - - this._trackMetaMetricsEvent({ - event: 'Transaction Added', - category: 'Transactions', - sensitiveProperties: { - type, - status, - gasPrice, - referrer, - source, - network, - chain_id: chainId, - }, - }); + this._trackTransactionMetricsEvent(txMeta, TRANSACTION_EVENTS.ADDED); } /** @@ -607,12 +592,13 @@ export default class TransactionController extends EventEmitter { // sign transaction const rawTx = await this.signTransaction(txId); await this.publishTransaction(txId, rawTx); + this._trackTransactionMetricsEvent(txMeta, TRANSACTION_EVENTS.APPROVED); // must set transaction to submitted/failed before releasing lock nonceLock.releaseLock(); } catch (err) { // this is try-catch wrapped so that we can guarantee that the nonceLock is released try { - this.txStateManager.setTxStatusFailed(txId, err); + this._failTransaction(txId, err); } catch (err2) { log.error(err2); } @@ -695,6 +681,11 @@ export default class TransactionController extends EventEmitter { this.setTxHash(txId, txHash); this.txStateManager.setTxStatusSubmitted(txId); + + const { gas } = txMeta.txParams; + this._trackTransactionMetricsEvent(txMeta, TRANSACTION_EVENTS.SUBMITTED, { + gas_limit: gas, + }); } /** @@ -727,6 +718,29 @@ export default class TransactionController extends EventEmitter { this.txStateManager.setTxStatusConfirmed(txId); this._markNonceDuplicatesDropped(txId); + const { submittedTime } = txMeta; + const { blockNumber } = txReceipt; + const metricsParams = { gas_used: gasUsed }; + const completionTime = await this._getTransactionCompletionTime( + blockNumber, + submittedTime, + ); + + if (completionTime) { + metricsParams.completion_time = completionTime; + } + + if (txReceipt.status === '0x0') { + metricsParams.status = 'failed on-chain'; + // metricsParams.error = TODO: figure out a way to get the on-chain failure reason + } + + this._trackTransactionMetricsEvent( + txMeta, + TRANSACTION_EVENTS.FINALIZED, + metricsParams, + ); + this.txStateManager.updateTransaction( txMeta, 'transactions#confirmTransaction - add txReceipt', @@ -760,7 +774,9 @@ export default class TransactionController extends EventEmitter { @returns {Promise} */ async cancelTransaction(txId) { + const txMeta = this.txStateManager.getTransaction(txId); this.txStateManager.setTxStatusRejected(txId); + this._trackTransactionMetricsEvent(txMeta, TRANSACTION_EVENTS.REJECTED); } /** @@ -843,7 +859,7 @@ export default class TransactionController extends EventEmitter { txMeta, 'failed to estimate gas during boot cleanup.', ); - this.txStateManager.setTxStatusFailed(txMeta.id, error); + this._failTransaction(txMeta.id, error); }); }); @@ -857,7 +873,7 @@ export default class TransactionController extends EventEmitter { const txSignError = new Error( 'Transaction found as "approved" during boot - possibly stuck during signing', ); - this.txStateManager.setTxStatusFailed(txMeta.id, txSignError); + this._failTransaction(txMeta.id, txSignError); }); } @@ -877,17 +893,15 @@ export default class TransactionController extends EventEmitter { 'transactions/pending-tx-tracker#event: tx:warning', ); }); - this.pendingTxTracker.on( - 'tx:failed', - this.txStateManager.setTxStatusFailed.bind(this.txStateManager), - ); + this.pendingTxTracker.on('tx:failed', (txId, error) => { + this._failTransaction(txId, error); + }); this.pendingTxTracker.on('tx:confirmed', (txId, transactionReceipt) => this.confirmTransaction(txId, transactionReceipt), ); - this.pendingTxTracker.on( - 'tx:dropped', - this.txStateManager.setTxStatusDropped.bind(this.txStateManager), - ); + this.pendingTxTracker.on('tx:dropped', (txId) => { + this._dropTransaction(txId); + }); this.pendingTxTracker.on('tx:block-update', (txMeta, latestBlockNumber) => { if (!txMeta.firstRetryBlockNumber) { txMeta.firstRetryBlockNumber = latestBlockNumber; @@ -997,7 +1011,7 @@ export default class TransactionController extends EventEmitter { txMeta, 'transactions/pending-tx-tracker#event: tx:confirmed reference to confirmed txHash with same nonce', ); - this.txStateManager.setTxStatusDropped(otherTxMeta.id); + this._dropTransaction(otherTxMeta.id); }); } @@ -1090,4 +1104,71 @@ export default class TransactionController extends EventEmitter { } } } + + /** + * Extracts relevant properties from a transaction meta + * object and uses them to create and send metrics for various transaction + * events. + * @param {Object} txMeta - the txMeta object + * @param {string} event - the name of the transaction event + * @param {Object} extraParams - optional props and values to include in sensitiveProperties + */ + _trackTransactionMetricsEvent(txMeta, event, extraParams = {}) { + const { + type, + time, + status, + chainId, + origin: referrer, + txParams: { gasPrice }, + metamaskNetworkId: network, + } = txMeta; + const source = referrer === 'metamask' ? 'user' : 'dapp'; + + this._trackMetaMetricsEvent({ + event, + category: 'Transactions', + sensitiveProperties: { + type, + status, + referrer, + source, + network, + chain_id: chainId, + gas_price: gasPrice, + first_seen: time, + ...extraParams, + }, + }); + } + + async _getTransactionCompletionTime(blockNumber, submittedTime) { + const transactionBlock = await this.query.getBlockByNumber( + blockNumber.toString(16), + false, + ); + + if (!transactionBlock) { + return ''; + } + + return new BigNumber(transactionBlock.timestamp, 10) + .minus(submittedTime / 1000) + .round() + .toString(10); + } + + _failTransaction(txId, error) { + this.txStateManager.setTxStatusFailed(txId, error); + const txMeta = this.txStateManager.getTransaction(txId); + this._trackTransactionMetricsEvent(txMeta, TRANSACTION_EVENTS.FINALIZED, { + error: error.message, + }); + } + + _dropTransaction(txId) { + this.txStateManager.setTxStatusDropped(txId); + const txMeta = this.txStateManager.getTransaction(txId); + this._trackTransactionMetricsEvent(txMeta, TRANSACTION_EVENTS.FINALIZED); + } } diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index 54a1f7271..c60c5d6ea 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -15,7 +15,7 @@ import { } from '../../../../shared/constants/transaction'; import { SECOND } from '../../../../shared/constants/time'; import { METAMASK_CONTROLLER_EVENTS } from '../../metamask-controller'; -import TransactionController from '.'; +import TransactionController, { TRANSACTION_EVENTS } from '.'; const noop = () => true; const currentNetworkId = '42'; @@ -419,17 +419,17 @@ describe('Transaction Controller', function () { }); describe('#addTransaction', function () { - let trackMetaMetricsEventSpy; + let trackTransactionMetricsEventSpy; beforeEach(function () { - trackMetaMetricsEventSpy = sinon.spy( + trackTransactionMetricsEventSpy = sinon.spy( txController, - '_trackMetaMetricsEvent', + '_trackTransactionMetricsEvent', ); }); afterEach(function () { - trackMetaMetricsEventSpy.restore(); + trackTransactionMetricsEventSpy.restore(); }); it('should emit updates', function (done) { @@ -470,7 +470,7 @@ describe('Transaction Controller', function () { txController.addTransaction(txMeta); }); - it('should call _trackMetaMetricsEvent with the correct payload (one)', function () { + it('should call _trackTransactionMetricsEvent with the correct params', function () { const txMeta = { id: 1, status: TRANSACTION_STATUSES.UNAPPROVED, @@ -484,65 +484,20 @@ describe('Transaction Controller', function () { type: 'sentEther', origin: 'metamask', chainId: currentChainId, + time: 1624408066355, metamaskNetworkId: currentNetworkId, }; - const expectedPayload = { - event: 'Transaction Added', - category: 'Transactions', - sensitiveProperties: { - chain_id: '0x2a', - gasPrice: '0x77359400', - network: '42', - referrer: 'metamask', - source: 'user', - status: 'unapproved', - type: 'sentEther', - }, - }; txController.addTransaction(txMeta); - assert.equal(trackMetaMetricsEventSpy.callCount, 1); - assert.deepEqual( - trackMetaMetricsEventSpy.getCall(0).args[0], - expectedPayload, - ); - }); - it('should call _trackMetaMetricsEvent with the correct payload (two)', function () { - const txMeta = { - id: 1, - status: TRANSACTION_STATUSES.UNAPPROVED, - txParams: { - from: fromAccount.address, - to: '0x1678a085c290ebd122dc42cba69373b5953b831d', - gasPrice: '0x77359400', - gas: '0x7b0d', - nonce: '0x4b', - }, - type: 'sentEther', - origin: 'other', - chainId: '0x3', - metamaskNetworkId: '3', - }; - const expectedPayload = { - event: 'Transaction Added', - category: 'Transactions', - sensitiveProperties: { - chain_id: '0x3', - gasPrice: '0x77359400', - network: '3', - referrer: 'other', - source: 'dapp', - status: 'unapproved', - type: 'sentEther', - }, - }; - - txController.addTransaction(txMeta); - assert.equal(trackMetaMetricsEventSpy.callCount, 1); + assert.equal(trackTransactionMetricsEventSpy.callCount, 1); assert.deepEqual( - trackMetaMetricsEventSpy.getCall(0).args[0], - expectedPayload, + trackTransactionMetricsEventSpy.getCall(0).args[0], + txMeta, + ); + assert.equal( + trackTransactionMetricsEventSpy.getCall(0).args[1], + TRANSACTION_EVENTS.ADDED, ); }); }); @@ -817,7 +772,8 @@ describe('Transaction Controller', function () { }); describe('#publishTransaction', function () { - let hash, txMeta; + let hash, txMeta, trackTransactionMetricsEventSpy; + beforeEach(function () { hash = '0x2a5523c6fa98b47b7d9b6c8320179785150b42a16bcff36b398c5062b65657e8'; @@ -825,12 +781,21 @@ describe('Transaction Controller', function () { id: 1, status: TRANSACTION_STATUSES.UNAPPROVED, txParams: { + gas: '0x7b0d', to: VALID_ADDRESS, from: VALID_ADDRESS_TWO, }, metamaskNetworkId: currentNetworkId, }; providerResultStub.eth_sendRawTransaction = hash; + trackTransactionMetricsEventSpy = sinon.spy( + txController, + '_trackTransactionMetricsEvent', + ); + }); + + afterEach(function () { + trackTransactionMetricsEventSpy.restore(); }); it('should publish a tx, updates the rawTx when provided a one', async function () { @@ -858,6 +823,25 @@ describe('Transaction Controller', function () { ); assert.equal(publishedTx.status, TRANSACTION_STATUSES.SUBMITTED); }); + + it('should call _trackTransactionMetricsEvent with the correct params', async function () { + const rawTx = + '0x477b2e6553c917af0db0388ae3da62965ff1a184558f61b749d1266b2e6d024c'; + txController.txStateManager.addTransaction(txMeta); + await txController.publishTransaction(txMeta.id, rawTx); + assert.equal(trackTransactionMetricsEventSpy.callCount, 1); + assert.deepEqual( + trackTransactionMetricsEventSpy.getCall(0).args[0], + txMeta, + ); + assert.equal( + trackTransactionMetricsEventSpy.getCall(0).args[1], + TRANSACTION_EVENTS.SUBMITTED, + ); + assert.deepEqual(trackTransactionMetricsEventSpy.getCall(0).args[2], { + gas_limit: txMeta.txParams.gas, + }); + }); }); describe('#_markNonceDuplicatesDropped', function () { @@ -1199,4 +1183,154 @@ describe('Transaction Controller', function () { ); }); }); + + describe('#_trackTransactionMetricsEvent', function () { + let trackMetaMetricsEventSpy; + + beforeEach(function () { + trackMetaMetricsEventSpy = sinon.spy( + txController, + '_trackMetaMetricsEvent', + ); + }); + + afterEach(function () { + trackMetaMetricsEventSpy.restore(); + }); + + it('should call _trackMetaMetricsEvent with the correct payload (user source)', function () { + const txMeta = { + id: 1, + status: TRANSACTION_STATUSES.UNAPPROVED, + txParams: { + from: fromAccount.address, + to: '0x1678a085c290ebd122dc42cba69373b5953b831d', + gasPrice: '0x77359400', + gas: '0x7b0d', + nonce: '0x4b', + }, + type: 'sentEther', + origin: 'metamask', + chainId: currentChainId, + time: 1624408066355, + metamaskNetworkId: currentNetworkId, + }; + const expectedPayload = { + event: 'Transaction Added', + category: 'Transactions', + sensitiveProperties: { + chain_id: '0x2a', + gas_price: '0x77359400', + first_seen: 1624408066355, + network: '42', + referrer: 'metamask', + source: 'user', + status: 'unapproved', + type: 'sentEther', + }, + }; + + txController._trackTransactionMetricsEvent( + txMeta, + TRANSACTION_EVENTS.ADDED, + ); + assert.equal(trackMetaMetricsEventSpy.callCount, 1); + assert.deepEqual( + trackMetaMetricsEventSpy.getCall(0).args[0], + expectedPayload, + ); + }); + + it('should call _trackMetaMetricsEvent with the correct payload (dapp source)', function () { + const txMeta = { + id: 1, + status: TRANSACTION_STATUSES.UNAPPROVED, + txParams: { + from: fromAccount.address, + to: '0x1678a085c290ebd122dc42cba69373b5953b831d', + gasPrice: '0x77359400', + gas: '0x7b0d', + nonce: '0x4b', + }, + type: 'sentEther', + origin: 'other', + chainId: currentChainId, + time: 1624408066355, + metamaskNetworkId: currentNetworkId, + }; + const expectedPayload = { + event: 'Transaction Added', + category: 'Transactions', + sensitiveProperties: { + chain_id: '0x2a', + gas_price: '0x77359400', + first_seen: 1624408066355, + network: '42', + referrer: 'other', + source: 'dapp', + status: 'unapproved', + type: 'sentEther', + }, + }; + + txController._trackTransactionMetricsEvent( + txMeta, + TRANSACTION_EVENTS.ADDED, + ); + assert.equal(trackMetaMetricsEventSpy.callCount, 1); + assert.deepEqual( + trackMetaMetricsEventSpy.getCall(0).args[0], + expectedPayload, + ); + }); + + it('should call _trackMetaMetricsEvent with the correct payload (extra params)', function () { + const txMeta = { + id: 1, + status: TRANSACTION_STATUSES.UNAPPROVED, + txParams: { + from: fromAccount.address, + to: '0x1678a085c290ebd122dc42cba69373b5953b831d', + gasPrice: '0x77359400', + gas: '0x7b0d', + nonce: '0x4b', + }, + type: 'sentEther', + origin: 'other', + chainId: currentChainId, + time: 1624408066355, + metamaskNetworkId: currentNetworkId, + }; + const expectedPayload = { + event: 'Transaction Added', + category: 'Transactions', + sensitiveProperties: { + baz: 3.0, + foo: 'bar', + chain_id: '0x2a', + gas_price: '0x77359400', + first_seen: 1624408066355, + network: '42', + referrer: 'other', + source: 'dapp', + status: 'unapproved', + type: 'sentEther', + }, + }; + + txController._trackTransactionMetricsEvent( + txMeta, + TRANSACTION_EVENTS.ADDED, + { + baz: 3.0, + foo: 'bar', + }, + ); + assert.equal(trackMetaMetricsEventSpy.callCount, 1); + assert.deepEqual( + trackMetaMetricsEventSpy.getCall(0).args[0], + expectedPayload, + ); + }); + }); });