From 2cd60d94e8a3c947cb2ff97d0acb9789c6ffa654 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Thu, 17 Aug 2023 09:04:30 -0230 Subject: [PATCH] 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 | 21 ++- app/scripts/migrations/077.test.js | 70 +++++++++ app/scripts/migrations/092.1.test.ts | 139 ++++++++++++++++++ app/scripts/migrations/092.1.ts | 53 +++++++ app/scripts/migrations/index.js | 2 + test/e2e/tests/errors.spec.js | 2 - .../errors-after-init-opt-in-ui-state.json | 1 + 7 files changed, 284 insertions(+), 4 deletions(-) create mode 100644 app/scripts/migrations/092.1.test.ts create mode 100644 app/scripts/migrations/092.1.ts diff --git a/app/scripts/migrations/077.js b/app/scripts/migrations/077.js index 5a5d4f1ac..9adb293ba 100644 --- a/app/scripts/migrations/077.js +++ b/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; diff --git a/app/scripts/migrations/077.test.js b/app/scripts/migrations/077.test.js index 53efb5cd5..67de1a8fa 100644 --- a/app/scripts/migrations/077.test.js +++ b/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: { diff --git a/app/scripts/migrations/092.1.test.ts b/app/scripts/migrations/092.1.test.ts new file mode 100644 index 000000000..7ab1045ca --- /dev/null +++ b/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: {}, + }); + }); +}); diff --git a/app/scripts/migrations/092.1.ts b/app/scripts/migrations/092.1.ts new file mode 100644 index 000000000..62b47fb04 --- /dev/null +++ b/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; +}) { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + versionedData.data = transformState(versionedData.data); + return versionedData; +} + +function transformState(state: Record) { + 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; +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index dc4fcd4e0..f5c169a1f 100644 --- a/app/scripts/migrations/index.js +++ b/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; diff --git a/test/e2e/tests/errors.spec.js b/test/e2e/tests/errors.spec.js index 5f2128d32..36f1921b3 100644 --- a/test/e2e/tests/errors.spec.js +++ b/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', ]; /** diff --git a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json index 129ae885a..89bb21722 100644 --- a/test/e2e/tests/state-snapshots/errors-after-init-opt-in-ui-state.json +++ b/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",