refactor: Refactor migration 120.2 to prepare for additions

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. * When the `SnapController` state is invalid, we now capture a Sentry exception * A test has been added for this case as well * 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 * 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. Relates to #26297
MetaMask · Aug 1, 2024 · bfbb0e5 · bfbb0e5
1 parent be24e25
commit bfbb0e5
Show file tree

Hide file tree

Showing 2 changed files with 204 additions and 97 deletions.
diff --git a/app/scripts/migrations/120.2.test.ts b/app/scripts/migrations/120.2.test.ts
@@ -1,134 +1,233 @@
+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('sets SelectedNetworkController state to an object containing an empty object "domains" if SelectedNetworkController state is not itself an object', async () => {
+      const oldState = {
+        SelectedNetworkController: 'foo',
+      };
+      const expectedState = {
+        SelectedNetworkController: { domains: {} },
+      };
 
-    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(expectedState);
     });
 
-    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 "perDomainNetwork" property and resets "domains" object in 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 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(expectedState);
     });
 
-    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 expectedState = {
+        SelectedNetworkController: {
+          domains: {},
+        },
+      };
+
+      const transformedState = await migrate({
+        meta: { version: oldVersion },
+        data: cloneDeep(oldState),
+      });
+
+      expect(transformedState.data.SelectedNetworkController).toEqual(
+        expectedState.SelectedNetworkController,
+      );
+    });
   });
 
-  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,33 @@ export async function migrate(
   return versionedData;
 }
 
-function transformState(state: Record<string, unknown>) {
-  if (hasProperty(state, 'SnapController') && isObject(state.SnapController)) {
-    delete state.SnapController.snapErrors;
+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;
   }
 
-  if (!hasProperty(state, 'SelectedNetworkController')) {
-    return state;
-  }
+  delete state.SnapController.snapErrors;
+}
 
-  if (!isObject(state.SelectedNetworkController)) {
+function removeObsoleteSelectedNetworkControllerState(state: Record<string, unknown>): void {
+  if (!hasProperty(state, 'SelectedNetworkController')) {
+    return;
+  } else 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: {} };
   } else if (hasProperty(state.SelectedNetworkController, 'perDomainNetwork')) {
     state.SelectedNetworkController = {
       domains: {},
     };
   }
+}
 
-  return state;
+function transformState(state: Record<string, unknown>): void {
+  removeObsoleteSnapControllerState(state);
+  removeObsoleteSelectedNetworkControllerState(state);
 }