Remove the network controller previousProviderStore (#18593)

* Remove the network controller `previousProviderStore`

The `previousProvider` network controller state has been replaced with
a private internal property. This was only used internally, it did not
need to be part of state.

This relates to https://github.com/MetaMask/metamask-extension/issues/18303

* Remove redundant tests

.storybook/test-data.js

@@ -606,13 +606,6 @@ const state = {
       rpcUrl: '',
       chainId: '0x5',
     },
-    previousProviderStore: {
-      type: 'goerli',
-      ticker: 'ETH',
-      nickname: '',
-      rpcUrl: '',
-      chainId: '0x5',
-    },
     network: '5',
     accounts: {
       '0x64a845a5b02460acf8a3d84503b0d68d028b4bb4': {

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

@@ -647,12 +647,6 @@ describe('NetworkController', () => {
               },
               "networkId": null,
               "networkStatus": "unknown",
-              "previousProviderStore": {
-                "chainId": "0x9999",
-                "nickname": "Test initial state",
-                "rpcUrl": "http://example-custom-rpc.metamask.io",
-                "type": "rpc",
-              },
               "provider": {
                 "chainId": "0x9999",
                 "nickname": "Test initial state",
@@ -677,13 +671,6 @@ describe('NetworkController', () => {
             },
             "networkId": null,
             "networkStatus": "unknown",
-            "previousProviderStore": {
-              "chainId": "0x539",
-              "nickname": "Localhost 8545",
-              "rpcUrl": "http://localhost:8545",
-              "ticker": "ETH",
-              "type": "rpc",
-            },
             "provider": {
               "chainId": "0x539",
               "nickname": "Localhost 8545",
@@ -4116,46 +4103,6 @@ describe('NetworkController', () => {
       );
     });
 
-    it('stores the current provider configuration before overwriting it', async () => {
-      await withController(
-        {
-          state: {
-            provider: {
-              type: 'rpc',
-              rpcUrl: 'https://mock-rpc-url-1',
-              chainId: '0x111',
-              ticker: 'TEST',
-            },
-            networkConfigurations: {
-              testNetworkConfigurationId2: {
-                id: 'testNetworkConfigurationId2',
-                rpcUrl: 'https://mock-rpc-url-2',
-                chainId: '0x222',
-                ticker: 'ABC',
-              },
-            },
-          },
-        },
-        async ({ controller }) => {
-          const network = new CustomNetworkCommunications({
-            customRpcUrl: 'https://mock-rpc-url-2',
-          });
-          network.mockEssentialRpcCalls();
-
-          controller.setActiveNetwork('testNetworkConfigurationId2');
-
-          expect(
-            controller.store.getState().previousProviderStore,
-          ).toStrictEqual({
-            type: 'rpc',
-            rpcUrl: 'https://mock-rpc-url-1',
-            chainId: '0x111',
-            ticker: 'TEST',
-          });
-        },
-      );
-    });
-
     it('overwrites the provider configuration given a networkConfigurationId that matches a configured networkConfiguration', async () => {
       await withController(
         {
@@ -4638,68 +4585,6 @@ describe('NetworkController', () => {
   describe('setProviderType', () => {
     for (const { networkType, chainId, ticker } of INFURA_NETWORKS) {
       describe(`given a type of "${networkType}"`, () => {
-        it('stores the current provider configuration before overwriting it', async () => {
-          await withController(
-            {
-              state: {
-                provider: {
-                  type: 'rpc',
-                  rpcUrl: 'http://mock-rpc-url-2',
-                  chainId: '0xtest2',
-                  nickname: 'test-chain-2',
-                  ticker: 'TEST2',
-                  rpcPrefs: {
-                    blockExplorerUrl: 'test-block-explorer-2.com',
-                  },
-                },
-                networkConfigurations: {
-                  testNetworkConfigurationId1: {
-                    rpcUrl: 'https://mock-rpc-url-1',
-                    chainId: '0xtest',
-                    nickname: 'test-chain',
-                    ticker: 'TEST',
-                    rpcPrefs: {
-                      blockExplorerUrl: 'test-block-explorer.com',
-                    },
-                    id: 'testNetworkConfigurationId1',
-                  },
-                  testNetworkConfigurationId2: {
-                    rpcUrl: 'http://mock-rpc-url-2',
-                    chainId: '0xtest2',
-                    nickname: 'test-chain-2',
-                    ticker: 'TEST2',
-                    rpcPrefs: {
-                      blockExplorerUrl: 'test-block-explorer-2.com',
-                    },
-                    id: 'testNetworkConfigurationId2',
-                  },
-                },
-              },
-            },
-            async ({ controller }) => {
-              const network = new InfuraNetworkCommunications({
-                infuraNetwork: networkType,
-              });
-              network.mockEssentialRpcCalls();
-
-              controller.setProviderType(networkType);
-
-              expect(
-                controller.store.getState().previousProviderStore,
-              ).toStrictEqual({
-                type: 'rpc',
-                rpcUrl: 'http://mock-rpc-url-2',
-                chainId: '0xtest2',
-                nickname: 'test-chain-2',
-                ticker: 'TEST2',
-                rpcPrefs: {
-                  blockExplorerUrl: 'test-block-explorer-2.com',
-                },
-              });
-            },
-          );
-        });
-
         it(`overwrites the provider configuration using type: "${networkType}", chainId: "${chainId}", and ticker "${ticker}", clearing rpcUrl and nickname, and removing rpcPrefs`, async () => {
           await withController(
             {

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

@@ -267,7 +267,6 @@ type NetworkConfigurations = Record<
  */
 export type NetworkControllerState = {
   provider: ProviderConfiguration;
-  previousProviderStore: ProviderConfiguration;
   networkId: NetworkIdState;
   networkStatus: NetworkStatus;
   networkDetails: NetworkDetails;
@@ -424,7 +423,7 @@ export class NetworkController extends EventEmitter {
    * Observable store containing the provider configuration for the previously
    * configured network.
    */
-  previousProviderStore: ObservableStore<ProviderConfiguration>;
+  #previousProviderConfig: ProviderConfiguration;
 
   /**
    * Observable store containing the network ID for the current network or null
@@ -489,9 +488,7 @@ export class NetworkController extends EventEmitter {
     this.providerStore = new ObservableStore(
       state.provider || buildDefaultProviderConfigState(),
     );
-    this.previousProviderStore = new ObservableStore(
+    this.#previousProviderConfig = this.providerStore.getState();
-      this.providerStore.getState(),
-    );
     this.networkIdStore = new ObservableStore(buildDefaultNetworkIdState());
     this.networkStatusStore = new ObservableStore(
       buildDefaultNetworkStatusState(),
@@ -511,7 +508,6 @@ export class NetworkController extends EventEmitter {
 
     this.store = new ComposedStore<NetworkControllerState>({
       provider: this.providerStore,
-      previousProviderStore: this.previousProviderStore,
       networkId: this.networkIdStore,
       networkStatus: this.networkStatusStore,
       networkDetails: this.networkDetails,
@@ -792,7 +788,7 @@ export class NetworkController extends EventEmitter {
    * calling `resetConnection`).
    */
   rollbackToPreviousProvider(): void {
-    const config = this.previousProviderStore.getState();
+    const config = this.#previousProviderConfig;
     this.providerStore.putState(config);
     this._switchNetwork(config);
   }
@@ -871,7 +867,7 @@ export class NetworkController extends EventEmitter {
    * @param providerConfig - The provider configuration.
    */
   _setProviderConfig(providerConfig: ProviderConfiguration): void {
-    this.previousProviderStore.putState(this.providerStore.getState());
+    this.#previousProviderConfig = this.providerStore.getState();
     this.providerStore.putState(providerConfig);
     this._switchNetwork(providerConfig);
   }

app/scripts/migrations/085.test.js

@@ -0,0 +1,91 @@
+import { migrate, version } from './085';
+
+jest.mock('uuid', () => {
+  const actual = jest.requireActual('uuid');
+
+  return {
+    ...actual,
+    v4: jest.fn(),
+  };
+});
+
+describe('migration #85', () => {
+  it('should update the version metadata', async () => {
+    const oldStorage = {
+      meta: {
+        version: 84,
+      },
+      data: {},
+    };
+
+    const newStorage = await migrate(oldStorage);
+    expect(newStorage.meta).toStrictEqual({
+      version,
+    });
+  });
+
+  it('should return state unaltered if there is no network controller state', async () => {
+    const oldData = {
+      other: 'data',
+    };
+    const oldStorage = {
+      meta: {
+        version: 84,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(oldStorage);
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should return state unaltered if there is no network controller previous provider state', async () => {
+    const oldData = {
+      other: 'data',
+      NetworkController: {
+        provider: {
+          some: 'provider',
+        },
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: 84,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(oldStorage);
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should remove the previous provider state', async () => {
+    const oldData = {
+      other: 'data',
+      NetworkController: {
+        previousProviderStore: {
+          example: 'config',
+        },
+        provider: {
+          some: 'provider',
+        },
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: 84,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(oldStorage);
+    expect(newStorage.data).toStrictEqual({
+      other: 'data',
+      NetworkController: {
+        provider: {
+          some: 'provider',
+        },
+      },
+    });
+  });
+});

app/scripts/migrations/085.ts

@@ -0,0 +1,33 @@
+import { cloneDeep } from 'lodash';
+import { isObject } from '@metamask/utils';
+
+export const version = 85;
+
+/**
+ * Remove the now-obsolete network controller `previousProviderStore` state.
+ *
+ * @param originalVersionedData - Versioned MetaMask extension state, exactly what we persist to dist.
+ * @param originalVersionedData.meta - State metadata.
+ * @param originalVersionedData.meta.version - The current state version.
+ * @param originalVersionedData.data - The persisted MetaMask state, keyed by controller.
+ * @returns Updated versioned MetaMask extension state.
+ */
+export async function migrate(originalVersionedData: {
+  meta: { version: number };
+  data: Record<string, unknown>;
+}) {
+  const versionedData = cloneDeep(originalVersionedData);
+  versionedData.meta.version = version;
+  versionedData.data = transformState(versionedData.data);
+  return versionedData;
+}
+
+function transformState(state: Record<string, unknown>) {
+  if (!isObject(state.NetworkController)) {
+    return state;
+  }
+
+  delete state.NetworkController.previousProviderStore;
+
+  return state;
+}

app/scripts/migrations/index.js

@@ -88,6 +88,7 @@ import * as m081 from './081';
 import * as m082 from './082';
 import * as m083 from './083';
 import * as m084 from './084';
+import * as m085 from './085';
 
 const migrations = [
   m002,
@@ -173,6 +174,7 @@ const migrations = [
   m082,
   m083,
   m084,
+  m085,
 ];
 
 export default migrations;