From f3cad47aa13bba0209430087cce373987b71d491 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Fri, 20 Dec 2024 17:05:29 -0500 Subject: [PATCH 01/18] unmask accounts data in sentry logs --- .../sentry/__snapshots__/utils.test.ts.snap | 44 +++++++++- app/util/sentry/utils.js | 55 +++++++++--- app/util/sentry/utils.test.ts | 83 +++++++++++++++++-- 3 files changed, 164 insertions(+), 18 deletions(-) diff --git a/app/util/sentry/__snapshots__/utils.test.ts.snap b/app/util/sentry/__snapshots__/utils.test.ts.snap index 8a5cfd0b599..93a65d6ce02 100644 --- a/app/util/sentry/__snapshots__/utils.test.ts.snap +++ b/app/util/sentry/__snapshots__/utils.test.ts.snap @@ -32,8 +32,48 @@ exports[`captureSentryFeedback maskObject masks initial root state fixture 1`] = }, "AccountsController": { "internalAccounts": { - "accounts": "object", - "selectedAccount": "string", + "accounts": { + "1be55f5b-eba9-41a7-a9ed-a6a8274aca27": { + "address": "string", + "id": "1be55f5b-eba9-41a7-a9ed-a6a8274aca27", + "metadata": { + "importTime": 1720023898234, + "keyring": { + "type": "HD Key Tree", + }, + "lastSelected": 1720023898236, + "name": "Account 1", + }, + "methods": [ + "personal_sign", + "eth_signTransaction", + "eth_signTypedData_v1", + "eth_signTypedData_v3", + "eth_signTypedData_v4", + ], + "options": {}, + "type": "eip155:eoa", + }, + "2be55f5b-eba9-41a7-a9ed-a6a8274aca28": { + "address": "string", + "id": "2be55f5b-eba9-41a7-a9ed-a6a8274aca28", + "metadata": { + "importTime": 1720023898235, + "keyring": { + "type": "HD Key Tree", + }, + "lastSelected": 1720023898237, + "name": "Account 2", + }, + "methods": [ + "personal_sign", + "eth_signTransaction", + ], + "options": {}, + "type": "eip155:eoa", + }, + }, + "selectedAccount": "1be55f5b-eba9-41a7-a9ed-a6a8274aca27", }, }, "AddressBookController": { diff --git a/app/util/sentry/utils.js b/app/util/sentry/utils.js index 44f55fc5123..d7c7e439dae 100644 --- a/app/util/sentry/utils.js +++ b/app/util/sentry/utils.js @@ -32,7 +32,30 @@ export const sentryStateMask = { }, AccountsController: { internalAccounts: { - [AllProperties]: false, + accounts: { + [AllProperties]: { + id: true, + address: false, + type: true, + options: true, + methods: true, + metadata: { + name: true, + importTime: true, + keyring: { + type: true, + }, + nameLastUpdatedAt: true, + snap: { + id: true, + name: true, + enabled: true, + }, + lastSelected: true, + }, + }, + }, + selectedAccount: true, }, }, AddressBookController: { @@ -336,29 +359,39 @@ function removeSES(report) { */ export function maskObject(objectToMask, mask = {}) { if (!objectToMask) return {}; - let maskAllProperties = false; - if (Object.keys(mask).includes(AllProperties)) { - if (Object.keys(mask).length > 1) { - throw new Error('AllProperties mask key does not support sibling keys'); - } - maskAllProperties = true; - } + + // Include both string and symbol keys. + const maskKeys = Reflect.ownKeys(mask); + const allPropertiesMask = maskKeys.includes(AllProperties) + ? mask[AllProperties] + : undefined; return Object.keys(objectToMask).reduce((maskedObject, key) => { - const maskKey = maskAllProperties ? mask[AllProperties] : mask[key]; + // Start with the AllProperties mask if available + let maskKey = allPropertiesMask; + + // If a key-specific mask exists, it overrides the AllProperties mask + if (mask[key] !== undefined && mask[key] !== AllProperties) { + maskKey = mask[key]; + } + const shouldPrintValue = maskKey === true; const shouldIterateSubMask = - Boolean(maskKey) && typeof maskKey === 'object'; + Boolean(maskKey) && + typeof maskKey === 'object' && + maskKey !== AllProperties; const shouldPrintType = maskKey === undefined || maskKey === false; + if (shouldPrintValue) { maskedObject[key] = objectToMask[key]; } else if (shouldIterateSubMask) { maskedObject[key] = maskObject(objectToMask[key], maskKey); } else if (shouldPrintType) { - // Since typeof null is object, it is more valuable to us having the null instead of object + // For excluded fields, return their type or a placeholder maskedObject[key] = objectToMask[key] === null ? 'null' : typeof objectToMask[key]; } + return maskedObject; }, {}); } diff --git a/app/util/sentry/utils.test.ts b/app/util/sentry/utils.test.ts index c36fe4d6c8b..21560216d16 100644 --- a/app/util/sentry/utils.test.ts +++ b/app/util/sentry/utils.test.ts @@ -180,6 +180,21 @@ describe('captureSentryFeedback', () => { options: {}, type: 'eip155:eoa', }, + '2be55f5b-eba9-41a7-a9ed-a6a8274aca28': { + address: '0x1234567890abcdef1234567890abcdef12345678', + id: '2be55f5b-eba9-41a7-a9ed-a6a8274aca28', + metadata: { + importTime: 1720023898235, + keyring: { + type: 'HD Key Tree', + }, + lastSelected: 1720023898237, + name: 'Account 2', + }, + methods: ['personal_sign', 'eth_signTransaction'], + options: {}, + type: 'eip155:eoa', + }, }, selectedAccount: '1be55f5b-eba9-41a7-a9ed-a6a8274aca27', }, @@ -467,9 +482,9 @@ describe('captureSentryFeedback', () => { it('masks initial root state fixture', () => { const maskedState = maskObject(rootState, sentryStateMask); - expect(maskedState).toMatchSnapshot(); }); + it('handles undefined mask', () => { const maskedState = maskObject(rootState, undefined); expect(maskedState).toEqual({ @@ -502,10 +517,12 @@ describe('captureSentryFeedback', () => { wizard: 'object', }); }); + it('handles empty rootState', () => { const maskedState = maskObject({}, sentryStateMask); expect(maskedState).toEqual({}); }); + it('handles rootState with more keys than what is defined in the mask', () => { const maskedState = maskObject(rootState, { legalNotices: true }); expect(maskedState).toEqual({ @@ -541,6 +558,7 @@ describe('captureSentryFeedback', () => { wizard: 'object', }); }); + it('handles submask with { [AllProperties]: false, enabled: true }', () => { const submask = { [AllProperties]: false, @@ -577,13 +595,66 @@ describe('captureSentryFeedback', () => { wizard: 'object', }); }); + + it('masks sensitive data in AccountsController while preserving other fields', () => { + const maskedState = maskObject(rootState, sentryStateMask) as any; + const maskedAccounts = + maskedState.engine.backgroundState.AccountsController.internalAccounts + .accounts; + + const maskedAccount1 = + maskedAccounts['1be55f5b-eba9-41a7-a9ed-a6a8274aca27']; + const maskedAccount2 = + maskedAccounts['2be55f5b-eba9-41a7-a9ed-a6a8274aca28']; + + // Verify addresses are masked + expect(maskedAccount1.address).toBe('string'); + expect(maskedAccount2.address).toBe('string'); + + // Verify non-sensitive data is preserved for account 1 + expect(maskedAccount1.id).toBe('1be55f5b-eba9-41a7-a9ed-a6a8274aca27'); + expect(maskedAccount1.type).toBe('eip155:eoa'); + expect(maskedAccount1.options).toEqual({}); + expect(maskedAccount1.methods).toEqual([ + 'personal_sign', + 'eth_signTransaction', + 'eth_signTypedData_v1', + 'eth_signTypedData_v3', + 'eth_signTypedData_v4', + ]); + expect(maskedAccount1.metadata).toEqual({ + importTime: 1720023898234, + keyring: { type: 'HD Key Tree' }, + lastSelected: 1720023898236, + name: 'Account 1', + }); + + // Verify non-sensitive data is preserved for account 2 + expect(maskedAccount2.id).toBe('2be55f5b-eba9-41a7-a9ed-a6a8274aca28'); + expect(maskedAccount2.type).toBe('eip155:eoa'); + expect(maskedAccount2.options).toEqual({}); + expect(maskedAccount2.methods).toEqual([ + 'personal_sign', + 'eth_signTransaction', + ]); + expect(maskedAccount2.metadata).toEqual({ + importTime: 1720023898235, + keyring: { type: 'HD Key Tree' }, + lastSelected: 1720023898237, + name: 'Account 2', + }); + + // Verify selected account is preserved + const selectedAccountId = + maskedState.engine.backgroundState.AccountsController.internalAccounts + .selectedAccount; + expect(selectedAccountId).toBe('1be55f5b-eba9-41a7-a9ed-a6a8274aca27'); + }); }); it('handle root state with value null and mask false', () => { const submask = { - SnapsController: { - [AllProperties]: false, - }, + [AllProperties]: false, }; const maskedState = maskObject( { @@ -593,7 +664,9 @@ describe('captureSentryFeedback', () => { exampleObj: {}, }, }, - submask, + { + SnapsController: submask, + }, ); expect(maskedState).toEqual({ SnapsController: { From 8e1ab0f7f369aa823234090340bd467739a9f084 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Fri, 20 Dec 2024 17:48:36 -0500 Subject: [PATCH 02/18] unmask non sensetive keyring data --- .../sentry/__snapshots__/utils.test.ts.snap | 14 ++++- app/util/sentry/utils.js | 10 ++++ app/util/sentry/utils.test.ts | 55 ++++++++++++++++++- 3 files changed, 77 insertions(+), 2 deletions(-) diff --git a/app/util/sentry/__snapshots__/utils.test.ts.snap b/app/util/sentry/__snapshots__/utils.test.ts.snap index 93a65d6ce02..8af1d0305c7 100644 --- a/app/util/sentry/__snapshots__/utils.test.ts.snap +++ b/app/util/sentry/__snapshots__/utils.test.ts.snap @@ -103,7 +103,19 @@ exports[`captureSentryFeedback maskObject masks initial root state fixture 1`] = }, "KeyringController": { "isUnlocked": true, - "keyrings": "object", + "keyrings": { + "0": { + "accounts": { + "0": "string", + "1": "string", + }, + "type": "HD Key Tree", + }, + "1": { + "accounts": {}, + "type": "QR Hardware Wallet Device", + }, + }, "vault": "string", }, "LoggingController": { diff --git a/app/util/sentry/utils.js b/app/util/sentry/utils.js index d7c7e439dae..87a5397d290 100644 --- a/app/util/sentry/utils.js +++ b/app/util/sentry/utils.js @@ -76,6 +76,16 @@ export const sentryStateMask = { }, KeyringController: { isUnlocked: true, + vault: false, + keyrings: { + [AllProperties]: { + type: true, + // Each keyring contains an array of accounts (addresses), all of which should be masked + accounts: { + [AllProperties]: false, + }, + }, + }, }, LoggingController: { [AllProperties]: false, diff --git a/app/util/sentry/utils.test.ts b/app/util/sentry/utils.test.ts index 21560216d16..52770640230 100644 --- a/app/util/sentry/utils.test.ts +++ b/app/util/sentry/utils.test.ts @@ -228,9 +228,16 @@ describe('captureSentryFeedback', () => { isUnlocked: true, keyrings: [ { - accounts: ['0x6312c98831d74754f86dd4936668a13b7e9ba411'], + accounts: [ + '0x6312c98831d74754f86dd4936668a13b7e9ba411', + '0xf2ecb579d1225711c2c117f3ab22554357372c822', + ], type: 'HD Key Tree', }, + { + type: 'QR Hardware Wallet Device', + accounts: [], + }, ], vault: '{"cipher":""}', }, @@ -650,6 +657,52 @@ describe('captureSentryFeedback', () => { .selectedAccount; expect(selectedAccountId).toBe('1be55f5b-eba9-41a7-a9ed-a6a8274aca27'); }); + + it('masks sensitive data in KeyringController while preserving type', () => { + const maskedState = maskObject(rootState, sentryStateMask) as any; + + const maskedKeyringController = + maskedState.engine.backgroundState.KeyringController; + + expect(maskedKeyringController.keyrings).toEqual({ + '0': { + accounts: { + '0': 'string', + '1': 'string', + }, + type: 'HD Key Tree', + }, + '1': { + accounts: {}, + type: 'QR Hardware Wallet Device', + }, + }); + + // Verify vault is masked + expect(maskedKeyringController.vault).toBe('string'); + + // keyrings should now be an object with numeric keys instead of an array + expect(typeof maskedKeyringController.keyrings).toBe('object'); + + const firstKeyring = maskedKeyringController.keyrings['0']; + const secondKeyring = maskedKeyringController.keyrings['1']; + + // Verify first keyring type is preserved + expect(firstKeyring.type).toBe('HD Key Tree'); + + // Accounts are now an object as well + expect(typeof firstKeyring.accounts).toBe('object'); + // Check that accounts have numeric keys and their values are masked + expect(firstKeyring.accounts['0']).toBe('string'); + expect(firstKeyring.accounts['1']).toBe('string'); + + // Verify second keyring type is preserved + expect(secondKeyring.type).toBe('QR Hardware Wallet Device'); + + // The second keyring has no accounts, so it should be an empty object + expect(typeof secondKeyring.accounts).toBe('object'); + expect(Object.keys(secondKeyring.accounts).length).toBe(0); + }); }); it('handle root state with value null and mask false', () => { From afb092702cd6313ad04855431f935d29fef0b837 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Fri, 20 Dec 2024 18:03:17 -0500 Subject: [PATCH 03/18] add for engine creation and accounts controller creation --- app/core/Engine/Engine.ts | 33 +++++++++++++------ .../controllers/AccountsController/utils.ts | 14 ++++++++ app/core/EngineService/EngineService.ts | 30 +++++++++++++++-- 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index a586b2c2393..91dc9b015c2 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -276,6 +276,15 @@ export class Engine { initialState: Partial = {}, initialKeyringState?: KeyringControllerState | null, ) { + if (Object.keys(initialState).length === 0) { + Logger.log('Engine initialized with empty state'); + } else { + Logger.log('Engine initialized with non-empty state', { + hasAccountsState: !!initialState.AccountsController, + hasKeyringState: !!initialState.KeyringController, + }); + } + this.controllerMessenger = new ExtendedControllerMessenger(); const isBasicFunctionalityToggleEnabled = () => @@ -971,8 +980,8 @@ export class Engine { disableSnaps: !isBasicFunctionalityToggleEnabled(), }), clientCryptography: { - pbkdf2Sha512: pbkdf2 - } + pbkdf2Sha512: pbkdf2, + }, }); const authenticationController = new AuthenticationController.Controller({ @@ -1173,7 +1182,7 @@ export class Engine { return Boolean( hasProperty(showIncomingTransactions, currentChainId) && - showIncomingTransactions?.[currentHexChainId], + showIncomingTransactions?.[currentHexChainId], ); }, updateTransactions: true, @@ -1302,7 +1311,9 @@ export class Engine { .addProperties({ token_standard: 'ERC20', asset_type: 'token', - chain_id: getDecimalChainId(getGlobalChainId(networkController)), + chain_id: getDecimalChainId( + getGlobalChainId(networkController), + ), }) .build(), ), @@ -1514,7 +1525,7 @@ export class Engine { if ( state.networksMetadata[state.selectedNetworkClientId].status === NetworkStatus.Available && - getGlobalChainId(networkController) !== currentChainId + getGlobalChainId(networkController) !== currentChainId ) { // We should add a state or event emitter saying the provider changed setTimeout(() => { @@ -1534,7 +1545,9 @@ export class Engine { } catch (error) { console.error( error, - `Network ID not changed, current chainId: ${getGlobalChainId(networkController)}`, + `Network ID not changed, current chainId: ${getGlobalChainId( + networkController, + )}`, ); } }, @@ -1772,7 +1785,7 @@ export class Engine { const tokenBalances = allTokenBalances?.[selectedInternalAccount.address as Hex]?.[ - chainId + chainId ] ?? {}; tokens.forEach( (item: { address: string; balance?: string; decimals: number }) => { @@ -1783,9 +1796,9 @@ export class Engine { item.balance || (item.address in tokenBalances ? renderFromTokenMinimalUnit( - tokenBalances[item.address as Hex], - item.decimals, - ) + tokenBalances[item.address as Hex], + item.decimals, + ) : undefined); const tokenBalanceFiat = balanceToFiatNumber( // TODO: Fix this by handling or eliminating the undefined case diff --git a/app/core/Engine/controllers/AccountsController/utils.ts b/app/core/Engine/controllers/AccountsController/utils.ts index 881e86e43ef..65c39a67d82 100644 --- a/app/core/Engine/controllers/AccountsController/utils.ts +++ b/app/core/Engine/controllers/AccountsController/utils.ts @@ -28,6 +28,20 @@ export const createAccountsController = ({ initialState?: AccountsControllerState; }): AccountsController => { try { + // Add logging for AccountsController creation + if (!initialState) { + Logger.log('Creating AccountsController with default state', { + defaultState: defaultAccountsControllerState, + }); + } else { + Logger.log('Creating AccountsController with provided initial state', { + hasSelectedAccount: !!initialState.internalAccounts?.selectedAccount, + accountsCount: Object.keys( + initialState.internalAccounts?.accounts || {}, + ).length, + }); + } + const accountsController = new AccountsController({ messenger, state: initialState ?? defaultAccountsControllerState, diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 6ba3e827cb9..646c2c276c8 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -49,9 +49,7 @@ export class EngineService { tags: getTraceTags(reduxState), }); const state = reduxState?.engine?.backgroundState || {}; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const Engine = UntypedEngine as any; + const Engine = UntypedEngine; try { Engine.init(state); this.updateControllers(Engine); @@ -235,6 +233,32 @@ export class EngineService { const keyringState = await getVaultFromBackup(); const reduxState = ReduxService.store.getState(); const state = reduxState?.engine?.backgroundState || {}; + + // Log the state we're initializing with + Logger.log( + 'Initializing Engine from backup vault', + { + hasKeyringState: !!keyringState, + hasReduxState: Object.keys(state).length > 0, + hasAccountsState: !!state.AccountsController, + }, + true, + ); + + if (state.AccountsController) { + Logger.log( + 'Initializing Engine from backup with AccountsController state', + { + hasSelectedAccount: + !!state.AccountsController.internalAccounts?.selectedAccount, + accountsCount: Object.keys( + state.AccountsController.internalAccounts?.accounts || {}, + ).length, + }, + true, + ); + } + // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any const Engine = UntypedEngine as any; From 3ee2acd1b933b6c2529bdb1d21e6c4676f534d5d Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Fri, 20 Dec 2024 18:08:57 -0500 Subject: [PATCH 04/18] cleanup --- .../controllers/AccountsController/utils.ts | 1 - app/core/EngineService/EngineService.ts | 30 ++----------------- 2 files changed, 3 insertions(+), 28 deletions(-) diff --git a/app/core/Engine/controllers/AccountsController/utils.ts b/app/core/Engine/controllers/AccountsController/utils.ts index 65c39a67d82..8dd08e57125 100644 --- a/app/core/Engine/controllers/AccountsController/utils.ts +++ b/app/core/Engine/controllers/AccountsController/utils.ts @@ -28,7 +28,6 @@ export const createAccountsController = ({ initialState?: AccountsControllerState; }): AccountsController => { try { - // Add logging for AccountsController creation if (!initialState) { Logger.log('Creating AccountsController with default state', { defaultState: defaultAccountsControllerState, diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 646c2c276c8..6ba3e827cb9 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -49,7 +49,9 @@ export class EngineService { tags: getTraceTags(reduxState), }); const state = reduxState?.engine?.backgroundState || {}; - const Engine = UntypedEngine; + // TODO: Replace "any" with type + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const Engine = UntypedEngine as any; try { Engine.init(state); this.updateControllers(Engine); @@ -233,32 +235,6 @@ export class EngineService { const keyringState = await getVaultFromBackup(); const reduxState = ReduxService.store.getState(); const state = reduxState?.engine?.backgroundState || {}; - - // Log the state we're initializing with - Logger.log( - 'Initializing Engine from backup vault', - { - hasKeyringState: !!keyringState, - hasReduxState: Object.keys(state).length > 0, - hasAccountsState: !!state.AccountsController, - }, - true, - ); - - if (state.AccountsController) { - Logger.log( - 'Initializing Engine from backup with AccountsController state', - { - hasSelectedAccount: - !!state.AccountsController.internalAccounts?.selectedAccount, - accountsCount: Object.keys( - state.AccountsController.internalAccounts?.accounts || {}, - ).length, - }, - true, - ); - } - // TODO: Replace "any" with type // eslint-disable-next-line @typescript-eslint/no-explicit-any const Engine = UntypedEngine as any; From 134fc65cd1fe78ee3d499ee938461e352118256d Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Sat, 21 Dec 2024 13:17:06 -0500 Subject: [PATCH 05/18] fix lint --- app/util/sentry/utils.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/util/sentry/utils.test.ts b/app/util/sentry/utils.test.ts index 52770640230..32071388370 100644 --- a/app/util/sentry/utils.test.ts +++ b/app/util/sentry/utils.test.ts @@ -604,6 +604,7 @@ describe('captureSentryFeedback', () => { }); it('masks sensitive data in AccountsController while preserving other fields', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any const maskedState = maskObject(rootState, sentryStateMask) as any; const maskedAccounts = maskedState.engine.backgroundState.AccountsController.internalAccounts @@ -659,6 +660,7 @@ describe('captureSentryFeedback', () => { }); it('masks sensitive data in KeyringController while preserving type', () => { + // eslint-disable-next-line @typescript-eslint/no-explicit-any const maskedState = maskObject(rootState, sentryStateMask) as any; const maskedKeyringController = From 0764a770fe1e3e3fbeb2dbc03d4c5476f3cf05c4 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Sat, 21 Dec 2024 13:47:11 -0500 Subject: [PATCH 06/18] extract engine log logic into util function --- app/core/Engine/Engine.ts | 10 ++-------- app/core/Engine/utils/logger.ts | 20 ++++++++++++++++++++ 2 files changed, 22 insertions(+), 8 deletions(-) create mode 100644 app/core/Engine/utils/logger.ts diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index 91dc9b015c2..64b5a39b199 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -213,6 +213,7 @@ import { getGlobalChainId, getGlobalNetworkClientId, } from '../../util/networks/global-network'; +import { logEngineCreation } from './utils/logger'; const NON_EMPTY = 'NON_EMPTY'; @@ -276,14 +277,7 @@ export class Engine { initialState: Partial = {}, initialKeyringState?: KeyringControllerState | null, ) { - if (Object.keys(initialState).length === 0) { - Logger.log('Engine initialized with empty state'); - } else { - Logger.log('Engine initialized with non-empty state', { - hasAccountsState: !!initialState.AccountsController, - hasKeyringState: !!initialState.KeyringController, - }); - } + logEngineCreation(initialState, initialKeyringState); this.controllerMessenger = new ExtendedControllerMessenger(); diff --git a/app/core/Engine/utils/logger.ts b/app/core/Engine/utils/logger.ts new file mode 100644 index 00000000000..ceee1d234d2 --- /dev/null +++ b/app/core/Engine/utils/logger.ts @@ -0,0 +1,20 @@ +import { KeyringControllerState } from '@metamask/keyring-controller'; +import { EngineState } from '../types'; +import Logger from '../../../util/Logger'; + +export function logEngineCreation( + initialState: Partial = {}, + initialKeyringState?: KeyringControllerState | null, +) { + if (Object.keys(initialState).length === 0) { + Logger.log('Engine initialized with empty state', { + keyringStateFromBackup: !!initialKeyringState, + }); + } else { + Logger.log('Engine initialized with non-empty state', { + hasAccountsState: !!initialState.AccountsController, + hasKeyringState: !!initialState.KeyringController, + keyringStateFromBackup: !!initialKeyringState, + }); + } +} From dc3ce924c11a8dec4d0dd71561a1db0645d42568 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Sat, 21 Dec 2024 13:57:57 -0500 Subject: [PATCH 07/18] test engine logs --- app/core/Engine/utils/test/logger.test.ts | 111 ++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 app/core/Engine/utils/test/logger.test.ts diff --git a/app/core/Engine/utils/test/logger.test.ts b/app/core/Engine/utils/test/logger.test.ts new file mode 100644 index 00000000000..0d43dfc2fd2 --- /dev/null +++ b/app/core/Engine/utils/test/logger.test.ts @@ -0,0 +1,111 @@ +import Logger from '../../../../util/Logger'; +import { logEngineCreation } from '../logger'; + +jest.mock('../../../../util/Logger'); + +describe('logEngineCreation', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('logs empty state initialization', () => { + logEngineCreation({}, null); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with empty state', + { + keyringStateFromBackup: false, + }, + ); + }); + + it('logs empty state initialization with keyring backup', () => { + logEngineCreation( + {}, + { vault: 'test-vault', keyrings: [], isUnlocked: false }, + ); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with empty state', + { + keyringStateFromBackup: true, + }, + ); + }); + + it('logs non-empty state initialization with accounts', () => { + const initialState = { + AccountsController: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }, + }; + + logEngineCreation(initialState, null); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with non-empty state', + { + hasAccountsState: true, + hasKeyringState: false, + keyringStateFromBackup: false, + }, + ); + }); + + it('logs non-empty state initialization with keyring state', () => { + const initialState = { + KeyringController: { + vault: 'test-vault', + keyrings: [], + isUnlocked: false, + }, + }; + + logEngineCreation(initialState, null); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with non-empty state', + { + hasAccountsState: false, + hasKeyringState: true, + keyringStateFromBackup: false, + }, + ); + }); + + it('logs non-empty state initialization with both accounts and keyring', () => { + const initialState = { + AccountsController: { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }, + KeyringController: { + vault: 'test-vault', + keyrings: [], + isUnlocked: false, + }, + }; + + const keyringBackup = { + vault: 'backup-vault', + keyrings: [], + isUnlocked: false, + }; + + logEngineCreation(initialState, keyringBackup); + + expect(Logger.log).toHaveBeenCalledWith( + 'Engine initialized with non-empty state', + { + hasAccountsState: true, + hasKeyringState: true, + keyringStateFromBackup: true, + }, + ); + }); +}); From 1e82dbeb82530cc4cf7b6ec0da3574c91c0a472b Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Sat, 21 Dec 2024 15:55:19 -0500 Subject: [PATCH 08/18] add test for accounts controller logs --- .../AccountsController/logger.test.ts | 54 +++++++++++++++++++ .../controllers/AccountsController/logger.ts | 19 +++++++ .../AccountsController/utils.test.ts | 36 +++++++++++++ .../controllers/AccountsController/utils.ts | 15 +----- 4 files changed, 111 insertions(+), 13 deletions(-) create mode 100644 app/core/Engine/controllers/AccountsController/logger.test.ts create mode 100644 app/core/Engine/controllers/AccountsController/logger.ts diff --git a/app/core/Engine/controllers/AccountsController/logger.test.ts b/app/core/Engine/controllers/AccountsController/logger.test.ts new file mode 100644 index 00000000000..bdc58f5a83b --- /dev/null +++ b/app/core/Engine/controllers/AccountsController/logger.test.ts @@ -0,0 +1,54 @@ +import Logger from '../../../../util/Logger'; +import { logAccountsControllerCreation } from './logger'; +import { defaultAccountsControllerState } from './utils'; +import { MOCK_ACCOUNTS_CONTROLLER_STATE } from '../../../../util/test/accountsControllerTestUtils'; + +jest.mock('../../../../util/Logger'); + +describe('logAccountsControllerCreation', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('logs creation with default state when no initial state provided', () => { + logAccountsControllerCreation(); + + expect(Logger.log).toHaveBeenCalledWith( + 'Creating AccountsController with default state', + { + defaultState: defaultAccountsControllerState, + }, + ); + }); + + it('logs creation with empty initial state', () => { + const initialState = { + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }; + + logAccountsControllerCreation(initialState); + + expect(Logger.log).toHaveBeenCalledWith( + 'Creating AccountsController with provided initial state', + { + hasSelectedAccount: false, + accountsCount: 0, + }, + ); + }); + + it('logs creation with populated initial state', () => { + logAccountsControllerCreation(MOCK_ACCOUNTS_CONTROLLER_STATE); + + expect(Logger.log).toHaveBeenCalledWith( + 'Creating AccountsController with provided initial state', + { + hasSelectedAccount: true, + accountsCount: 2, + }, + ); + }); +}); diff --git a/app/core/Engine/controllers/AccountsController/logger.ts b/app/core/Engine/controllers/AccountsController/logger.ts new file mode 100644 index 00000000000..381d8e3c7a3 --- /dev/null +++ b/app/core/Engine/controllers/AccountsController/logger.ts @@ -0,0 +1,19 @@ +import { AccountsControllerState } from '@metamask/accounts-controller'; +import Logger from '../../../../util/Logger'; +import { defaultAccountsControllerState } from './utils'; + +export function logAccountsControllerCreation( + initialState?: AccountsControllerState, +) { + if (!initialState) { + Logger.log('Creating AccountsController with default state', { + defaultState: defaultAccountsControllerState, + }); + } else { + Logger.log('Creating AccountsController with provided initial state', { + hasSelectedAccount: !!initialState.internalAccounts?.selectedAccount, + accountsCount: Object.keys(initialState.internalAccounts?.accounts || {}) + .length, + }); + } +} diff --git a/app/core/Engine/controllers/AccountsController/utils.test.ts b/app/core/Engine/controllers/AccountsController/utils.test.ts index 67b640cb448..7165e1239a3 100644 --- a/app/core/Engine/controllers/AccountsController/utils.test.ts +++ b/app/core/Engine/controllers/AccountsController/utils.test.ts @@ -10,11 +10,19 @@ import { import { withScope } from '@sentry/react-native'; import { AGREED, METRICS_OPT_IN } from '../../../../constants/storage'; import StorageWrapper from '../../../../store/storage-wrapper'; +import { logAccountsControllerCreation } from './logger'; jest.mock('@sentry/react-native', () => ({ withScope: jest.fn(), })); +jest.mock('./logger', () => ({ + logAccountsControllerCreation: jest.fn(), +})); + const mockedWithScope = jest.mocked(withScope); +const mockedLogAccountsControllerCreation = jest.mocked( + logAccountsControllerCreation, +); describe('accountControllersUtils', () => { describe('createAccountsController', () => { @@ -50,12 +58,38 @@ describe('accountControllersUtils', () => { jest.resetAllMocks(); }); + it('logs creation with default state when no initial state provided', () => { + createAccountsController({ + messenger: accountsControllerMessenger, + }); + expect(mockedLogAccountsControllerCreation).toHaveBeenCalledWith( + undefined, + ); + }); + + it('logs creation with provided initial state', () => { + const initialState = { + internalAccounts: { + accounts: {}, + selectedAccount: '0x1', + }, + }; + createAccountsController({ + messenger: accountsControllerMessenger, + initialState, + }); + expect(mockedLogAccountsControllerCreation).toHaveBeenCalledWith( + initialState, + ); + }); + it('AccountsController state should be default state when no initial state is passed in', () => { const accountsController = createAccountsController({ messenger: accountsControllerMessenger, }); expect(accountsController.state).toEqual(defaultAccountsControllerState); }); + it('AccountsController state should be initial state when initial state is passed in', () => { const initialAccountsControllerState: AccountsControllerState = { internalAccounts: { @@ -69,6 +103,7 @@ describe('accountControllersUtils', () => { }); expect(accountsController.state).toEqual(initialAccountsControllerState); }); + it('AccountsController name should be AccountsController', () => { const accountsControllerName = 'AccountsController'; const accountsController = createAccountsController({ @@ -76,6 +111,7 @@ describe('accountControllersUtils', () => { }); expect(accountsController.name).toEqual(accountsControllerName); }); + it('should detect and log an error when controller fails to initialize', async () => { const brokenAccountsControllerMessenger = 'controllerMessenger' as unknown as AccountsControllerMessenger; diff --git a/app/core/Engine/controllers/AccountsController/utils.ts b/app/core/Engine/controllers/AccountsController/utils.ts index 8dd08e57125..d081bed84e5 100644 --- a/app/core/Engine/controllers/AccountsController/utils.ts +++ b/app/core/Engine/controllers/AccountsController/utils.ts @@ -4,6 +4,7 @@ import { AccountsControllerState, } from '@metamask/accounts-controller'; import Logger from '../../../../util/Logger'; +import { logAccountsControllerCreation } from './logger'; // Default AccountsControllerState export const defaultAccountsControllerState: AccountsControllerState = { @@ -28,19 +29,7 @@ export const createAccountsController = ({ initialState?: AccountsControllerState; }): AccountsController => { try { - if (!initialState) { - Logger.log('Creating AccountsController with default state', { - defaultState: defaultAccountsControllerState, - }); - } else { - Logger.log('Creating AccountsController with provided initial state', { - hasSelectedAccount: !!initialState.internalAccounts?.selectedAccount, - accountsCount: Object.keys( - initialState.internalAccounts?.accounts || {}, - ).length, - }); - } - + logAccountsControllerCreation(initialState); const accountsController = new AccountsController({ messenger, state: initialState ?? defaultAccountsControllerState, From 2f900a071ac10dba2dff8245b2d8a669a7ce2069 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Sat, 21 Dec 2024 16:45:48 -0500 Subject: [PATCH 09/18] validate post migration state --- app/store/index.ts | 5 ++ app/store/validateState.ts | 111 +++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) create mode 100644 app/store/validateState.ts diff --git a/app/store/index.ts b/app/store/index.ts index e158b1e5f81..a14e3728698 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -14,6 +14,7 @@ import persistConfig from './persistConfig'; import getUIStartupSpan from '../core/Performance/UIStartup'; import ReduxService from '../core/redux'; import { onPersistedDataLoaded } from '../actions/user'; +import { validatePostMigrationState } from './validateState'; // TODO: Improve type safety by using real Action types instead of `any` // TODO: Replace "any" with type @@ -69,6 +70,10 @@ const createStoreAndPersistor = async () => { endTrace({ name: TraceName.StoreInit }); // Signal that persisted data has been loaded store.dispatch(onPersistedDataLoaded()); + + // validate the state after migration + const currentState = store.getState(); + validatePostMigrationState(currentState); }; persistor = persistStore(store, null, onPersistComplete); diff --git a/app/store/validateState.ts b/app/store/validateState.ts new file mode 100644 index 00000000000..eedcc84970e --- /dev/null +++ b/app/store/validateState.ts @@ -0,0 +1,111 @@ +import Logger from '../util/Logger'; +import { RootState } from '../reducers'; +import { AccountsControllerState } from '@metamask/accounts-controller'; + +/** + * Each validation check is a function that: + * - Takes the final (post-migration) Redux state, + * - Returns an array of error strings if any validations fail or an empty array otherwise. + */ +type ValidationCheck = (state: RootState) => string[]; + +const LOG_TAG = 'MIGRATION_VALIDATE_STATE_ERROR'; + +/** + * Verifies that the engine is initialized + */ +const checkEngineInitialized: ValidationCheck = (state) => { + const errors: string[] = []; + if (!state.engine?.backgroundState) { + errors.push(`${LOG_TAG}: Engine backgroundState not found.`); + } + return errors; +}; + +/** + * Validates the AccountsControllerState to ensure required + * fields exist and match the expected structure. + * + * @param rootState - The Redux state to validate. + * @returns An array of error messages. If empty, the state is valid. + */ +const checkAccountsController: ValidationCheck = (rootState) => { + const errors: string[] = []; + + // Safely access the AccountsController state in the engine backgroundState + const accountsState: AccountsControllerState | undefined = + rootState.engine?.backgroundState?.AccountsController; + + // If it's missing altogether, return an error + if (!accountsState) { + errors.push( + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ); + return errors; + } + + // 1. Check that internalAccounts exists + if (!accountsState.internalAccounts) { + errors.push( + `${LOG_TAG}: AccountsController No internalAccounts object found on AccountsControllerState.`, + ); + return errors; + } + + const { selectedAccount, accounts } = accountsState.internalAccounts; + + // 2. Confirm there is at least one account + if (!accounts || Object.keys(accounts).length === 0) { + errors.push( + `${LOG_TAG}: AccountsController No accounts found in internalAccounts.accounts.`, + ); + return errors; + } + + // 3. Check that selectedAccount is non-empty + if (!selectedAccount) { + errors.push( + `${LOG_TAG}: AccountsController selectedAccount is missing or empty.`, + ); + return errors; + } + + // 4. Confirm the selectedAccount ID exists in internalAccounts.accounts + if (!accounts[selectedAccount]) { + errors.push( + `${LOG_TAG}: AccountsController The selectedAccount '${selectedAccount}' does not exist in the accounts record.`, + ); + } + + return errors; +}; + +/** + * If you have more checks, simply define them above and add them to this array. + */ +const checks: ValidationCheck[] = [ + checkEngineInitialized, + checkAccountsController, +]; + +/** + * Runs all validations and logs any errors, but doesn’t throw. + * This makes sure your app keeps running even if some data is unexpected. + */ +export function validatePostMigrationState(state: RootState): void { + const allErrors: string[] = []; + + for (const check of checks) { + const errors = check(state); + if (errors.length > 0) { + allErrors.push(...errors); + } + } + + // If there are any errors, log them + if (allErrors.length > 0) { + Logger.error(new Error('Migration validation errors'), { + message: `State validation found these issues: ${allErrors.join(', ')}`, + }); + } +} From 40aa7af3e6282b3fd8cfdb8f8da1270f6619741e Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 14:54:21 -0500 Subject: [PATCH 10/18] validate keyring controller migration --- app/store/index.ts | 2 +- .../accountsController.ts} | 57 +------------------ .../validateMigration/keyringController.ts | 42 ++++++++++++++ .../validateMigration/validateMigration.ts | 46 +++++++++++++++ .../validateMigration.types.ts | 10 ++++ 5 files changed, 102 insertions(+), 55 deletions(-) rename app/store/{validateState.ts => validateMigration/accountsController.ts} (51%) create mode 100644 app/store/validateMigration/keyringController.ts create mode 100644 app/store/validateMigration/validateMigration.ts create mode 100644 app/store/validateMigration/validateMigration.types.ts diff --git a/app/store/index.ts b/app/store/index.ts index a14e3728698..7dca2b0e1c8 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -14,7 +14,7 @@ import persistConfig from './persistConfig'; import getUIStartupSpan from '../core/Performance/UIStartup'; import ReduxService from '../core/redux'; import { onPersistedDataLoaded } from '../actions/user'; -import { validatePostMigrationState } from './validateState'; +import { validatePostMigrationState } from './validateMigration/validateMigration'; // TODO: Improve type safety by using real Action types instead of `any` // TODO: Replace "any" with type diff --git a/app/store/validateState.ts b/app/store/validateMigration/accountsController.ts similarity index 51% rename from app/store/validateState.ts rename to app/store/validateMigration/accountsController.ts index eedcc84970e..574af6b54a5 100644 --- a/app/store/validateState.ts +++ b/app/store/validateMigration/accountsController.ts @@ -1,26 +1,6 @@ -import Logger from '../util/Logger'; -import { RootState } from '../reducers'; -import { AccountsControllerState } from '@metamask/accounts-controller'; - -/** - * Each validation check is a function that: - * - Takes the final (post-migration) Redux state, - * - Returns an array of error strings if any validations fail or an empty array otherwise. - */ -type ValidationCheck = (state: RootState) => string[]; - -const LOG_TAG = 'MIGRATION_VALIDATE_STATE_ERROR'; +import { ValidationCheck, LOG_TAG } from './validateMigration.types'; -/** - * Verifies that the engine is initialized - */ -const checkEngineInitialized: ValidationCheck = (state) => { - const errors: string[] = []; - if (!state.engine?.backgroundState) { - errors.push(`${LOG_TAG}: Engine backgroundState not found.`); - } - return errors; -}; +import { AccountsControllerState } from '@metamask/accounts-controller'; /** * Validates the AccountsControllerState to ensure required @@ -29,10 +9,9 @@ const checkEngineInitialized: ValidationCheck = (state) => { * @param rootState - The Redux state to validate. * @returns An array of error messages. If empty, the state is valid. */ -const checkAccountsController: ValidationCheck = (rootState) => { +export const validateAccountsController: ValidationCheck = (rootState) => { const errors: string[] = []; - // Safely access the AccountsController state in the engine backgroundState const accountsState: AccountsControllerState | undefined = rootState.engine?.backgroundState?.AccountsController; @@ -79,33 +58,3 @@ const checkAccountsController: ValidationCheck = (rootState) => { return errors; }; - -/** - * If you have more checks, simply define them above and add them to this array. - */ -const checks: ValidationCheck[] = [ - checkEngineInitialized, - checkAccountsController, -]; - -/** - * Runs all validations and logs any errors, but doesn’t throw. - * This makes sure your app keeps running even if some data is unexpected. - */ -export function validatePostMigrationState(state: RootState): void { - const allErrors: string[] = []; - - for (const check of checks) { - const errors = check(state); - if (errors.length > 0) { - allErrors.push(...errors); - } - } - - // If there are any errors, log them - if (allErrors.length > 0) { - Logger.error(new Error('Migration validation errors'), { - message: `State validation found these issues: ${allErrors.join(', ')}`, - }); - } -} diff --git a/app/store/validateMigration/keyringController.ts b/app/store/validateMigration/keyringController.ts new file mode 100644 index 00000000000..554342ca839 --- /dev/null +++ b/app/store/validateMigration/keyringController.ts @@ -0,0 +1,42 @@ +import { KeyringControllerState } from '@metamask/keyring-controller'; +import { ValidationCheck, LOG_TAG } from './validateMigration.types'; + +/** + * Validates the KeyringControllerState to ensure required + * fields exist and match the expected structure. + * + * @param rootState - The Redux state to validate. + * @returns An array of error messages. If empty, the state is valid. + */ +export const validateKeyringController: ValidationCheck = (rootState) => { + const errors: string[] = []; + + const keyringControllerState: KeyringControllerState | undefined = + rootState.engine?.backgroundState?.KeyringController; + + // If it's missing altogether, return an error + if (!keyringControllerState) { + errors.push( + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ); + return errors; + } + + // 1. Check that vault exists + if (!keyringControllerState.vault) { + errors.push( + `${LOG_TAG}: KeyringController No vault in KeyringControllerState.`, + ); + return errors; + } + + const keyrings = keyringControllerState.keyrings; + + // 2. Confirm there is at least one account + if (!keyrings || Object.keys(keyrings).length === 0) { + errors.push(`${LOG_TAG}: KeyringController No keyrings found.`); + return errors; + } + + return errors; +}; diff --git a/app/store/validateMigration/validateMigration.ts b/app/store/validateMigration/validateMigration.ts new file mode 100644 index 00000000000..f74fc6ab504 --- /dev/null +++ b/app/store/validateMigration/validateMigration.ts @@ -0,0 +1,46 @@ +import Logger from '../../util/Logger'; +import { RootState } from '../../reducers'; +import { LOG_TAG, ValidationCheck } from './validateMigration.types'; + +// checks +import { validateAccountsController } from './accountsController'; +import { validateKeyringController } from './keyringController'; + +/** + * Verifies that the engine is initialized + */ +const checkEngineInitialized: ValidationCheck = (state) => { + const errors: string[] = []; + if (!state.engine?.backgroundState) { + errors.push(`${LOG_TAG}: Engine backgroundState not found.`); + } + return errors; +}; + +const checks: ValidationCheck[] = [ + checkEngineInitialized, + validateAccountsController, + validateKeyringController, +]; + +/** + * Runs all validations and logs any errors, but doesn’t throw. + * This makes sure your app keeps running even if some data is unexpected. + */ +export function validatePostMigrationState(state: RootState): void { + const allErrors: string[] = []; + + for (const check of checks) { + const errors = check(state); + if (errors.length > 0) { + allErrors.push(...errors); + } + } + + // If there are any errors, log them + if (allErrors.length > 0) { + Logger.error(new Error('Migration validation errors'), { + message: `State validation found these issues: ${allErrors.join(', ')}`, + }); + } +} diff --git a/app/store/validateMigration/validateMigration.types.ts b/app/store/validateMigration/validateMigration.types.ts new file mode 100644 index 00000000000..fb9da96148f --- /dev/null +++ b/app/store/validateMigration/validateMigration.types.ts @@ -0,0 +1,10 @@ +import { RootState } from '../../reducers'; + +/** + * Each validation check is a function that: + * - Takes the final (post-migration) Redux state, + * - Returns an array of error strings if any validations fail or an empty array otherwise. + */ +export type ValidationCheck = (state: RootState) => string[]; + +export const LOG_TAG = 'MIGRATION_VALIDATE_STATE_ERROR'; From 861de5a3cdb1b503e604ae24cc931c60a2fad720 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 15:31:35 -0500 Subject: [PATCH 11/18] add tests --- .../accountsController.test.ts | 154 ++++++++++++++++++ .../engineBackgroundState.test.ts | 31 ++++ .../engineBackgroundState.ts | 12 ++ .../keyringController.test.ts | 77 +++++++++ .../validateMigration.test.ts | 67 ++++++++ .../validateMigration/validateMigration.ts | 16 +- 6 files changed, 344 insertions(+), 13 deletions(-) create mode 100644 app/store/validateMigration/accountsController.test.ts create mode 100644 app/store/validateMigration/engineBackgroundState.test.ts create mode 100644 app/store/validateMigration/engineBackgroundState.ts create mode 100644 app/store/validateMigration/keyringController.test.ts create mode 100644 app/store/validateMigration/validateMigration.test.ts diff --git a/app/store/validateMigration/accountsController.test.ts b/app/store/validateMigration/accountsController.test.ts new file mode 100644 index 00000000000..21d707b33ae --- /dev/null +++ b/app/store/validateMigration/accountsController.test.ts @@ -0,0 +1,154 @@ +import { validateAccountsController } from './accountsController'; +import { LOG_TAG } from './validateMigration.types'; +import { RootState } from '../../reducers'; +import { AccountsControllerState } from '@metamask/accounts-controller'; +import { EngineState } from '../../core/Engine/types'; +import { Json } from '@metamask/utils'; + +describe('validateAccountsController', () => { + const createMockState = ( + accountsState?: Partial, + ): Partial => ({ + engine: accountsState + ? { + backgroundState: { + AccountsController: accountsState, + } as EngineState, + } + : undefined, + }); + + const mockValidAccountsState: Partial = { + internalAccounts: { + accounts: { + 'account-1': { + id: 'account-1', + address: '0x123', + type: 'eip155:eoa', + options: {} as Record, + methods: [], + metadata: { + name: 'Account 1', + lastSelected: 0, + importTime: Date.now(), + keyring: { + type: 'HD Key Tree', + }, + }, + }, + }, + selectedAccount: 'account-1', + }, + }; + + it('returns no errors for valid state', () => { + const errors = validateAccountsController( + createMockState(mockValidAccountsState) as RootState, + ); + expect(errors).toEqual([]); + }); + + it('returns error if AccountsController state is missing', () => { + const errors = validateAccountsController( + createMockState(undefined) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ]); + }); + + it('returns error if internalAccounts is missing', () => { + const errors = validateAccountsController(createMockState({}) as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController No internalAccounts object found on AccountsControllerState.`, + ]); + }); + + it('returns error if accounts is empty', () => { + const errors = validateAccountsController( + createMockState({ + internalAccounts: { + accounts: {}, + selectedAccount: '', + }, + }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController No accounts found in internalAccounts.accounts.`, + ]); + }); + + it('returns error if selectedAccount is empty', () => { + const errors = validateAccountsController( + createMockState({ + internalAccounts: { + accounts: { + 'account-1': { + id: 'account-1', + address: '0x123', + type: 'eip155:eoa', + options: {} as Record, + methods: [], + metadata: { + name: 'Account 1', + lastSelected: 0, + importTime: Date.now(), + keyring: { + type: 'HD Key Tree', + }, + }, + }, + }, + selectedAccount: '', + }, + }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController selectedAccount is missing or empty.`, + ]); + }); + + it('returns error if selectedAccount does not exist in accounts', () => { + const errors = validateAccountsController( + createMockState({ + internalAccounts: { + accounts: { + 'account-1': { + id: 'account-1', + address: '0x123', + type: 'eip155:eoa', + options: {} as Record, + methods: [], + metadata: { + name: 'Account 1', + lastSelected: 0, + importTime: Date.now(), + keyring: { + type: 'HD Key Tree', + }, + }, + }, + }, + selectedAccount: 'non-existent-account', + }, + }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController The selectedAccount 'non-existent-account' does not exist in the accounts record.`, + ]); + }); + + it('handles undefined engine state', () => { + const errors = validateAccountsController({} as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ]); + }); + + it('handles undefined backgroundState', () => { + const errors = validateAccountsController({ engine: {} } as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, + ]); + }); +}); diff --git a/app/store/validateMigration/engineBackgroundState.test.ts b/app/store/validateMigration/engineBackgroundState.test.ts new file mode 100644 index 00000000000..5999e3bcd59 --- /dev/null +++ b/app/store/validateMigration/engineBackgroundState.test.ts @@ -0,0 +1,31 @@ +import { validateEngineInitialized } from './engineBackgroundState'; +import { LOG_TAG } from './validateMigration.types'; +import { RootState } from '../../reducers'; +import { backgroundState } from '../../util/test/initial-root-state'; + +describe('validateEngineInitialized', () => { + const createMockState = ( + hasBackgroundState: boolean, + ): Partial => ({ + engine: hasBackgroundState ? { backgroundState } : undefined, + }); + + it('returns no errors when backgroundState exists', () => { + const errors = validateEngineInitialized( + createMockState(true) as RootState, + ); + expect(errors).toEqual([]); + }); + + it('returns error if engine state is missing', () => { + const errors = validateEngineInitialized({} as RootState); + expect(errors).toEqual([`${LOG_TAG}: Engine backgroundState not found.`]); + }); + + it('returns error if backgroundState is missing', () => { + const errors = validateEngineInitialized( + createMockState(false) as RootState, + ); + expect(errors).toEqual([`${LOG_TAG}: Engine backgroundState not found.`]); + }); +}); diff --git a/app/store/validateMigration/engineBackgroundState.ts b/app/store/validateMigration/engineBackgroundState.ts new file mode 100644 index 00000000000..17e9d3c4e2e --- /dev/null +++ b/app/store/validateMigration/engineBackgroundState.ts @@ -0,0 +1,12 @@ +import { ValidationCheck, LOG_TAG } from './validateMigration.types'; + +/** + * Verifies that the engine is initialized + */ +export const validateEngineInitialized: ValidationCheck = (state) => { + const errors: string[] = []; + if (!state.engine?.backgroundState) { + errors.push(`${LOG_TAG}: Engine backgroundState not found.`); + } + return errors; +}; diff --git a/app/store/validateMigration/keyringController.test.ts b/app/store/validateMigration/keyringController.test.ts new file mode 100644 index 00000000000..a8c1e812365 --- /dev/null +++ b/app/store/validateMigration/keyringController.test.ts @@ -0,0 +1,77 @@ +import { validateKeyringController } from './keyringController'; +import { LOG_TAG } from './validateMigration.types'; +import { RootState } from '../../reducers'; +import { KeyringControllerState } from '@metamask/keyring-controller'; +import { EngineState } from '../../core/Engine/types'; + +describe('validateKeyringController', () => { + const createMockState = ( + keyringState?: Partial, + ): Partial => ({ + engine: keyringState + ? { + backgroundState: { + KeyringController: keyringState, + } as EngineState, + } + : undefined, + }); + + const mockValidKeyringState: Partial = { + vault: 'mock-vault', + keyrings: [ + { + type: 'HD Key Tree', + accounts: ['0x123'], + }, + ], + }; + + it('returns no errors for valid state', () => { + const errors = validateKeyringController( + createMockState(mockValidKeyringState) as RootState, + ); + expect(errors).toEqual([]); + }); + + it('returns error if KeyringController state is missing', () => { + const errors = validateKeyringController( + createMockState(undefined) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ]); + }); + + it('returns error if vault is missing', () => { + const errors = validateKeyringController( + createMockState({ keyrings: [] }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController No vault in KeyringControllerState.`, + ]); + }); + + it('returns error if keyrings is missing or empty', () => { + const errors = validateKeyringController( + createMockState({ vault: 'mock-vault', keyrings: [] }) as RootState, + ); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController No keyrings found.`, + ]); + }); + + it('handles undefined engine state', () => { + const errors = validateKeyringController({} as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ]); + }); + + it('handles undefined backgroundState', () => { + const errors = validateKeyringController({ engine: {} } as RootState); + expect(errors).toEqual([ + `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, + ]); + }); +}); diff --git a/app/store/validateMigration/validateMigration.test.ts b/app/store/validateMigration/validateMigration.test.ts new file mode 100644 index 00000000000..9708fa28261 --- /dev/null +++ b/app/store/validateMigration/validateMigration.test.ts @@ -0,0 +1,67 @@ +import { validatePostMigrationState } from './validateMigration'; +import { RootState } from '../../reducers'; +import Logger from '../../util/Logger'; +import { validateAccountsController } from './accountsController'; +import { validateKeyringController } from './keyringController'; +import { validateEngineInitialized } from './engineBackgroundState'; + +jest.mock('../../util/Logger', () => ({ + error: jest.fn(), +})); + +jest.mock('./accountsController'); +jest.mock('./keyringController'); +jest.mock('./engineBackgroundState'); + +const TOTAL_VALIDATION_CHECKS = 3; + +describe('validatePostMigrationState', () => { + beforeEach(() => { + jest.clearAllMocks(); + (validateEngineInitialized as jest.Mock).mockReturnValue([]); + (validateAccountsController as jest.Mock).mockReturnValue([]); + (validateKeyringController as jest.Mock).mockReturnValue([]); + }); + + it('runs all validation checks', () => { + const mockState = {} as RootState; + validatePostMigrationState(mockState); + + expect(validateEngineInitialized).toHaveBeenCalledWith(mockState); + expect(validateKeyringController).toHaveBeenCalledWith(mockState); + expect(validateAccountsController).toHaveBeenCalledWith(mockState); + + const totalCalls = + (validateAccountsController as jest.Mock).mock.calls.length + + (validateKeyringController as jest.Mock).mock.calls.length + + (validateEngineInitialized as jest.Mock).mock.calls.length; + + expect(totalCalls).toBe(TOTAL_VALIDATION_CHECKS); + }); + + it('logs errors when validation checks return errors', () => { + const mockState = {} as RootState; + const mockError = 'Mock validation error'; + + // Mock one of the validation checks to return an error + (validateAccountsController as jest.Mock).mockReturnValue([mockError]); + + validatePostMigrationState(mockState); + + // Verify error was logged + expect(Logger.error).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ + message: expect.stringContaining(mockError), + }), + ); + }); + + it('does not log when no validation errors occur', () => { + const mockState = {} as RootState; + validatePostMigrationState(mockState); + + // Verify no errors were logged + expect(Logger.error).not.toHaveBeenCalled(); + }); +}); diff --git a/app/store/validateMigration/validateMigration.ts b/app/store/validateMigration/validateMigration.ts index f74fc6ab504..21d6cae5db2 100644 --- a/app/store/validateMigration/validateMigration.ts +++ b/app/store/validateMigration/validateMigration.ts @@ -1,24 +1,14 @@ import Logger from '../../util/Logger'; import { RootState } from '../../reducers'; -import { LOG_TAG, ValidationCheck } from './validateMigration.types'; +import { ValidationCheck } from './validateMigration.types'; // checks import { validateAccountsController } from './accountsController'; import { validateKeyringController } from './keyringController'; - -/** - * Verifies that the engine is initialized - */ -const checkEngineInitialized: ValidationCheck = (state) => { - const errors: string[] = []; - if (!state.engine?.backgroundState) { - errors.push(`${LOG_TAG}: Engine backgroundState not found.`); - } - return errors; -}; +import { validateEngineInitialized } from './engineBackgroundState'; const checks: ValidationCheck[] = [ - checkEngineInitialized, + validateEngineInitialized, validateAccountsController, validateKeyringController, ]; From fa9a84c757969a7cc23531f70c952a03a2cb5cc2 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 15:52:35 -0500 Subject: [PATCH 12/18] better types and logs in engine service --- app/core/Engine/Engine.ts | 5 ++- app/core/EngineService/EngineService.test.ts | 35 ++++++++++++++++++-- app/core/EngineService/EngineService.ts | 23 ++++++++----- 3 files changed, 52 insertions(+), 11 deletions(-) diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index 64b5a39b199..fc3ea4d2510 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -2157,7 +2157,10 @@ export default { instance = null; }, - init(state: Partial | undefined, keyringState = null) { + init( + state: Partial | undefined, + keyringState: KeyringControllerState | null = null, + ) { instance = Engine.instance || new Engine(state, keyringState); Object.freeze(instance); return instance; diff --git a/app/core/EngineService/EngineService.test.ts b/app/core/EngineService/EngineService.test.ts index 99bc238b2f5..5cfdffd720b 100644 --- a/app/core/EngineService/EngineService.test.ts +++ b/app/core/EngineService/EngineService.test.ts @@ -16,6 +16,7 @@ jest.mock('../NavigationService', () => ({ // Mock Logger jest.mock('../../util/Logger', () => ({ error: jest.fn(), + log: jest.fn(), })); jest.mock('../BackupVault', () => ({ @@ -109,7 +110,7 @@ describe('EngineService', () => { jest.clearAllMocks(); jest.resetAllMocks(); jest.spyOn(ReduxService, 'store', 'get').mockReturnValue({ - getState: () => ({ engine: { backgroundState: {} } }), + getState: () => ({ engine: { backgroundState: { someController: {} } } }), } as unknown as ReduxStore); engineService = new EngineService(); @@ -120,11 +121,41 @@ describe('EngineService', () => { expect(Engine.context).toBeDefined(); }); - it('should have recovered vault on redux store ', async () => { + it('should log Engine initialization with state info', () => { + engineService.start(); + expect(Logger.log).toHaveBeenCalledWith( + 'EngineService: Initializing Engine:', + { + hasState: true, + }, + ); + }); + + it('should log Engine initialization with empty state', () => { + jest.spyOn(ReduxService, 'store', 'get').mockReturnValue({ + getState: () => ({ engine: { backgroundState: {} } }), + } as unknown as ReduxStore); + + engineService.start(); + expect(Logger.log).toHaveBeenCalledWith( + 'EngineService: Initializing Engine:', + { + hasState: false, + }, + ); + }); + + it('should have recovered vault on redux store and log initialization', async () => { engineService.start(); const { success } = await engineService.initializeVaultFromBackup(); expect(success).toBeTruthy(); expect(Engine.context.KeyringController.state.vault).toBeDefined(); + expect(Logger.log).toHaveBeenCalledWith( + 'EngineService: Initializing Engine from backup:', + { + hasState: true, + }, + ); }); it('should navigate to vault recovery if Engine fails to initialize', () => { diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 6ba3e827cb9..0e103d522bc 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -1,4 +1,4 @@ -import UntypedEngine from '../Engine'; +import Engine from '../Engine'; import AppConstants from '../AppConstants'; import { getVaultFromBackup } from '../BackupVault'; import Logger from '../../util/Logger'; @@ -12,6 +12,9 @@ import getUIStartupSpan from '../Performance/UIStartup'; import ReduxService from '../redux'; import NavigationService from '../NavigationService'; import Routes from '../../constants/navigation/Routes'; +import { KeyringControllerState } from '@metamask/keyring-controller'; + +const LOG_TAG = 'EngineService'; interface InitializeEngineResult { success: boolean; @@ -49,10 +52,11 @@ export class EngineService { tags: getTraceTags(reduxState), }); const state = reduxState?.engine?.backgroundState || {}; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const Engine = UntypedEngine as any; try { + Logger.log(`${LOG_TAG}: Initializing Engine:`, { + hasState: Object.keys(state).length > 0, + }); + Engine.init(state); this.updateControllers(Engine); } catch (error) { @@ -235,17 +239,20 @@ export class EngineService { const keyringState = await getVaultFromBackup(); const reduxState = ReduxService.store.getState(); const state = reduxState?.engine?.backgroundState || {}; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const Engine = UntypedEngine as any; // This ensures we create an entirely new engine await Engine.destroyEngine(); this.engineInitialized = false; if (keyringState) { - const newKeyringState = { + const newKeyringState: KeyringControllerState = { keyrings: [], vault: keyringState.vault, + isUnlocked: false, }; + + Logger.log(`${LOG_TAG}: Initializing Engine from backup:`, { + hasState: Object.keys(state).length > 0, + }); + const instance = Engine.init(state, newKeyringState); if (instance) { this.updateControllers(instance); From 108bc550e6843d3d46534cf85d8af1b3ebe2d665 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 16:14:07 -0500 Subject: [PATCH 13/18] better name --- app/core/EngineService/EngineService.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/core/EngineService/EngineService.test.ts b/app/core/EngineService/EngineService.test.ts index 5cfdffd720b..1afd30585b5 100644 --- a/app/core/EngineService/EngineService.test.ts +++ b/app/core/EngineService/EngineService.test.ts @@ -110,7 +110,9 @@ describe('EngineService', () => { jest.clearAllMocks(); jest.resetAllMocks(); jest.spyOn(ReduxService, 'store', 'get').mockReturnValue({ - getState: () => ({ engine: { backgroundState: { someController: {} } } }), + getState: () => ({ + engine: { backgroundState: { KeyringController: {} } }, + }), } as unknown as ReduxStore); engineService = new EngineService(); From c090d3d4ae421d9c7a4f1ee278654f06fdc0d575 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 16:18:42 -0500 Subject: [PATCH 14/18] verify that function does not throw --- .../validateMigration.test.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/app/store/validateMigration/validateMigration.test.ts b/app/store/validateMigration/validateMigration.test.ts index 9708fa28261..32375b18917 100644 --- a/app/store/validateMigration/validateMigration.test.ts +++ b/app/store/validateMigration/validateMigration.test.ts @@ -64,4 +64,24 @@ describe('validatePostMigrationState', () => { // Verify no errors were logged expect(Logger.error).not.toHaveBeenCalled(); }); + + it('does not throw when validation checks fail', () => { + const mockState = {} as RootState; + // Mock all validation checks to return errors + const mockErrors = ['Error 1', 'Error 2', 'Error 3']; + (validateEngineInitialized as jest.Mock).mockReturnValue([mockErrors[0]]); + (validateAccountsController as jest.Mock).mockReturnValue([mockErrors[1]]); + (validateKeyringController as jest.Mock).mockReturnValue([mockErrors[2]]); + + // Verify that calling validatePostMigrationState does not throw + expect(() => validatePostMigrationState(mockState)).not.toThrow(); + + // Verify that errors were logged but execution continued + expect(Logger.error).toHaveBeenCalledWith( + expect.any(Error), + expect.objectContaining({ + message: expect.stringContaining(mockErrors.join(', ')), + }), + ); + }); }); From 6287a64fce11c469a0fc2567fc7def9dcd034471 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 16:28:22 -0500 Subject: [PATCH 15/18] do not throw with malformed state --- .../accountsController.test.ts | 60 +++++++++++++++++++ .../validateMigration/accountsController.ts | 2 +- .../engineBackgroundState.test.ts | 25 ++++++++ .../engineBackgroundState.ts | 2 +- .../keyringController.test.ts | 32 ++++++++++ .../validateMigration/keyringController.ts | 2 +- 6 files changed, 120 insertions(+), 3 deletions(-) diff --git a/app/store/validateMigration/accountsController.test.ts b/app/store/validateMigration/accountsController.test.ts index 21d707b33ae..63a636f8243 100644 --- a/app/store/validateMigration/accountsController.test.ts +++ b/app/store/validateMigration/accountsController.test.ts @@ -151,4 +151,64 @@ describe('validateAccountsController', () => { `${LOG_TAG}: AccountsController state is missing in engine backgroundState.`, ]); }); + + it('does not throw with malformed state', () => { + // Test with various malformed states + const testStates = [ + undefined, + null, + {}, + { engine: undefined }, + { engine: null }, + { engine: { backgroundState: undefined } }, + { engine: { backgroundState: null } }, + { engine: { backgroundState: { AccountsController: undefined } } }, + { engine: { backgroundState: { AccountsController: null } } }, + { + engine: { + backgroundState: { + AccountsController: { internalAccounts: undefined }, + }, + }, + }, + { + engine: { + backgroundState: { AccountsController: { internalAccounts: null } }, + }, + }, + { + engine: { + backgroundState: { + AccountsController: { + internalAccounts: { + accounts: undefined, + selectedAccount: undefined, + }, + }, + }, + }, + }, + { + engine: { + backgroundState: { + AccountsController: { + internalAccounts: { accounts: null, selectedAccount: null }, + }, + }, + }, + }, + ]; + + testStates.forEach((state) => { + // Verify no throw + expect(() => { + validateAccountsController(state as unknown as RootState); + }).not.toThrow(); + + // Verify returns errors array + const errors = validateAccountsController(state as unknown as RootState); + expect(Array.isArray(errors)).toBe(true); + expect(errors.length).toBeGreaterThan(0); + }); + }); }); diff --git a/app/store/validateMigration/accountsController.ts b/app/store/validateMigration/accountsController.ts index 574af6b54a5..20d608fb8ef 100644 --- a/app/store/validateMigration/accountsController.ts +++ b/app/store/validateMigration/accountsController.ts @@ -13,7 +13,7 @@ export const validateAccountsController: ValidationCheck = (rootState) => { const errors: string[] = []; const accountsState: AccountsControllerState | undefined = - rootState.engine?.backgroundState?.AccountsController; + rootState?.engine?.backgroundState?.AccountsController; // If it's missing altogether, return an error if (!accountsState) { diff --git a/app/store/validateMigration/engineBackgroundState.test.ts b/app/store/validateMigration/engineBackgroundState.test.ts index 5999e3bcd59..d729b9cd45b 100644 --- a/app/store/validateMigration/engineBackgroundState.test.ts +++ b/app/store/validateMigration/engineBackgroundState.test.ts @@ -28,4 +28,29 @@ describe('validateEngineInitialized', () => { ); expect(errors).toEqual([`${LOG_TAG}: Engine backgroundState not found.`]); }); + + it('does not throw with malformed state', () => { + // Test with various malformed states + const testStates = [ + undefined, + null, + {}, + { engine: undefined }, + { engine: null }, + { engine: { backgroundState: undefined } }, + { engine: { backgroundState: null } }, + ]; + + testStates.forEach((state) => { + // Verify no throw + expect(() => { + validateEngineInitialized(state as unknown as RootState); + }).not.toThrow(); + + // Verify returns errors array + const errors = validateEngineInitialized(state as unknown as RootState); + expect(Array.isArray(errors)).toBe(true); + expect(errors.length).toBeGreaterThan(0); + }); + }); }); diff --git a/app/store/validateMigration/engineBackgroundState.ts b/app/store/validateMigration/engineBackgroundState.ts index 17e9d3c4e2e..9eda6e4f9b6 100644 --- a/app/store/validateMigration/engineBackgroundState.ts +++ b/app/store/validateMigration/engineBackgroundState.ts @@ -5,7 +5,7 @@ import { ValidationCheck, LOG_TAG } from './validateMigration.types'; */ export const validateEngineInitialized: ValidationCheck = (state) => { const errors: string[] = []; - if (!state.engine?.backgroundState) { + if (!state?.engine?.backgroundState) { errors.push(`${LOG_TAG}: Engine backgroundState not found.`); } return errors; diff --git a/app/store/validateMigration/keyringController.test.ts b/app/store/validateMigration/keyringController.test.ts index a8c1e812365..25881d55802 100644 --- a/app/store/validateMigration/keyringController.test.ts +++ b/app/store/validateMigration/keyringController.test.ts @@ -74,4 +74,36 @@ describe('validateKeyringController', () => { `${LOG_TAG}: KeyringController state is missing in engine backgroundState.`, ]); }); + + it('does not throw with malformed state', () => { + // Test with various malformed states + const testStates = [ + undefined, + null, + {}, + { engine: undefined }, + { engine: { backgroundState: undefined } }, + { engine: { backgroundState: { KeyringController: undefined } } }, + { engine: { backgroundState: { KeyringController: {} } } }, + { + engine: { + backgroundState: { + KeyringController: { vault: undefined, keyrings: undefined }, + }, + }, + }, + ]; + + testStates.forEach((state) => { + // Verify no throw + expect(() => { + validateKeyringController(state as unknown as RootState); + }).not.toThrow(); + + // Verify returns errors array + const errors = validateKeyringController(state as unknown as RootState); + expect(Array.isArray(errors)).toBe(true); + expect(errors.length).toBeGreaterThan(0); + }); + }); }); diff --git a/app/store/validateMigration/keyringController.ts b/app/store/validateMigration/keyringController.ts index 554342ca839..fa932eb09dc 100644 --- a/app/store/validateMigration/keyringController.ts +++ b/app/store/validateMigration/keyringController.ts @@ -12,7 +12,7 @@ export const validateKeyringController: ValidationCheck = (rootState) => { const errors: string[] = []; const keyringControllerState: KeyringControllerState | undefined = - rootState.engine?.backgroundState?.KeyringController; + rootState?.engine?.backgroundState?.KeyringController; // If it's missing altogether, return an error if (!keyringControllerState) { From 05f78fca2cb20641458126381402b4f1da8d00cc Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Mon, 6 Jan 2025 16:33:04 -0500 Subject: [PATCH 16/18] await destroy engine --- app/core/Engine/Engine.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index fc3ea4d2510..55604d04a56 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -2152,8 +2152,8 @@ export default { return instance.resetState(); }, - destroyEngine() { - instance?.destroyEngineInstance(); + destroyEngine: async () => { + await instance?.destroyEngineInstance(); instance = null; }, From a09ede8cbd726dff433e6662d3c02c006ab2a00c Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Tue, 7 Jan 2025 17:51:22 -0500 Subject: [PATCH 17/18] simplify validate migration function --- app/store/validateMigration/validateMigration.ts | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/app/store/validateMigration/validateMigration.ts b/app/store/validateMigration/validateMigration.ts index 21d6cae5db2..0638d65b680 100644 --- a/app/store/validateMigration/validateMigration.ts +++ b/app/store/validateMigration/validateMigration.ts @@ -18,14 +18,7 @@ const checks: ValidationCheck[] = [ * This makes sure your app keeps running even if some data is unexpected. */ export function validatePostMigrationState(state: RootState): void { - const allErrors: string[] = []; - - for (const check of checks) { - const errors = check(state); - if (errors.length > 0) { - allErrors.push(...errors); - } - } + const allErrors = checks.flatMap((check) => check(state)); // If there are any errors, log them if (allErrors.length > 0) { From 28b903dac9b07df7642fb3e456a11a111f4da7c5 Mon Sep 17 00:00:00 2001 From: Owen Craston Date: Wed, 8 Jan 2025 12:39:40 -0500 Subject: [PATCH 18/18] new test build --- android/app/build.gradle | 4 ++-- bitrise.yml | 26 +++++++++++++------------- ios/MetaMask.xcodeproj/project.pbxproj | 24 ++++++++++++------------ 3 files changed, 27 insertions(+), 27 deletions(-) diff --git a/android/app/build.gradle b/android/app/build.gradle index 5692a52de02..a3762f16533 100644 --- a/android/app/build.gradle +++ b/android/app/build.gradle @@ -178,8 +178,8 @@ android { applicationId "io.metamask" minSdkVersion rootProject.ext.minSdkVersion targetSdkVersion rootProject.ext.targetSdkVersion - versionName "7.37.1" - versionCode 1520 + versionName "7.38.1" + versionCode 1524 testBuildType System.getProperty('testBuildType', 'debug') missingDimensionStrategy 'react-native-camera', 'general' testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner" diff --git a/bitrise.yml b/bitrise.yml index e46fa35dea7..2be40bb4604 100644 --- a/bitrise.yml +++ b/bitrise.yml @@ -114,7 +114,7 @@ stages: workflows: - deploy_android_to_store: {} create_build_qa_and_expo: - workflows: + workflows: - build_android_devbuild: {} - build_android_qa: {} - build_ios_devbuild: {} @@ -1247,20 +1247,20 @@ workflows: - content: |- # Define the source path of the generated APK SOURCE_APK_PATH="$PROJECT_LOCATION/app/build/outputs/apk/prod/debug/app-prod-debug.apk" - + # Define the destination path with the new name DEST_APK_PATH="$BITRISE_DEPLOY_DIR/android-expo-dev-build.apk" - + # Copy and rename the APK cp "$SOURCE_APK_PATH" "$DEST_APK_PATH" - + # Optionally, print the new path for verification echo "APK has been copied and renamed to: $DEST_APK_PATH" - deploy-to-bitrise-io@2.2.3: is_always_run: false is_skippable: true inputs: - - deploy_path: "$BITRISE_DEPLOY_DIR/android-expo-dev-build.apk" + - deploy_path: '$BITRISE_DEPLOY_DIR/android-expo-dev-build.apk' title: Bitrise Deploy APK meta: bitrise.io: @@ -1537,13 +1537,13 @@ workflows: - content: |- # Define the source path of the generated IPA SOURCE_IPA_PATH="ios/build/output/MetaMask.ipa" - + # Define the destination path with the new name DEST_IPA_PATH="$BITRISE_DEPLOY_DIR/ios-expo-dev-build.ipa" - + # Copy and rename the IPA cp "$SOURCE_IPA_PATH" "$DEST_IPA_PATH" - + # Optionally, print the new path for verification echo "IPA has been copied and renamed to: $DEST_IPA_PATH" - deploy-to-bitrise-io@2.2.3: @@ -1551,7 +1551,7 @@ workflows: is_skippable: true inputs: - pipeline_intermediate_files: ios/build/output/MetaMask.ipa:BITRISE_APP_STORE_IPA_PATH - - deploy_path: "$BITRISE_DEPLOY_DIR/ios-expo-dev-build.ipa" + - deploy_path: '$BITRISE_DEPLOY_DIR/ios-expo-dev-build.ipa' title: Deploy iOS IPA build_ios_qa: envs: @@ -1793,16 +1793,16 @@ app: PROJECT_LOCATION_IOS: ios - opts: is_expand: false - VERSION_NAME: 7.37.1 + VERSION_NAME: 7.38.1 - opts: is_expand: false - VERSION_NUMBER: 1520 + VERSION_NUMBER: 1524 - opts: is_expand: false - FLASK_VERSION_NAME: 7.37.1 + FLASK_VERSION_NAME: 7.38.1 - opts: is_expand: false - FLASK_VERSION_NUMBER: 1520 + FLASK_VERSION_NUMBER: 1524 - opts: is_expand: false ANDROID_APK_LINK: '' diff --git a/ios/MetaMask.xcodeproj/project.pbxproj b/ios/MetaMask.xcodeproj/project.pbxproj index fde045b95df..b0475b8b659 100644 --- a/ios/MetaMask.xcodeproj/project.pbxproj +++ b/ios/MetaMask.xcodeproj/project.pbxproj @@ -1380,7 +1380,7 @@ CODE_SIGN_IDENTITY = "Apple Development"; "CODE_SIGN_IDENTITY[sdk=iphoneos*]" = "iPhone Developer"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1520; + CURRENT_PROJECT_VERSION = 1524; DEAD_CODE_STRIPPING = YES; DEBUG_INFORMATION_FORMAT = dwarf; DEVELOPMENT_TEAM = 48XVW22RCG; @@ -1418,7 +1418,7 @@ "${inherited}", ); LLVM_LTO = YES; - MARKETING_VERSION = 7.37.1; + MARKETING_VERSION = 7.38.1; ONLY_ACTIVE_ARCH = YES; OTHER_CFLAGS = "$(inherited)"; OTHER_LDFLAGS = ( @@ -1449,7 +1449,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMask.entitlements; CODE_SIGN_IDENTITY = "iPhone Distribution"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1520; + CURRENT_PROJECT_VERSION = 1524; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = 48XVW22RCG; "DEVELOPMENT_TEAM[sdk=iphoneos*]" = 48XVW22RCG; @@ -1484,7 +1484,7 @@ "${inherited}", ); LLVM_LTO = YES; - MARKETING_VERSION = 7.37.1; + MARKETING_VERSION = 7.38.1; ONLY_ACTIVE_ARCH = NO; OTHER_CFLAGS = "$(inherited)"; OTHER_LDFLAGS = ( @@ -1514,7 +1514,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMaskDebug.entitlements; CODE_SIGN_IDENTITY = "Apple Development"; CODE_SIGN_STYLE = Automatic; - CURRENT_PROJECT_VERSION = 1520; + CURRENT_PROJECT_VERSION = 1524; DEAD_CODE_STRIPPING = YES; DEBUG_INFORMATION_FORMAT = dwarf; DEVELOPMENT_TEAM = 48XVW22RCG; @@ -1547,7 +1547,7 @@ ); LIBRARY_SEARCH_PATHS = "$(SDKROOT)/usr/lib/swift$(inherited)"; LLVM_LTO = YES; - MARKETING_VERSION = 7.37.1; + MARKETING_VERSION = 7.38.1; ONLY_ACTIVE_ARCH = YES; OTHER_CFLAGS = "$(inherited)"; OTHER_LDFLAGS = ( @@ -1576,7 +1576,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMask.entitlements; CODE_SIGN_IDENTITY = "iPhone Distribution"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1520; + CURRENT_PROJECT_VERSION = 1524; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = 48XVW22RCG; "DEVELOPMENT_TEAM[sdk=iphoneos*]" = 48XVW22RCG; @@ -1607,7 +1607,7 @@ ); LIBRARY_SEARCH_PATHS = "$(SDKROOT)/usr/lib/swift$(inherited)"; LLVM_LTO = YES; - MARKETING_VERSION = 7.37.1; + MARKETING_VERSION = 7.38.1; ONLY_ACTIVE_ARCH = NO; OTHER_CFLAGS = "$(inherited)"; OTHER_LDFLAGS = ( @@ -1731,7 +1731,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMaskDebug.entitlements; CODE_SIGN_IDENTITY = "Apple Development"; CODE_SIGN_STYLE = Automatic; - CURRENT_PROJECT_VERSION = 1520; + CURRENT_PROJECT_VERSION = 1524; DEAD_CODE_STRIPPING = YES; DEBUG_INFORMATION_FORMAT = dwarf; DEVELOPMENT_TEAM = 48XVW22RCG; @@ -1768,7 +1768,7 @@ "\"$(SRCROOT)/MetaMask/System/Library/Frameworks\"", ); LLVM_LTO = YES; - MARKETING_VERSION = 7.37.1; + MARKETING_VERSION = 7.38.1; ONLY_ACTIVE_ARCH = YES; OTHER_CFLAGS = ( "$(inherited)", @@ -1800,7 +1800,7 @@ CODE_SIGN_ENTITLEMENTS = MetaMask/MetaMask.entitlements; CODE_SIGN_IDENTITY = "iPhone Distribution"; CODE_SIGN_STYLE = Manual; - CURRENT_PROJECT_VERSION = 1520; + CURRENT_PROJECT_VERSION = 1524; DEBUG_INFORMATION_FORMAT = "dwarf-with-dsym"; DEVELOPMENT_TEAM = 48XVW22RCG; "DEVELOPMENT_TEAM[sdk=iphoneos*]" = 48XVW22RCG; @@ -1835,7 +1835,7 @@ "\"$(SRCROOT)/MetaMask/System/Library/Frameworks\"", ); LLVM_LTO = YES; - MARKETING_VERSION = 7.37.1; + MARKETING_VERSION = 7.38.1; ONLY_ACTIVE_ARCH = NO; OTHER_CFLAGS = ( "$(inherited)",