From 1f1c26e76d5f0f8df982535b9da1fff88e4330cc Mon Sep 17 00:00:00 2001 From: Michele Esposito Date: Wed, 28 Jan 2026 17:54:59 +0100 Subject: [PATCH 1/2] refactor: remove `KeyringController:getState` dep, `init()` and `destroy()` --- .../src/ConfigRegistryController.test.ts | 278 ++---------------- .../src/ConfigRegistryController.ts | 93 +----- 2 files changed, 38 insertions(+), 333 deletions(-) diff --git a/packages/config-registry-controller/src/ConfigRegistryController.test.ts b/packages/config-registry-controller/src/ConfigRegistryController.test.ts index c94312d15d5..687735c3698 100644 --- a/packages/config-registry-controller/src/ConfigRegistryController.test.ts +++ b/packages/config-registry-controller/src/ConfigRegistryController.test.ts @@ -1,4 +1,8 @@ -import type { MockAnyNamespace } from '@metamask/messenger'; +import type { + MockAnyNamespace, + MessengerActions, + MessengerEvents, +} from '@metamask/messenger'; import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger'; import { useFakeTimers } from 'sinon'; @@ -19,20 +23,11 @@ import { advanceTime } from '../../../tests/helpers'; const namespace = 'ConfigRegistryController' as const; -type RootMessenger = Messenger< - MockAnyNamespace, - | { type: 'RemoteFeatureFlagController:getState'; handler: () => unknown } - | { - type: 'KeyringController:getState'; - handler: () => { isUnlocked: boolean }; - } - | { - type: 'ConfigRegistryApiService:fetchConfig'; - handler: (options?: { etag?: string }) => Promise; - }, - | { type: 'KeyringController:unlock'; payload: [] } - | { type: 'KeyringController:lock'; payload: [] } ->; +type AllActions = MessengerActions; + +type AllEvents = MessengerEvents; + +type RootMessenger = Messenger; /** * Constructs a messenger for ConfigRegistryController. @@ -43,42 +38,28 @@ function getConfigRegistryControllerMessenger(): { messenger: ConfigRegistryMessenger; rootMessenger: RootMessenger; } { - const rootMessenger = new Messenger< - MockAnyNamespace, - | { type: 'RemoteFeatureFlagController:getState'; handler: () => unknown } - | { - type: 'KeyringController:getState'; - handler: () => { isUnlocked: boolean }; - } - | { - type: 'ConfigRegistryApiService:fetchConfig'; - handler: (options?: { etag?: string }) => Promise; - }, - | { type: 'KeyringController:unlock'; payload: [] } - | { type: 'KeyringController:lock'; payload: [] } - >({ + const rootMessenger: RootMessenger = new Messenger({ namespace: MOCK_ANY_NAMESPACE, }); - const configRegistryControllerMessenger = new Messenger< - typeof namespace, - never, - | { type: 'KeyringController:unlock'; payload: [] } - | { type: 'KeyringController:lock'; payload: [] }, - typeof rootMessenger - >({ - namespace, - parent: rootMessenger, - }) as ConfigRegistryMessenger; + const configRegistryControllerMessenger: ConfigRegistryMessenger = + new Messenger< + typeof namespace, + AllActions, + AllEvents, + typeof rootMessenger + >({ + namespace, + parent: rootMessenger, + }); rootMessenger.delegate({ messenger: configRegistryControllerMessenger, actions: [ 'RemoteFeatureFlagController:getState', - 'KeyringController:getState', 'ConfigRegistryApiService:fetchConfig', - ] as never[], - events: ['KeyringController:unlock', 'KeyringController:lock'] as never[], + ], + events: ['KeyringController:unlock', 'KeyringController:lock'], }); return { messenger: configRegistryControllerMessenger, rootMessenger }; @@ -168,7 +149,6 @@ type WithControllerCallback = (args: { messenger: ConfigRegistryMessenger; mockApiServiceHandler: jest.Mock; mockRemoteFeatureFlagGetState: jest.Mock; - mockKeyringControllerGetState: jest.Mock; }) => Promise | ReturnValue; type WithControllerOptions = { @@ -204,23 +184,11 @@ async function withController( mockRemoteFeatureFlagGetState, ); - const mockKeyringControllerGetState = jest.fn().mockReturnValue({ - isUnlocked: false, - }); - - rootMessenger.registerActionHandler( - 'KeyringController:getState', - mockKeyringControllerGetState, - ); - const controller = new ConfigRegistryController({ messenger, ...options, }); - // Initialize the controller after creation - controller.init(); - try { return await testFunction({ controller, @@ -229,11 +197,9 @@ async function withController( messenger, mockApiServiceHandler, mockRemoteFeatureFlagGetState, - mockKeyringControllerGetState, }); } finally { controller.stopPolling(); - controller.destroy(); clock.restore(); mockApiServiceHandler.mockReset(); } @@ -522,10 +488,6 @@ describe('ConfigRegistryController', () => { 'RemoteFeatureFlagController:getState', mockRemoteFeatureFlagGetState, ); - testRootMessenger.registerActionHandler( - 'KeyringController:getState', - jest.fn().mockReturnValue({ isUnlocked: false }), - ); testRootMessenger.delegate({ messenger: testMessenger, actions: [ @@ -2296,103 +2258,6 @@ describe('ConfigRegistryController', () => { }); describe('KeyringController event listeners', () => { - it('starts polling when KeyringController is already unlocked on initialization', async () => { - await withController(async ({ clock }) => { - const mockKeyringControllerGetState = jest.fn().mockReturnValue({ - isUnlocked: true, - }); - - const testRootMessenger = new Messenger< - MockAnyNamespace, - | { - type: 'RemoteFeatureFlagController:getState'; - handler: () => unknown; - } - | { - type: 'KeyringController:getState'; - handler: () => { isUnlocked: boolean }; - } - | { - type: 'ConfigRegistryApiService:fetchConfig'; - handler: (options?: { - etag?: string; - }) => Promise; - }, - | { type: 'KeyringController:unlock'; payload: [] } - | { type: 'KeyringController:lock'; payload: [] } - >({ - namespace: MOCK_ANY_NAMESPACE, - }); - - const testMessenger = new Messenger< - typeof namespace, - never, - | { type: 'KeyringController:unlock'; payload: [] } - | { type: 'KeyringController:lock'; payload: [] }, - typeof testRootMessenger - >({ - namespace, - parent: testRootMessenger, - }) as ConfigRegistryMessenger; - - testRootMessenger.registerActionHandler( - 'ConfigRegistryApiService:fetchConfig', - jest.fn(buildMockApiServiceHandler()), - ); - testRootMessenger.registerActionHandler( - 'RemoteFeatureFlagController:getState', - jest.fn().mockReturnValue({ - remoteFeatureFlags: { - configRegistryApiEnabled: true, - }, - cacheTimestamp: Date.now(), - }), - ); - testRootMessenger.registerActionHandler( - 'KeyringController:getState', - mockKeyringControllerGetState, - ); - testRootMessenger.delegate({ - messenger: testMessenger, - actions: [ - 'RemoteFeatureFlagController:getState', - 'KeyringController:getState', - 'ConfigRegistryApiService:fetchConfig', - ] as never[], - events: [ - 'KeyringController:unlock', - 'KeyringController:lock', - ] as never[], - }); - - const controller = new ConfigRegistryController({ - messenger: testMessenger, - }); - - // Initialize the controller after creation - controller.init(); - - const executePollSpy = jest.spyOn(controller, '_executePoll'); - - await advanceTime({ clock, duration: 100 }); - - expect(mockKeyringControllerGetState).toHaveBeenCalled(); - expect(executePollSpy).toHaveBeenCalledTimes(1); - - controller.stopPolling(); - }); - }); - - it('handles KeyringController:getState error gracefully when KeyringController is unavailable', async () => { - await withController(({ controller, mockKeyringControllerGetState }) => { - mockKeyringControllerGetState.mockImplementation(() => { - throw new Error('KeyringController not available'); - }); - - expect(controller.state.lastFetched).toBeNull(); - }); - }); - it('starts polling when KeyringController:unlock event is published', async () => { await withController(async ({ controller, clock, rootMessenger }) => { const executePollSpy = jest.spyOn(controller, '_executePoll'); @@ -2412,27 +2277,16 @@ describe('ConfigRegistryController', () => { }); it('stops polling when KeyringController:lock event is published', async () => { - await withController( - async ({ - controller, - clock, - rootMessenger, - mockKeyringControllerGetState, - }) => { - mockKeyringControllerGetState.mockReturnValue({ - isUnlocked: true, - }); - - const stopPollingSpy = jest.spyOn(controller, 'stopPolling'); + await withController(async ({ controller, clock, rootMessenger }) => { + const stopPollingSpy = jest.spyOn(controller, 'stopPolling'); - await advanceTime({ clock, duration: 0 }); - expect(stopPollingSpy).not.toHaveBeenCalled(); + await advanceTime({ clock, duration: 0 }); + expect(stopPollingSpy).not.toHaveBeenCalled(); - rootMessenger.publish('KeyringController:lock'); + rootMessenger.publish('KeyringController:lock'); - expect(stopPollingSpy).toHaveBeenCalled(); - }, - ); + expect(stopPollingSpy).toHaveBeenCalled(); + }); }); it('calls startPolling with default parameter when called without arguments', async () => { @@ -2638,76 +2492,4 @@ describe('ConfigRegistryController', () => { }); }); }); - - describe('destroy', () => { - it('cleans up event listeners and action handlers', async () => { - await withController(({ controller, messenger }) => { - const unsubscribeSpy = jest.spyOn(messenger, 'unsubscribe'); - const unregisterActionHandlerSpy = jest.spyOn( - messenger, - 'unregisterActionHandler', - ); - const stopPollingSpy = jest.spyOn(controller, 'stopPolling'); - - controller.destroy(); - - expect(stopPollingSpy).toHaveBeenCalled(); - expect(unsubscribeSpy).toHaveBeenCalledWith( - 'KeyringController:unlock', - expect.any(Function), - ); - expect(unsubscribeSpy).toHaveBeenCalledWith( - 'KeyringController:lock', - expect.any(Function), - ); - expect(unregisterActionHandlerSpy).toHaveBeenCalledWith( - 'ConfigRegistryController:startPolling', - ); - expect(unregisterActionHandlerSpy).toHaveBeenCalledWith( - 'ConfigRegistryController:stopPolling', - ); - }); - }); - - it('handles unsubscribe errors gracefully', async () => { - await withController(({ controller, messenger }) => { - jest.spyOn(messenger, 'unsubscribe').mockImplementation(() => { - throw new Error('Handler not subscribed'); - }); - - // Should not throw even if unsubscribe fails - expect(() => controller.destroy()).not.toThrow(); - }); - }); - - it('clears pending timeout when destroying', async () => { - const pollingInterval = 10000; - const recentTimestamp = Date.now() - 2000; - await withController( - { - options: { - pollingInterval, - state: { - lastFetched: recentTimestamp, - }, - }, - }, - async ({ controller, clock }) => { - const executePollSpy = jest.spyOn(controller, '_executePoll'); - controller.startPolling(null); - - // Verify timeout was set - await advanceTime({ clock, duration: 0 }); - expect(executePollSpy).not.toHaveBeenCalled(); - - // Destroy should clear the timeout - controller.destroy(); - - // Advance time past when the timeout would have fired - await advanceTime({ clock, duration: pollingInterval }); - expect(executePollSpy).not.toHaveBeenCalled(); - }, - ); - }); - }); }); diff --git a/packages/config-registry-controller/src/ConfigRegistryController.ts b/packages/config-registry-controller/src/ConfigRegistryController.ts index e2ce7f5dd50..d11ebcb7cd7 100644 --- a/packages/config-registry-controller/src/ConfigRegistryController.ts +++ b/packages/config-registry-controller/src/ConfigRegistryController.ts @@ -20,6 +20,7 @@ import type { } from './config-registry-api-service'; import { filterNetworks } from './config-registry-api-service'; import { isConfigRegistryApiEnabled as defaultIsConfigRegistryApiEnabled } from './utils/feature-flags'; +import { RemoteFeatureFlagControllerGetStateAction } from '@metamask/remote-feature-flag-controller'; const controllerName = 'ConfigRegistryController'; @@ -130,14 +131,7 @@ export type ConfigRegistryControllerActions = | ConfigRegistryControllerGetStateAction | ConfigRegistryControllerStartPollingAction | ConfigRegistryControllerStopPollingAction - | { - type: 'RemoteFeatureFlagController:getState'; - handler: () => RemoteFeatureFlagControllerState; - } - | { - type: 'KeyringController:getState'; - handler: () => { isUnlocked: boolean }; - } + | RemoteFeatureFlagControllerGetStateAction | { type: 'ConfigRegistryApiService:fetchConfig'; handler: (options?: FetchConfigOptions) => Promise; @@ -173,10 +167,6 @@ export class ConfigRegistryController extends StaticIntervalPollingController boolean; - readonly #unlockHandler: () => void; - - readonly #lockHandler: () => void; - #delayedPollTimeoutId: ReturnType | null = null; readonly #delayedPollTokenMap: Map = new Map(); @@ -217,15 +207,6 @@ export class ConfigRegistryController extends StaticIntervalPollingController { - this.startPolling(null); - }; - - this.#lockHandler = (): void => { - this.stopPolling(); - }; - this.messenger.registerActionHandler( `${controllerName}:startPolling`, (input: null) => this.startPolling(input), @@ -236,26 +217,13 @@ export class ConfigRegistryController extends StaticIntervalPollingController this.stopPolling(token), ); - this.#registerKeyringEventListeners(); - } + this.messenger.subscribe('KeyringController:unlock', () => + this.startPolling(null), + ); - /** - * Initializes the controller by checking the KeyringController unlock state - * and starting polling if already unlocked. - * - * This method should be called after all controllers have been initialized - * to avoid runtime dependencies during construction. If KeyringController - * is not available, this method will silently skip initialization. - */ - init(): void { - try { - const { isUnlocked } = this.messenger.call('KeyringController:getState'); - if (isUnlocked) { - this.startPolling(null); - } - } catch { - // KeyringController may not be available, silently handle - } + this.messenger.subscribe('KeyringController:lock', () => + this.stopPolling(), + ); } /** @@ -402,18 +370,6 @@ export class ConfigRegistryController extends StaticIntervalPollingController Date: Wed, 28 Jan 2026 18:08:29 +0100 Subject: [PATCH 2/2] remove unused imports --- .../config-registry-controller/src/ConfigRegistryController.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/config-registry-controller/src/ConfigRegistryController.ts b/packages/config-registry-controller/src/ConfigRegistryController.ts index d11ebcb7cd7..247f69fafbf 100644 --- a/packages/config-registry-controller/src/ConfigRegistryController.ts +++ b/packages/config-registry-controller/src/ConfigRegistryController.ts @@ -9,7 +9,7 @@ import type { } from '@metamask/keyring-controller'; import type { Messenger } from '@metamask/messenger'; import { StaticIntervalPollingController } from '@metamask/polling-controller'; -import type { RemoteFeatureFlagControllerState } from '@metamask/remote-feature-flag-controller'; +import { RemoteFeatureFlagControllerGetStateAction } from '@metamask/remote-feature-flag-controller'; import { Duration, inMilliseconds } from '@metamask/utils'; import type { Json } from '@metamask/utils'; @@ -20,7 +20,6 @@ import type { } from './config-registry-api-service'; import { filterNetworks } from './config-registry-api-service'; import { isConfigRegistryApiEnabled as defaultIsConfigRegistryApiEnabled } from './utils/feature-flags'; -import { RemoteFeatureFlagControllerGetStateAction } from '@metamask/remote-feature-flag-controller'; const controllerName = 'ConfigRegistryController';