refactor: Refactor migration 120.2 to prepare for additions (#26298)

## **Description** Migration 120.2 has been refactored to prepare for the addition of further obsolete state cleanup steps. A few minor functional changes and test changes have been made as well. The changes are: * The migration has been refactored to group together changes for each controller in separate functions. * The `SelectedNetworkController` migration has been updated to delete state rather than set it to default. This is functionally equivalent (for this controller) and it simplified the migration and tests a bit, avoiding the need to verify that the default state was correct. * When the `SnapController` state is invalid, we now capture a Sentry exception * A test has been added for this case as well * JSDoc comments have been added to the migration * Tests have been grouped together by controller * Tests have been added to ensure corrupted controller state does not impact the obsolete state removal steps of other controllers * Effectively each obsolete state removal function is acting as an independent migration here * A test was added for the case where `SnapController` state is not set * The tests have been updated to deep clone the input to guard against the migration failing to do so * The `transformState` function was given an explicit return type, and the unused return value was removed. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26298?quickstart=1) ## **Related issues** Relates to #26297 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.
MetaMask · Aug 2, 2024 · 5e7c2d5 · 5e7c2d5
1 parent 8e220f4
commit 5e7c2d5
Show file tree

Hide file tree

Showing 2 changed files with 223 additions and 99 deletions.
diff --git a/app/scripts/migrations/120.2.test.ts b/app/scripts/migrations/120.2.test.ts
@@ -1,134 +1,218 @@
+import { cloneDeep } from 'lodash';
 import { migrate, version } from './120.2';
 
+const sentryCaptureExceptionMock = jest.fn();
+
+global.sentry = {
+  captureException: sentryCaptureExceptionMock,
+};
+
 const oldVersion = 120.1;
 
 describe('migration #120.2', () => {
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+
   it('updates the version metadata', async () => {
     const oldStorage = {
       meta: { version: oldVersion },
       data: {},
     };
 
-    const newStorage = await migrate(oldStorage);
+    const newStorage = await migrate(cloneDeep(oldStorage));
 
     expect(newStorage.meta).toStrictEqual({ version });
   });
 
-  it('does nothing if SelectedNetworkController state is not set', async () => {
-    const oldState = {
-      OtherController: {},
-    };
+  describe('SelectedNetworkController', () => {
+    it('does nothing if SelectedNetworkController state is not set', async () => {
+      const oldState = {
+        OtherController: {},
+      };
 
-    const transformedState = await migrate({
-      meta: { version: oldVersion },
-      data: oldState,
-    });
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
 
-    expect(transformedState.data).toEqual(oldState);
-  });
+      expect(transformedState.data).toEqual(oldState);
+    });
 
-  it('sets SelectedNetworkController state to an object containing an empty object "domains" if SelectedNetworkController state is not itself an object', async () => {
-    const oldState = {
-      SelectedNetworkController: 'foo',
-    };
+    it('removes SelectedNetworkController state if SelectedNetworkController state is not itself an object', async () => {
+      const oldState = {
+        SelectedNetworkController: 'foo',
+      };
 
-    const expectedState = {
-      SelectedNetworkController: { domains: {} },
-    };
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
 
-    const transformedState = await migrate({
-      meta: { version: oldVersion },
-      data: oldState,
+      expect(transformedState.data).toEqual({});
     });
 
-    expect(transformedState.data).toEqual(expectedState);
-  });
-
-  it('removes "perDomainNetwork" property and resets "domains" object in SelectedNetworkController state if "perDomainNetwork" property is present', async () => {
-    const oldState = {
-      SelectedNetworkController: {
-        domains: {
-          'https://metamask.io': {
-            network: 'mainnet',
+    it('removes SelectedNetworkController state if "perDomainNetwork" property is present', async () => {
+      const oldState = {
+        SelectedNetworkController: {
+          domains: {
+            'https://metamask.io': {
+              network: 'mainnet',
+            },
           },
+          perDomainNetwork: true,
         },
-        perDomainNetwork: true,
-      },
-    };
+      };
 
-    const expectedState = {
-      SelectedNetworkController: {
-        domains: {},
-      },
-    };
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
 
-    const transformedState = await migrate({
-      meta: { version: oldVersion },
-      data: oldState,
+      expect(transformedState.data).toEqual({});
     });
 
-    expect(transformedState.data).toEqual(expectedState);
-  });
-
-  it('leaves "domains" state unchanged in SelectedNetworkController if "perDomainNetwork" property is not present in SelectedNetworkController state', async () => {
-    const oldState = {
-      SelectedNetworkController: {
-        domains: {
-          'https://metamask.io': {
-            network: 'mainnet',
-          },
-          'https://test.io': {
-            network: 'linea',
-          },
-          'https://uniswap.io': {
-            network: 'optimism',
+    it('leaves "domains" state unchanged in SelectedNetworkController if "perDomainNetwork" property is not present in SelectedNetworkController state', async () => {
+      const oldState = {
+        SelectedNetworkController: {
+          domains: {
+            'https://metamask.io': {
+              network: 'mainnet',
+            },
+            'https://test.io': {
+              network: 'linea',
+            },
+            'https://uniswap.io': {
+              network: 'optimism',
+            },
           },
         },
-      },
-    };
+      };
 
-    const transformedState = await migrate({
-      meta: { version: oldVersion },
-      data: oldState,
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data).toEqual(oldState);
     });
 
-    expect(transformedState.data).toEqual(oldState);
+    it('still migrates SelectedNetworkController state if other controllers have invalid state', async () => {
+      const oldState = {
+        SelectedNetworkController: {
+          domains: {
+            'https://metamask.io': {
+              network: 'mainnet',
+            },
+          },
+          perDomainNetwork: true,
+        },
+        SnapController: 'invalid',
+      };
+
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data.SelectedNetworkController).toBeUndefined();
+    });
   });
 
-  it('strips SnapController.snapErrors if it exists', async () => {
-    const oldState = {
-      SnapController: {
-        snapErrors: {},
-        snapStates: {},
-        unencryptedSnapStates: {},
-        snaps: {},
-      },
-    };
+  describe('SnapController', () => {
+    it('does nothing if SnapController state is not set', async () => {
+      const oldState = {
+        PreferencesController: {},
+      };
 
-    const transformedState = await migrate({
-      meta: { version: oldVersion },
-      data: oldState,
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data).toEqual(oldState);
     });
 
-    expect(transformedState.data).toEqual({
-      SnapController: { snapStates: {}, unencryptedSnapStates: {}, snaps: {} },
+    it('captures an error and leaves state unchanged if SnapController state is corrupted', async () => {
+      const oldState = {
+        SnapController: 'invalid',
+      };
+
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data).toEqual(oldState);
+      expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
+        new Error(
+          `Migration ${version}: Invalid SnapController state of type 'string'`,
+        ),
+      );
     });
-  });
 
-  it('does nothing if SnapController.snapErrors doesnt exist', async () => {
-    const oldState = {
-      SnapController: {
+    it('strips SnapController.snapErrors if it exists', async () => {
+      const oldState = {
+        SnapController: {
+          snapErrors: {},
+          snapStates: {},
+          unencryptedSnapStates: {},
+          snaps: {},
+        },
+      };
+
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data).toEqual({
+        SnapController: {
+          snapStates: {},
+          unencryptedSnapStates: {},
+          snaps: {},
+        },
+      });
+    });
+
+    it('does nothing if SnapController.snapErrors doesnt exist', async () => {
+      const oldState = {
+        SnapController: {
+          snapStates: {},
+          unencryptedSnapStates: {},
+          snaps: {},
+        },
+      };
+
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data).toEqual(oldState);
+    });
+
+    it('still migrates SnapController state if other controllers have invalid state', async () => {
+      const oldState = {
+        SelectedNetworkController: 'invalid',
+        SnapController: {
+          snapErrors: {},
+          snapStates: {},
+          unencryptedSnapStates: {},
+          snaps: {},
+        },
+      };
+
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data.SnapController).toEqual({
         snapStates: {},
         unencryptedSnapStates: {},
         snaps: {},
-      },
-    };
-
-    const transformedState = await migrate({
-      meta: { version: oldVersion },
-      data: oldState,
+      });
     });
-
-    expect(transformedState.data).toEqual(oldState);
   });
 });
diff --git a/app/scripts/migrations/120.2.ts b/app/scripts/migrations/120.2.ts
@@ -26,25 +26,65 @@ export async function migrate(
   return versionedData;
 }
 
-function transformState(state: Record<string, unknown>) {
-  if (hasProperty(state, 'SnapController') && isObject(state.SnapController)) {
-    delete state.SnapController.snapErrors;
+/**
+ * Remove obsolete SnapController state
+ *
+ * The `snapErrors` property was never intended to be persisted, but the initial state for this
+ * property was accidentally persisted for some users due to a bug. See #26280 for details.
+ *
+ * @param state - The persisted MetaMask state, keyed by controller.
+ */
+function removeObsoleteSnapControllerState(
+  state: Record<string, unknown>,
+): void {
+  if (!hasProperty(state, 'SnapController')) {
+    return;
+  } else if (!isObject(state.SnapController)) {
+    global.sentry.captureException(
+      new Error(
+        `Migration ${version}: Invalid SnapController state of type '${typeof state.SnapController}'`,
+      ),
+    );
+    return;
   }
 
+  delete state.SnapController.snapErrors;
+}
+
+/**
+ * Remove obsolete `perDomainNetwork` property from SelectedNetworkController state.
+ *
+ * We don't know exactly why yet, but we see from Sentry that some users have this property still
+ * in state. It is no longer used.
+ *
+ * If we detect that the state is corrupted or that this property is present, we are fixing it by
+ * erasing the state. The consequences of this state being erased are minimal, and this was easier
+ * than fixing state corruption without resetting it.
+ *
+ * @param state - The persisted MetaMask state, keyed by controller.
+ */
+function removeObsoleteSelectedNetworkControllerState(
+  state: Record<string, unknown>,
+): void {
   if (!hasProperty(state, 'SelectedNetworkController')) {
-    return state;
+    return;
   }
-
   if (!isObject(state.SelectedNetworkController)) {
     console.error(
-      `Unexpected state encountered during migration version ${version}: state.SelectedNetworkController is type: ${typeof state.SelectedNetworkController}`,
+      `Migration ${version}: Invalid SelectedNetworkController state of type '${typeof state.SelectedNetworkController}'`,
     );
-    state.SelectedNetworkController = { domains: {} };
+    delete state.SelectedNetworkController;
   } else if (hasProperty(state.SelectedNetworkController, 'perDomainNetwork')) {
-    state.SelectedNetworkController = {
-      domains: {},
-    };
+    delete state.SelectedNetworkController;
   }
+}
 
-  return state;
+/**
+ * Remove obsolete controller state.
+ *
+ * @param state - The persisted MetaMask state, keyed by controller.
+ */
+function transformState(state: Record<string, unknown>): void {
+  removeObsoleteSnapControllerState(state);
+  removeObsoleteSelectedNetworkControllerState(state);
 }