From 44c2275a04ebd82d734dab5c316857dbe7066c2f Mon Sep 17 00:00:00 2001 From: Aslau Mario-Daniel Date: Wed, 19 Jun 2024 19:38:07 +0100 Subject: [PATCH] feat: Log the validity of the keyringController in the top 3 Migrations that appear in Sentry (#9785) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …migrations that have been reporting errors ## **Description** As part of the Vault Corruption Tactics: https://www.notion.so/metamask-consensys/Vault-Corruption-98cdfc486e75419fa3e53b7bfe244060 We have to increase our logging around possible areas that could be leading to Vault Corruption, to gather more information. Add Sentry logs to log the validity of the keyringController in the top 3 Migrations that appear in Sentry, with the highest number of issues. Issue: https://github.com/MetaMask/mobile-planning/issues/1712 Fixes: vault check in EngineService ## **Related issues** ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** ## **Pre-merge author checklist** - [ ] I’ve followed [MetaMask Coding Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.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/core/EngineService/EngineService.ts | 4 ++-- app/store/migrations/023.js | 10 ++++++++++ app/store/migrations/026.js | 10 ++++++++++ app/store/migrations/026.test.js | 5 +++++ app/store/migrations/035.ts | 7 +++++++ app/store/migrations/036.test.ts | 15 ++++++++++++++- app/store/migrations/036.ts | 10 ++++++++++ app/store/migrations/037.test.ts | 2 ++ app/store/migrations/037.ts | 10 ++++++++++ 9 files changed, 70 insertions(+), 3 deletions(-) diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 67062a4e1ed..c42f44cae48 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -119,7 +119,7 @@ class EngineService { } engine?.datamodel?.subscribe?.(() => { - if (!engine.context.KeyringController.vault) { + if (!engine.context.KeyringController.metadata.vault) { Logger.message('keyringController vault missing for INIT_BG_STATE_KEY'); } if (!this.engineInitialized) { @@ -131,7 +131,7 @@ class EngineService { controllers.forEach((controller) => { const { name, key = undefined } = controller; const update_bg_state_cb = () => { - if (!engine.context.KeyringController.vault) { + if (!engine.context.KeyringController.metadata.vault) { Logger.message( 'keyringController vault missing for UPDATE_BG_STATE_KEY', ); diff --git a/app/store/migrations/023.js b/app/store/migrations/023.js index e9799778afa..502b6ec0966 100644 --- a/app/store/migrations/023.js +++ b/app/store/migrations/023.js @@ -24,6 +24,16 @@ import ambiguousNetworks from './migration-data/amibiguous-networks.json'; * **/ export default function migrate(state) { + const keyringControllerState = state.engine.backgroundState.KeyringController; + if (!isObject(keyringControllerState)) { + captureException( + // @ts-expect-error We are not returning state not to stop the flow of Vault recovery + new Error( + `Migration 23: Invalid vault in KeyringController: '${typeof keyringControllerState}'`, + ), + ); + } + const networkControllerState = state.engine.backgroundState.NetworkController; const addressBookControllerState = state.engine.backgroundState.AddressBookController; diff --git a/app/store/migrations/026.js b/app/store/migrations/026.js index 45d42544940..7d1f89d2372 100644 --- a/app/store/migrations/026.js +++ b/app/store/migrations/026.js @@ -1,4 +1,5 @@ import { captureException } from '@sentry/react-native'; +import { isObject } from '@metamask/utils'; /** * This migration is to free space of unused data in the user devices @@ -6,6 +7,15 @@ import { captureException } from '@sentry/react-native'; * **/ export default function migrate(state) { + const keyringControllerState = state.engine.backgroundState.KeyringController; + if (!isObject(keyringControllerState)) { + captureException( + // @ts-expect-error We are not returning state not to stop the flow of Vault recovery + new Error( + `Migration 26: Invalid vault in KeyringController: '${typeof keyringControllerState}'`, + ), + ); + } const phishingControllerState = state.engine.backgroundState.PhishingController; if (phishingControllerState?.listState) { diff --git a/app/store/migrations/026.test.js b/app/store/migrations/026.test.js index 87be9ebcc63..3b4b47130a4 100644 --- a/app/store/migrations/026.test.js +++ b/app/store/migrations/026.test.js @@ -19,6 +19,7 @@ describe('Migration #26', () => { PhishingController: { listState: {}, }, + KeyringController: { vault: {} }, }, }, }; @@ -28,6 +29,7 @@ describe('Migration #26', () => { engine: { backgroundState: { PhishingController: {}, + KeyringController: { vault: {} }, }, }, }); @@ -41,6 +43,7 @@ describe('Migration #26', () => { hotlistLastFetched: 1, stalelistLastFetched: 1, }, + KeyringController: { vault: {} }, }, }, }; @@ -53,6 +56,7 @@ describe('Migration #26', () => { hotlistLastFetched: 0, stalelistLastFetched: 0, }, + KeyringController: { vault: {} }, }, }, }); @@ -63,6 +67,7 @@ describe('Migration #26', () => { engine: { backgroundState: { PhishingController: {}, + KeyringController: { vault: {} }, }, }, }; diff --git a/app/store/migrations/035.ts b/app/store/migrations/035.ts index a5afd306217..deb17288db3 100644 --- a/app/store/migrations/035.ts +++ b/app/store/migrations/035.ts @@ -38,6 +38,13 @@ export default async function migrate(stateAsync: unknown) { return state; } + const keyringControllerState = state.engine.backgroundState.KeyringController; + if (!isObject(keyringControllerState)) { + captureException( + `Migration 35: Invalid vault in KeyringController: '${typeof keyringControllerState}'`, + ); + } + const networkControllerState = state.engine.backgroundState.NetworkController; const newNetworkControllerState = state.engine.backgroundState .NetworkController as NetworkState; diff --git a/app/store/migrations/036.test.ts b/app/store/migrations/036.test.ts index 85df6708ddc..0a3ff51760a 100644 --- a/app/store/migrations/036.test.ts +++ b/app/store/migrations/036.test.ts @@ -81,6 +81,7 @@ function createMockState( PreferencesController: { ...preferenceState, }, + KeyringController: { vault: {} }, }, }, }; @@ -117,6 +118,7 @@ describe('Migration #036', () => { engine: { backgroundState: { PreferencesController: undefined, + KeyringController: { vault: {} }, }, }, }; @@ -132,6 +134,7 @@ describe('Migration #036', () => { engine: { backgroundState: { PreferencesController: {}, + KeyringController: { vault: {} }, }, }, }; @@ -154,12 +157,12 @@ describe('Migration #036', () => { selectedAddress: MOCK_ADDRESS_1, }); const newState = migrate(oldState); - const expectedUuid = getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS_1); const resultInternalAccount = expectedInternalAccount( MOCK_ADDRESS_1, 'Account 1', ); + expect(newState).toStrictEqual({ engine: { backgroundState: { @@ -181,6 +184,7 @@ describe('Migration #036', () => { }, selectedAddress: '0x0', }, + KeyringController: { vault: {} }, }, }, }); @@ -218,6 +222,7 @@ describe('Migration #036', () => { }, }, PreferencesController: expect.any(Object), + KeyringController: { vault: {} }, }, }, }); @@ -247,6 +252,7 @@ describe('Migration #036', () => { selectedAccount: expectedUuid, }, }, + KeyringController: { vault: {} }, }, }, }); @@ -282,6 +288,7 @@ describe('Migration #036', () => { }, }, PreferencesController: expect.any(Object), + KeyringController: { vault: {} }, }, }, }); @@ -306,6 +313,7 @@ describe('Migration #036', () => { selectedAccount: '', // Expect no account to be selected }, }, + KeyringController: { vault: {} }, }, }, }); @@ -332,6 +340,7 @@ describe('Migration #036', () => { getUUIDFromAddressOfNormalAccount(MOCK_ADDRESS_1), }, }, + KeyringController: { vault: {} }, }, }, }); @@ -345,6 +354,7 @@ describe('Migration #036', () => { identities: {}, selectedAddress: '', }, + KeyringController: { vault: {} }, }, }, }; @@ -359,6 +369,7 @@ describe('Migration #036', () => { selectedAccount: '', }, }, + KeyringController: { vault: {} }, }, }, }); @@ -393,6 +404,7 @@ describe('Migration #036', () => { }, }, }, + KeyringController: { vault: {} }, }, }, }); @@ -436,6 +448,7 @@ describe('Migration #036', () => { selectedAccount: expectedUuid, }, }, + KeyringController: { vault: {} }, }, }, }); diff --git a/app/store/migrations/036.ts b/app/store/migrations/036.ts index 675a7ab5c83..fc269148cd8 100644 --- a/app/store/migrations/036.ts +++ b/app/store/migrations/036.ts @@ -40,6 +40,16 @@ export default function migrate(state: unknown) { ); return state; } + + const keyringControllerState = state.engine.backgroundState.KeyringController; + if (!isObject(keyringControllerState)) { + captureException( + new Error( + `Migration 36: Invalid vault in KeyringController: '${typeof keyringControllerState}'`, + ), + ); + } + if (!isObject(state.engine.backgroundState.PreferencesController)) { captureException( new Error( diff --git a/app/store/migrations/037.test.ts b/app/store/migrations/037.test.ts index 072610e8e3a..6e316f26c71 100644 --- a/app/store/migrations/037.test.ts +++ b/app/store/migrations/037.test.ts @@ -68,6 +68,7 @@ describe('Migration #37', () => { NetworkController: { networkId: '1', }, + KeyringController: { vault: {} }, }, }, }; @@ -77,6 +78,7 @@ describe('Migration #37', () => { engine: { backgroundState: { NetworkController: {}, + KeyringController: { vault: {} }, }, }, }; diff --git a/app/store/migrations/037.ts b/app/store/migrations/037.ts index 13b9b9f6e62..4c69a94cd45 100644 --- a/app/store/migrations/037.ts +++ b/app/store/migrations/037.ts @@ -29,6 +29,16 @@ export default async function migrate(stateAsync: unknown) { ); return state; } + + const keyringControllerState = state.engine.backgroundState.KeyringController; + if (!isObject(keyringControllerState)) { + captureException( + new Error( + `Migration 37: Invalid vault in KeyringController: '${typeof keyringControllerState}'`, + ), + ); + } + const networkControllerState = state.engine.backgroundState.NetworkController; if (!isObject(networkControllerState)) {