From 582a05981787cc0cd94a00c6eebe213cd969e15e Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 27 Mar 2025 16:09:46 +0100 Subject: [PATCH 1/3] fix: Don't refresh phishing list when updating interfaces --- packages/snaps-controllers/coverage.json | 2 +- .../SnapInterfaceController.test.tsx | 70 ++----------------- .../src/interface/SnapInterfaceController.ts | 15 +--- 3 files changed, 7 insertions(+), 80 deletions(-) diff --git a/packages/snaps-controllers/coverage.json b/packages/snaps-controllers/coverage.json index 6df3e67f1f..6d44c6623f 100644 --- a/packages/snaps-controllers/coverage.json +++ b/packages/snaps-controllers/coverage.json @@ -2,5 +2,5 @@ "branches": 93.34, "functions": 97.37, "lines": 98.33, - "statements": 98.07 + "statements": 98.06 } diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx index 563deeca04..e703024069 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.test.tsx @@ -147,11 +147,6 @@ describe('SnapInterfaceController', () => { expect(rootMessenger.call).toHaveBeenNthCalledWith( 2, - 'PhishingController:maybeUpdateState', - ); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 3, 'PhishingController:testOrigin', 'https://foo.bar/', ); @@ -187,7 +182,7 @@ describe('SnapInterfaceController', () => { ); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 4, + 3, 'MultichainAssetsController:getState', ); @@ -243,11 +238,6 @@ describe('SnapInterfaceController', () => { expect(rootMessenger.call).toHaveBeenNthCalledWith( 2, - 'PhishingController:maybeUpdateState', - ); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 3, 'PhishingController:testOrigin', 'https://foo.bar/', ); @@ -366,11 +356,6 @@ describe('SnapInterfaceController', () => { false, ); - rootMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - jest.fn(), - ); - rootMessenger.registerActionHandler( 'PhishingController:testOrigin', () => ({ result: true, type: 'all' }), @@ -399,11 +384,6 @@ describe('SnapInterfaceController', () => { expect(rootMessenger.call).toHaveBeenNthCalledWith( 2, - 'PhishingController:maybeUpdateState', - ); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 3, 'PhishingController:testOrigin', 'https://foo.bar/', ); @@ -416,11 +396,6 @@ describe('SnapInterfaceController', () => { false, ); - rootMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - jest.fn(), - ); - rootMessenger.registerActionHandler( 'PhishingController:testOrigin', () => ({ result: true, type: 'all' }), @@ -449,11 +424,6 @@ describe('SnapInterfaceController', () => { expect(rootMessenger.call).toHaveBeenNthCalledWith( 2, - 'PhishingController:maybeUpdateState', - ); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 3, 'PhishingController:testOrigin', 'https://foo.bar/', ); @@ -466,11 +436,6 @@ describe('SnapInterfaceController', () => { false, ); - rootMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - jest.fn(), - ); - rootMessenger.registerActionHandler( 'SnapController:get', () => undefined, @@ -500,7 +465,7 @@ describe('SnapInterfaceController', () => { ); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 3, + 2, 'SnapController:get', MOCK_SNAP_ID, ); @@ -513,11 +478,6 @@ describe('SnapInterfaceController', () => { false, ); - rootMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - jest.fn(), - ); - rootMessenger.registerActionHandler( 'PhishingController:testOrigin', () => ({ result: true, type: 'all' }), @@ -563,7 +523,7 @@ describe('SnapInterfaceController', () => { ); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 3, + 2, 'AccountsController:getAccountByAddress', '7S3P4HxJpyyigGzodYwHtCxZyUQe9JiBMHyRWXArAaKv', ); @@ -933,11 +893,6 @@ describe('SnapInterfaceController', () => { false, ); - rootMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - jest.fn(), - ); - rootMessenger.registerActionHandler( 'PhishingController:testOrigin', () => ({ result: true, type: 'all' }), @@ -977,12 +932,7 @@ describe('SnapInterfaceController', () => { ).rejects.toThrow('Invalid URL: The specified URL is not allowed.'); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 4, - 'PhishingController:maybeUpdateState', - ); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 5, + 3, 'PhishingController:testOrigin', 'https://foo.bar/', ); @@ -995,11 +945,6 @@ describe('SnapInterfaceController', () => { false, ); - rootMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - jest.fn(), - ); - rootMessenger.registerActionHandler( 'PhishingController:testOrigin', () => ({ result: true, type: 'all' }), @@ -1043,12 +988,7 @@ describe('SnapInterfaceController', () => { ).rejects.toThrow('Invalid URL: The specified URL is not allowed.'); expect(rootMessenger.call).toHaveBeenNthCalledWith( - 4, - 'PhishingController:maybeUpdateState', - ); - - expect(rootMessenger.call).toHaveBeenNthCalledWith( - 5, + 3, 'PhishingController:testOrigin', 'https://foo.bar/', ); diff --git a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts index 487a11adf5..f8143fd2c5 100644 --- a/packages/snaps-controllers/src/interface/SnapInterfaceController.ts +++ b/packages/snaps-controllers/src/interface/SnapInterfaceController.ts @@ -8,10 +8,7 @@ import type { ControllerStateChangeEvent, } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; -import type { - MaybeUpdateState, - TestOrigin, -} from '@metamask/phishing-controller'; +import type { TestOrigin } from '@metamask/phishing-controller'; import type { InterfaceState, SnapId, @@ -91,7 +88,6 @@ type MultichainAssetsControllerGetStateAction = ControllerGetStateAction< export type SnapInterfaceControllerAllowedActions = | TestOrigin - | MaybeUpdateState | HasApprovalRequest | AcceptRequest | GetSnap @@ -406,13 +402,6 @@ export class SnapInterfaceController extends BaseController< ); } - /** - * Trigger a Phishing list update if needed. - */ - async #triggerPhishingListUpdate() { - await this.messagingSystem.call('PhishingController:maybeUpdateState'); - } - /** * Check an origin against the phishing list. * @@ -500,8 +489,6 @@ export class SnapInterfaceController extends BaseController< `A Snap UI may not be larger than ${MAX_UI_CONTENT_SIZE / 1000000} MB.`, ); - await this.#triggerPhishingListUpdate(); - validateJsxElements(element, { isOnPhishingList: this.#checkPhishingList.bind(this), getSnap: this.#getSnap.bind(this), From 7a24a827001324b454793f92093a6018a7ab7d92 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 27 Mar 2025 16:17:31 +0100 Subject: [PATCH 2/3] Remove action --- .../snaps-controllers/src/test-utils/controller.ts | 7 ------- packages/snaps-jest/src/test-utils/controller.ts | 6 ------ packages/snaps-simulation/src/controllers.ts | 1 - packages/snaps-simulation/src/simulation.test.ts | 8 -------- packages/snaps-simulation/src/simulation.ts | 5 ----- packages/snaps-simulation/src/test-utils/controller.ts | 6 ------ .../snaps-simulator/src/features/simulation/sagas.ts | 10 +--------- .../src/features/simulation/test/controllers.ts | 5 +---- 8 files changed, 2 insertions(+), 46 deletions(-) diff --git a/packages/snaps-controllers/src/test-utils/controller.ts b/packages/snaps-controllers/src/test-utils/controller.ts index 7dc1efd3cc..f6c162cbb7 100644 --- a/packages/snaps-controllers/src/test-utils/controller.ts +++ b/packages/snaps-controllers/src/test-utils/controller.ts @@ -491,7 +491,6 @@ export const getSnapControllerMessenger = ( 'PermissionController:revokePermissionForAllSubjects', 'PermissionController:updateCaveat', 'PermissionController:getSubjectNames', - 'PhishingController:maybeUpdateState', 'PhishingController:testOrigin', 'SnapController:get', 'SnapController:handleRequest', @@ -781,7 +780,6 @@ export const getRestrictedSnapInterfaceControllerMessenger = ( name: 'SnapInterfaceController', allowedActions: [ 'PhishingController:testOrigin', - 'PhishingController:maybeUpdateState', 'ApprovalController:hasRequest', 'ApprovalController:acceptRequest', 'MultichainAssetsController:getState', @@ -795,11 +793,6 @@ export const getRestrictedSnapInterfaceControllerMessenger = ( }); if (mocked) { - messenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - async () => Promise.resolve(), - ); - messenger.registerActionHandler('PhishingController:testOrigin', () => ({ result: false, type: 'all', diff --git a/packages/snaps-jest/src/test-utils/controller.ts b/packages/snaps-jest/src/test-utils/controller.ts index e2e1e7fec7..e32e5c5bf2 100644 --- a/packages/snaps-jest/src/test-utils/controller.ts +++ b/packages/snaps-jest/src/test-utils/controller.ts @@ -10,11 +10,6 @@ export const getRootControllerMessenger = (mocked = true) => { >(); if (mocked) { - messenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - async () => Promise.resolve(), - ); - messenger.registerActionHandler('PhishingController:testOrigin', () => ({ result: false, type: 'all', @@ -51,7 +46,6 @@ export const getRestrictedSnapInterfaceControllerMessenger = ( name: 'SnapInterfaceController', allowedActions: [ 'PhishingController:testOrigin', - 'PhishingController:maybeUpdateState', 'ApprovalController:hasRequest', 'ApprovalController:acceptRequest', ], diff --git a/packages/snaps-simulation/src/controllers.ts b/packages/snaps-simulation/src/controllers.ts index 1e5559a8fc..69a732e0ea 100644 --- a/packages/snaps-simulation/src/controllers.ts +++ b/packages/snaps-simulation/src/controllers.ts @@ -84,7 +84,6 @@ export function getControllers(options: GetControllersOptions): Controllers { messenger: controllerMessenger.getRestricted({ name: 'SnapInterfaceController', allowedActions: [ - 'PhishingController:maybeUpdateState', 'PhishingController:testOrigin', 'ApprovalController:hasRequest', 'ApprovalController:acceptRequest', diff --git a/packages/snaps-simulation/src/simulation.test.ts b/packages/snaps-simulation/src/simulation.test.ts index d40a9291a4..2de781ab7d 100644 --- a/packages/snaps-simulation/src/simulation.test.ts +++ b/packages/snaps-simulation/src/simulation.test.ts @@ -631,14 +631,6 @@ describe('registerActions', () => { const { runSaga, store } = createStore(getMockOptions()); const controllerMessenger = getRootControllerMessenger(false); - it('registers `PhishingController:maybeUpdateState`', async () => { - registerActions(controllerMessenger, runSaga); - - expect( - await controllerMessenger.call('PhishingController:maybeUpdateState'), - ).toBeUndefined(); - }); - it('registers `PhishingController:testOrigin`', async () => { registerActions(controllerMessenger, runSaga); diff --git a/packages/snaps-simulation/src/simulation.ts b/packages/snaps-simulation/src/simulation.ts index e04be1e572..a1f1334cea 100644 --- a/packages/snaps-simulation/src/simulation.ts +++ b/packages/snaps-simulation/src/simulation.ts @@ -470,11 +470,6 @@ export function registerActions( controllerMessenger: RootControllerMessenger, runSaga: RunSagaFunction, ) { - controllerMessenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - async () => Promise.resolve(), - ); - controllerMessenger.registerActionHandler( 'PhishingController:testOrigin', () => ({ result: false, type: PhishingDetectorResultType.All }), diff --git a/packages/snaps-simulation/src/test-utils/controller.ts b/packages/snaps-simulation/src/test-utils/controller.ts index ac264f7cff..2e70a084e0 100644 --- a/packages/snaps-simulation/src/test-utils/controller.ts +++ b/packages/snaps-simulation/src/test-utils/controller.ts @@ -14,11 +14,6 @@ export const getRootControllerMessenger = (mocked = true) => { >(); if (mocked) { - messenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - async () => Promise.resolve(), - ); - messenger.registerActionHandler('PhishingController:testOrigin', () => ({ result: false, type: PhishingDetectorResultType.All, @@ -56,7 +51,6 @@ export const getRestrictedSnapInterfaceControllerMessenger = ( name: 'SnapInterfaceController', allowedActions: [ 'PhishingController:testOrigin', - 'PhishingController:maybeUpdateState', 'ApprovalController:hasRequest', 'ApprovalController:acceptRequest', ], diff --git a/packages/snaps-simulator/src/features/simulation/sagas.ts b/packages/snaps-simulator/src/features/simulation/sagas.ts index c5ad17c914..453e699a45 100644 --- a/packages/snaps-simulator/src/features/simulation/sagas.ts +++ b/packages/snaps-simulator/src/features/simulation/sagas.ts @@ -98,11 +98,6 @@ export function registerActions(messenger: Messenger) { messenger.registerActionHandler('PhishingController:testOrigin', () => ({ result: false, })); - - messenger.registerActionHandler( - 'PhishingController:maybeUpdateState', - async () => Promise.resolve(), - ); } /** @@ -202,10 +197,7 @@ export function* initSaga({ payload }: PayloadAction) { const snapInterfaceController = new SnapInterfaceController({ messenger: messenger.getRestricted({ name: 'SnapInterfaceController', - allowedActions: [ - `PhishingController:testOrigin`, - `PhishingController:maybeUpdateState`, - ], + allowedActions: [`PhishingController:testOrigin`], allowedEvents: [ 'NotificationServicesController:notificationsListUpdated', ], diff --git a/packages/snaps-simulator/src/features/simulation/test/controllers.ts b/packages/snaps-simulator/src/features/simulation/test/controllers.ts index 0cbd31b409..7a2dc6ca24 100644 --- a/packages/snaps-simulator/src/features/simulation/test/controllers.ts +++ b/packages/snaps-simulator/src/features/simulation/test/controllers.ts @@ -16,10 +16,7 @@ export function getSnapInterfaceController() { return new SnapInterfaceController({ messenger: messenger.getRestricted({ name: 'SnapInterfaceController', - allowedActions: [ - 'PhishingController:maybeUpdateState', - 'PhishingController:testOrigin', - ], + allowedActions: ['PhishingController:testOrigin'], allowedEvents: [ 'NotificationServicesController:notificationsListUpdated', ], From 92de5e4e29f81ca23c4361b87ac32e02bf4024e6 Mon Sep 17 00:00:00 2001 From: Frederik Bolding Date: Thu, 27 Mar 2025 16:27:31 +0100 Subject: [PATCH 3/3] Fix tests --- packages/snaps-simulation/src/request.test.tsx | 10 +++++----- packages/snaps-simulation/src/simulation.test.ts | 8 ++++---- packages/snaps-simulator/jest.config.js | 6 +++--- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/packages/snaps-simulation/src/request.test.tsx b/packages/snaps-simulation/src/request.test.tsx index da93e74348..4dc803490a 100644 --- a/packages/snaps-simulation/src/request.test.tsx +++ b/packages/snaps-simulation/src/request.test.tsx @@ -419,7 +419,7 @@ describe('getInterfaceApi', () => { await snapInterface.clickElement('foo'); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 5, + 4, 'ExecutionService:handleRpcRequest', MOCK_SNAP_ID, { @@ -466,7 +466,7 @@ describe('getInterfaceApi', () => { await snapInterface.typeInField('foo', 'bar'); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ExecutionService:handleRpcRequest', MOCK_SNAP_ID, { @@ -519,7 +519,7 @@ describe('getInterfaceApi', () => { await snapInterface.selectInDropdown('foo', 'option2'); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ExecutionService:handleRpcRequest', MOCK_SNAP_ID, { @@ -572,7 +572,7 @@ describe('getInterfaceApi', () => { await snapInterface.selectFromRadioGroup('foo', 'option2'); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ExecutionService:handleRpcRequest', MOCK_SNAP_ID, { @@ -629,7 +629,7 @@ describe('getInterfaceApi', () => { await snapInterface.selectFromSelector('foo', 'option2'); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 6, + 5, 'ExecutionService:handleRpcRequest', MOCK_SNAP_ID, { diff --git a/packages/snaps-simulation/src/simulation.test.ts b/packages/snaps-simulation/src/simulation.test.ts index 2de781ab7d..94977b2a0b 100644 --- a/packages/snaps-simulation/src/simulation.test.ts +++ b/packages/snaps-simulation/src/simulation.test.ts @@ -491,7 +491,7 @@ describe('getPermittedHooks', () => { await updateInterface(id, content); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 3, + 2, 'SnapInterfaceController:updateInterface', snapId, id, @@ -532,7 +532,7 @@ describe('getPermittedHooks', () => { const result = getInterfaceState(id); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 3, + 2, 'SnapInterfaceController:getInterface', snapId, id, @@ -573,7 +573,7 @@ describe('getPermittedHooks', () => { const result = getInterfaceContext(id); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 3, + 2, 'SnapInterfaceController:getInterface', snapId, id, @@ -616,7 +616,7 @@ describe('getPermittedHooks', () => { await resolveInterface(id, 'foobar'); expect(controllerMessenger.call).toHaveBeenNthCalledWith( - 2, + 1, 'SnapInterfaceController:resolveInterface', snapId, id, diff --git a/packages/snaps-simulator/jest.config.js b/packages/snaps-simulator/jest.config.js index 1bf2add3f6..52d7602a27 100644 --- a/packages/snaps-simulator/jest.config.js +++ b/packages/snaps-simulator/jest.config.js @@ -8,9 +8,9 @@ module.exports = deepmerge(baseConfig, { coverageThreshold: { global: { branches: 54.33, - functions: 60.43, - lines: 80.49, - statements: 80.79, + functions: 60.32, + lines: 80.47, + statements: 80.77, }, }, setupFiles: ['./jest.setup.js'],