From 06d87fb98b9386a3d79a8a2b659b403913ec870e Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Tue, 13 Dec 2022 15:43:12 -0330 Subject: [PATCH] Refactor how network state is passed to transaction controller (#16922) The network state is now passed to the TransactionController via a getter function and a subscription function, instead of passing one of the network controller stores directly. This way of passing the state makes further refactoring easier, as we don't have to change the input when the store is changed or replaced. It's also more aligned with our conventions today. This change was made as part of a larger refactor of the network controller, as part of the effort to merge the mobile and extension network controllers. --- app/scripts/controllers/transactions/index.js | 16 +++++++--------- .../controllers/transactions/index.test.js | 16 ++++++++++++---- .../transactions/tx-state-manager.js | 19 ++++++++++++------- .../transactions/tx-state-manager.test.js | 8 ++++---- app/scripts/metamask-controller.js | 4 +++- app/scripts/metamask-controller.test.js | 2 +- 6 files changed, 39 insertions(+), 26 deletions(-) diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 71601f2af..61ab7df62 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -113,7 +113,8 @@ const METRICS_STATUS_FAILED = 'failed on-chain'; * * @param {object} opts * @param {object} opts.initState - initial transaction list default is an empty array - * @param {object} opts.networkStore - an observable store for network number + * @param {Function} opts.getNetworkState - Get the current network state. + * @param {Function} opts.onNetworkStateChange - Subscribe to network state change events. * @param {object} opts.blockTracker - An instance of eth-blocktracker * @param {object} opts.provider - A network provider. * @param {Function} opts.signTransaction - function the signs an @ethereumjs/tx @@ -126,7 +127,7 @@ const METRICS_STATUS_FAILED = 'failed on-chain'; export default class TransactionController extends EventEmitter { constructor(opts) { super(); - this.networkStore = opts.networkStore || new ObservableStore({}); + this.getNetworkState = opts.getNetworkState; this._getCurrentChainId = opts.getCurrentChainId; this.getProviderConfig = opts.getProviderConfig; this._getCurrentNetworkEIP1559Compatibility = @@ -163,7 +164,7 @@ export default class TransactionController extends EventEmitter { this.txStateManager = new TransactionStateManager({ initState: opts.initState, txHistoryLimit: opts.txHistoryLimit, - getNetwork: this.getNetwork.bind(this), + getNetworkState: this.getNetworkState, getCurrentChainId: opts.getCurrentChainId, }); @@ -205,7 +206,7 @@ export default class TransactionController extends EventEmitter { // memstore is computed from a few different stores this._updateMemstore(); this.txStateManager.store.subscribe(() => this._updateMemstore()); - this.networkStore.subscribe(() => { + opts.onNetworkStateChange(() => { this._onBootCleanUp(); this._updateMemstore(); }); @@ -222,7 +223,7 @@ export default class TransactionController extends EventEmitter { * @returns {number} The numerical chainId. */ getChainId() { - const networkState = this.networkStore.getState(); + const networkState = this.getNetworkState(); const chainId = this._getCurrentChainId(); const integerChainId = parseInt(chainId, 16); if (networkState === 'loading' || Number.isNaN(integerChainId)) { @@ -273,7 +274,7 @@ export default class TransactionController extends EventEmitter { // name, chainId and networkId properties. This is done using the // `forCustomChain` static method on the Common class. const chainId = parseInt(this._getCurrentChainId(), 16); - const networkId = this.networkStore.getState(); + const networkId = this.getNetworkState(); const customChainParams = { name, @@ -1764,9 +1765,6 @@ export default class TransactionController extends EventEmitter { /** @returns {object} the state in transaction controller */ this.getState = () => this.memStore.getState(); - /** @returns {string|number} the network number stored in networkStore */ - this.getNetwork = () => this.networkStore.getState(); - /** @returns {string} the user selected address */ this.getSelectedAddress = () => this.preferencesStore.getState().selectedAddress; diff --git a/app/scripts/controllers/transactions/index.test.js b/app/scripts/controllers/transactions/index.test.js index 7c9be16b9..db0c6a496 100644 --- a/app/scripts/controllers/transactions/index.test.js +++ b/app/scripts/controllers/transactions/index.test.js @@ -41,7 +41,12 @@ const VALID_ADDRESS = '0x0000000000000000000000000000000000000000'; const VALID_ADDRESS_TWO = '0x0000000000000000000000000000000000000001'; describe('Transaction Controller', function () { - let txController, provider, providerResultStub, fromAccount, fragmentExists; + let txController, + provider, + providerResultStub, + fromAccount, + fragmentExists, + networkStore; beforeEach(function () { fragmentExists = false; @@ -57,6 +62,8 @@ describe('Transaction Controller', function () { chainId: currentNetworkId, }).provider; + networkStore = new ObservableStore(currentNetworkId); + fromAccount = getTestAccounts()[0]; const blockTrackerStub = new EventEmitter(); blockTrackerStub.getCurrentBlock = noop; @@ -66,7 +73,8 @@ describe('Transaction Controller', function () { getGasPrice() { return '0xee6b2800'; }, - networkStore: new ObservableStore(currentNetworkId), + getNetworkState: () => networkStore.getState(), + onNetworkStateChange: (listener) => networkStore.subscribe(listener), getCurrentNetworkEIP1559Compatibility: () => Promise.resolve(false), getCurrentAccountEIP1559Compatibility: () => false, txHistoryLimit: 10, @@ -455,7 +463,7 @@ describe('Transaction Controller', function () { }); it('should fail if netId is loading', async function () { - txController.networkStore = new ObservableStore('loading'); + networkStore.putState('loading'); await assert.rejects( () => txController.addUnapprovedTransaction({ @@ -1067,7 +1075,7 @@ describe('Transaction Controller', function () { describe('#getChainId', function () { it('returns 0 when the chainId is NaN', function () { - txController.networkStore = new ObservableStore('loading'); + networkStore.putState('loading'); assert.equal(txController.getChainId(), 0); }); }); diff --git a/app/scripts/controllers/transactions/tx-state-manager.js b/app/scripts/controllers/transactions/tx-state-manager.js index d426cfe61..07d19736e 100644 --- a/app/scripts/controllers/transactions/tx-state-manager.js +++ b/app/scripts/controllers/transactions/tx-state-manager.js @@ -54,10 +54,15 @@ export const ERROR_SUBMITTING = * transactions list keyed by id * @param {number} [opts.txHistoryLimit] - limit for how many finished * transactions can hang around in state - * @param {Function} opts.getNetwork - return network number + * @param {Function} opts.getNetworkState - Get the current network state. */ export default class TransactionStateManager extends EventEmitter { - constructor({ initState, txHistoryLimit, getNetwork, getCurrentChainId }) { + constructor({ + initState, + txHistoryLimit, + getNetworkState, + getCurrentChainId, + }) { super(); this.store = new ObservableStore({ @@ -65,7 +70,7 @@ export default class TransactionStateManager extends EventEmitter { ...initState, }); this.txHistoryLimit = txHistoryLimit; - this.getNetwork = getNetwork; + this.getNetworkState = getNetworkState; this.getCurrentChainId = getCurrentChainId; } @@ -81,7 +86,7 @@ export default class TransactionStateManager extends EventEmitter { * @returns {TransactionMeta} the default txMeta object */ generateTxMeta(opts = {}) { - const netId = this.getNetwork(); + const netId = this.getNetworkState(); const chainId = this.getCurrentChainId(); if (netId === 'loading') { throw new Error('MetaMask is having trouble connecting to the network'); @@ -144,7 +149,7 @@ export default class TransactionStateManager extends EventEmitter { */ getUnapprovedTxList() { const chainId = this.getCurrentChainId(); - const network = this.getNetwork(); + const network = this.getNetworkState(); return pickBy( this.store.getState().transactions, (transaction) => @@ -408,7 +413,7 @@ export default class TransactionStateManager extends EventEmitter { limit, } = {}) { const chainId = this.getCurrentChainId(); - const network = this.getNetwork(); + const network = this.getNetworkState(); // searchCriteria is an object that might have values that aren't predicate // methods. When providing any other value type (string, number, etc), we // consider this shorthand for "check the value at key for strict equality @@ -600,7 +605,7 @@ export default class TransactionStateManager extends EventEmitter { wipeTransactions(address) { // network only tx const { transactions } = this.store.getState(); - const network = this.getNetwork(); + const network = this.getNetworkState(); const chainId = this.getCurrentChainId(); // Update state diff --git a/app/scripts/controllers/transactions/tx-state-manager.test.js b/app/scripts/controllers/transactions/tx-state-manager.test.js index 3638eefa0..f1354d146 100644 --- a/app/scripts/controllers/transactions/tx-state-manager.test.js +++ b/app/scripts/controllers/transactions/tx-state-manager.test.js @@ -54,7 +54,7 @@ describe('TransactionStateManager', function () { transactions: {}, }, txHistoryLimit: 10, - getNetwork: () => currentNetworkId, + getNetworkState: () => currentNetworkId, getCurrentChainId: () => currentChainId, }); }); @@ -181,7 +181,7 @@ describe('TransactionStateManager', function () { [confirmedTx.id]: confirmedTx, }, }, - getNetwork: () => currentNetworkId, + getNetworkState: () => currentNetworkId, getCurrentChainId: () => currentChainId, }); @@ -246,7 +246,7 @@ describe('TransactionStateManager', function () { [confirmedTx3.id]: confirmedTx3, }, }, - getNetwork: () => currentNetworkId, + getNetworkState: () => currentNetworkId, getCurrentChainId: () => currentChainId, }); @@ -355,7 +355,7 @@ describe('TransactionStateManager', function () { [failedTx3Dupe.id]: failedTx3Dupe, }, }, - getNetwork: () => currentNetworkId, + getNetworkState: () => currentNetworkId, getCurrentChainId: () => currentChainId, }); diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4615a3216..eb271d645 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -838,7 +838,9 @@ export default class MetamaskController extends EventEmitter { ), getCurrentAccountEIP1559Compatibility: this.getCurrentAccountEIP1559Compatibility.bind(this), - networkStore: this.networkController.networkStore, + getNetworkState: () => this.networkController.networkStore.getState(), + onNetworkStateChange: (listener) => + this.networkController.networkStore.subscribe(listener), getCurrentChainId: this.networkController.getCurrentChainId.bind( this.networkController, ), diff --git a/app/scripts/metamask-controller.test.js b/app/scripts/metamask-controller.test.js index 232e98550..389739283 100644 --- a/app/scripts/metamask-controller.test.js +++ b/app/scripts/metamask-controller.test.js @@ -709,7 +709,7 @@ describe('MetaMaskController', function () { ); const getNetworkstub = sinon.stub( metamaskController.txController.txStateManager, - 'getNetwork', + 'getNetworkState', ); selectedAddressStub.returns('0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc');