chore(network-controller): Simplify network switch detection in lookupNetwork

`lookupNetwork` operates on an arbitrary network or, if given nothing,
the globally selected network. The behavior between these two cases is
largely the same, except that in the "selected network" case, it
publishes one of two events, `NetworkController:infuraIsBlocked` or
`NetworkController:infuraIsUnblocked`. This happens after making a
request to the network, and while the request is running, it is possible
for the network to change. This creates a potential problem because if
that happens, the events that we want to fire would no longer represent
the network they were intended to represent.

`lookupNetwork` accounts for this by subscribing to
`NetworkController:networkDidChange` before starting the request; if
it's fired, then it exits early (with the presumption that the network
switch called its own version of `lookupNetwork` and will publish
`NetworkController:infuraIsBlocked` or
`NetworkController:infuraIsUnblocked` at that point in time).

However, this approach to detect a network switch is complicated and
unnecessary. We can simply capture `selectedNetworkClientId` before we
start the request, ask what it is afterward, and compare the two to know
whether the network switched. This means that we can bring the
implementation of the two branches within `lookupNetwork` closer
together (and we can eliminate some tests).

Author: mcmire

packages/network-controller/src/NetworkController.ts

@@ -1558,8 +1558,8 @@ export class NetworkController extends BaseController<
   }
 
   /**
-   * Uses a request for the latest block to gather the following information on
-   * the given network:
+   * Uses a request for the latest block to gather and record the following
+   * information on the given network:
    *
    * - The connectivity status: whether it is available, geo-blocked (Infura
    * only), unavailable, or unknown
@@ -1570,7 +1570,7 @@ export class NetworkController extends BaseController<
    * If no ID is provided, uses the currently selected network.
    * @returns The resulting metadata for the network.
    */
-  async #determineNetworkMetadata(networkClientId: NetworkClientId) {
+  async #captureNetworkMetadata(networkClientId: NetworkClientId) {
     // Force TypeScript to use one of the two overloads explicitly
     const networkClient = isInfuraNetworkType(networkClientId)
       ? this.getNetworkClientById(networkClientId)
@@ -1631,6 +1631,22 @@ export class NetworkController extends BaseController<
       }
     }
 
+    this.update((state) => {
+      if (state.networksMetadata[networkClientId] === undefined) {
+        state.networksMetadata[networkClientId] = {
+          status: NetworkStatus.Unknown,
+          EIPS: {},
+        };
+      }
+      const meta = state.networksMetadata[networkClientId];
+      meta.status = networkStatus;
+      if (isEIP1559Compatible === undefined) {
+        delete meta.EIPS[1559];
+      } else {
+        meta.EIPS[1559] = isEIP1559Compatible;
+      }
+    });
+
     return { isInfura, networkStatus, isEIP1559Compatible };
   }
 
@@ -1686,13 +1702,7 @@ export class NetworkController extends BaseController<
    * @param networkClientId - The ID of the network client to inspect.
    */
   async #lookupGivenNetwork(networkClientId: NetworkClientId) {
-    const { networkStatus, isEIP1559Compatible } =
-      await this.#determineNetworkMetadata(networkClientId);
-    this.#updateMetadataForNetwork(
-      networkClientId,
-      networkStatus,
-      isEIP1559Compatible,
-    );
+    await this.#captureNetworkMetadata(networkClientId);
   }
 
   /**
@@ -1713,113 +1723,30 @@ export class NetworkController extends BaseController<
       return;
     }
 
-    let networkChanged = false;
-    const listener = () => {
-      networkChanged = true;
-      try {
-        this.messagingSystem.unsubscribe(
-          'NetworkController:networkDidChange',
-          listener,
-        );
-      } catch (error) {
-        // In theory, this `catch` should not be necessary given that this error
-        // would occur "inside" of the call to `#determineEIP1559Compatibility`
-        // below and so it should be caught by the `try`/`catch` below (it is
-        // impossible to reproduce in tests for that reason). However, somehow
-        // it occurs within Mobile and so we have to add our own `try`/`catch`
-        // here.
-        /* istanbul ignore next */
-        if (
-          !(error instanceof Error) ||
-          error.message !==
-            'Subscription not found for event: NetworkController:networkDidChange'
-        ) {
-          // Again, this error should not happen and is impossible to reproduce
-          // in tests.
-          /* istanbul ignore next */
-          throw error;
-        }
-      }
-    };
-    this.messagingSystem.subscribe(
-      'NetworkController:networkDidChange',
-      listener,
-    );
-
-    const { isInfura, networkStatus, isEIP1559Compatible } =
-      await this.#determineNetworkMetadata(this.state.selectedNetworkClientId);
-
-    if (networkChanged) {
-      // If the network has changed, then `lookupNetwork` either has been or is
-      // in the process of being called, so we don't need to go further.
-      return;
-    }
+    // Capture up front in case the network is switched while awaiting the
+    // network request
+    const { selectedNetworkClientId } = this.state;
 
-    try {
-      this.messagingSystem.unsubscribe(
-        'NetworkController:networkDidChange',
-        listener,
-      );
-    } catch (error) {
-      if (
-        !(error instanceof Error) ||
-        error.message !==
-          'Subscription not found for event: NetworkController:networkDidChange'
-      ) {
-        throw error;
-      }
-    }
-
-    this.#updateMetadataForNetwork(
+    const { isInfura, networkStatus } = await this.#captureNetworkMetadata(
       this.state.selectedNetworkClientId,
-      networkStatus,
-      isEIP1559Compatible,
     );
 
-    if (isInfura) {
-      if (networkStatus === NetworkStatus.Available) {
+    if (selectedNetworkClientId === this.state.selectedNetworkClientId) {
+      if (isInfura) {
+        if (networkStatus === NetworkStatus.Available) {
+          this.messagingSystem.publish('NetworkController:infuraIsUnblocked');
+        } else if (networkStatus === NetworkStatus.Blocked) {
+          this.messagingSystem.publish('NetworkController:infuraIsBlocked');
+        }
+      } else {
+        // Always publish infuraIsUnblocked regardless of network status to
+        // prevent consumers from being stuck in a blocked state if they were
+        // previously connected to an Infura network that was blocked
         this.messagingSystem.publish('NetworkController:infuraIsUnblocked');
-      } else if (networkStatus === NetworkStatus.Blocked) {
-        this.messagingSystem.publish('NetworkController:infuraIsBlocked');
       }
-    } else {
-      // Always publish infuraIsUnblocked regardless of network status to
-      // prevent consumers from being stuck in a blocked state if they were
-      // previously connected to an Infura network that was blocked
-      this.messagingSystem.publish('NetworkController:infuraIsUnblocked');
     }
   }
 
-  /**
-   * Updates the metadata for the given network in state.
-   *
-   * @param networkClientId - The associated network client ID.
-   * @param networkStatus - The network status to store in state.
-   * @param isEIP1559Compatible - The EIP-1559 compatibility status to
-   * store in state.
-   */
-  #updateMetadataForNetwork(
-    networkClientId: NetworkClientId,
-    networkStatus: NetworkStatus,
-    isEIP1559Compatible: boolean | undefined,
-  ) {
-    this.update((state) => {
-      if (state.networksMetadata[networkClientId] === undefined) {
-        state.networksMetadata[networkClientId] = {
-          status: NetworkStatus.Unknown,
-          EIPS: {},
-        };
-      }
-      const meta = state.networksMetadata[networkClientId];
-      meta.status = networkStatus;
-      if (isEIP1559Compatible === undefined) {
-        delete meta.EIPS[1559];
-      } else {
-        meta.EIPS[1559] = isEIP1559Compatible;
-      }
-    });
-  }
-
   /**
    * Convenience method to update provider network type settings.
    *

packages/network-controller/tests/NetworkController.test.ts

@@ -2225,55 +2225,6 @@ describe('NetworkController', () => {
             });
           });
 
-          describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
-            it('re-throws the error', async () => {
-              const infuraProjectId = 'some-infura-project-id';
-
-              await withController(
-                {
-                  state: {
-                    selectedNetworkClientId: infuraNetworkType,
-                  },
-                  infuraProjectId,
-                },
-                async ({ controller, messenger }) => {
-                  const fakeProvider = buildFakeProvider([
-                    // Called during provider initialization
-                    {
-                      request: {
-                        method: 'eth_getBlockByNumber',
-                      },
-                      response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
-                    },
-                    // Called via `lookupNetwork` directly
-                    {
-                      request: {
-                        method: 'eth_getBlockByNumber',
-                      },
-                      response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
-                    },
-                  ]);
-                  const fakeNetworkClient = buildFakeClient(fakeProvider);
-                  createNetworkClientMock.mockReturnValue(fakeNetworkClient);
-                  await controller.initializeProvider();
-
-                  const lookupNetworkPromise = controller.lookupNetwork();
-                  const error = new Error('oops');
-                  jest
-                    .spyOn(messenger, 'unsubscribe')
-                    .mockImplementation((eventType) => {
-                      // This is okay.
-                      // eslint-disable-next-line jest/no-conditional-in-test
-                      if (eventType === 'NetworkController:networkDidChange') {
-                        throw error;
-                      }
-                    });
-                  await expect(lookupNetworkPromise).rejects.toThrow(error);
-                },
-              );
-            });
-          });
-
           lookupNetworkTests({
             expectedNetworkClientType: NetworkClientType.Infura,
             expectedNetworkClientId: infuraNetworkType,
@@ -2726,65 +2677,6 @@ describe('NetworkController', () => {
           });
         });
 
-        describe('if removing the networkDidChange subscription fails for an unknown reason', () => {
-          it('re-throws the error', async () => {
-            const infuraProjectId = 'some-infura-project-id';
-
-            await withController(
-              {
-                state: {
-                  selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',
-                  networkConfigurationsByChainId: {
-                    '0x1337': buildCustomNetworkConfiguration({
-                      chainId: '0x1337',
-                      rpcEndpoints: [
-                        buildCustomRpcEndpoint({
-                          networkClientId: 'AAAA-AAAA-AAAA-AAAA',
-                        }),
-                      ],
-                    }),
-                  },
-                },
-                infuraProjectId,
-              },
-              async ({ controller, messenger }) => {
-                const fakeProvider = buildFakeProvider([
-                  // Called during provider initialization
-                  {
-                    request: {
-                      method: 'eth_getBlockByNumber',
-                    },
-                    response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
-                  },
-                  // Called via `lookupNetwork` directly
-                  {
-                    request: {
-                      method: 'eth_getBlockByNumber',
-                    },
-                    response: SUCCESSFUL_ETH_GET_BLOCK_BY_NUMBER_RESPONSE,
-                  },
-                ]);
-                const fakeNetworkClient = buildFakeClient(fakeProvider);
-                createNetworkClientMock.mockReturnValue(fakeNetworkClient);
-                await controller.initializeProvider();
-
-                const lookupNetworkPromise = controller.lookupNetwork();
-                const error = new Error('oops');
-                jest
-                  .spyOn(messenger, 'unsubscribe')
-                  .mockImplementation((eventType) => {
-                    // This is okay.
-                    // eslint-disable-next-line jest/no-conditional-in-test
-                    if (eventType === 'NetworkController:networkDidChange') {
-                      throw error;
-                    }
-                  });
-                await expect(lookupNetworkPromise).rejects.toThrow(error);
-              },
-            );
-          });
-        });
-
         lookupNetworkTests({
           expectedNetworkClientType: NetworkClientType.Custom,
           expectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA',