refactor incoming tx controller (#10639)

app/scripts/background.js

@@ -70,21 +70,7 @@ if (inTest || process.env.METAMASK_DEBUG) {
 initialize().catch(log.error);
 
 /**
- * An object representing a transaction, in whatever state it is in.
- * @typedef TransactionMeta
- *
- * @property {number} id - An internally unique tx identifier.
- * @property {number} time - Time the tx was first suggested, in unix epoch time (ms).
- * @property {string} status - The current transaction status (unapproved, signed, submitted, dropped, failed, rejected), as defined in `tx-state-manager.js`.
- * @property {string} metamaskNetworkId - The transaction's network ID, used for EIP-155 compliance.
- * @property {boolean} loadingDefaults - TODO: Document
- * @property {Object} txParams - The tx params as passed to the network provider.
- * @property {Object[]} history - A history of mutations to this TransactionMeta object.
- * @property {string} origin - A string representing the interface that suggested the transaction.
- * @property {Object} nonceDetails - A metadata object containing information used to derive the suggested nonce, useful for debugging nonce issues.
- * @property {string} rawTx - A hex string of the final signed transaction, ready to submit to the network.
- * @property {string} hash - A hex string of the transaction hash, used to identify the transaction on the network.
- * @property {number} submittedTime - The time the transaction was submitted to the network, in Unix epoch time (ms).
+ * @typedef {import('../../shared/constants/transaction').TransactionMeta} TransactionMeta
  */
 
 /**

app/scripts/controllers/incoming-transactions.js

@@ -12,21 +12,37 @@ import {
 import {
   CHAIN_ID_TO_NETWORK_ID_MAP,
   CHAIN_ID_TO_TYPE_MAP,
-  GOERLI,
   GOERLI_CHAIN_ID,
-  KOVAN,
   KOVAN_CHAIN_ID,
-  MAINNET,
   MAINNET_CHAIN_ID,
-  RINKEBY,
   RINKEBY_CHAIN_ID,
-  ROPSTEN,
   ROPSTEN_CHAIN_ID,
 } from '../../../shared/constants/network';
-import { NETWORK_EVENTS } from './network';
 
 const fetchWithTimeout = getFetchWithTimeout(30000);
 
+/**
+ * @typedef {import('../../../shared/constants/transaction').TransactionMeta} TransactionMeta
+ */
+
+/**
+ * A transaction object in the format returned by the Etherscan API.
+ *
+ * Note that this is not an exhaustive type definiton; only the properties we use are defined
+ *
+ * @typedef {Object} EtherscanTransaction
+ * @property {string} blockNumber - The number of the block this transaction was found in, in decimal
+ * @property {string} from - The hex-prefixed address of the sender
+ * @property {string} gas - The gas limit, in decimal WEI
+ * @property {string} gasPrice - The gas price, in decimal WEI
+ * @property {string} hash - The hex-prefixed transaction hash
+ * @property {string} isError - Whether the transaction was confirmed or failed (0 for confirmed, 1 for failed)
+ * @property {string} nonce - The transaction nonce, in decimal
+ * @property {string} timeStamp - The timestamp for the transaction, in seconds
+ * @property {string} to - The hex-prefixed address of the recipient
+ * @property {string} value - The amount of ETH sent in this transaction, in decimal WEI
+ */
+
 /**
  * This controller is responsible for retrieving incoming transactions. Etherscan is polled once every block to check
  * for new incoming transactions for the current selected account on the current network
@@ -44,35 +60,37 @@ const etherscanSupportedNetworks = [
 
 export default class IncomingTransactionsController {
   constructor(opts = {}) {
-    const { blockTracker, networkController, preferencesController } = opts;
+    const {
+      blockTracker,
+      onNetworkDidChange,
+      getCurrentChainId,
+      preferencesController,
+    } = opts;
     this.blockTracker = blockTracker;
-    this.networkController = networkController;
+    this.getCurrentChainId = getCurrentChainId;
     this.preferencesController = preferencesController;
 
     this._onLatestBlock = async (newBlockNumberHex) => {
       const selectedAddress = this.preferencesController.getSelectedAddress();
       const newBlockNumberDec = parseInt(newBlockNumberHex, 16);
-      await this._update({
-        address: selectedAddress,
-        newBlockNumberDec,
-      });
+      await this._update(selectedAddress, newBlockNumberDec);
     };
 
     const initState = {
       incomingTransactions: {},
-      incomingTxLastFetchedBlocksByNetwork: {
-        [GOERLI]: null,
-        [KOVAN]: null,
-        [MAINNET]: null,
-        [RINKEBY]: null,
-        [ROPSTEN]: null,
+      incomingTxLastFetchedBlockByChainId: {
+        [GOERLI_CHAIN_ID]: null,
+        [KOVAN_CHAIN_ID]: null,
+        [MAINNET_CHAIN_ID]: null,
+        [RINKEBY_CHAIN_ID]: null,
+        [ROPSTEN_CHAIN_ID]: null,
       },
       ...opts.initState,
     };
     this.store = new ObservableStore(initState);
 
     this.preferencesController.store.subscribe(
-      pairwise((prevState, currState) => {
+      previousValueComparator((prevState, currState) => {
         const {
           featureFlags: {
             showIncomingTransactions: prevShowIncomingTransactions,
@@ -94,29 +112,24 @@ export default class IncomingTransactionsController {
         }
 
         this.start();
-      }),
+      }, this.preferencesController.store.getState()),
     );
 
     this.preferencesController.store.subscribe(
-      pairwise(async (prevState, currState) => {
+      previousValueComparator(async (prevState, currState) => {
         const { selectedAddress: prevSelectedAddress } = prevState;
         const { selectedAddress: currSelectedAddress } = currState;
 
         if (currSelectedAddress === prevSelectedAddress) {
           return;
         }
-
-        await this._update({
-          address: currSelectedAddress,
-        });
-      }),
+        await this._update(currSelectedAddress);
+      }, this.preferencesController.store.getState()),
     );
 
-    this.networkController.on(NETWORK_EVENTS.NETWORK_DID_CHANGE, async () => {
+    onNetworkDidChange(async () => {
       const address = this.preferencesController.getSelectedAddress();
-      await this._update({
-        address,
-      });
+      await this._update(address);
     });
   }
 
@@ -136,85 +149,79 @@ export default class IncomingTransactionsController {
     this.blockTracker.removeListener('latest', this._onLatestBlock);
   }
 
-  async _update({ address, newBlockNumberDec } = {}) {
-    const chainId = this.networkController.getCurrentChainId();
-    if (!etherscanSupportedNetworks.includes(chainId)) {
+  /**
+   * Determines the correct block number to begin looking for new transactions
+   * from, fetches the transactions and then saves them and the next block
+   * number to begin fetching from in state. Block numbers and transactions are
+   * stored per chainId.
+   * @private
+   * @param {string} address - address to lookup transactions for
+   * @param {number} [newBlockNumberDec] - block number to begin fetching from
+   * @returns {void}
+   */
+  async _update(address, newBlockNumberDec) {
+    const chainId = this.getCurrentChainId();
+    if (!etherscanSupportedNetworks.includes(chainId) || !address) {
       return;
     }
     try {
-      const dataForUpdate = await this._getDataForUpdate({
+      const currentState = this.store.getState();
+      const currentBlock = parseInt(this.blockTracker.getCurrentBlock(), 16);
+
+      const mostRecentlyFetchedBlock =
+        currentState.incomingTxLastFetchedBlockByChainId[chainId];
+      const blockToFetchFrom =
+        mostRecentlyFetchedBlock ?? newBlockNumberDec ?? currentBlock;
+
+      const newIncomingTxs = await this._getNewIncomingTransactions(
         address,
+        blockToFetchFrom,
         chainId,
-        newBlockNumberDec,
+      );
+
+      let newMostRecentlyFetchedBlock = blockToFetchFrom;
+
+      newIncomingTxs.forEach((tx) => {
+        if (
+          tx.blockNumber &&
+          parseInt(newMostRecentlyFetchedBlock, 10) <
+            parseInt(tx.blockNumber, 10)
+        ) {
+          newMostRecentlyFetchedBlock = parseInt(tx.blockNumber, 10);
+        }
+      });
+
+      this.store.updateState({
+        incomingTxLastFetchedBlockByChainId: {
+          ...currentState.incomingTxLastFetchedBlockByChainId,
+          [chainId]: newMostRecentlyFetchedBlock + 1,
+        },
+        incomingTransactions: newIncomingTxs.reduce(
+          (transactions, tx) => {
+            transactions[tx.hash] = tx;
+            return transactions;
+          },
+          {
+            ...currentState.incomingTransactions,
+          },
+        ),
       });
-      this._updateStateWithNewTxData(dataForUpdate);
     } catch (err) {
       log.error(err);
     }
   }
 
-  async _getDataForUpdate({ address, chainId, newBlockNumberDec } = {}) {
-    const {
-      incomingTransactions: currentIncomingTxs,
-      incomingTxLastFetchedBlocksByNetwork: currentBlocksByNetwork,
-    } = this.store.getState();
-
-    const lastFetchBlockByCurrentNetwork =
-      currentBlocksByNetwork[CHAIN_ID_TO_TYPE_MAP[chainId]];
-    let blockToFetchFrom = lastFetchBlockByCurrentNetwork || newBlockNumberDec;
-    if (blockToFetchFrom === undefined) {
-      blockToFetchFrom = parseInt(this.blockTracker.getCurrentBlock(), 16);
-    }
-
-    const { latestIncomingTxBlockNumber, txs: newTxs } = await this._fetchAll(
-      address,
-      blockToFetchFrom,
-      chainId,
-    );
-
-    return {
-      latestIncomingTxBlockNumber,
-      newTxs,
-      currentIncomingTxs,
-      currentBlocksByNetwork,
-      fetchedBlockNumber: blockToFetchFrom,
-      chainId,
-    };
-  }
-
-  _updateStateWithNewTxData({
-    latestIncomingTxBlockNumber,
-    newTxs,
-    currentIncomingTxs,
-    currentBlocksByNetwork,
-    fetchedBlockNumber,
-    chainId,
-  }) {
-    const newLatestBlockHashByNetwork = latestIncomingTxBlockNumber
-      ? parseInt(latestIncomingTxBlockNumber, 10) + 1
-      : fetchedBlockNumber + 1;
-    const newIncomingTransactions = {
-      ...currentIncomingTxs,
-    };
-    newTxs.forEach((tx) => {
-      newIncomingTransactions[tx.hash] = tx;
-    });
-
-    this.store.updateState({
-      incomingTxLastFetchedBlocksByNetwork: {
-        ...currentBlocksByNetwork,
-        [CHAIN_ID_TO_TYPE_MAP[chainId]]: newLatestBlockHashByNetwork,
-      },
-      incomingTransactions: newIncomingTransactions,
-    });
-  }
-
-  async _fetchAll(address, fromBlock, chainId) {
-    const fetchedTxResponse = await this._fetchTxs(address, fromBlock, chainId);
-    return this._processTxFetchResponse(fetchedTxResponse);
-  }
-
-  async _fetchTxs(address, fromBlock, chainId) {
+  /**
+   * fetches transactions for the given address and chain, via etherscan, then
+   * processes the data into the necessary shape for usage in this controller.
+   *
+   * @private
+   * @param {string} [address] - Address to fetch transactions for
+   * @param {number} [fromBlock] - Block to look for transactions at
+   * @param {string} [chainId] - The chainId for the current network
+   * @returns {TransactionMeta[]}
+   */
+  async _getNewIncomingTransactions(address, fromBlock, chainId) {
     const etherscanSubdomain =
       chainId === MAINNET_CHAIN_ID
         ? 'api'
@@ -227,16 +234,8 @@ export default class IncomingTransactionsController {
       url += `&startBlock=${parseInt(fromBlock, 10)}`;
     }
     const response = await fetchWithTimeout(url);
-    const parsedResponse = await response.json();
-
-    return {
-      ...parsedResponse,
-      address,
-      chainId,
-    };
-  }
-
-  _processTxFetchResponse({ status, result = [], address, chainId }) {
+    const { status, result } = await response.json();
+    let newIncomingTxs = [];
     if (status === '1' && Array.isArray(result) && result.length > 0) {
       const remoteTxList = {};
       const remoteTxs = [];
@@ -247,70 +246,70 @@ export default class IncomingTransactionsController {
         }
       });
 
-      const incomingTxs = remoteTxs.filter(
+      newIncomingTxs = remoteTxs.filter(
         (tx) => tx.txParams?.to?.toLowerCase() === address.toLowerCase(),
       );
-      incomingTxs.sort((a, b) => (a.time < b.time ? -1 : 1));
-
-      let latestIncomingTxBlockNumber = null;
-      incomingTxs.forEach((tx) => {
-        if (
-          tx.blockNumber &&
-          (!latestIncomingTxBlockNumber ||
-            parseInt(latestIncomingTxBlockNumber, 10) <
-              parseInt(tx.blockNumber, 10))
-        ) {
-          latestIncomingTxBlockNumber = tx.blockNumber;
-        }
-      });
-      return {
-        latestIncomingTxBlockNumber,
-        txs: incomingTxs,
-      };
+      newIncomingTxs.sort((a, b) => (a.time < b.time ? -1 : 1));
     }
-    return {
-      latestIncomingTxBlockNumber: null,
-      txs: [],
-    };
+    return newIncomingTxs;
   }
 
-  _normalizeTxFromEtherscan(txMeta, chainId) {
-    const time = parseInt(txMeta.timeStamp, 10) * 1000;
+  /**
+   * Transmutes a EtherscanTransaction into a TransactionMeta
+   * @param {EtherscanTransaction} etherscanTransaction - the transaction to normalize
+   * @param {string} chainId - The chainId of the current network
+   * @returns {TransactionMeta}
+   */
+  _normalizeTxFromEtherscan(etherscanTransaction, chainId) {
+    const time = parseInt(etherscanTransaction.timeStamp, 10) * 1000;
     const status =
-      txMeta.isError === '0'
+      etherscanTransaction.isError === '0'
         ? TRANSACTION_STATUSES.CONFIRMED
         : TRANSACTION_STATUSES.FAILED;
     return {
-      blockNumber: txMeta.blockNumber,
+      blockNumber: etherscanTransaction.blockNumber,
       id: createId(),
       chainId,
       metamaskNetworkId: CHAIN_ID_TO_NETWORK_ID_MAP[chainId],
       status,
       time,
       txParams: {
-        from: txMeta.from,
-        gas: bnToHex(new BN(txMeta.gas)),
-        gasPrice: bnToHex(new BN(txMeta.gasPrice)),
-        nonce: bnToHex(new BN(txMeta.nonce)),
-        to: txMeta.to,
-        value: bnToHex(new BN(txMeta.value)),
+        from: etherscanTransaction.from,
+        gas: bnToHex(new BN(etherscanTransaction.gas)),
+        gasPrice: bnToHex(new BN(etherscanTransaction.gasPrice)),
+        nonce: bnToHex(new BN(etherscanTransaction.nonce)),
+        to: etherscanTransaction.to,
+        value: bnToHex(new BN(etherscanTransaction.value)),
       },
-      hash: txMeta.hash,
+      hash: etherscanTransaction.hash,
       type: TRANSACTION_TYPES.INCOMING,
     };
   }
 }
 
-function pairwise(fn) {
+/**
+ * Returns a function with arity 1 that caches the argument that the function
+ * is called with and invokes the comparator with both the cached, previous,
+ * value and the current value. If specified, the initialValue will be passed
+ * in as the previous value on the first invocation of the returned method.
+ * @template A
+ * @params {A=} type of compared value
+ * @param {(prevValue: A, nextValue: A) => void} comparator - method to compare
+ *  previous and next values.
+ * @param {A} [initialValue] - initial value to supply to prevValue
+ *  on first call of the method.
+ * @returns {void}
+ */
+function previousValueComparator(comparator, initialValue) {
   let first = true;
   let cache;
   return (value) => {
     try {
       if (first) {
         first = false;
-        return fn(value, value);
+        return comparator(initialValue ?? value, value);
       }
-      return fn(cache, value);
+      return comparator(cache, value);
     } finally {
       cache = value;
     }

app/scripts/lib/setupSentry.js

@@ -31,7 +31,7 @@ export const SENTRY_STATE = {
     featureFlags: true,
     firstTimeFlowType: true,
     forgottenPassword: true,
-    incomingTxLastFetchedBlocksByNetwork: true,
+    incomingTxLastFetchedBlockByChainId: true,
     ipfsGateway: true,
     isAccountMenuOpen: true,
     isInitialized: true,

app/scripts/metamask-controller.js

@@ -189,7 +189,13 @@ export default class MetamaskController extends EventEmitter {
 
     this.incomingTransactionsController = new IncomingTransactionsController({
       blockTracker: this.blockTracker,
-      networkController: this.networkController,
+      onNetworkDidChange: this.networkController.on.bind(
+        this.networkController,
+        NETWORK_EVENTS.NETWORK_DID_CHANGE,
+      ),
+      getCurrentChainId: this.networkController.getCurrentChainId.bind(
+        this.networkController,
+      ),
       preferencesController: this.preferencesController,
       initState: initState.IncomingTransactionsController,
     });

app/scripts/migrations/055.js

@@ -0,0 +1,32 @@
+import { cloneDeep, mapKeys } from 'lodash';
+import { NETWORK_TYPE_TO_ID_MAP } from '../../../shared/constants/network';
+
+const version = 55;
+
+/**
+ * replace 'incomingTxLastFetchedBlocksByNetwork' with 'incomingTxLastFetchedBlockByChainId'
+ */
+export default {
+  version,
+  async migrate(originalVersionedData) {
+    const versionedData = cloneDeep(originalVersionedData);
+    versionedData.meta.version = version;
+    const state = versionedData.data;
+    versionedData.data = transformState(state);
+    return versionedData;
+  },
+};
+
+function transformState(state) {
+  if (
+    state?.IncomingTransactionsController?.incomingTxLastFetchedBlocksByNetwork
+  ) {
+    state.IncomingTransactionsController.incomingTxLastFetchedBlockByChainId = mapKeys(
+      state.IncomingTransactionsController.incomingTxLastFetchedBlocksByNetwork,
+      (_, key) => NETWORK_TYPE_TO_ID_MAP[key].chainId,
+    );
+    delete state.IncomingTransactionsController
+      .incomingTxLastFetchedBlocksByNetwork;
+  }
+  return state;
+}

app/scripts/migrations/055.test.js

@@ -0,0 +1,96 @@
+import { strict as assert } from 'assert';
+import {
+  GOERLI,
+  GOERLI_CHAIN_ID,
+  KOVAN,
+  KOVAN_CHAIN_ID,
+  MAINNET,
+  MAINNET_CHAIN_ID,
+  RINKEBY,
+  RINKEBY_CHAIN_ID,
+  ROPSTEN,
+  ROPSTEN_CHAIN_ID,
+} from '../../../shared/constants/network';
+import migration55 from './055';
+
+describe('migration #55', function () {
+  it('should update the version metadata', async function () {
+    const oldStorage = {
+      meta: {
+        version: 54,
+      },
+      data: {},
+    };
+
+    const newStorage = await migration55.migrate(oldStorage);
+    assert.deepEqual(newStorage.meta, {
+      version: 55,
+    });
+  });
+
+  it('should replace incomingTxLastFetchedBlocksByNetwork with incomingTxLastFetchedBlockByChainId, and carry over old values', async function () {
+    const oldStorage = {
+      meta: {},
+      data: {
+        IncomingTransactionsController: {
+          incomingTransactions: {
+            test: {
+              transactionCategory: 'incoming',
+              txParams: {
+                foo: 'bar',
+              },
+            },
+          },
+          incomingTxLastFetchedBlocksByNetwork: {
+            [MAINNET]: 1,
+            [ROPSTEN]: 2,
+            [RINKEBY]: 3,
+            [GOERLI]: 4,
+            [KOVAN]: 5,
+          },
+        },
+        foo: 'bar',
+      },
+    };
+
+    const newStorage = await migration55.migrate(oldStorage);
+    assert.deepEqual(newStorage.data, {
+      IncomingTransactionsController: {
+        incomingTransactions:
+          oldStorage.data.IncomingTransactionsController.incomingTransactions,
+        incomingTxLastFetchedBlockByChainId: {
+          [MAINNET_CHAIN_ID]: 1,
+          [ROPSTEN_CHAIN_ID]: 2,
+          [RINKEBY_CHAIN_ID]: 3,
+          [GOERLI_CHAIN_ID]: 4,
+          [KOVAN_CHAIN_ID]: 5,
+        },
+      },
+      foo: 'bar',
+    });
+  });
+
+  it('should do nothing if incomingTxLastFetchedBlocksByNetwork key is not populated', async function () {
+    const oldStorage = {
+      meta: {},
+      data: {
+        IncomingTransactionsController: {
+          foo: 'baz',
+        },
+        foo: 'bar',
+      },
+    };
+
+    const newStorage = await migration55.migrate(oldStorage);
+    assert.deepEqual(oldStorage.data, newStorage.data);
+  });
+  it('should do nothing if state is empty', async function () {
+    const oldStorage = {
+      meta: {},
+      data: {},
+    };
+
+    const newStorage = await migration55.migrate(oldStorage);
+    assert.deepEqual(oldStorage.data, newStorage.data);
+  });
+});

app/scripts/migrations/index.js

@@ -59,6 +59,7 @@ const migrations = [
   require('./052').default,
   require('./053').default,
   require('./054').default,
+  require('./055').default,
 ];
 
 export default migrations;

shared/constants/transaction.js

@@ -30,6 +30,12 @@
  *  the same nonce and higher gas fees.
  */
 
+/**
+ * This type will work anywhere you expect a string that can be one of the
+ * above transaction types.
+ * @typedef {TransactionTypes[keyof TransactionTypes]} TransactionTypeString
+ */
+
 /**
  * @type {TransactionTypes}
  */
@@ -65,6 +71,12 @@ export const TRANSACTION_TYPES = {
  * @property {'confirmed'} CONFIRMED - The transaction was confirmed by the network
  */
 
+/**
+ * This type will work anywhere you expect a string that can be one of the
+ * above transaction statuses.
+ * @typedef {TransactionStatuses[keyof TransactionStatuses]} TransactionStatusString
+ */
+
 /**
  * @type {TransactionStatuses}
  */
@@ -132,3 +144,45 @@ export const TRANSACTION_GROUP_CATEGORIES = {
   SIGNATURE_REQUEST: 'signature-request',
   SWAP: 'swap',
 };
+
+/**
+ * @typedef {Object} TxParams
+ * @property {string} from - The address the transaction is sent from
+ * @property {string} to - The address the transaction is sent to
+ * @property {string} value - The amount of wei, in hexadecimal, to send
+ * @property {number} nonce - The transaction count for the current account/network
+ * @property {string} gasPrice - The amount of gwei, in hexadecimal, per unit of gas
+ * @property {string} gas - The max amount of gwei, in hexadecimal, the user is willing to pay
+ * @property {string} [data] - Hexadecimal encoded string representing calls to the EVM's ABI
+ */
+/**
+ * An object representing a transaction, in whatever state it is in.
+ * @typedef {Object} TransactionMeta
+ *
+ * @property {string} [blockNumber] - The block number this transaction was
+ *  included in. Currently only present on incoming transactions!
+ * @property {number} id - An internally unique tx identifier.
+ * @property {number} time - Time the transaction was first suggested, in unix
+ *  epoch time (ms).
+ * @property {TransactionTypeString} type - The type of transaction this txMeta
+ *  represents.
+ * @property {TransactionStatusString} status - The current status of the
+ *  transaction.
+ * @property {string} metamaskNetworkId - The transaction's network ID, used
+ *  for EIP-155 compliance.
+ * @property {boolean} loadingDefaults - TODO: Document
+ * @property {TxParams} txParams - The transaction params as passed to the
+ *  network provider.
+ * @property {Object[]} history - A history of mutations to this
+ *  TransactionMeta object.
+ * @property {string} origin - A string representing the interface that
+ *  suggested the transaction.
+ * @property {Object} nonceDetails - A metadata object containing information
+ *  used to derive the suggested nonce, useful for debugging nonce issues.
+ * @property {string} rawTx - A hex string of the final signed transaction,
+ *  ready to submit to the network.
+ * @property {string} hash - A hex string of the transaction hash, used to
+ *  identify the transaction on the network.
+ * @property {number} submittedTime - The time the transaction was submitted to
+ *  the network, in Unix epoch time (ms).
+ */