Correct invalid initial selectedNetworkClientId

Currently, when NetworkController is instantiated with pre-existing
state that contains an invalid `selectedNetworkClientId` — that is, no
RPC endpoint exists which has the same network client ID — then it
throws an error. This was intentionally done to bring attention to
possible bugs in NetworkController, but this has the unfortunate side
effect of bricking users' wallets.

To fix this, we now correct an invalid `selectedNetworkClientId` to
point to the default RPC endpoint of the first network sorted by chain
ID (which in the vast majority of cases will be Mainnet). We still do
want to know about this, though, so we log the error in Sentry.

Author: mcmire

packages/network-controller/CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Rather than throwing an error, NetworkController now corrects an invalid initial `selectedNetworkClientId` to point to the default RPC endpoint of the first network sorted by chain ID ([#5851](https://github.com/MetaMask/core/pull/5851))
+
 ## [23.5.0]
 
 ### Changed

packages/network-controller/package.json

@@ -49,6 +49,7 @@
   "dependencies": {
     "@metamask/base-controller": "^8.0.1",
     "@metamask/controller-utils": "^11.9.0",
+    "@metamask/error-reporting-service": "^0.0.0",
     "@metamask/eth-block-tracker": "^11.0.3",
     "@metamask/eth-json-rpc-infura": "^10.1.1",
     "@metamask/eth-json-rpc-middleware": "^16.0.1",

packages/network-controller/src/NetworkController.ts

@@ -17,6 +17,7 @@ import {
   BUILT_IN_CUSTOM_NETWORKS_RPC,
   BUILT_IN_NETWORKS,
 } from '@metamask/controller-utils';
+import type { ErrorReportingServiceCaptureExceptionAction } from '@metamask/error-reporting-service';
 import type { PollingBlockTrackerOptions } from '@metamask/eth-block-tracker';
 import EthQuery from '@metamask/eth-query';
 import { errorCodes } from '@metamask/rpc-errors';
@@ -25,8 +26,7 @@ import type { SwappableProxy } from '@metamask/swappable-obj-proxy';
 import type { Hex } from '@metamask/utils';
 import { hasProperty, isPlainObject, isStrictHexString } from '@metamask/utils';
 import deepEqual from 'fast-deep-equal';
-import type { Draft } from 'immer';
-import { cloneDeep } from 'lodash';
+import { produce, current, type Draft } from 'immer';
 import type { Logger } from 'loglevel';
 import { createSelector } from 'reselect';
 import * as URI from 'uri-js';
@@ -498,6 +498,11 @@ export type NetworkControllerEvents =
   | NetworkControllerRpcEndpointDegradedEvent
   | NetworkControllerRpcEndpointRequestRetriedEvent;
 
+/**
+ * All events that {@link NetworkController} calls internally.
+ */
+type AllowedEvents = never;
+
 export type NetworkControllerGetStateAction = ControllerGetStateAction<
   typeof controllerName,
   NetworkState
@@ -590,12 +595,17 @@ export type NetworkControllerActions =
   | NetworkControllerRemoveNetworkAction
   | NetworkControllerUpdateNetworkAction;
 
+/**
+ * All actions that {@link NetworkController} calls internally.
+ */
+type AllowedActions = ErrorReportingServiceCaptureExceptionAction;
+
 export type NetworkControllerMessenger = RestrictedMessenger<
   typeof controllerName,
-  NetworkControllerActions,
-  NetworkControllerEvents,
-  never,
-  never
+  NetworkControllerActions | AllowedActions,
+  NetworkControllerEvents | AllowedEvents,
+  AllowedActions['type'],
+  AllowedEvents['type']
 >;
 
 /**
@@ -1003,7 +1013,7 @@ function deriveInfuraNetworkNameFromRpcEndpointUrl(
  * @param state - The NetworkController state to verify.
  * @throws if the state is invalid in some way.
  */
-function validateNetworkControllerState(state: NetworkState) {
+function validateInitialState(state: NetworkState) {
   const networkConfigurationEntries = Object.entries(
     state.networkConfigurationsByChainId,
   );
@@ -1054,14 +1064,44 @@ function validateNetworkControllerState(state: NetworkState) {
       'NetworkController state has invalid `networkConfigurationsByChainId`: Every RPC endpoint across all network configurations must have a unique `networkClientId`',
     );
   }
+}
 
-  if (!networkClientIds.includes(state.selectedNetworkClientId)) {
-    throw new Error(
-      // This ESLint rule mistakenly produces an error.
-      // eslint-disable-next-line @typescript-eslint/restrict-template-expressions
-      `NetworkController state is invalid: \`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration`,
-    );
-  }
+/**
+ * Checks that the given initial NetworkController state is internally
+ * consistent similar to `validateInitialState`, but if an anomaly is detected,
+ * it does its best to correct the state and logs an error to Sentry.
+ *
+ * @param state - The NetworkController state to verify.
+ * @param messenger - The NetworkController messenger.
+ * @returns The corrected state.
+ */
+function correctInitialState(
+  state: NetworkState,
+  messenger: NetworkControllerMessenger,
+): NetworkState {
+  const networkConfigurationsSortedByChainId = getNetworkConfigurations(
+    state,
+  ).sort((a, b) => a.chainId.localeCompare(b.chainId));
+  const networkClientIds = getAvailableNetworkClientIds(
+    networkConfigurationsSortedByChainId,
+  );
+
+  return produce(state, (newState) => {
+    if (!networkClientIds.includes(state.selectedNetworkClientId)) {
+      const firstNetworkConfiguration = networkConfigurationsSortedByChainId[0];
+      const newSelectedNetworkClientId =
+        firstNetworkConfiguration.rpcEndpoints[
+          firstNetworkConfiguration.defaultRpcEndpointIndex
+        ].networkClientId;
+      messenger.call(
+        'ErrorReportingService:captureException',
+        new Error(
+          `\`selectedNetworkClientId\` '${state.selectedNetworkClientId}' does not refer to an RPC endpoint within a network configuration; correcting to '${newSelectedNetworkClientId}'`,
+        ),
+      );
+      newState.selectedNetworkClientId = newSelectedNetworkClientId;
+    }
+  });
 }
 
 /**
@@ -1146,7 +1186,9 @@ export class NetworkController extends BaseController<
       ...getDefaultNetworkControllerState(additionalDefaultNetworks),
       ...state,
     };
-    validateNetworkControllerState(initialState);
+    validateInitialState(initialState);
+    const correctedInitialState = correctInitialState(initialState, messenger);
+
     if (!infuraProjectId || typeof infuraProjectId !== 'string') {
       throw new Error('Invalid Infura project ID');
     }
@@ -1168,7 +1210,7 @@ export class NetworkController extends BaseController<
         },
       },
       messenger,
-      state: initialState,
+      state: correctedInitialState,
     });
 
     this.#infuraProjectId = infuraProjectId;
@@ -2875,7 +2917,7 @@ export class NetworkController extends BaseController<
 
     this.#networkConfigurationsByNetworkClientId =
       buildNetworkConfigurationsByNetworkClientId(
-        cloneDeep(state.networkConfigurationsByChainId),
+        current(state.networkConfigurationsByChainId),
       );
   }
 

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

@@ -1,7 +1,6 @@
 // A lot of the tests in this file have conditionals.
 /* eslint-disable jest/no-conditional-in-test */
 
-import type { Messenger } from '@metamask/base-controller';
 import {
   BuiltInNetworkName,
   ChainId,
@@ -37,6 +36,7 @@ import {
   INFURA_NETWORKS,
   TESTNET,
 } from './helpers';
+import type { RootMessenger } from './helpers';
 import { FakeBlockTracker } from '../../../tests/fake-block-tracker';
 import type { FakeProviderStub } from '../../../tests/fake-provider';
 import { FakeProvider } from '../../../tests/fake-provider';
@@ -51,7 +51,6 @@ import type {
   InfuraRpcEndpoint,
   NetworkClientId,
   NetworkConfiguration,
-  NetworkControllerActions,
   NetworkControllerEvents,
   NetworkControllerMessenger,
   NetworkControllerOptions,
@@ -350,30 +349,90 @@ describe('NetworkController', () => {
       );
     });
 
-    it('throws if selectedNetworkClientId does not match the networkClientId of an RPC endpoint in networkConfigurationsByChainId', () => {
-      const messenger = buildRootMessenger();
-      const restrictedMessenger = buildNetworkControllerMessenger(messenger);
-      expect(
-        () =>
-          new NetworkController({
-            messenger: restrictedMessenger,
-            state: {
-              selectedNetworkClientId: 'nonexistent',
-              networkConfigurationsByChainId: {
-                '0x1337': buildCustomNetworkConfiguration({
-                  chainId: '0x1337',
-                }),
-              },
+    describe('if selectedNetworkClientId does not match the networkClientId of an RPC endpoint in networkConfigurationsByChainId', () => {
+      it('corrects selectedNetworkClientId to the default RPC endpoint of the first chain', () => {
+        const messenger = buildRootMessenger();
+        messenger.registerActionHandler(
+          'ErrorReportingService:captureException',
+          jest.fn(),
+        );
+        const restrictedMessenger = buildNetworkControllerMessenger(messenger);
+        const controller = new NetworkController({
+          messenger: restrictedMessenger,
+          state: {
+            selectedNetworkClientId: 'nonexistent',
+            networkConfigurationsByChainId: {
+              '0x1': buildCustomNetworkConfiguration({
+                chainId: '0x1',
+                defaultRpcEndpointIndex: 1,
+                rpcEndpoints: [
+                  buildCustomRpcEndpoint({
+                    networkClientId: 'AAAA-AAAA-AAAA-AAAA',
+                  }),
+                  buildCustomRpcEndpoint({
+                    networkClientId: 'BBBB-BBBB-BBBB-BBBB',
+                  }),
+                ],
+              }),
+              '0x2': buildCustomNetworkConfiguration({ chainId: '0x2' }),
+              '0x3': buildCustomNetworkConfiguration({ chainId: '0x3' }),
             },
-            infuraProjectId: 'infura-project-id',
-            getRpcServiceOptions: () => ({
-              fetch,
-              btoa,
-            }),
+          },
+          infuraProjectId: 'infura-project-id',
+          getRpcServiceOptions: () => ({
+            fetch,
+            btoa,
           }),
-      ).toThrow(
-        "NetworkController state is invalid: `selectedNetworkClientId` 'nonexistent' does not refer to an RPC endpoint within a network configuration",
-      );
+        });
+
+        expect(controller.state.selectedNetworkClientId).toBe(
+          'BBBB-BBBB-BBBB-BBBB',
+        );
+      });
+
+      it('logs a Sentry error', () => {
+        const messenger = buildRootMessenger();
+        const captureExceptionMock = jest.fn();
+        messenger.registerActionHandler(
+          'ErrorReportingService:captureException',
+          captureExceptionMock,
+        );
+        const restrictedMessenger = buildNetworkControllerMessenger(messenger);
+
+        new NetworkController({
+          messenger: restrictedMessenger,
+          state: {
+            selectedNetworkClientId: 'nonexistent',
+            networkConfigurationsByChainId: {
+              '0x1': buildCustomNetworkConfiguration({
+                chainId: '0x1',
+                defaultRpcEndpointIndex: 1,
+                rpcEndpoints: [
+                  buildCustomRpcEndpoint({
+                    networkClientId: 'AAAA-AAAA-AAAA-AAAA',
+                  }),
+                  buildCustomRpcEndpoint({
+                    networkClientId: 'BBBB-BBBB-BBBB-BBBB',
+                  }),
+                ],
+              }),
+              '0x2': buildCustomNetworkConfiguration({ chainId: '0x2' }),
+              '0x3': buildCustomNetworkConfiguration({ chainId: '0x3' }),
+            },
+          },
+          infuraProjectId: 'infura-project-id',
+          getRpcServiceOptions: () => ({
+            fetch,
+            btoa,
+          }),
+        });
+
+        expect(captureExceptionMock).toHaveBeenCalledWith(
+          new Error(
+            "`selectedNetworkClientId` 'nonexistent' does not refer to an RPC endpoint within a network configuration; correcting to 'BBBB-BBBB-BBBB-BBBB'",
+          ),
+        );
+      });
     });
 
     const invalidInfuraProjectIds = [undefined, null, {}, 1];
@@ -1326,10 +1385,7 @@ describe('NetworkController', () => {
         {
           messenger,
         }: {
-          messenger: Messenger<
-            NetworkControllerActions,
-            NetworkControllerEvents
-          >;
+          messenger: RootMessenger;
         },
         args: Parameters<NetworkController['findNetworkClientIdByChainId']>,
       ): ReturnType<NetworkController['findNetworkClientIdByChainId']> =>
@@ -3278,13 +3334,7 @@ describe('NetworkController', () => {
     ],
     [
       'NetworkController:getNetworkConfigurationByChainId',
-      ({
-        messenger,
-        chainId,
-      }: {
-        messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
-        chainId: Hex;
-      }) =>
+      ({ messenger, chainId }: { messenger: RootMessenger; chainId: Hex }) =>
         messenger.call(
           'NetworkController:getNetworkConfigurationByChainId',
           chainId,
@@ -3397,7 +3447,7 @@ describe('NetworkController', () => {
         messenger,
         networkClientId,
       }: {
-        messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
+        messenger: RootMessenger;
         networkClientId: NetworkClientId;
       }) =>
         messenger.call(
@@ -15171,7 +15221,7 @@ type WithControllerCallback<ReturnValue> = ({
   controller,
 }: {
   controller: NetworkController;
-  messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
+  messenger: RootMessenger;
   networkControllerMessenger: NetworkControllerMessenger;
 }) => Promise<ReturnValue> | ReturnValue;
 
@@ -15350,7 +15400,7 @@ async function waitForPublishedEvents<E extends NetworkControllerEvents>({
     // do nothing
   },
 }: {
-  messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
+  messenger: RootMessenger;
   eventType: E['type'];
   count?: number;
   filter?: (payload: E['payload']) => boolean;
@@ -15481,7 +15531,7 @@ async function waitForStateChanges({
   operation,
   beforeResolving,
 }: {
-  messenger: Messenger<NetworkControllerActions, NetworkControllerEvents>;
+  messenger: RootMessenger;
   propertyPath?: string[];
   count?: number;
   wait?: number;