Move networkVersion logic from base to inpage provider

jest.config.js

@@ -20,18 +20,18 @@ module.exports = {
   coveragePathIgnorePatterns: ['/node_modules/', '/mocks/', '/test/'],
   coverageThreshold: {
     global: {
-      branches: 51.35,
-      functions: 47.67,
-      lines: 55.05,
-      statements: 55.21,
+      branches: 55.61,
+      functions: 52.81,
+      lines: 58.33,
+      statements: 58.63,
     },
   },
   projects: [
     {
       ...baseConfig,
       displayName: 'StreamProvider',
       testEnvironment: 'node',
-      testMatch: ['**/StreamProvider.test.ts'],
+      testMatch: ['**/StreamProvider.test.ts', '**/utils.test.ts'],
     },
     {
       ...baseConfig,

src/BaseProvider.ts

@@ -10,7 +10,12 @@ import {
   JsonRpcMiddleware,
 } from 'json-rpc-engine';
 import messages from './messages';
-import { getRpcPromiseCallback, ConsoleLike, Maybe } from './utils';
+import {
+  getRpcPromiseCallback,
+  ConsoleLike,
+  Maybe,
+  isValidChainId,
+} from './utils';
 
 export interface UnvalidatedJsonRpcRequest {
   id?: JsonRpcId;
@@ -207,6 +212,9 @@ export abstract class BaseProvider extends SafeEventEmitter {
    * Sets initial state if provided and marks this provider as initialized.
    * Throws if called more than once.
    *
+   * Permits the `networkVersion` field in the parameter object for
+   * compatibility with child classes that use this value.
+   *
    * @param initialState - The provider's initial state.
    * @emits BaseProvider#_initialized
    * @emits BaseProvider#connect - If `initialState` is defined.
@@ -215,7 +223,7 @@ export abstract class BaseProvider extends SafeEventEmitter {
     accounts: string[];
     chainId: string;
     isUnlocked: boolean;
-    networkVersion: string;
+    networkVersion?: string;
   }) {
     if (this._state.initialized === true) {
       throw new Error('Provider already initialized.');
@@ -332,44 +340,31 @@ export abstract class BaseProvider extends SafeEventEmitter {
   }
 
   /**
-   * Upon receipt of a new chainId and networkVersion, emits corresponding
-   * events and sets relevant public state.
-   * Does nothing if neither the chainId nor the networkVersion are different
-   * from existing values.
+   * Upon receipt of a new `chainId`, emits the corresponding event and sets
+   * and sets relevant public state. Does nothing if the given `chainId` is
+   * equivalent to the existing value.
    *
-   * @emits MetamaskInpageProvider#chainChanged
+   * Permits the `networkVersion` field in the parameter object for
+   * compatibility with child classes that use this value.
+   *
+   * @emits BaseProvider#chainChanged
    * @param networkInfo - An object with network info.
    * @param networkInfo.chainId - The latest chain ID.
-   * @param networkInfo.networkVersion - The latest network ID.
    */
   protected _handleChainChanged({
     chainId,
-    networkVersion,
   }: { chainId?: string; networkVersion?: string } = {}) {
-    if (
-      !chainId ||
-      typeof chainId !== 'string' ||
-      !chainId.startsWith('0x') ||
-      !networkVersion ||
-      typeof networkVersion !== 'string'
-    ) {
-      this._log.error(
-        'MetaMask: Received invalid network parameters. Please report this bug.',
-        { chainId, networkVersion },
-      );
+    if (!isValidChainId(chainId)) {
+      this._log.error(messages.errors.invalidNetworkParams(), { chainId });
       return;
     }
 
-    if (networkVersion === 'loading') {
-      this._handleDisconnect(true);
-    } else {
-      this._handleConnect(chainId);
+    this._handleConnect(chainId);
 
-      if (chainId !== this.chainId) {
-        this.chainId = chainId;
-        if (this._state.initialized) {
-          this.emit('chainChanged', this.chainId);
-        }
+    if (chainId !== this.chainId) {
+      this.chainId = chainId;
+      if (this._state.initialized) {
+        this.emit('chainChanged', this.chainId);
       }
     }
   }

src/MetaMaskInpageProvider.test.ts

@@ -5,6 +5,35 @@ import {
 } from './MetaMaskInpageProvider';
 import messages from './messages';
 
+async function getInitializedProvider({
+  accounts = [],
+  chainId = '0x0',
+  isUnlocked = true,
+  networkVersion = '0',
+}: Partial<Parameters<MetaMaskInpageProvider['_initializeState']>[0]> = {}) {
+  // This will be called via the constructor
+  const requestMock = jest
+    .spyOn(MetaMaskInpageProvider.prototype, 'request')
+    .mockImplementationOnce(async () => {
+      return {
+        accounts,
+        chainId,
+        isUnlocked,
+        networkVersion,
+      };
+    });
+
+  const mockStream = new MockDuplexStream();
+  const inpageProvider = new MetaMaskInpageProvider(mockStream);
+
+  await new Promise<void>((resolve) => setTimeout(() => resolve(), 1));
+  // Sanity check
+  expect(requestMock).toHaveBeenCalledTimes(1);
+
+  // Return the initialized provider and its stream
+  return [inpageProvider, mockStream] as const;
+}
+
 describe('MetaMaskInpageProvider: RPC', () => {
   const MOCK_ERROR_MESSAGE = 'Did you specify a mock return value?';
 
@@ -621,13 +650,11 @@ describe('MetaMaskInpageProvider: RPC', () => {
   });
 
   describe('provider events', () => {
-    it('calls chainChanged when it chainId changes ', async () => {
-      const mockStream = new MockDuplexStream();
-      const inpageProvider = new MetaMaskInpageProvider(mockStream);
-      (inpageProvider as any)._state.initialized = true;
+    it('calls chainChanged when receiving a new chainId ', async () => {
+      const [inpageProvider, mockStream] = await getInitializedProvider();
 
       await new Promise((resolve) => {
-        inpageProvider.on('chainChanged', (newChainId) => {
+        inpageProvider.once('chainChanged', (newChainId) => {
           expect(newChainId).toBe('0x1');
           resolve(undefined);
         });
@@ -637,20 +664,18 @@ describe('MetaMaskInpageProvider: RPC', () => {
           data: {
             jsonrpc: '2.0',
             method: 'metamask_chainChanged',
-            params: { chainId: '0x1', networkVersion: '0x1' },
+            params: { chainId: '0x1', networkVersion: '1' },
           },
         });
       });
     });
 
-    it('calls networkChanged when it networkVersion changes ', async () => {
-      const mockStream = new MockDuplexStream();
-      const inpageProvider = new MetaMaskInpageProvider(mockStream);
-      (inpageProvider as any)._state.initialized = true;
+    it('calls networkChanged when receiving a new networkVersion ', async () => {
+      const [inpageProvider, mockStream] = await getInitializedProvider();
 
       await new Promise((resolve) => {
-        inpageProvider.on('networkChanged', (newNetworkId) => {
-          expect(newNetworkId).toBe('0x1');
+        inpageProvider.once('networkChanged', (newNetworkId) => {
+          expect(newNetworkId).toBe('1');
           resolve(undefined);
         });
 
@@ -659,10 +684,78 @@ describe('MetaMaskInpageProvider: RPC', () => {
           data: {
             jsonrpc: '2.0',
             method: 'metamask_chainChanged',
-            params: { chainId: '0x1', networkVersion: '0x1' },
+            params: { chainId: '0x1', networkVersion: '1' },
+          },
+        });
+      });
+    });
+
+    it('handles chain changes with intermittent disconnection', async () => {
+      const [inpageProvider, mockStream] = await getInitializedProvider();
+
+      // We check this mostly for the readability of this test.
+      expect(inpageProvider.isConnected()).toBe(true);
+      expect(inpageProvider.chainId).toBe('0x0');
+      expect(inpageProvider.networkVersion).toBe('0');
+
+      const emitSpy = jest.spyOn(inpageProvider, 'emit');
+
+      inpageProvider.once('chainChanged', (newChainId) => {
+        expect(newChainId).toBe('0x1');
+      });
+
+      await new Promise<void>((resolve) => {
+        inpageProvider.once('disconnect', (error) => {
+          expect((error as any).code).toBe(1013);
+          resolve();
+        });
+
+        mockStream.push({
+          name: MetaMaskInpageProviderStreamName,
+          data: {
+            jsonrpc: '2.0',
+            method: 'metamask_chainChanged',
+            // A "loading" networkVersion indicates the network is changing.
+            // Although the chainId is different, chainChanged should not be
+            // emitted in this case.
+            params: { chainId: '0x1', networkVersion: 'loading' },
           },
         });
       });
+
+      // Only once, for "disconnect".
+      expect(emitSpy).toHaveBeenCalledTimes(1);
+      emitSpy.mockClear(); // Clear the mock to avoid keeping a count.
+
+      expect(inpageProvider.isConnected()).toBe(false);
+      // These should be unchanged.
+      expect(inpageProvider.chainId).toBe('0x0');
+      expect(inpageProvider.networkVersion).toBe('0');
+
+      await new Promise<void>((resolve) => {
+        inpageProvider.once('chainChanged', (newChainId) => {
+          expect(newChainId).toBe('0x1');
+          resolve();
+        });
+
+        mockStream.push({
+          name: MetaMaskInpageProviderStreamName,
+          data: {
+            jsonrpc: '2.0',
+            method: 'metamask_chainChanged',
+            params: { chainId: '0x1', networkVersion: '1' },
+          },
+        });
+      });
+
+      expect(emitSpy).toHaveBeenCalledTimes(3);
+      expect(emitSpy).toHaveBeenNthCalledWith(1, 'connect', { chainId: '0x1' });
+      expect(emitSpy).toHaveBeenCalledWith('chainChanged', '0x1');
+      expect(emitSpy).toHaveBeenCalledWith('networkChanged', '1');
+
+      expect(inpageProvider.isConnected()).toBe(true);
+      expect(inpageProvider.chainId).toBe('0x1');
+      expect(inpageProvider.networkVersion).toBe('1');
     });
   });
 });
@@ -733,6 +826,30 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
           }),
       ).not.toThrow();
     });
+
+    it('gets initial state', async () => {
+      // This will be called via the constructor
+      const requestMock = jest
+        .spyOn(MetaMaskInpageProvider.prototype, 'request')
+        .mockImplementationOnce(async () => {
+          return {
+            accounts: ['0xabc'],
+            chainId: '0x0',
+            isUnlocked: true,
+            networkVersion: '0',
+          };
+        });
+
+      const mockStream = new MockDuplexStream();
+      const inpageProvider = new MetaMaskInpageProvider(mockStream);
+
+      await new Promise<void>((resolve) => setTimeout(() => resolve(), 1));
+      expect(requestMock).toHaveBeenCalledTimes(1);
+      expect(inpageProvider.chainId).toBe('0x0');
+      expect(inpageProvider.networkVersion).toBe('0');
+      expect(inpageProvider.selectedAddress).toBe('0xabc');
+      expect(inpageProvider.isConnected()).toBe(true);
+    });
   });
 
   describe('isConnected', () => {

src/MetaMaskInpageProvider.ts

@@ -7,6 +7,8 @@ import {
   EMITTED_NOTIFICATIONS,
   getDefaultExternalMiddleware,
   getRpcPromiseCallback,
+  isValidChainId,
+  isValidNetworkVersion,
   NOOP,
 } from './utils';
 import type { UnvalidatedJsonRpcRequest } from './BaseProvider';
@@ -421,11 +423,10 @@ export class MetaMaskInpageProvider extends AbstractStreamProvider {
 
   /**
    * Upon receipt of a new chainId and networkVersion, emits corresponding
-   * events and sets relevant public state.
-   * Does nothing if neither the chainId nor the networkVersion are different
-   * from existing values.
+   * events and sets relevant public state. Does nothing if neither the chainId
+   * nor the networkVersion are different from existing values.
    *
-   * @emits MetamaskInpageProvider#chainChanged
+   * @emits BaseProvider#chainChanged
    * @emits MetamaskInpageProvider#networkChanged
    * @param networkInfo - An object with network info.
    * @param networkInfo.chainId - The latest chain ID.
@@ -435,13 +436,19 @@ export class MetaMaskInpageProvider extends AbstractStreamProvider {
     chainId,
     networkVersion,
   }: { chainId?: string; networkVersion?: string } = {}) {
-    super._handleChainChanged({ chainId, networkVersion });
+    if (!isValidChainId(chainId) || !isValidNetworkVersion(networkVersion)) {
+      this._log.error(messages.errors.invalidNetworkParams(), {
+        chainId,
+        networkVersion,
+      });
+      return;
+    }
+
+    if (networkVersion === 'loading') {
+      this._handleDisconnect(true);
+    } else if (networkVersion !== this.networkVersion) {
+      super._handleChainChanged({ chainId });
 
-    if (
-      networkVersion &&
-      networkVersion !== 'loading' &&
-      networkVersion !== this.networkVersion
-    ) {
       this.networkVersion = networkVersion;
       if (this._state.initialized) {
         this.emit('networkChanged', this.networkVersion);

src/messages.ts

@@ -9,6 +9,8 @@ const messages = {
     unsupportedSync: (method: string) =>
       `MetaMask: The MetaMask Ethereum provider does not support synchronous methods like ${method} without a callback parameter.`,
     invalidDuplexStream: () => 'Must provide a Node.js-style duplex stream.',
+    invalidNetworkParams: () =>
+      'MetaMask: Received invalid network parameters. Please report this bug.',
     invalidRequestArgs: () => `Expected a single, non-array, object argument.`,
     invalidRequestMethod: () => `'args.method' must be a non-empty string.`,
     invalidRequestParams: () =>

src/utils.ts

@@ -78,4 +78,26 @@ export const getRpcPromiseCallback =
     }
   };
 
+/**
+ * Checks whether the given chain ID is valid, meaning if it is non-empty,
+ * '0x'-prefixed string.
+ *
+ * @param chainId - The chain ID to validate.
+ * @returns Whether the given chain ID is valid.
+ */
+export const isValidChainId = (chainId: unknown): chainId is string =>
+  Boolean(chainId) && typeof chainId === 'string' && chainId.startsWith('0x');
+
+/**
+ * Checks whether the given network version is valid, meaning if it is non-empty
+ * string.
+ *
+ * @param networkVersion - The network version to validate.
+ * @returns Whether the given network version is valid.
+ */
+export const isValidNetworkVersion = (
+  networkVersion: unknown,
+): networkVersion is string =>
+  Boolean(networkVersion) && typeof networkVersion === 'string';
+
 export const NOOP = () => undefined;