Update swaps controller ethereum provider on demand instead of on net…

…work events (#21958)

While QAing v11.5.2, this issue was discovered:

1. Add a network, through the popular network list, that you have tokens
on.
2. use "+ Import tokens" form to import a token that that you have on
this network
3. try to swap that token for the network's native currency, fetching
quotes will fail and an "Error fetching quotes" message will be shown

We thought we fixed this with
#21923, but the case
of adding a network and then immediately trying to swap an erc20 token
on that network, was not covered by that solution.

The reasons for that are detailed extensively here
https://consensys.slack.com/archives/GTQAGKY5V/p1700700399190349

tldr: When the `NetworkDidChange` event occurs during the network adding
flow, the network's status is not yet "Available" and so the swaps
controller subscriber for network changes does allow the provider to
update

The fix in this PR, which also covers the problem addressed by the
aforementioned PR, is to only update the swaps controller's
ethersProvider at the time it is needed, within the `fetchAndSetQuotes`,
and to stop subscribing to network controller changes.

1. Add a network, through the popular network list, that you have tokens
on.
2. use "+ Import tokens" form to import a token that that you have on
this network
3. try to swap that token for the network's native currency, fetching
quotes should succeed

https://github.com/MetaMask/metamask-extension/assets/7499938/acc62c6c-ac67-41be-a4a9-78e7d9941043

https://github.com/MetaMask/metamask-extension/assets/7499938/3eeea1c2-9d14-4f56-9a66-4669af9ae0e1

- [x] I’ve followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [x] I've clearly explained what problem this PR is solving and how it
is solved.
- [x] I've linked related issues
- [x] I've included manual testing steps
- [x] I've included screenshots/recordings if applicable
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.
- [x] I’ve properly set the pull request status:
  - [ ] In case it's not yet "ready for review", I've set it to "draft".
- [ ] In case it's "ready for review", I've changed it from "draft" to
"non-draft".

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

app/scripts/controllers/swaps.js

@@ -18,7 +18,7 @@ import {
   SWAPS_CHAINID_CONTRACT_ADDRESS_MAP,
 } from '../../../shared/constants/swaps';
 import { GasEstimateTypes } from '../../../shared/constants/gas';
-import { CHAIN_IDS, NetworkStatus } from '../../../shared/constants/network';
+import { CHAIN_IDS } from '../../../shared/constants/network';
 import {
   MetaMetricsEventCategory,
   MetaMetricsEventName,
@@ -113,14 +113,12 @@ export default class SwapsController {
   constructor(
     {
       getBufferedGasLimit,
-      networkController,
       provider,
       getProviderConfig,
       getTokenRatesState,
       fetchTradesInfo = defaultFetchTradesInfo,
       getCurrentChainId,
       getEIP1559GasFeeEstimates,
-      onNetworkStateChange,
       trackMetaMetricsEvent,
     },
     state,
@@ -154,24 +152,9 @@ export default class SwapsController {
 
     this.indexOfNewestCallInFlight = 0;
 
+    this.provider = provider;
     this.ethersProvider = new Web3Provider(provider);
-    this._currentChainId = networkController.state.providerConfig.chainId;
-    onNetworkStateChange(() => {
-      const {
-        networksMetadata,
-        selectedNetworkClientId,
-        providerConfig: { chainId },
-      } = networkController.state;
-      const selectedNetworkStatus =
-        networksMetadata[selectedNetworkClientId]?.status;
-      if (
-        selectedNetworkStatus === NetworkStatus.Available &&
-        chainId !== this._currentChainId
-      ) {
-        this._currentChainId = chainId;
-        this.ethersProvider = new Web3Provider(provider);
-      }
-    });
+    this._ethersProviderChainId = this._getCurrentChainId();
   }
 
   async fetchSwapsNetworkConfig(chainId) {
@@ -273,6 +256,12 @@ export default class SwapsController {
     isPolledRequest,
   ) {
     const { chainId } = fetchParamsMetaData;
+
+    if (chainId !== this._ethersProviderChainId) {
+      this.ethersProvider = new Web3Provider(this.provider);
+      this._ethersProviderChainId = chainId;
+    }
+
     const {
       swapsState: { quotesPollingLimitEnabled, saveFetchedQuotes },
     } = this.store.getState();

app/scripts/controllers/swaps.test.js

@@ -4,8 +4,7 @@ import sinon from 'sinon';
 import { BigNumber } from '@ethersproject/bignumber';
 import { mapValues } from 'lodash';
 import BigNumberjs from 'bignumber.js';
-import { NetworkType } from '@metamask/controller-utils';
-import { CHAIN_IDS, NetworkStatus } from '../../../shared/constants/network';
+import { CHAIN_IDS } from '../../../shared/constants/network';
 import { ETH_SWAPS_TOKEN_OBJECT } from '../../../shared/constants/swaps';
 import { createTestProviderTools } from '../../../test/stub/provider';
 import { SECOND } from '../../../shared/constants/time';
@@ -95,23 +94,6 @@ const MOCK_GET_BUFFERED_GAS_LIMIT = async () => ({
   simulationFails: undefined,
 });
 
-function getMockNetworkController() {
-  return {
-    state: {
-      selectedNetworkClientId: NetworkType.goerli,
-      networksMetadata: {
-        [NetworkType.goerli]: {
-          EIPS: {},
-          status: NetworkStatus.Available,
-        },
-      },
-      providerConfig: {
-        chainId: CHAIN_IDS.GOERLI,
-      },
-    },
-  };
-}
-
 const EMPTY_INIT_STATE = {
   swapsState: {
     quotes: {},
@@ -144,7 +126,7 @@ const EMPTY_INIT_STATE = {
 };
 
 const sandbox = sinon.createSandbox();
-const fetchTradesInfoStub = sandbox.stub();
+let fetchTradesInfoStub = sandbox.stub();
 const getCurrentChainIdStub = sandbox.stub();
 getCurrentChainIdStub.returns(CHAIN_IDS.MAINNET);
 const getEIP1559GasFeeEstimatesStub = sandbox.stub(() => {
@@ -162,8 +144,6 @@ describe('SwapsController', function () {
   const getSwapsController = (_provider = provider) => {
     return new SwapsController({
       getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT,
-      networkController: getMockNetworkController(),
-      onNetworkStateChange: sinon.stub(),
       provider: _provider,
       getProviderConfig: MOCK_GET_PROVIDER_CONFIG,
       getTokenRatesState: MOCK_TOKEN_RATES_STORE,
@@ -208,126 +188,6 @@ describe('SwapsController', function () {
         MOCK_GET_PROVIDER_CONFIG,
       );
     });
-
-    it('should replace ethers instance when network changes', function () {
-      const networkController = getMockNetworkController();
-      let networkStateChangeListener;
-      const onNetworkStateChange = (listener) => {
-        networkStateChangeListener = listener;
-      };
-      const swapsController = new SwapsController({
-        getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT,
-        networkController,
-        onNetworkStateChange,
-        provider,
-        getProviderConfig: MOCK_GET_PROVIDER_CONFIG,
-        getTokenRatesState: MOCK_TOKEN_RATES_STORE,
-        fetchTradesInfo: fetchTradesInfoStub,
-        getCurrentChainId: getCurrentChainIdStub,
-      });
-      const currentEthersInstance = swapsController.ethersProvider;
-
-      networkController.state = {
-        selectedNetworkClientId: NetworkType.mainnet,
-        networksMetadata: {
-          [NetworkType.mainnet]: {
-            EIPS: {},
-            status: NetworkStatus.Available,
-          },
-        },
-        providerConfig: {
-          chainId: CHAIN_IDS.MAINNET,
-        },
-      };
-      networkStateChangeListener();
-
-      const newEthersInstance = swapsController.ethersProvider;
-      assert.notStrictEqual(
-        currentEthersInstance,
-        newEthersInstance,
-        'Ethers provider should be replaced',
-      );
-    });
-
-    it('should not replace ethers instance when network changes to loading', function () {
-      const networkController = getMockNetworkController();
-      let networkStateChangeListener;
-      const onNetworkStateChange = (listener) => {
-        networkStateChangeListener = listener;
-      };
-      const swapsController = new SwapsController({
-        getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT,
-        networkController,
-        onNetworkStateChange,
-        provider,
-        getProviderConfig: MOCK_GET_PROVIDER_CONFIG,
-        getTokenRatesState: MOCK_TOKEN_RATES_STORE,
-        fetchTradesInfo: fetchTradesInfoStub,
-        getCurrentChainId: getCurrentChainIdStub,
-      });
-      const currentEthersInstance = swapsController.ethersProvider;
-
-      networkController.state = {
-        selectedNetworkClientId: NetworkType.goerli,
-        networksMetadata: {
-          [NetworkType.goerli]: {
-            EIPS: {},
-            status: NetworkStatus.Unknown,
-          },
-        },
-        providerConfig: {
-          chainId: CHAIN_IDS.GOERLI,
-        },
-      };
-      networkStateChangeListener();
-
-      const newEthersInstance = swapsController.ethersProvider;
-      assert.strictEqual(
-        currentEthersInstance,
-        newEthersInstance,
-        'Ethers provider should not be replaced',
-      );
-    });
-
-    it('should not replace ethers instance when network changes to the same network', function () {
-      const networkController = getMockNetworkController();
-      let networkStateChangeListener;
-      const onNetworkStateChange = (listener) => {
-        networkStateChangeListener = listener;
-      };
-      const swapsController = new SwapsController({
-        getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT,
-        networkController,
-        onNetworkStateChange,
-        provider,
-        getProviderConfig: MOCK_GET_PROVIDER_CONFIG,
-        getTokenRatesState: MOCK_TOKEN_RATES_STORE,
-        fetchTradesInfo: fetchTradesInfoStub,
-        getCurrentChainId: getCurrentChainIdStub,
-      });
-      const currentEthersInstance = swapsController.ethersProvider;
-
-      networkController.state = {
-        selectedNetworkClientId: NetworkType.goerli,
-        networksMetadata: {
-          [NetworkType.goerli]: {
-            EIPS: {},
-            status: NetworkStatus.Available,
-          },
-        },
-        providerConfig: {
-          chainId: CHAIN_IDS.GOERLI,
-        },
-      };
-      networkStateChangeListener();
-
-      const newEthersInstance = swapsController.ethersProvider;
-      assert.strictEqual(
-        currentEthersInstance,
-        newEthersInstance,
-        'Ethers provider should not be replaced',
-      );
-    });
   });
 
   describe('API', function () {
@@ -938,6 +798,128 @@ describe('SwapsController', function () {
 
         assert.strictEqual(newQuotes[topAggId].isBestQuote, undefined);
       });
+
+      it('should replace ethers instance when called with a different chainId than was current when the controller was instantiated', async function () {
+        fetchTradesInfoStub = sandbox.stub();
+
+        const _swapsController = getSwapsController();
+
+        const currentEthersInstance = _swapsController.ethersProvider;
+
+        await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, {
+          ...MOCK_FETCH_METADATA,
+          chainId: CHAIN_IDS.GOERLI,
+        });
+
+        const newEthersInstance = _swapsController.ethersProvider;
+        assert.notStrictEqual(
+          currentEthersInstance,
+          newEthersInstance,
+          'Ethers provider should be replaced',
+        );
+      });
+
+      it('should not replace ethers instance when called with the same chainId that was current when the controller was instantiated', async function () {
+        const _swapsController = new SwapsController({
+          getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT,
+          provider,
+          getProviderConfig: MOCK_GET_PROVIDER_CONFIG,
+          getTokenRatesState: MOCK_TOKEN_RATES_STORE,
+          fetchTradesInfo: fetchTradesInfoStub,
+          getCurrentChainId: getCurrentChainIdStub,
+        });
+        const currentEthersInstance = _swapsController.ethersProvider;
+
+        await swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, {
+          ...MOCK_FETCH_METADATA,
+          chainId: CHAIN_IDS.MAINNET,
+        });
+
+        const newEthersInstance = _swapsController.ethersProvider;
+        assert.strictEqual(
+          currentEthersInstance,
+          newEthersInstance,
+          'Ethers provider should not be replaced',
+        );
+      });
+
+      it('should replace ethers instance, and _ethersProviderChainId, twice when called twice with two different chainIds, and successfully set the _ethersProviderChainId when returning to the original chain', async function () {
+        const _swapsController = new SwapsController({
+          getBufferedGasLimit: MOCK_GET_BUFFERED_GAS_LIMIT,
+          provider,
+          getProviderConfig: MOCK_GET_PROVIDER_CONFIG,
+          getTokenRatesState: MOCK_TOKEN_RATES_STORE,
+          fetchTradesInfo: fetchTradesInfoStub,
+          getCurrentChainId: getCurrentChainIdStub,
+        });
+        const firstEthersInstance = _swapsController.ethersProvider;
+        const firstEthersProviderChainId =
+          _swapsController._ethersProviderChainId;
+
+        await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, {
+          ...MOCK_FETCH_METADATA,
+          chainId: CHAIN_IDS.GOERLI,
+        });
+
+        const secondEthersInstance = _swapsController.ethersProvider;
+        const secondEthersProviderChainId =
+          _swapsController._ethersProviderChainId;
+
+        assert.notStrictEqual(
+          firstEthersInstance,
+          secondEthersInstance,
+          'Ethers provider should be replaced',
+        );
+        assert.notStrictEqual(
+          firstEthersInstance,
+          secondEthersProviderChainId,
+          'Ethers provider chainId should be replaced',
+        );
+
+        await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, {
+          ...MOCK_FETCH_METADATA,
+          chainId: CHAIN_IDS.LOCALHOST,
+        });
+
+        const thirdEthersInstance = _swapsController.ethersProvider;
+        const thirdEthersProviderChainId =
+          _swapsController._ethersProviderChainId;
+
+        assert.notStrictEqual(
+          firstEthersProviderChainId,
+          thirdEthersInstance,
+          'Ethers provider should be replaced',
+        );
+        assert.notStrictEqual(
+          secondEthersInstance,
+          thirdEthersInstance,
+          'Ethers provider should be replaced',
+        );
+        assert.notStrictEqual(
+          firstEthersInstance,
+          thirdEthersProviderChainId,
+          'Ethers provider chainId should be replaced',
+        );
+        assert.notStrictEqual(
+          secondEthersProviderChainId,
+          thirdEthersProviderChainId,
+          'Ethers provider chainId should be replaced',
+        );
+
+        await _swapsController.fetchAndSetQuotes(MOCK_FETCH_PARAMS, {
+          ...MOCK_FETCH_METADATA,
+          chainId: CHAIN_IDS.MAINNET,
+        });
+
+        const lastEthersProviderChainId =
+          _swapsController._ethersProviderChainId;
+
+        assert.strictEqual(
+          firstEthersProviderChainId,
+          lastEthersProviderChainId,
+          'Ethers provider chainId should match what it was originally',
+        );
+      });
     });
 
     describe('resetSwapsState', function () {

app/scripts/metamask-controller.js

@@ -1548,14 +1548,6 @@ export default class MetamaskController extends EventEmitter {
           this.txController.txGasUtil.getBufferedGasLimit.bind(
             this.txController.txGasUtil,
           ),
-        networkController: this.networkController,
-        // This handler is misnamed. We must listen to networkDidChange
-        // to ensure the network provider has been set by the time we
-        // try to use it in this controller
-        onNetworkStateChange: networkControllerMessenger.subscribe.bind(
-          networkControllerMessenger,
-          'NetworkController:networkDidChange',
-        ),
         provider: this.provider,
         getProviderConfig: () => this.networkController.state.providerConfig,
         getTokenRatesState: () => this.tokenRatesController.state,