From 19b6baefb4508e70da3209354c85940936ed8071 Mon Sep 17 00:00:00 2001 From: Pedro Figueiredo Date: Mon, 28 Aug 2023 17:55:56 +0100 Subject: [PATCH] fix: Remove obsolete network controller state properties (#20586) * fix: remove obsolete network controller state properties * address comments * address comments * address comments * Add additional precautions for data mutation in migration tests --------- Co-authored-by: Mark Stacey --- app/scripts/background.js | 2 +- app/scripts/migrations/092.2.test.ts | 99 ++++++++++++++++++++++++++++ app/scripts/migrations/092.2.ts | 75 +++++++++++++++++++++ app/scripts/migrations/index.js | 2 + 4 files changed, 177 insertions(+), 1 deletion(-) create mode 100644 app/scripts/migrations/092.2.test.ts create mode 100644 app/scripts/migrations/092.2.ts diff --git a/app/scripts/background.js b/app/scripts/background.js index 44f9d177d..dd6848f55 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -360,7 +360,7 @@ async function loadPhishingWarningPage() { } catch (error) { if (error instanceof PhishingWarningPageTimeoutError) { console.warn( - 'Phishing warning page timeout; page not guaraneteed to work offline.', + 'Phishing warning page timeout; page not guaranteed to work offline.', ); } else { console.error('Failed to initialize phishing warning page', error); diff --git a/app/scripts/migrations/092.2.test.ts b/app/scripts/migrations/092.2.test.ts new file mode 100644 index 000000000..513ac4f0f --- /dev/null +++ b/app/scripts/migrations/092.2.test.ts @@ -0,0 +1,99 @@ +import { NetworkType, toHex } from '@metamask/controller-utils'; +import { NetworkStatus } from '@metamask/network-controller'; +import { cloneDeep } from 'lodash'; +import { version as currentStateVersion, migrate } from './092.2'; + +const TEST_NETWORK_CONTROLLER_STATE = { + networkId: 'network-id', + networkStatus: NetworkStatus.Available, + providerConfig: { + type: NetworkType.rpc, + chainId: toHex(42), + nickname: 'Funky Town Chain', + ticker: 'ETH', + id: 'test-network-client-id', + }, + networkDetails: { EIPS: {} }, + networkConfigurations: { + 'network-configuration-id-1': { + chainId: toHex(42), + nickname: 'Localhost 8545', + rpcPrefs: {}, + rpcUrl: 'http://localhost:8545', + ticker: 'ETH', + }, + }, +}; + +const anyPreviousStateVersion = 91; + +describe('migration #96', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('should update the state version number in the appropriate metadata field', async () => { + const originalVersionedState = { + meta: { version: anyPreviousStateVersion }, + data: {}, + }; + + const newStorage = await migrate(cloneDeep(originalVersionedState)); + + expect(newStorage.meta).toStrictEqual({ version: currentStateVersion }); + }); + + it('should return state unaltered if there is no network controller state', async () => { + const originalMetaMaskState = { + anotherController: 'another-controller-state', + }; + const originalVersionedState = { + meta: { version: anyPreviousStateVersion }, + data: originalMetaMaskState, + }; + + const updatedVersionedState = await migrate( + cloneDeep(originalVersionedState), + ); + expect(updatedVersionedState.data).toStrictEqual(originalMetaMaskState); + }); + + it('should return unaltered state if there are no obsolete network controller state properties', async () => { + const originalMetaMaskState = { + anotherController: 'another-controller-state', + NetworkController: TEST_NETWORK_CONTROLLER_STATE, + }; + const originalVersionedState = { + meta: { version: anyPreviousStateVersion }, + data: originalMetaMaskState, + }; + + const updatedVersionedState = await migrate( + cloneDeep(originalVersionedState), + ); + expect(updatedVersionedState.data).toStrictEqual(originalMetaMaskState); + }); + + it('should return updated state without obsolete network controller state properties', async () => { + const originalMetaMaskState = { + anotherController: 'another-controller-state', + NetworkController: { + ...TEST_NETWORK_CONTROLLER_STATE, + someSortOfRogueObsoleteStateProperty: 'exists', + }, + }; + const originalVersionedState = { + meta: { version: anyPreviousStateVersion }, + data: originalMetaMaskState, + }; + + const updatedVersionedState = await migrate( + cloneDeep(originalVersionedState), + ); + expect(updatedVersionedState.data).not.toStrictEqual(originalMetaMaskState); + expect(updatedVersionedState.data).toStrictEqual({ + anotherController: 'another-controller-state', + NetworkController: TEST_NETWORK_CONTROLLER_STATE, + }); + }); +}); diff --git a/app/scripts/migrations/092.2.ts b/app/scripts/migrations/092.2.ts new file mode 100644 index 000000000..ef36a9fa9 --- /dev/null +++ b/app/scripts/migrations/092.2.ts @@ -0,0 +1,75 @@ +import { hasProperty } from '@metamask/utils'; +import { captureException } from '@sentry/browser'; +import { cloneDeep, isObject, pick } from 'lodash'; + +type MetaMaskState = Record; +type VersionedState = { + meta: { version: number }; + data: MetaMaskState; +}; + +export const version = 92.2; + +/** + * This migration removes obsolete NetworkController state properties. + * + * @param originalVersionedState - Versioned MetaMask extension state, exactly what we persist to dist. + * @param originalVersionedState.meta - State metadata. + * @param originalVersionedState.meta.version - The current state version. + * @param originalVersionedState.data - The persisted MetaMask state, keyed by controller. + * @returns Updated versioned of MetaMask extension state. + */ +export async function migrate( + originalVersionedState: VersionedState, +): Promise { + const updatedVersionedState = cloneDeep(originalVersionedState); + + updatedVersionedState.meta.version = version; + updatedVersionedState.data = transformState(updatedVersionedState.data); + + return updatedVersionedState; +} + +function transformState(originalState: MetaMaskState): MetaMaskState { + const updatedState = + filterOutObsoleteNetworkControllerStateProperties(originalState); + + return updatedState; +} + +function filterOutObsoleteNetworkControllerStateProperties( + state: MetaMaskState, +): MetaMaskState { + // https://github.com/MetaMask/core/blob/%40metamask/network-controller%4010.3.1/packages/network-controller/src/NetworkController.ts#L336-L342 + const CURRENT_NETWORK_CONTROLLER_STATE_PROPS = [ + 'networkId', + 'networkStatus', + 'providerConfig', + 'networkDetails', + 'networkConfigurations', + ]; + + if ( + !hasProperty(state, 'NetworkController') || + !isObject(state.NetworkController) + ) { + captureException( + `Migration ${version}: Invalid NetworkController state: ${typeof state.NetworkController}`, + ); + + return state; + } + + const networkControllerState = state.NetworkController; + + // delete network state properties that are not currently in use + const updatedNetworkController = pick( + networkControllerState, + CURRENT_NETWORK_CONTROLLER_STATE_PROPS, + ); + + return { + ...state, + NetworkController: updatedNetworkController, + }; +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 3dcbf1b26..a07a71fa0 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -97,6 +97,7 @@ import * as m090 from './090'; import * as m091 from './091'; import * as m092 from './092'; import * as m092point1 from './092.1'; +import * as m092point2 from './092.2'; import * as m092point3 from './092.3'; const migrations = [ @@ -192,6 +193,7 @@ const migrations = [ m091, m092, m092point1, + m092point2, m092point3, ];