Ensure custom provider configs have chain ID and RPC URL (#19296)

Update NetworkController so that when it is creating the network client
based on configuration for a custom RPC endpoint, it verifies that the
configuration object contains both a chain ID and RPC URL. This check is
already present on the core side; bringing it over makes the tests more
consistent so we can compare them more easily.

app/scripts/controllers/network/network-controller.test.ts

@@ -345,76 +345,190 @@ describe('NetworkController', () => {
     }
 
     describe(`when the type in the provider configuration is "rpc"`, () => {
-      it('initializes a provider pointed to the given RPC URL whose chain ID matches the configured chain ID', async () => {
+      describe('if chainId and rpcUrl are present in the provider config', () => {
-        await withController(
+        it('initializes a provider pointed to the given RPC URL whose chain ID matches the configured chain ID', async () => {
-          {
+          await withController(
-            state: {
+            {
-              providerConfig: {
+              state: {
-                type: 'rpc',
+                providerConfig: {
-                chainId: '0x1337',
+                  type: 'rpc',
-                rpcUrl: 'https://mock-rpc-url',
-                ticker: 'TEST',
-              },
-              networkConfigurations: {
-                testNetworkConfigurationId: {
-                  rpcUrl: 'https://mock-rpc-url',
                   chainId: '0x1337',
+                  rpcUrl: 'https://mock-rpc-url',
                   ticker: 'TEST',
-                  id: 'testNetworkConfigurationId',
+                },
+                networkConfigurations: {
+                  testNetworkConfigurationId: {
+                    rpcUrl: 'https://mock-rpc-url',
+                    chainId: '0x1337',
+                    ticker: 'TEST',
+                    id: 'testNetworkConfigurationId',
+                  },
                 },
               },
             },
-          },
+            async ({ controller }) => {
-          async ({ controller }) => {
+              const fakeProvider = buildFakeProvider([
-            const fakeProvider = buildFakeProvider([
+                {
-              {
+                  request: {
-                request: {
+                    method: 'test',
-                  method: 'test',
+                  },
+                  response: {
+                    result: 'test response',
+                  },
                 },
-                response: {
+              ]);
-                  result: 'test response',
+              const fakeNetworkClient = buildFakeClient(fakeProvider);
-                },
+              mockCreateNetworkClient()
-              },
+                .calledWith({
-            ]);
+                  chainId: '0x1337',
-            const fakeNetworkClient = buildFakeClient(fakeProvider);
+                  rpcUrl: 'https://mock-rpc-url',
-            mockCreateNetworkClient()
+                  type: NetworkClientType.Custom,
-              .calledWith({
+                })
-                chainId: '0x1337',
+                .mockReturnValue(fakeNetworkClient);
-                rpcUrl: 'https://mock-rpc-url',
+              mockCreateNetworkClient().mockReturnValue(fakeNetworkClient);
-                type: NetworkClientType.Custom,
-              })
-              .mockReturnValue(fakeNetworkClient);
-            mockCreateNetworkClient().mockReturnValue(fakeNetworkClient);
 
-            await controller.initializeProvider();
+              await controller.initializeProvider();
 
-            const { provider } = controller.getProviderAndBlockTracker();
+              const { provider } = controller.getProviderAndBlockTracker();
-            assert(provider, 'Provider is somehow unset');
+              assert(provider, 'Provider is somehow unset');
-            const promisifiedSendAsync = promisify(provider.sendAsync).bind(
+              const promisifiedSendAsync = promisify(provider.sendAsync).bind(
-              provider,
+                provider,
-            );
+              );
-            const response = await promisifiedSendAsync({
+              const response = await promisifiedSendAsync({
-              id: '1',
+                id: '1',
-              jsonrpc: '2.0',
+                jsonrpc: '2.0',
-              method: 'test',
+                method: 'test',
-            });
+              });
-            expect(response.result).toBe('test response');
+              expect(response.result).toBe('test response');
-          },
+            },
-        );
+          );
-      });
+        });
 
-      lookupNetworkTests({
+        lookupNetworkTests({
-        expectedProviderConfig: buildProviderConfig({
+          expectedProviderConfig: buildProviderConfig({
-          type: NETWORK_TYPES.RPC,
-        }),
-        initialState: {
-          providerConfig: buildProviderConfig({
             type: NETWORK_TYPES.RPC,
           }),
-        },
+          initialState: {
-        operation: async (controller: NetworkController) => {
+            providerConfig: buildProviderConfig({
-          await controller.initializeProvider();
+              type: NETWORK_TYPES.RPC,
-        },
+            }),
+          },
+          operation: async (controller: NetworkController) => {
+            await controller.initializeProvider();
+          },
+        });
+      });
+
+      describe('if chainId is missing from the provider config', () => {
+        it('throws', async () => {
+          await withController(
+            {
+              state: {
+                providerConfig: buildProviderConfig({
+                  type: NETWORK_TYPES.RPC,
+                  chainId: undefined,
+                }),
+              },
+            },
+            async ({ controller }) => {
+              const fakeProvider = buildFakeProvider();
+              const fakeNetworkClient = buildFakeClient(fakeProvider);
+              createNetworkClientMock.mockReturnValue(fakeNetworkClient);
+
+              await expect(() =>
+                controller.initializeProvider(),
+              ).rejects.toThrow(
+                'chainId must be provided for custom RPC endpoints',
+              );
+            },
+          );
+        });
+
+        it('does not create a network client or capture a provider', async () => {
+          await withController(
+            {
+              state: {
+                providerConfig: buildProviderConfig({
+                  type: NETWORK_TYPES.RPC,
+                  chainId: undefined,
+                }),
+              },
+            },
+            async ({ controller }) => {
+              const fakeProvider = buildFakeProvider();
+              const fakeNetworkClient = buildFakeClient(fakeProvider);
+              createNetworkClientMock.mockReturnValue(fakeNetworkClient);
+
+              try {
+                await controller.initializeProvider();
+              } catch {
+                // ignore the error
+              }
+
+              expect(createNetworkClientMock).not.toHaveBeenCalled();
+              const { provider, blockTracker } =
+                controller.getProviderAndBlockTracker();
+              expect(provider).toBeNull();
+              expect(blockTracker).toBeNull();
+            },
+          );
+        });
+      });
+
+      describe('if rpcUrl is missing from the provider config', () => {
+        it('throws', async () => {
+          await withController(
+            {
+              state: {
+                providerConfig: buildProviderConfig({
+                  type: NETWORK_TYPES.RPC,
+                  rpcUrl: undefined,
+                }),
+              },
+            },
+            async ({ controller }) => {
+              const fakeProvider = buildFakeProvider();
+              const fakeNetworkClient = buildFakeClient(fakeProvider);
+              createNetworkClientMock.mockReturnValue(fakeNetworkClient);
+
+              await expect(() =>
+                controller.initializeProvider(),
+              ).rejects.toThrow(
+                'rpcUrl must be provided for custom RPC endpoints',
+              );
+            },
+          );
+        });
+
+        it('does not create a network client or capture a provider', async () => {
+          await withController(
+            {
+              state: {
+                providerConfig: buildProviderConfig({
+                  type: NETWORK_TYPES.RPC,
+                  rpcUrl: undefined,
+                }),
+              },
+            },
+            async ({ controller }) => {
+              const fakeProvider = buildFakeProvider();
+              const fakeNetworkClient = buildFakeClient(fakeProvider);
+              createNetworkClientMock.mockReturnValue(fakeNetworkClient);
+
+              try {
+                await controller.initializeProvider();
+              } catch {
+                // ignore the error
+              }
+
+              expect(createNetworkClientMock).not.toHaveBeenCalled();
+              const { provider, blockTracker } =
+                controller.getProviderAndBlockTracker();
+              expect(provider).toBeNull();
+              expect(blockTracker).toBeNull();
+            },
+          );
+        });
       });
     });
   });
@@ -900,102 +1014,6 @@ describe('NetworkController', () => {
       });
     });
 
-    describe('if the provider has initialized, but the current network has no chainId', () => {
-      it('does not update state in any way', async () => {
-        await withController(
-          /* @ts-expect-error We are intentionally not including a chainId in the provider config. */
-          {
-            state: {
-              providerConfig: {
-                type: 'rpc',
-                rpcUrl: 'http://example-custom-rpc.metamask.io',
-              },
-              networkDetails: {
-                EIPS: {
-                  1559: true,
-                },
-              },
-            },
-          },
-          async ({ controller }) => {
-            const fakeProvider = buildFakeProvider();
-            const fakeNetworkClient = buildFakeClient(fakeProvider);
-            mockCreateNetworkClient().mockReturnValue(fakeNetworkClient);
-            await controller.initializeProvider();
-            const stateAfterInitialization = controller.store.getState();
-
-            await controller.lookupNetwork();
-
-            expect(controller.store.getState()).toStrictEqual(
-              stateAfterInitialization,
-            );
-          },
-        );
-      });
-
-      it('does not emit infuraIsUnblocked', async () => {
-        await withController(
-          /* @ts-expect-error We are intentionally not including a chainId in the provider config. */
-          {
-            state: {
-              providerConfig: {
-                type: 'rpc',
-                rpcUrl: 'http://example-custom-rpc.metamask.io',
-              },
-            },
-          },
-          async ({ controller, messenger }) => {
-            const fakeProvider = buildFakeProvider();
-            const fakeNetworkClient = buildFakeClient(fakeProvider);
-            mockCreateNetworkClient().mockReturnValue(fakeNetworkClient);
-            await controller.initializeProvider();
-
-            const promiseForNoInfuraIsUnblockedEvents = waitForPublishedEvents({
-              messenger,
-              eventType: 'NetworkController:infuraIsUnblocked',
-              count: 0,
-              operation: async () => {
-                await controller.lookupNetwork();
-              },
-            });
-
-            expect(await promiseForNoInfuraIsUnblockedEvents).toBeTruthy();
-          },
-        );
-      });
-
-      it('does not emit infuraIsBlocked', async () => {
-        await withController(
-          /* @ts-expect-error We are intentionally not including a chainId in the provider config. */
-          {
-            state: {
-              providerConfig: {
-                type: 'rpc',
-                rpcUrl: 'http://example-custom-rpc.metamask.io',
-              },
-            },
-          },
-          async ({ controller, messenger }) => {
-            const fakeProvider = buildFakeProvider();
-            const fakeNetworkClient = buildFakeClient(fakeProvider);
-            mockCreateNetworkClient().mockReturnValue(fakeNetworkClient);
-            await controller.initializeProvider();
-
-            const promiseForNoInfuraIsBlockedEvents = waitForPublishedEvents({
-              messenger,
-              eventType: 'NetworkController:infuraIsBlocked',
-              count: 0,
-              operation: async () => {
-                await controller.lookupNetwork();
-              },
-            });
-
-            expect(await promiseForNoInfuraIsBlockedEvents).toBeTruthy();
-          },
-        );
-      });
-    });
-
     INFURA_NETWORKS.forEach(({ networkType }) => {
       describe(`when the type in the provider configuration is "${networkType}"`, () => {
         describe('if the network was switched after the eth_getBlockByNumber request started but before it completed', () => {

app/scripts/controllers/network/network-controller.ts

@@ -570,7 +570,7 @@ export class NetworkController extends EventEmitter {
    * blocking requests, or if the network is not Infura-supported.
    */
   async lookupNetwork(): Promise<void> {
-    const { chainId, type } = this.store.getState().providerConfig;
+    const { type } = this.store.getState().providerConfig;
     const { provider } = this.getProviderAndBlockTracker();
     let networkChanged = false;
     let networkId: NetworkIdState = null;
@@ -584,16 +584,6 @@ export class NetworkController extends EventEmitter {
       return;
     }
 
-    if (!chainId) {
-      log.warn(
-        'NetworkController - lookupNetwork aborted due to missing chainId',
-      );
-      this.#resetNetworkId();
-      this.#resetNetworkStatus();
-      this.#resetNetworkDetails();
-      return;
-    }
-
     const isInfura = isInfuraProviderType(type);
 
     const listener = () => {
@@ -874,11 +864,12 @@ export class NetworkController extends EventEmitter {
    * the new network.
    */
   async #switchNetwork(providerConfig: ProviderConfiguration) {
+    const { type, rpcUrl, chainId } = providerConfig;
     this.#messenger.publish('NetworkController:networkWillChange');
     this.#resetNetworkId();
     this.#resetNetworkStatus();
     this.#resetNetworkDetails();
-    this.#configureProvider(providerConfig);
+    this.#configureProvider({ type, rpcUrl, chainId });
     this.#messenger.publish('NetworkController:networkDidChange');
     await this.lookupNetwork();
   }
@@ -888,8 +879,7 @@ export class NetworkController extends EventEmitter {
    * block tracker) to talk to a network.
    *
    * @param args - The arguments.
-   * @param args.type - The shortname of an Infura-supported network (see
+   * @param args.type - The provider type.
-   * {@link NETWORK_TYPES}).
    * @param args.rpcUrl - The URL of the RPC endpoint that represents the
    * network. Only used for non-Infura networks.
    * @param args.chainId - The chain ID of the network (as per EIP-155). Only
@@ -897,16 +887,28 @@ export class NetworkController extends EventEmitter {
    * any Infura-supported network).
    * @throws if the `type` if not a known Infura-supported network.
    */
-  #configureProvider({ type, rpcUrl, chainId }: ProviderConfiguration): void {
+  #configureProvider({
+    type,
+    rpcUrl,
+    chainId,
+  }: {
+    type: ProviderType;
+    rpcUrl: string | undefined;
+    chainId: Hex | undefined;
+  }): void {
     const isInfura = isInfuraProviderType(type);
     if (isInfura) {
-      // infura type-based endpoints
       this.#configureInfuraProvider({
         type,
         infuraProjectId: this.#infuraProjectId,
       });
-    } else if (type === NETWORK_TYPES.RPC && rpcUrl) {
+    } else if (type === NETWORK_TYPES.RPC) {
-      // url-based rpc endpoints
+      if (chainId === undefined) {
+        throw new Error('chainId must be provided for custom RPC endpoints');
+      }
+      if (rpcUrl === undefined) {
+        throw new Error('rpcUrl must be provided for custom RPC endpoints');
+      }
       this.#configureStandardProvider(rpcUrl, chainId);
     } else {
       throw new Error(