From 69e57d82bc7470fc8c06b1a50906fcebcc815859 Mon Sep 17 00:00:00 2001 From: Mark Stacey Date: Fri, 2 Aug 2024 10:45:22 -0230 Subject: [PATCH] fix: Remove obsolete NetworkController state (#26302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## **Description** Migration 120.2 has been updated to remove obsolete NetworkController state as well. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/26302?quickstart=1) ## **Related issues** Fixes #26297 ## **Manual testing steps** N/A ## **Screenshots/Recordings** N/A ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. --- app/scripts/migrations/120.2.test.ts | 98 ++++++++++++++++++++++++++++ app/scripts/migrations/120.2.ts | 32 +++++++++ 2 files changed, 130 insertions(+) diff --git a/app/scripts/migrations/120.2.test.ts b/app/scripts/migrations/120.2.test.ts index 4e9e4df6b728..531fe4b0fb8c 100644 --- a/app/scripts/migrations/120.2.test.ts +++ b/app/scripts/migrations/120.2.test.ts @@ -99,6 +99,7 @@ describe('migration #120.2', () => { it('still migrates SelectedNetworkController state if other controllers have invalid state', async () => { const oldState = { + NetworkController: 'invalid', SelectedNetworkController: { domains: { 'https://metamask.io': { @@ -194,6 +195,7 @@ describe('migration #120.2', () => { it('still migrates SnapController state if other controllers have invalid state', async () => { const oldState = { + NetworkController: 'invalid', SelectedNetworkController: 'invalid', SnapController: { snapErrors: {}, @@ -215,4 +217,100 @@ describe('migration #120.2', () => { }); }); }); + + describe('NetworkController', () => { + it('does nothing if NetworkController state is not set', async () => { + const oldState = { + PreferencesController: {}, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + }); + + it('captures an error and leaves state unchanged if NetworkController state is corrupted', async () => { + const oldState = { + NetworkController: 'invalid', + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + expect(sentryCaptureExceptionMock).toHaveBeenCalledWith( + new Error( + `Migration ${version}: Invalid NetworkController state of type 'string'`, + ), + ); + }); + + it('does nothing if obsolete properties are not set', async () => { + const oldState = { + NetworkController: { + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual(oldState); + }); + + it('removes all obsolete properties', async () => { + const oldState = { + NetworkController: { + networkDetails: {}, + networkId: 'example', + networkStatus: 'example', + previousProviderStore: 'example', + provider: 'example', + selectedNetworkClientId: 'example', + }, + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data).toEqual({ + NetworkController: { + selectedNetworkClientId: 'example', + }, + }); + }); + + it('still migrates NetworkController state if other controllers have invalid state', async () => { + const oldState = { + NetworkController: { + networkDetails: {}, + networkId: 'example', + networkStatus: 'example', + previousProviderStore: 'example', + provider: 'example', + selectedNetworkClientId: 'example', + }, + SelectedNetworkController: 'invalid', + SnapController: 'invalid', + }; + + const transformedState = await migrate({ + meta: { version: oldVersion }, + data: cloneDeep(oldState), + }); + + expect(transformedState.data.NetworkController).toEqual({ + selectedNetworkClientId: 'example', + }); + }); + }); }); diff --git a/app/scripts/migrations/120.2.ts b/app/scripts/migrations/120.2.ts index 7a6d9f512eb9..70452a710747 100644 --- a/app/scripts/migrations/120.2.ts +++ b/app/scripts/migrations/120.2.ts @@ -79,6 +79,37 @@ function removeObsoleteSelectedNetworkControllerState( } } +/** + * Remove obsolete NetworkController state. + * + * We don't know exactly why yet, but we see from Sentry that some users have these properties + * in state. They should have been removed by migrations long ago. They are no longer used. + * + * @param state - The persisted MetaMask state, keyed by controller. + */ +function removeObsoleteNetworkControllerState( + state: Record, +): void { + if (!hasProperty(state, 'NetworkController')) { + return; + } else if (!isObject(state.NetworkController)) { + global.sentry.captureException( + new Error( + `Migration ${version}: Invalid NetworkController state of type '${typeof state.NetworkController}'`, + ), + ); + return; + } + + const networkControllerState = state.NetworkController; + + delete networkControllerState.networkDetails; + delete networkControllerState.networkId; + delete networkControllerState.networkStatus; + delete networkControllerState.previousProviderStore; + delete networkControllerState.provider; +} + /** * Remove obsolete controller state. * @@ -87,4 +118,5 @@ function removeObsoleteSelectedNetworkControllerState( function transformState(state: Record): void { removeObsoleteSnapControllerState(state); removeObsoleteSelectedNetworkControllerState(state); + removeObsoleteNetworkControllerState(state); }