From f95bf2d2441d106d1913a1aee953be9239801753 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 22 Jul 2024 14:36:08 +0100 Subject: [PATCH 1/6] fix: add migration for profile syncing controller we want existing users to have profile syncing disabled, this will remove/nullify the isEnableProfileSyncing state field. --- .../user-storage/user-storage-controller.ts | 4 +- app/scripts/migrations/120.1.test.ts | 35 ++++++++++++++++++ app/scripts/migrations/120.1.ts | 37 +++++++++++++++++++ app/scripts/migrations/index.js | 1 + 4 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 app/scripts/migrations/120.1.test.ts create mode 100644 app/scripts/migrations/120.1.ts diff --git a/app/scripts/controllers/user-storage/user-storage-controller.ts b/app/scripts/controllers/user-storage/user-storage-controller.ts index bc5c0b77c2bb..1015a9cc46f3 100644 --- a/app/scripts/controllers/user-storage/user-storage-controller.ts +++ b/app/scripts/controllers/user-storage/user-storage-controller.ts @@ -27,7 +27,7 @@ export type UserStorageControllerState = { /** * Condition used by UI and to determine if we can use some of the User Storage methods. */ - isProfileSyncingEnabled: boolean; + isProfileSyncingEnabled: boolean | null; /** * Loading state for the profile syncing update */ @@ -35,7 +35,7 @@ export type UserStorageControllerState = { }; const defaultState: UserStorageControllerState = { - isProfileSyncingEnabled: true, + isProfileSyncingEnabled: null, isProfileSyncingUpdateLoading: false, }; diff --git a/app/scripts/migrations/120.1.test.ts b/app/scripts/migrations/120.1.test.ts new file mode 100644 index 000000000000..b207cd675775 --- /dev/null +++ b/app/scripts/migrations/120.1.test.ts @@ -0,0 +1,35 @@ +import { migrate, version } from './120.1'; + +const oldVersion = 120; + +describe('migration #120.1', () => { + afterEach(() => jest.resetAllMocks()); + + it('updates the version metadata', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: {}, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.meta).toStrictEqual({ version }); + }); + + it('sets isProfileSyncing enabled to false', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + UserStorageController: { + isProfileSyncingEnabled: true, + isProfileSyncingUpdateLoading: false, + }, + }, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.data.UserStorageController).toStrictEqual({ + isProfileSyncingEnabled: null, + isProfileSyncingUpdateLoading: false, + }); + }); +}); diff --git a/app/scripts/migrations/120.1.ts b/app/scripts/migrations/120.1.ts new file mode 100644 index 000000000000..fc14017aca27 --- /dev/null +++ b/app/scripts/migrations/120.1.ts @@ -0,0 +1,37 @@ +import { cloneDeep } from 'lodash'; +import { hasProperty } from '@metamask/utils'; +import { InternalAccount } from '@metamask/keyring-api'; + +type VersionedData = { + meta: { version: number }; + data: Record; +}; + +export const version = 120.1; + +/** + * Add a default value for importTime in the InternalAccount + * + * @param originalVersionedData + */ +export async function migrate( + originalVersionedData: VersionedData, +): Promise { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + transformState(versionedData.data); + return versionedData; +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function transformState(state: Record) { + if (!hasProperty(state, 'UserStorageController')) { + return state; + } + + if (hasProperty(state.UserStorageController, 'isProfileSyncingEnabled')) { + state.UserStorageController.isProfileSyncingEnabled = null; + } + + return state; +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 794a5b660974..1b680645a331 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -131,6 +131,7 @@ const migrations = [ require('./118'), require('./119'), require('./120'), + require('./120.1'), require('./121'), require('./123'), From e543fe5aac54d2d51520d753b7e4fbd78022bbf5 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 22 Jul 2024 14:49:04 +0100 Subject: [PATCH 2/6] test: add some additional tests for this migration --- app/scripts/migrations/120.1.test.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/app/scripts/migrations/120.1.test.ts b/app/scripts/migrations/120.1.test.ts index b207cd675775..f27535efd1d4 100644 --- a/app/scripts/migrations/120.1.test.ts +++ b/app/scripts/migrations/120.1.test.ts @@ -15,7 +15,7 @@ describe('migration #120.1', () => { expect(newStorage.meta).toStrictEqual({ version }); }); - it('sets isProfileSyncing enabled to false', async () => { + it('sets isProfileSyncingEnabled to null', async () => { const oldStorage = { meta: { version: oldVersion }, data: { @@ -32,4 +32,16 @@ describe('migration #120.1', () => { isProfileSyncingUpdateLoading: false, }); }); + + it('does nothing if user storage is not initialized', async () => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + OtherController: {}, + }, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }); }); From 4c332fd483c3927eb7388ccca98b580b741353ee Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 22 Jul 2024 15:09:37 +0100 Subject: [PATCH 3/6] fix: fix small mistake and also cover other edge cases during the migration we need to also handle migration of new users who migrated from 11.16 that might have UserStorage in their state --- .../user-storage/user-storage-controller.ts | 2 +- app/scripts/migrations/120.1.test.ts | 9 +++++++- app/scripts/migrations/120.1.ts | 21 ++++++++++++++----- 3 files changed, 25 insertions(+), 7 deletions(-) diff --git a/app/scripts/controllers/user-storage/user-storage-controller.ts b/app/scripts/controllers/user-storage/user-storage-controller.ts index 1015a9cc46f3..57f4930d1d2f 100644 --- a/app/scripts/controllers/user-storage/user-storage-controller.ts +++ b/app/scripts/controllers/user-storage/user-storage-controller.ts @@ -35,7 +35,7 @@ export type UserStorageControllerState = { }; const defaultState: UserStorageControllerState = { - isProfileSyncingEnabled: null, + isProfileSyncingEnabled: true, isProfileSyncingUpdateLoading: false, }; diff --git a/app/scripts/migrations/120.1.test.ts b/app/scripts/migrations/120.1.test.ts index f27535efd1d4..a32d56237b93 100644 --- a/app/scripts/migrations/120.1.test.ts +++ b/app/scripts/migrations/120.1.test.ts @@ -41,7 +41,14 @@ describe('migration #120.1', () => { }, }; + const expectedStorageData = { + OtherController: {}, + UserStorageController: { + isProfileSyncingEnabled: null, + }, + }; + const newStorage = await migrate(oldStorage); - expect(newStorage.data).toStrictEqual(oldStorage.data); + expect(newStorage.data).toStrictEqual(expectedStorageData); }); }); diff --git a/app/scripts/migrations/120.1.ts b/app/scripts/migrations/120.1.ts index fc14017aca27..58e3e51919e4 100644 --- a/app/scripts/migrations/120.1.ts +++ b/app/scripts/migrations/120.1.ts @@ -1,6 +1,5 @@ -import { cloneDeep } from 'lodash'; +import { cloneDeep, isObject } from 'lodash'; import { hasProperty } from '@metamask/utils'; -import { InternalAccount } from '@metamask/keyring-api'; type VersionedData = { meta: { version: number }; @@ -23,14 +22,26 @@ export async function migrate( return versionedData; } -// eslint-disable-next-line @typescript-eslint/no-explicit-any -function transformState(state: Record) { +function transformState( + state: Record, +): Record { + // Existing users who do not have UserStorageController state. + // Provide some initial state & nullify `isProfileSyncingEnabled` if (!hasProperty(state, 'UserStorageController')) { + state.UserStorageController = { + isProfileSyncingEnabled: null, + }; return state; } - if (hasProperty(state.UserStorageController, 'isProfileSyncingEnabled')) { + // Existing users who do have UserStorageController state. + // nullify `isProfileSyncingEnabled` + if ( + isObject(state.UserStorageController) && + hasProperty(state.UserStorageController, 'isProfileSyncingEnabled') + ) { state.UserStorageController.isProfileSyncingEnabled = null; + return state; } return state; From c3d453f95a5f597d56af57ff42a52eb855019d2b Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 22 Jul 2024 15:35:21 +0100 Subject: [PATCH 4/6] test: update test names --- app/scripts/migrations/120.1.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/migrations/120.1.test.ts b/app/scripts/migrations/120.1.test.ts index a32d56237b93..1ea0bb55d3cd 100644 --- a/app/scripts/migrations/120.1.test.ts +++ b/app/scripts/migrations/120.1.test.ts @@ -33,7 +33,7 @@ describe('migration #120.1', () => { }); }); - it('does nothing if user storage is not initialized', async () => { + it('initializes a default user storage state if it did not exist before', async () => { const oldStorage = { meta: { version: oldVersion }, data: { From 5c0894201a57ca920ba697cffefdaa4fc6afce02 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 22 Jul 2024 16:29:25 +0100 Subject: [PATCH 5/6] chore: update e2e test snapshots --- .../errors-after-init-opt-in-background-state.json | 2 +- .../state-snapshots/errors-after-init-opt-in-ui-state.json | 2 +- .../errors-before-init-opt-in-background-state.json | 2 +- .../state-snapshots/errors-before-init-opt-in-ui-state.json | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json b/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json index 8108f769aca5..e5e6a217f42f 100644 --- a/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json +++ b/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-background-state.json @@ -299,7 +299,7 @@ }, "UserOperationController": { "userOperations": "object" }, "UserStorageController": { - "isProfileSyncingEnabled": true, + "isProfileSyncingEnabled": null, "isProfileSyncingUpdateLoading": "boolean" } } diff --git a/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json b/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json index 4708b678fbf3..bf71f9095fbb 100644 --- a/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json +++ b/test/e2e/tests/metrics/state-snapshots/errors-after-init-opt-in-ui-state.json @@ -203,7 +203,7 @@ "nameSources": "object", "userOperations": "object", "isSignedIn": "boolean", - "isProfileSyncingEnabled": true, + "isProfileSyncingEnabled": null, "isProfileSyncingUpdateLoading": "boolean", "subscriptionAccountsSeen": "object", "isMetamaskNotificationsFeatureSeen": "boolean", diff --git a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json index d71759755ad0..922d92b9e360 100644 --- a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json +++ b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json @@ -1,7 +1,7 @@ { "data": { "AuthenticationController": { "isSignedIn": "boolean" }, - "UserStorageController": { "isProfileSyncingEnabled": true }, + "UserStorageController": { "isProfileSyncingEnabled": null }, "MetamaskNotificationsController": { "subscriptionAccountsSeen": "object", "isFeatureAnnouncementsEnabled": "boolean", diff --git a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json index 90c5786fa935..f9dfdee50d2c 100644 --- a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json +++ b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json @@ -1,7 +1,7 @@ { "data": { "AuthenticationController": { "isSignedIn": "boolean" }, - "UserStorageController": { "isProfileSyncingEnabled": true }, + "UserStorageController": { "isProfileSyncingEnabled": null }, "MetamaskNotificationsController": { "subscriptionAccountsSeen": "object", "isFeatureAnnouncementsEnabled": "boolean", From 5d46b4d86fa9c5fdddc3a8ad004e255df3deea10 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Mon, 22 Jul 2024 17:23:53 +0100 Subject: [PATCH 6/6] refactor: handle and capture migration failures due to malformed state --- app/scripts/migrations/120.1.test.ts | 21 +++++++++++++++++++ app/scripts/migrations/120.1.ts | 14 ++++++++----- ...s-before-init-opt-in-background-state.json | 2 +- .../errors-before-init-opt-in-ui-state.json | 2 +- 4 files changed, 32 insertions(+), 7 deletions(-) diff --git a/app/scripts/migrations/120.1.test.ts b/app/scripts/migrations/120.1.test.ts index 1ea0bb55d3cd..83cacd2e3c12 100644 --- a/app/scripts/migrations/120.1.test.ts +++ b/app/scripts/migrations/120.1.test.ts @@ -51,4 +51,25 @@ describe('migration #120.1', () => { const newStorage = await migrate(oldStorage); expect(newStorage.data).toStrictEqual(expectedStorageData); }); + + it('should do nothing if existing UserStorageController state is malformed', async () => { + const actAssertInvalidUserStorageState = async (state: unknown) => { + const oldStorage = { + meta: { version: oldVersion }, + data: { + OtherController: {}, + UserStorageController: state, + }, + }; + + const newStorage = await migrate(oldStorage); + expect(newStorage.data).toStrictEqual(oldStorage.data); + }; + + actAssertInvalidUserStorageState('user storage state is not an object'); + actAssertInvalidUserStorageState({ + // missing the isProfileSyncingEnabled field + isProfileSyncingUpdateLoading: false, + }); + }); }); diff --git a/app/scripts/migrations/120.1.ts b/app/scripts/migrations/120.1.ts index 58e3e51919e4..bf68b6c4815a 100644 --- a/app/scripts/migrations/120.1.ts +++ b/app/scripts/migrations/120.1.ts @@ -1,5 +1,6 @@ import { cloneDeep, isObject } from 'lodash'; import { hasProperty } from '@metamask/utils'; +import { captureException } from '@sentry/browser'; type VersionedData = { meta: { version: number }; @@ -34,15 +35,18 @@ function transformState( return state; } - // Existing users who do have UserStorageController state. - // nullify `isProfileSyncingEnabled` if ( - isObject(state.UserStorageController) && - hasProperty(state.UserStorageController, 'isProfileSyncingEnabled') + !isObject(state.UserStorageController) || + !hasProperty(state.UserStorageController, 'isProfileSyncingEnabled') ) { - state.UserStorageController.isProfileSyncingEnabled = null; + captureException( + `Migration ${version}: Invalid UserStorageController state: ${typeof state.UserStorageController}`, + ); return state; } + // Existing users who do have UserStorageController state. + // nullify `isProfileSyncingEnabled` + state.UserStorageController.isProfileSyncingEnabled = null; return state; } diff --git a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json index 922d92b9e360..d71759755ad0 100644 --- a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json +++ b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-background-state.json @@ -1,7 +1,7 @@ { "data": { "AuthenticationController": { "isSignedIn": "boolean" }, - "UserStorageController": { "isProfileSyncingEnabled": null }, + "UserStorageController": { "isProfileSyncingEnabled": true }, "MetamaskNotificationsController": { "subscriptionAccountsSeen": "object", "isFeatureAnnouncementsEnabled": "boolean", diff --git a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json index f9dfdee50d2c..90c5786fa935 100644 --- a/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json +++ b/test/e2e/tests/metrics/state-snapshots/errors-before-init-opt-in-ui-state.json @@ -1,7 +1,7 @@ { "data": { "AuthenticationController": { "isSignedIn": "boolean" }, - "UserStorageController": { "isProfileSyncingEnabled": null }, + "UserStorageController": { "isProfileSyncingEnabled": true }, "MetamaskNotificationsController": { "subscriptionAccountsSeen": "object", "isFeatureAnnouncementsEnabled": "boolean",