From c7fad8f4004023ad6a45dd61e42b3cfdbf7d17db Mon Sep 17 00:00:00 2001 From: Whymarrh Whitby Date: Thu, 16 Jul 2020 17:52:41 -0230 Subject: [PATCH] Limit number of transactions passed outside of TransactionController (#9010) Refs #8572 Refs #8991 This change limits the number of transactions (`txMeta`s) that are passed outside of the `TransactionController`, resulting in shorter serialization and deserialization times when state is moved between the background and UI contexts. `TransactionController#_updateMemstore` --------------------------------------- The `currentNetworkTxList` state of the `TransactionController` is used externally (i.e. outside of the controller) as the canonical source for the full transaction history. Prior to this change, the method would iterate the full transaction history and possibly return all of it. This change limits it to `MAX_MEMSTORE_TX_LIST_SIZE` to make sure that: 1. Calls to `_updateMemstore` are fast(er) 2. Passing `currentNetworkTxList` around is fast(er) (Shown in #8377, `_updateMemstore`, is called _frequently_ when a transaction is pending.) The list is iterated backwards because it is possible that new transactions are at the end of the list. [1] Results ------- In profiles before this change, with ~3k transactions locally, `PortDuplexStream._onMessage` took up to ~4.5s to complete when the set of transactions is included. [2] In profiles after this change, `PortDuplexStream._onMessage` took ~90ms to complete. [3] Before vs. after profile screenshots: ![Profile 1][2] ![Profile 2][3] [1]:https://github.com/MetaMask/metamask-extension/blob/5a3ae85b728096cb45c8cc6822249eed5555ee25/app/scripts/controllers/transactions/tx-state-manager.js#L172-L174 [2]:https://user-images.githubusercontent.com/1623628/87613203-36f51d80-c6e7-11ea-89bc-11a1cc2f3b1e.png [3]:https://user-images.githubusercontent.com/1623628/87613215-3bb9d180-c6e7-11ea-8d85-aff3acbd0374.png [8337]:https://github.com/MetaMask/metamask-extension/issues/8377 [8572]:https://github.com/MetaMask/metamask-extension/issues/8572 [8991]:https://github.com/MetaMask/metamask-extension/issues/8991 --- app/scripts/controllers/transactions/index.js | 5 +- .../transactions/tx-state-manager.js | 35 ++- .../transactions/tx-state-manager-test.js | 214 ++++++++++++++++++ 3 files changed, 247 insertions(+), 7 deletions(-) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 046925b1d..ed851fcc9 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -37,6 +37,7 @@ import { hexToBn, bnToHex, BnMultiplyByFraction } from '../../lib/util' import { TRANSACTION_NO_CONTRACT_ERROR_KEY } from '../../../../ui/app/helpers/constants/error-keys' const SIMPLE_GAS_COST = '0x5208' // Hex for 21000, cost of a simple send. +const MAX_MEMSTORE_TX_LIST_SIZE = 100 // Number of transactions (by unique nonces) to keep in memory /** Transaction Controller is an aggregate of sub-controllers and trackers @@ -789,9 +790,7 @@ export default class TransactionController extends EventEmitter { */ _updateMemstore () { const unapprovedTxs = this.txStateManager.getUnapprovedTxList() - const currentNetworkTxList = this.txStateManager.getFilteredTxList({ - metamaskNetworkId: this.getNetwork(), - }) + const currentNetworkTxList = this.txStateManager.getTxList(MAX_MEMSTORE_TX_LIST_SIZE) this.memStore.updateState({ unapprovedTxs, currentNetworkTxList }) } } diff --git a/app/scripts/controllers/transactions/tx-state-manager.js b/app/scripts/controllers/transactions/tx-state-manager.js index 1489a3bb1..ca07d3e88 100644 --- a/app/scripts/controllers/transactions/tx-state-manager.js +++ b/app/scripts/controllers/transactions/tx-state-manager.js @@ -57,12 +57,39 @@ export default class TransactionStateManager extends EventEmitter { } /** - @returns {array} - of txMetas that have been filtered for only the current network - */ - getTxList () { + * Returns the full tx list for the current network + * + * The list is iterated backwards as new transactions are pushed onto it. + * + * @param {number} [limit] a limit for the number of transactions to return + * @returns {Object[]} The {@code txMeta}s, filtered to the current network + */ + getTxList (limit) { const network = this.getNetwork() const fullTxList = this.getFullTxList() - return fullTxList.filter((txMeta) => txMeta.metamaskNetworkId === network) + + const nonces = new Set() + const txs = [] + for (let i = fullTxList.length - 1; i > -1; i--) { + const txMeta = fullTxList[i] + if (txMeta.metamaskNetworkId !== network) { + continue + } + + if (limit !== undefined) { + const { nonce } = txMeta.txParams + if (!nonces.has(nonce)) { + if (nonces.size < limit) { + nonces.add(nonce) + } else { + continue + } + } + } + + txs.unshift(txMeta) + } + return txs } /** diff --git a/test/unit/app/controllers/transactions/tx-state-manager-test.js b/test/unit/app/controllers/transactions/tx-state-manager-test.js index 7f0a53981..32acfb408 100644 --- a/test/unit/app/controllers/transactions/tx-state-manager-test.js +++ b/test/unit/app/controllers/transactions/tx-state-manager-test.js @@ -85,6 +85,220 @@ describe('TransactionStateManager', function () { assert.ok(Array.isArray(result)) assert.equal(result.length, 0) }) + + it('should return a full list of transactions', function () { + const submittedTx = { + id: 0, + metamaskNetworkId: currentNetworkId, + time: 0, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x0', + }, + status: 'submitted', + } + + const confirmedTx = { + id: 3, + metamaskNetworkId: currentNetworkId, + time: 3, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x3', + }, + status: 'confirmed', + } + + const txm = new TxStateManager({ + initState: { + transactions: [ + submittedTx, + confirmedTx, + ], + }, + getNetwork: () => currentNetworkId, + }) + + assert.deepEqual(txm.getTxList(), [ + submittedTx, + confirmedTx, + ]) + }) + + it('should return a list of transactions, limited by N unique nonces when there are NO duplicates', function () { + const submittedTx0 = { + id: 0, + metamaskNetworkId: currentNetworkId, + time: 0, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x0', + }, + status: 'submitted', + } + + const unapprovedTx1 = { + id: 1, + metamaskNetworkId: currentNetworkId, + time: 1, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x1', + }, + status: 'unapproved', + } + + const approvedTx2 = { + id: 2, + metamaskNetworkId: currentNetworkId, + time: 2, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x2', + }, + status: 'approved', + } + + const confirmedTx3 = { + id: 3, + metamaskNetworkId: currentNetworkId, + time: 3, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x3', + }, + status: 'confirmed', + } + + const txm = new TxStateManager({ + initState: { + transactions: [ + submittedTx0, + unapprovedTx1, + approvedTx2, + confirmedTx3, + ], + }, + getNetwork: () => currentNetworkId, + }) + + assert.deepEqual(txm.getTxList(2), [ + approvedTx2, + confirmedTx3, + ]) + }) + + it('should return a list of transactions, limited by N unique nonces when there ARE duplicates', function () { + const submittedTx0s = [ + { + id: 0, + metamaskNetworkId: currentNetworkId, + time: 0, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x0', + }, + status: 'submitted', + }, + { + id: 0, + metamaskNetworkId: currentNetworkId, + time: 0, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x0', + }, + status: 'submitted', + }, + ] + + const unapprovedTx1 = { + id: 1, + metamaskNetworkId: currentNetworkId, + time: 1, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x1', + }, + status: 'unapproved', + } + + const approvedTx2s = [ + { + id: 2, + metamaskNetworkId: currentNetworkId, + time: 2, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x2', + }, + status: 'approved', + }, + { + id: 2, + metamaskNetworkId: currentNetworkId, + time: 2, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x2', + }, + status: 'approved', + }, + ] + + const failedTx3s = [ + { + id: 3, + metamaskNetworkId: currentNetworkId, + time: 3, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x3', + }, + status: 'failed', + }, + { + id: 3, + metamaskNetworkId: currentNetworkId, + time: 3, + txParams: { + from: '0xAddress', + to: '0xRecipient', + nonce: '0x3', + }, + status: 'failed', + }, + ] + + const txm = new TxStateManager({ + initState: { + transactions: [ + ...submittedTx0s, + unapprovedTx1, + ...approvedTx2s, + ...failedTx3s, + ], + }, + getNetwork: () => currentNetworkId, + }) + + assert.deepEqual(txm.getTxList(2), [ + ...approvedTx2s, + ...failedTx3s, + ]) + }) }) describe('#addTx', function () {