Remove invalid tokensChainsCache state (#20495)

Migration #77 would set the `TokenListController.tokensChainsCache`
state to `undefined` if it wasn't already set to anything when that
migration was run. This is probably harmless except that it results
in Sentry errors during migrations, and it results in that property
having a value (at least temporarily) that doesn't match its type.

Migration #77 has been updated to prevent this property from being
set to `undefined` going forward. A new migration has been added to
delete this value for any users already affected by this problem. The
new migration was named "92.1" so that it could run after 92 but before
93, to make backporting this to v10.34.x easier (v10.34.x is currently
on migration 92). "92.1" is still a valid number so this should work
just as well as a whole number.

app/scripts/migrations/077.js

@@ -1,4 +1,6 @@
 import { cloneDeep } from 'lodash';
+import log from 'loglevel';
+import { hasProperty, isObject } from '@metamask/utils';
 import transformState077For082 from './077-supplements/077-supplement-for-082';
 import transformState077For084 from './077-supplements/077-supplement-for-084';
 import transformState077For086 from './077-supplements/077-supplement-for-086';
@@ -29,8 +31,23 @@ export default {
 };
 
 function transformState(state) {
-  const TokenListController = state?.TokenListController || {};
+  if (!hasProperty(state, 'TokenListController')) {
-
+    log.warn('Skipping migration, TokenListController state is missing');
+    return state;
+  } else if (!isObject(state.TokenListController)) {
+    global.sentry?.captureException?.(
+      new Error(
+        `typeof state.TokenListController is ${typeof state.TokenListController}`,
+      ),
+    );
+    return state;
+  } else if (!hasProperty(state.TokenListController, 'tokensChainsCache')) {
+    log.warn(
+      'Skipping migration, TokenListController.tokensChainsCache state is missing',
+    );
+    return state;
+  }
+  const { TokenListController } = state;
   const { tokensChainsCache } = TokenListController;
 
   let dataCache;

app/scripts/migrations/077.test.js

@@ -1,11 +1,22 @@
+import { cloneDeep } from 'lodash';
 import migration77 from './077';
 
+const sentryCaptureExceptionMock = jest.fn();
+
+global.sentry = {
+  startSession: jest.fn(),
+  endSession: jest.fn(),
+  toggleSession: jest.fn(),
+  captureException: sentryCaptureExceptionMock,
+};
+
 describe('migration #77', () => {
   it('should update the version metadata', async () => {
     const oldStorage = {
       meta: {
         version: 76,
       },
+      data: {},
     };
 
     const newStorage = await migration77.migrate(oldStorage);
@@ -13,6 +24,65 @@ describe('migration #77', () => {
       version: 77,
     });
   });
+
+  it('should return state unchanged if token list controller is missing', async () => {
+    const oldStorage = {
+      meta: {
+        version: 76,
+      },
+      data: {
+        Foo: {
+          bar: 'baz',
+        },
+      },
+    };
+
+    const newStorage = await migration77.migrate(cloneDeep(oldStorage));
+
+    expect(newStorage.data).toStrictEqual(oldStorage.data);
+  });
+
+  it('should capture an exception if the TokenListController state is invalid', async () => {
+    const oldStorage = {
+      meta: {
+        version: 76,
+      },
+      data: {
+        TokenListController: 'test',
+      },
+    };
+
+    await migration77.migrate(oldStorage);
+
+    expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1);
+    expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
+      new Error(`typeof state.TokenListController is string`),
+    );
+  });
+
+  it('should return state unchanged if tokenChainsCache is missing', async () => {
+    const oldStorage = {
+      meta: {
+        version: 76,
+      },
+      data: {
+        TokenListController: {
+          tokenList: {
+            '0x514910771af9ca656af840dff83e8264ecf986ca': {
+              address: '0x514910771af9ca656af840dff83e8264ecf986ca',
+              symbol: 'LINK',
+              decimals: 18,
+            },
+          },
+        },
+      },
+    };
+
+    const newStorage = await migration77.migrate(cloneDeep(oldStorage));
+
+    expect(newStorage.data).toStrictEqual(oldStorage.data);
+  });
+
   it('should change the data from array to object for a single network', async () => {
     const oldStorage = {
       meta: {

app/scripts/migrations/092.1.test.ts

@@ -0,0 +1,139 @@
+import { cloneDeep } from 'lodash';
+import { migrate, version } from './092.1';
+
+const PREVIOUS_VERSION = 92;
+
+const sentryCaptureExceptionMock = jest.fn();
+
+global.sentry = {
+  startSession: jest.fn(),
+  endSession: jest.fn(),
+  toggleSession: jest.fn(),
+  captureException: sentryCaptureExceptionMock,
+};
+
+describe('migration #92.1', () => {
+  afterEach(() => {
+    jest.resetAllMocks();
+  });
+
+  it('should update the version metadata', async () => {
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: {},
+    };
+
+    const newStorage = await migrate(oldStorage);
+    expect(newStorage.meta).toStrictEqual({
+      version,
+    });
+  });
+
+  it('should return state unaltered if there is no TokenListController state', async () => {
+    const oldData = {
+      other: 'data',
+    };
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('captures an exception if the TokenListController state is invalid', async () => {
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: { TokenListController: 'this is not valid' },
+    };
+
+    await migrate(oldStorage);
+
+    expect(sentryCaptureExceptionMock).toHaveBeenCalledTimes(1);
+    expect(sentryCaptureExceptionMock).toHaveBeenCalledWith(
+      new Error(`typeof state.TokenListController is string`),
+    );
+  });
+
+  it('should return state unaltered if there is no TokenListController tokensChainsCache state', async () => {
+    const oldData = {
+      other: 'data',
+      TokenListController: {
+        tokenList: {},
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should return state unaltered if the tokensChainsCache state is an unexpected type', async () => {
+    const oldData = {
+      other: 'data',
+      TokenListController: {
+        tokensChainsCache: 'unexpected string',
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should return state unaltered if the tokensChainsCache state is valid', async () => {
+    const oldData = {
+      other: 'data',
+      TokenListController: {
+        tokensChainsCache: {},
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual(oldData);
+  });
+
+  it('should remove undefined tokensChainsCache state', async () => {
+    const oldData = {
+      other: 'data',
+      TokenListController: {
+        tokensChainsCache: undefined,
+      },
+    };
+    const oldStorage = {
+      meta: {
+        version: PREVIOUS_VERSION,
+      },
+      data: oldData,
+    };
+
+    const newStorage = await migrate(cloneDeep(oldStorage));
+    expect(newStorage.data).toStrictEqual({
+      other: 'data',
+      TokenListController: {},
+    });
+  });
+});

app/scripts/migrations/092.1.ts

@@ -0,0 +1,53 @@
+import { cloneDeep } from 'lodash';
+import { hasProperty, isObject } from '@metamask/utils';
+import log from 'loglevel';
+
+export const version = 92.1;
+
+/**
+ * Check whether the `TokenListController.tokensChainsCache` state is
+ * `undefined`, and delete it if so.
+ *
+ * This property was accidentally set to `undefined` by an earlier revision of
+ * migration #77 in some cases.
+ *
+ * @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 (!hasProperty(state, 'TokenListController')) {
+    log.warn('Skipping migration, TokenListController state is missing');
+    return state;
+  } else if (!isObject(state.TokenListController)) {
+    global.sentry?.captureException?.(
+      new Error(
+        `typeof state.TokenListController is ${typeof state.TokenListController}`,
+      ),
+    );
+    return state;
+  } else if (!hasProperty(state.TokenListController, 'tokensChainsCache')) {
+    log.warn(
+      'Skipping migration, TokenListController.tokensChainsCache state is missing',
+    );
+    return state;
+  }
+
+  if (state.TokenListController.tokensChainsCache === undefined) {
+    delete state.TokenListController.tokensChainsCache;
+  }
+
+  return state;
+}

app/scripts/migrations/index.js

@@ -96,6 +96,7 @@ import * as m089 from './089';
 import * as m090 from './090';
 import * as m091 from './091';
 import * as m092 from './092';
+import * as m092point1 from './092.1';
 
 const migrations = [
   m002,
@@ -189,6 +190,7 @@ const migrations = [
   m090,
   m091,
   m092,
+  m092point1,
 ];
 
 export default migrations;

test/e2e/tests/errors.spec.js

@@ -20,7 +20,6 @@ const removedBackgroundFields = [
   // These properties are set to undefined, causing inconsistencies between Chrome and Firefox
   'AppStateController.currentPopupId',
   'AppStateController.timeoutMinutes',
-  'TokenListController.tokensChainsCache',
 ];
 
 const removedUiFields = [
@@ -29,7 +28,6 @@ const removedUiFields = [
   // These properties are set to undefined, causing inconsistencies between Chrome and Firefox
   'metamask.currentPopupId',
   'metamask.timeoutMinutes',
-  'metamask.tokensChainsCache',
 ];
 
 /**

test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json

@@ -130,6 +130,7 @@
     "estimatedGasFeeTimeBounds": "object",
     "gasEstimateType": "string",
     "tokenList": "object",
+    "tokensChainsCache": "object",
     "preventPollingOnNetworkRestart": "boolean",
     "tokens": "object",
     "ignoredTokens": "object",