From 62cae8a1bb8257a08ec65ad4de08ad2301a808a4 Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 3 Dec 2024 04:06:51 -0500 Subject: [PATCH 1/5] Optimize `EngineService` `stateChange` events subscription loop --- app/core/EngineService/EngineService.ts | 140 +++--------------------- 1 file changed, 15 insertions(+), 125 deletions(-) diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index bedb3e97534..261d14bf31e 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -1,5 +1,4 @@ import UntypedEngine from '../Engine'; -import AppConstants from '../AppConstants'; import { getVaultFromBackup } from '../BackupVault'; import { store as importedStore } from '../../store'; import Logger from '../../util/Logger'; @@ -10,6 +9,7 @@ import { import { getTraceTags } from '../../util/sentry/tags'; import { trace, endTrace, TraceName, TraceOperation } from '../../util/trace'; import getUIStartupSpan from '../Performance/UIStartup'; +import { BACKGROUND_STATE_CHANGE_EVENT_NAMES } from '../Engine/constants'; interface InitializeEngineResult { success: boolean; @@ -58,121 +58,6 @@ class EngineService { return; } - const controllers = [ - { - name: 'AddressBookController', - key: `${engine.context.AddressBookController.name}:stateChange`, - }, - { name: 'NftController', key: 'NftController:stateChange' }, - { - name: 'TokensController', - key: `${engine.context.TokensController.name}:stateChange`, - }, - { - name: 'KeyringController', - key: `${engine.context.KeyringController.name}:stateChange`, - }, - { - name: 'AccountTrackerController', - key: 'AccountTrackerController:stateChange', - }, - { - name: 'NetworkController', - key: AppConstants.NETWORK_STATE_CHANGE_EVENT, - }, - { - name: 'PhishingController', - key: `${engine.context.PhishingController.name}:stateChange`, - }, - { - name: 'PreferencesController', - key: `${engine.context.PreferencesController.name}:stateChange`, - }, - { - name: 'RemoteFeatureFlagController', - key: `${engine.context.RemoteFeatureFlagController.name}:stateChange`, - }, - { - name: 'SelectedNetworkController', - key: `${engine.context.SelectedNetworkController.name}:stateChange`, - }, - { - name: 'TokenBalancesController', - key: `${engine.context.TokenBalancesController.name}:stateChange`, - }, - { name: 'TokenRatesController', key: 'TokenRatesController:stateChange' }, - { - name: 'TransactionController', - key: `${engine.context.TransactionController.name}:stateChange`, - }, - { - name: 'SmartTransactionsController', - key: `${engine.context.SmartTransactionsController.name}:stateChange`, - }, - { - name: 'SwapsController', - key: `${engine.context.SwapsController.name}:stateChange`, - }, - { - name: 'TokenListController', - key: `${engine.context.TokenListController.name}:stateChange`, - }, - { - name: 'CurrencyRateController', - key: `${engine.context.CurrencyRateController.name}:stateChange`, - }, - { - name: 'GasFeeController', - key: `${engine.context.GasFeeController.name}:stateChange`, - }, - { - name: 'ApprovalController', - key: `${engine.context.ApprovalController.name}:stateChange`, - }, - ///: BEGIN:ONLY_INCLUDE_IF(preinstalled-snaps,external-snaps) - { - name: 'SnapController', - key: `${engine.context.SnapController.name}:stateChange`, - }, - { - name: 'SubjectMetadataController', - key: `${engine.context.SubjectMetadataController.name}:stateChange`, - }, - { - name: 'AuthenticationController', - key: 'AuthenticationController:stateChange', - }, - { - name: 'UserStorageController', - key: 'UserStorageController:stateChange', - }, - { - name: 'NotificationServicesController', - key: 'NotificationServicesController:stateChange', - }, - { - name: 'NotificationServicesPushController', - key: 'NotificationServicesPushController:stateChange', - }, - ///: END:ONLY_INCLUDE_IF - { - name: 'PermissionController', - key: `${engine.context.PermissionController.name}:stateChange`, - }, - { - name: 'LoggingController', - key: `${engine.context.LoggingController.name}:stateChange`, - }, - { - name: 'AccountsController', - key: `${engine.context.AccountsController.name}:stateChange`, - }, - { - name: 'PPOMController', - key: `${engine.context.PPOMController.name}:stateChange`, - }, - ]; - engine.controllerMessenger.subscribeOnceIf( 'ComposableController:stateChange', () => { @@ -185,15 +70,20 @@ class EngineService { () => !this.engineInitialized, ); - controllers.forEach((controller) => { - const { name, key } = controller; - const update_bg_state_cb = () => { - if (!engine.context.KeyringController.metadata.vault) { - Logger.log('keyringController vault missing for UPDATE_BG_STATE_KEY'); - } - store.dispatch({ type: UPDATE_BG_STATE_KEY, payload: { key: name } }); - }; - engine.controllerMessenger.subscribe(key, update_bg_state_cb); + const update_bg_state_cb = (controllerName: string) => { + if (!engine.context.KeyringController.metadata.vault) { + Logger.log('keyringController vault missing for UPDATE_BG_STATE_KEY'); + } + store.dispatch({ + type: UPDATE_BG_STATE_KEY, + payload: { key: controllerName }, + }); + }; + + BACKGROUND_STATE_CHANGE_EVENT_NAMES.forEach((eventName) => { + engine.controllerMessenger.subscribe(eventName, () => + update_bg_state_cb(eventName.split(':')[0]), + ); }); }; From cd6e3d7fbeea77821e71e05dca712d5bb0b781cd Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 3 Dec 2024 05:54:33 -0500 Subject: [PATCH 2/5] Define `ReduxStore`, `ReduxState` types --- app/store/index.ts | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/app/store/index.ts b/app/store/index.ts index 5f74eb552d9..5fefca2654c 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -1,4 +1,4 @@ -import { Store } from 'redux'; +import { AnyAction } from 'redux'; import { configureStore } from '@reduxjs/toolkit'; import { persistStore, persistReducer } from 'redux-persist'; import createSagaMiddleware from 'redux-saga'; @@ -16,17 +16,18 @@ import thunk from 'redux-thunk'; import persistConfig from './persistConfig'; import { AppStateEventProcessor } from '../core/AppStateEventListener'; import getUIStartupSpan from '../core/Performance/UIStartup'; +import { ToolkitStore } from '@reduxjs/toolkit/dist/configureStore'; // TODO: Improve type safety by using real Action types instead of `any` -// TODO: Replace "any" with type -// eslint-disable-next-line @typescript-eslint/no-explicit-any -const pReducer = persistReducer(persistConfig, rootReducer); +const pReducer = persistReducer( + persistConfig, + rootReducer, +); // TODO: Fix the Action type. It's set to `any` now because some of the // TypeScript reducers have invalid actions -// TODO: Replace "any" with type -// eslint-disable-next-line @typescript-eslint/no-explicit-any, import/no-mutable-exports -let store: Store, persistor; +// eslint-disable-next-line import/no-mutable-exports +let store: ReduxStore, persistor; const createStoreAndPersistor = async () => { trace({ name: TraceName.StoreInit, @@ -104,3 +105,11 @@ const createStoreAndPersistor = async () => { })(); export { store, persistor }; + +export type ReduxState = ReturnType; + +export type ReduxStore = ToolkitStore< + ReduxState, + AnyAction, + [ReturnType>, typeof thunk] +>; From 514ccbd4ee311809d5b1e3dbf31ad7d1f8c338dd Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 3 Dec 2024 05:54:38 -0500 Subject: [PATCH 3/5] Fix `any` types in `EngineService` --- app/core/Engine/Engine.test.ts | 5 ++--- app/core/Engine/Engine.ts | 7 +++++-- app/core/EngineService/EngineService.ts | 28 ++++++++++++------------- 3 files changed, 20 insertions(+), 20 deletions(-) diff --git a/app/core/Engine/Engine.test.ts b/app/core/Engine/Engine.test.ts index 5af5d8965c8..cdac27192ee 100644 --- a/app/core/Engine/Engine.test.ts +++ b/app/core/Engine/Engine.test.ts @@ -6,12 +6,11 @@ import { zeroAddress } from 'ethereumjs-util'; import { createMockAccountsControllerState } from '../../util/test/accountsControllerTestUtils'; import { mockNetworkState } from '../../util/test/network'; import MetaMetrics from '../Analytics/MetaMetrics'; -import { store } from '../../store'; +import { ReduxState, store } from '../../store'; import { MetaMetricsEvents } from '../Analytics'; import { NetworkState } from '@metamask/network-controller'; import { Hex } from '@metamask/utils'; import { TransactionMeta } from '@metamask/transaction-controller'; -import { RootState } from '../../reducers'; import { MetricsEventBuilder } from '../Analytics/MetricsEventBuilder'; jest.unmock('./Engine'); @@ -369,7 +368,7 @@ describe('Transaction event handlers', () => { beforeEach(() => { engine = Engine.init({}); jest.spyOn(MetaMetrics.getInstance(), 'trackEvent').mockImplementation(); - jest.spyOn(store, 'getState').mockReturnValue({} as RootState); + jest.spyOn(store, 'getState').mockReturnValue({} as ReduxState); }); afterEach(() => { diff --git a/app/core/Engine/Engine.ts b/app/core/Engine/Engine.ts index 294baf2b96f..f28317afd74 100644 --- a/app/core/Engine/Engine.ts +++ b/app/core/Engine/Engine.ts @@ -274,7 +274,7 @@ export class Engine { // eslint-disable-next-line @typescript-eslint/default-param-last constructor( initialState: Partial = {}, - initialKeyringState?: KeyringControllerState | null, + initialKeyringState?: Partial | null, ) { this.controllerMessenger = new ExtendedControllerMessenger(); @@ -2151,7 +2151,10 @@ export default { instance = null; }, - init(state: Partial | undefined, keyringState = null) { + init( + state: Partial | undefined, + keyringState: Partial | null = null, + ) { instance = Engine.instance || new Engine(state, keyringState); Object.freeze(instance); return instance; diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 261d14bf31e..20353d1a5aa 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -1,6 +1,7 @@ import UntypedEngine from '../Engine'; +import { Engine as TypedEngine } from '../Engine/Engine'; import { getVaultFromBackup } from '../BackupVault'; -import { store as importedStore } from '../../store'; +import { store as importedStore, ReduxStore } from '../../store'; import Logger from '../../util/Logger'; import { NO_VAULT_IN_BACKUP_ERROR, @@ -27,9 +28,7 @@ class EngineService { * @param store - Redux store */ - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - initalizeEngine = (store: any) => { + initalizeEngine = (store: ReduxStore) => { trace({ name: TraceName.EngineInitialization, op: TraceOperation.EngineInitialization, @@ -38,17 +37,14 @@ class EngineService { }); const reduxState = 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; + const Engine = UntypedEngine; Engine.init(state); - this.updateControllers(store, Engine); + // `Engine.init()` call mutates `typeof UntypedEngine` to `TypedEngine` + this.updateControllers(store, Engine as unknown as TypedEngine); endTrace({ name: TraceName.EngineInitialization }); }; - // TODO: Replace "any" with type - // eslint-disable-next-line @typescript-eslint/no-explicit-any - private updateControllers = (store: any, engine: any) => { + private updateControllers = (store: ReduxStore, engine: TypedEngine) => { if (!engine.context) { Logger.error( new Error( @@ -100,9 +96,7 @@ class EngineService { const keyringState = await getVaultFromBackup(); const reduxState = importedStore.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; + const Engine = UntypedEngine; // This ensures we create an entirely new engine await Engine.destroyEngine(); this.engineInitialized = false; @@ -111,7 +105,11 @@ class EngineService { keyrings: [], vault: keyringState.vault, }; - const instance = Engine.init(state, newKeyringState); + // `Engine.init()` call mutates `typeof UntypedEngine` to `Engine` + const instance = Engine.init( + state, + newKeyringState, + ) as unknown as TypedEngine; if (instance) { this.updateControllers(importedStore, instance); // this is a hack to give the engine time to reinitialize From 1f5952e843dbc6edf4422daabfb92985122833ff Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 3 Dec 2024 10:22:20 -0500 Subject: [PATCH 4/5] Prefer nullish coalescing operator for type safety --- app/core/EngineService/EngineService.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/core/EngineService/EngineService.ts b/app/core/EngineService/EngineService.ts index 20353d1a5aa..b214f3ec1d2 100644 --- a/app/core/EngineService/EngineService.ts +++ b/app/core/EngineService/EngineService.ts @@ -36,7 +36,7 @@ class EngineService { tags: getTraceTags(store.getState()), }); const reduxState = store.getState?.(); - const state = reduxState?.engine?.backgroundState || {}; + const state = reduxState?.engine?.backgroundState ?? {}; const Engine = UntypedEngine; Engine.init(state); // `Engine.init()` call mutates `typeof UntypedEngine` to `TypedEngine` @@ -95,7 +95,7 @@ class EngineService { async initializeVaultFromBackup(): Promise { const keyringState = await getVaultFromBackup(); const reduxState = importedStore.getState?.(); - const state = reduxState?.engine?.backgroundState || {}; + const state = reduxState?.engine?.backgroundState ?? {}; const Engine = UntypedEngine; // This ensures we create an entirely new engine await Engine.destroyEngine(); From c2e4ecbcc2ba2cd0accdb59db5d8d476cfc215ac Mon Sep 17 00:00:00 2001 From: Jongsun Suh Date: Tue, 10 Dec 2024 03:43:38 -0500 Subject: [PATCH 5/5] Remove `Redux{State,Store}` types defined in `app/store/index.ts` and replace with new types in `app/core/redux` --- app/core/Engine/Engine.test.ts | 5 +++-- app/store/index.ts | 15 ++------------- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/app/core/Engine/Engine.test.ts b/app/core/Engine/Engine.test.ts index cdac27192ee..5af5d8965c8 100644 --- a/app/core/Engine/Engine.test.ts +++ b/app/core/Engine/Engine.test.ts @@ -6,11 +6,12 @@ import { zeroAddress } from 'ethereumjs-util'; import { createMockAccountsControllerState } from '../../util/test/accountsControllerTestUtils'; import { mockNetworkState } from '../../util/test/network'; import MetaMetrics from '../Analytics/MetaMetrics'; -import { ReduxState, store } from '../../store'; +import { store } from '../../store'; import { MetaMetricsEvents } from '../Analytics'; import { NetworkState } from '@metamask/network-controller'; import { Hex } from '@metamask/utils'; import { TransactionMeta } from '@metamask/transaction-controller'; +import { RootState } from '../../reducers'; import { MetricsEventBuilder } from '../Analytics/MetricsEventBuilder'; jest.unmock('./Engine'); @@ -368,7 +369,7 @@ describe('Transaction event handlers', () => { beforeEach(() => { engine = Engine.init({}); jest.spyOn(MetaMetrics.getInstance(), 'trackEvent').mockImplementation(); - jest.spyOn(store, 'getState').mockReturnValue({} as ReduxState); + jest.spyOn(store, 'getState').mockReturnValue({} as RootState); }); afterEach(() => { diff --git a/app/store/index.ts b/app/store/index.ts index 0a339a90c4f..03e433ba75d 100644 --- a/app/store/index.ts +++ b/app/store/index.ts @@ -12,18 +12,15 @@ import thunk from 'redux-thunk'; import persistConfig from './persistConfig'; import getUIStartupSpan from '../core/Performance/UIStartup'; -import { ToolkitStore } from '@reduxjs/toolkit/dist/configureStore'; -import ReduxService from '../core/redux'; +import ReduxService, { ReduxStore } from '../core/redux'; import { onPersistedDataLoaded } from '../actions/user'; -// TODO: Improve type safety by using real Action types instead of `any` +// TODO: Improve type safety by using real Action types instead of `AnyAction` const pReducer = persistReducer( persistConfig, rootReducer, ); -// TODO: Fix the Action type. It's set to `any` now because some of the -// TypeScript reducers have invalid actions // eslint-disable-next-line import/no-mutable-exports let store: ReduxStore, persistor; const createStoreAndPersistor = async () => { @@ -80,11 +77,3 @@ const createStoreAndPersistor = async () => { })(); export { store, persistor }; - -export type ReduxState = ReturnType; - -export type ReduxStore = ToolkitStore< - ReduxState, - AnyAction, - [ReturnType>, typeof thunk] ->;