From 0ab7b748d16f4277e279dde36461bdae27ca9306 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Tue, 7 Oct 2025 15:39:44 +0200 Subject: [PATCH 1/3] refactor: migrate PhishingController to @metamask/messenger --- packages/phishing-controller/CHANGELOG.md | 5 + packages/phishing-controller/package.json | 1 + .../src/PhishingController.test.ts | 91 +++++++++++++------ .../src/PhishingController.ts | 46 +++++----- .../phishing-controller/tsconfig.build.json | 3 +- packages/phishing-controller/tsconfig.json | 3 +- yarn.lock | 1 + 7 files changed, 98 insertions(+), 52 deletions(-) diff --git a/packages/phishing-controller/CHANGELOG.md b/packages/phishing-controller/CHANGELOG.md index e1919def703..96058b52dd9 100644 --- a/packages/phishing-controller/CHANGELOG.md +++ b/packages/phishing-controller/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** Use new `Messenger` from `@metamask/messenger` ([#6535](https://github.com/MetaMask/core/pull/6535)) + - Previously, `PhishingController` accepted a `RestrictedMessenger` instance from `@metamask/base-controller`. + ## [14.1.0] ### Added diff --git a/packages/phishing-controller/package.json b/packages/phishing-controller/package.json index 73514b3aeb9..df368d2c6a6 100644 --- a/packages/phishing-controller/package.json +++ b/packages/phishing-controller/package.json @@ -49,6 +49,7 @@ "dependencies": { "@metamask/base-controller": "^8.4.0", "@metamask/controller-utils": "^11.14.0", + "@metamask/messenger": "^0.3.0", "@noble/hashes": "^1.8.0", "@types/punycode": "^2.1.0", "ethereum-cryptography": "^2.1.2", diff --git a/packages/phishing-controller/src/PhishingController.test.ts b/packages/phishing-controller/src/PhishingController.test.ts index e2a2ff23d6f..b22e2e6c017 100644 --- a/packages/phishing-controller/src/PhishingController.test.ts +++ b/packages/phishing-controller/src/PhishingController.test.ts @@ -1,5 +1,11 @@ -import { deriveStateFromMetadata, Messenger } from '@metamask/base-controller'; -import type { TransactionControllerStateChangeEvent } from '@metamask/transaction-controller'; +import { deriveStateFromMetadata } from '@metamask/base-controller/next'; +import { + Messenger, + MOCK_ANY_NAMESPACE, + type MessengerActions, + type MessengerEvents, + type MockAnyNamespace, +} from '@metamask/messenger'; import { strict as assert } from 'assert'; import nock, { cleanAll, isDone, pendingMocks } from 'nock'; import sinon from 'sinon'; @@ -10,8 +16,6 @@ import { METAMASK_STALELIST_FILE, PhishingController, PHISHING_CONFIG_BASE_URL, - type PhishingControllerActions, - type PhishingControllerEvents, type PhishingControllerOptions, CLIENT_SIDE_DETECION_BASE_URL, C2_DOMAIN_BLOCKLIST_ENDPOINT, @@ -19,6 +23,7 @@ import { PHISHING_DETECTION_SCAN_ENDPOINT, PHISHING_DETECTION_BULK_SCAN_ENDPOINT, type BulkPhishingDetectionScanResponse, + type PhishingControllerMessenger, } from './PhishingController'; import { createMockStateChangePayload, @@ -32,24 +37,58 @@ import { getHostnameFromUrl } from './utils'; const controllerName = 'PhishingController'; +type AllPhishingControllerActions = + MessengerActions; + +type AllPhishingControllerEvents = MessengerEvents; + +type RootMessenger = Messenger< + MockAnyNamespace, + AllPhishingControllerActions, + AllPhishingControllerEvents +>; + /** - * Constructs a restricted messenger with transaction events enabled. + * Creates and returns a root messenger for testing * - * @returns A restricted messenger that can listen to TransactionController events. + * @returns A messenger instance */ -function getRestrictedMessengerWithTransactionEvents() { +function getRootMessenger(): RootMessenger { + return new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); +} + +/** + * Constructs a messenger for use in PhishingController tests. + * + * @returns A messenger and the root messenger. + */ +function setupMessenger(): { + messenger: PhishingControllerMessenger; + rootMessenger: RootMessenger; +} { + const rootMessenger = getRootMessenger(); + const messenger = new Messenger< - PhishingControllerActions, - PhishingControllerEvents | TransactionControllerStateChangeEvent - >(); + typeof controllerName, + AllPhishingControllerActions, + AllPhishingControllerEvents, + RootMessenger + >({ + namespace: controllerName, + parent: rootMessenger, + }); + + rootMessenger.delegate({ + actions: [], + events: ['TransactionController:stateChange'], + messenger, + }); return { - messenger: messenger.getRestricted({ - name: controllerName, - allowedActions: [], - allowedEvents: ['TransactionController:stateChange'], - }), - globalMessenger: messenger, + messenger, + rootMessenger, }; } @@ -60,8 +99,9 @@ function getRestrictedMessengerWithTransactionEvents() { * @returns The constructed Phishing Controller. */ function getPhishingController(options?: Partial) { + const { messenger } = setupMessenger(); return new PhishingController({ - messenger: getRestrictedMessengerWithTransactionEvents().messenger, + messenger, ...options, }); } @@ -407,8 +447,9 @@ describe('PhishingController', () => { }); it('replaces existing phishing lists with completely new list from phishing detection API', async () => { + const { messenger } = setupMessenger(); const controller = new PhishingController({ - messenger: getRestrictedMessengerWithTransactionEvents().messenger, + messenger, stalelistRefreshInterval: 10, state: { phishingLists: [ @@ -3495,7 +3536,7 @@ describe('URL Scan Cache', () => { deriveStateFromMetadata( controller.state, controller.metadata, - 'anonymous', + 'includeInDebugSnapshot', ), ).toMatchInlineSnapshot(`Object {}`); }); @@ -3561,18 +3602,16 @@ describe('URL Scan Cache', () => { describe('Transaction Controller State Change Integration', () => { let controller: PhishingController; - let globalMessenger: Messenger< - PhishingControllerActions, - PhishingControllerEvents | TransactionControllerStateChangeEvent - >; + let globalMessenger: RootMessenger; let bulkScanTokensSpy: jest.SpyInstance; beforeEach(() => { - const messengerSetup = getRestrictedMessengerWithTransactionEvents(); - globalMessenger = messengerSetup.globalMessenger; + const { messenger, rootMessenger } = setupMessenger(); + + globalMessenger = rootMessenger; controller = new PhishingController({ - messenger: messengerSetup.messenger, + messenger, }); bulkScanTokensSpy = jest diff --git a/packages/phishing-controller/src/PhishingController.ts b/packages/phishing-controller/src/PhishingController.ts index 2bd79231fdd..c3bb20c6b9c 100644 --- a/packages/phishing-controller/src/PhishingController.ts +++ b/packages/phishing-controller/src/PhishingController.ts @@ -1,14 +1,14 @@ -import type { - ControllerGetStateAction, - ControllerStateChangeEvent, - RestrictedMessenger, - StateMetadata, -} from '@metamask/base-controller'; -import { BaseController } from '@metamask/base-controller'; +import { + BaseController, + type StateMetadata, + type ControllerGetStateAction, + type ControllerStateChangeEvent, +} from '@metamask/base-controller/next'; import { safelyExecute, safelyExecuteWithTimeout, } from '@metamask/controller-utils'; +import { type Messenger } from '@metamask/messenger'; import type { TransactionControllerStateChangeEvent, TransactionMeta, @@ -238,49 +238,49 @@ const metadata: StateMetadata = { phishingLists: { includeInStateLogs: false, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: false, }, whitelist: { includeInStateLogs: false, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: false, }, whitelistPaths: { includeInStateLogs: false, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: false, }, hotlistLastFetched: { includeInStateLogs: true, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: false, }, stalelistLastFetched: { includeInStateLogs: true, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: false, }, c2DomainBlocklistLastFetched: { includeInStateLogs: true, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: false, }, urlScanCache: { includeInStateLogs: false, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: true, }, tokenScanCache: { includeInStateLogs: false, persist: true, - anonymous: false, + includeInDebugSnapshot: false, usedInUi: true, }, }; @@ -399,12 +399,10 @@ type AllowedActions = never; */ export type AllowedEvents = TransactionControllerStateChangeEvent; -export type PhishingControllerMessenger = RestrictedMessenger< +export type PhishingControllerMessenger = Messenger< typeof controllerName, PhishingControllerActions | AllowedActions, - PhishingControllerEvents | AllowedEvents, - AllowedActions['type'], - AllowedEvents['type'] + PhishingControllerEvents | AllowedEvents >; /** @@ -521,7 +519,7 @@ export class PhishingController extends BaseController< } #subscribeToTransactionControllerStateChange() { - this.messagingSystem.subscribe( + this.messenger.subscribe( 'TransactionController:stateChange', this.#transactionControllerStateChangeHandler, ); @@ -532,22 +530,22 @@ export class PhishingController extends BaseController< * actions. */ #registerMessageHandlers(): void { - this.messagingSystem.registerActionHandler( + this.messenger.registerActionHandler( `${controllerName}:maybeUpdateState` as const, this.maybeUpdateState.bind(this), ); - this.messagingSystem.registerActionHandler( + this.messenger.registerActionHandler( `${controllerName}:testOrigin` as const, this.test.bind(this), ); - this.messagingSystem.registerActionHandler( + this.messenger.registerActionHandler( `${controllerName}:bulkScanUrls` as const, this.bulkScanUrls.bind(this), ); - this.messagingSystem.registerActionHandler( + this.messenger.registerActionHandler( `${controllerName}:bulkScanTokens` as const, this.bulkScanTokens.bind(this), ); diff --git a/packages/phishing-controller/tsconfig.build.json b/packages/phishing-controller/tsconfig.build.json index ef633b78ac6..25a4d3c697a 100644 --- a/packages/phishing-controller/tsconfig.build.json +++ b/packages/phishing-controller/tsconfig.build.json @@ -8,7 +8,8 @@ "references": [ { "path": "../base-controller/tsconfig.build.json" }, { "path": "../controller-utils/tsconfig.build.json" }, - { "path": "../transaction-controller/tsconfig.build.json" } + { "path": "../transaction-controller/tsconfig.build.json" }, + { "path": "../messenger/tsconfig.build.json" } ], "include": ["../../types", "./src"] } diff --git a/packages/phishing-controller/tsconfig.json b/packages/phishing-controller/tsconfig.json index 9c91d666a84..5f32f34e8aa 100644 --- a/packages/phishing-controller/tsconfig.json +++ b/packages/phishing-controller/tsconfig.json @@ -6,7 +6,8 @@ "references": [ { "path": "../base-controller" }, { "path": "../controller-utils" }, - { "path": "../transaction-controller" } + { "path": "../transaction-controller" }, + { "path": "../messenger" } ], "include": ["../../types", "./src", "./tests"] } diff --git a/yarn.lock b/yarn.lock index 8dbaec41939..b1a3502530c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4212,6 +4212,7 @@ __metadata: "@metamask/auto-changelog": "npm:^3.4.4" "@metamask/base-controller": "npm:^8.4.0" "@metamask/controller-utils": "npm:^11.14.0" + "@metamask/messenger": "npm:^0.3.0" "@metamask/transaction-controller": "npm:^60.6.0" "@noble/hashes": "npm:^1.8.0" "@types/jest": "npm:^27.4.1" From 141ad75fb20b890565f870182cc63360bed698df Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 15 Oct 2025 16:21:16 +0200 Subject: [PATCH 2/3] fix: PhishingController tests --- packages/phishing-controller/src/PhishingController.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/phishing-controller/src/PhishingController.test.ts b/packages/phishing-controller/src/PhishingController.test.ts index b22e2e6c017..52dce0de2b8 100644 --- a/packages/phishing-controller/src/PhishingController.test.ts +++ b/packages/phishing-controller/src/PhishingController.test.ts @@ -45,7 +45,8 @@ type AllPhishingControllerEvents = MessengerEvents; type RootMessenger = Messenger< MockAnyNamespace, AllPhishingControllerActions, - AllPhishingControllerEvents + AllPhishingControllerEvents, + RootMessenger >; /** From 3e1240ae3bc57fa1d68bac75e24e6897289ec714 Mon Sep 17 00:00:00 2001 From: Salah-Eddine Saakoun Date: Wed, 15 Oct 2025 16:36:24 +0200 Subject: [PATCH 3/3] fix: Bulk Token Scanning tests --- .../src/BulkTokenScan.test.ts | 67 ++++++++++++++----- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/packages/phishing-controller/src/BulkTokenScan.test.ts b/packages/phishing-controller/src/BulkTokenScan.test.ts index 9f84752578a..1773fd75de4 100644 --- a/packages/phishing-controller/src/BulkTokenScan.test.ts +++ b/packages/phishing-controller/src/BulkTokenScan.test.ts @@ -1,13 +1,17 @@ -import { Messenger } from '@metamask/base-controller'; import { safelyExecuteWithTimeout } from '@metamask/controller-utils'; -import type { TransactionControllerStateChangeEvent } from '@metamask/transaction-controller'; +import { + Messenger, + MOCK_ANY_NAMESPACE, + type MessengerActions, + type MessengerEvents, + type MockAnyNamespace, +} from '@metamask/messenger'; import nock, { cleanAll } from 'nock'; import sinon from 'sinon'; -import type { PhishingControllerEvents } from './PhishingController'; +import type { PhishingControllerMessenger } from './PhishingController'; import { PhishingController, - type PhishingControllerActions, type PhishingControllerOptions, SECURITY_ALERTS_BASE_URL, TOKEN_BULK_SCANNING_ENDPOINT, @@ -30,24 +34,55 @@ const mockSafelyExecuteWithTimeout = const controllerName = 'PhishingController'; +type AllPhishingControllerActions = + MessengerActions; + +type AllPhishingControllerEvents = MessengerEvents; + +type RootMessenger = Messenger< + MockAnyNamespace, + AllPhishingControllerActions, + AllPhishingControllerEvents, + RootMessenger +>; + +/** + * Creates and returns a root messenger for testing + * + * @returns A messenger instance + */ +function getRootMessenger(): RootMessenger { + return new Messenger({ + namespace: MOCK_ANY_NAMESPACE, + }); +} + /** - * Constructs a restricted messenger with transaction events enabled. + * Constructs a messenger with transaction events enabled. * * @returns A restricted messenger that can listen to TransactionController events. */ -function getRestrictedMessengerWithTransactionEvents() { +function getMessengerWithTransactionEvents() { + const rootMessenger = getRootMessenger(); + const messenger = new Messenger< - PhishingControllerActions, - PhishingControllerEvents | TransactionControllerStateChangeEvent - >(); + typeof controllerName, + AllPhishingControllerActions, + AllPhishingControllerEvents, + RootMessenger + >({ + namespace: controllerName, + parent: rootMessenger, + }); + + rootMessenger.delegate({ + actions: [], + events: ['TransactionController:stateChange'], + messenger, + }); return { - messenger: messenger.getRestricted({ - name: controllerName, - allowedActions: [], - allowedEvents: ['TransactionController:stateChange'], - }), - globalMessenger: messenger, + messenger, }; } @@ -59,7 +94,7 @@ function getRestrictedMessengerWithTransactionEvents() { */ function getPhishingController(options?: Partial) { return new PhishingController({ - messenger: getRestrictedMessengerWithTransactionEvents().messenger, + messenger: getMessengerWithTransactionEvents().messenger, ...options, }); }