refactor: migrate PhishingController data fetching to PhishingDataService

eslint-suppressions.json

@@ -1559,27 +1559,14 @@
       "count": 1
     }
   },
-  "packages/phishing-controller/src/CacheManager.test.ts": {
-    "@typescript-eslint/explicit-function-return-type": {
-      "count": 2
-    }
-  },
-  "packages/phishing-controller/src/CacheManager.ts": {
-    "@typescript-eslint/naming-convention": {
-      "count": 3
-    }
-  },
   "packages/phishing-controller/src/PathTrie.ts": {
     "@typescript-eslint/explicit-function-return-type": {
       "count": 2
     }
   },
   "packages/phishing-controller/src/PhishingController.ts": {
     "@typescript-eslint/explicit-function-return-type": {
-      "count": 14
-    },
-    "@typescript-eslint/naming-convention": {
-      "count": 1
+      "count": 11
     },
     "@typescript-eslint/prefer-nullish-coalescing": {
       "count": 6
@@ -1610,7 +1597,7 @@
   },
   "packages/phishing-controller/src/utils.ts": {
     "@typescript-eslint/explicit-function-return-type": {
-      "count": 5
+      "count": 4
     },
     "@typescript-eslint/prefer-nullish-coalescing": {
       "count": 1
@@ -2343,4 +2330,4 @@
       "count": 10
     }
   }
-}
+}

packages/phishing-controller/CHANGELOG.md

@@ -7,10 +7,33 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Added
+
+- Add `PhishingDataService`, a `BaseDataService` subclass that now performs all network requests for `PhishingController` (stalelist, hotlist diffs, C2 domain blocklist, URL/token/address scans, and approvals)
+  - Exposes the messenger actions `PhishingDataService:getStalelist`, `PhishingDataService:getHotlistDiffs`, `PhishingDataService:getC2DomainBlocklist`, `PhishingDataService:scanUrl`, `PhishingDataService:bulkScanUrls`, `PhishingDataService:scanToken`, `PhishingDataService:bulkScanTokens`, `PhishingDataService:scanAddress`, and `PhishingDataService:getApprovals`, making query results available to the UI via `@metamask/react-data-query`
+  - Requests are wrapped in a shared retry policy, configurable via the `policyOptions` constructor option; circuit breaking is disabled by default because the service spans four independent API hosts and a broken circuit caused by one host would pause phishing-list updates from the others
+  - Scan results are cached per URL hostname, token, and address for `SCAN_RESULT_STALE_TIME` (1 minute, matching the previous cache TTLs); bulk scans only request items without a fresh cached result and coalesce them into batched API calls (up to 50 URLs / 100 tokens per request), and single and bulk URL scans share cache entries; approvals are never cached
+  - The query cache is persisted between sessions by default (`persistenceConfig`, max age 5 minutes), which requires the `StorageService:setItem`, `StorageService:getItem`, and `StorageService:removeItem` messenger actions and an `init` call during client initialization (automatic with `@metamask/wallet`); pass `persistenceConfig: null` to disable
+
+- Export the `resolveChainName` utility, which maps chain IDs to the chain names used in scan query keys, enabling UI consumers to construct `PhishingDataService` query keys
+
 ### Changed
 
+- **BREAKING:** `PhishingController` no longer performs network requests directly; a `PhishingDataService` must be registered and its method actions delegated to the controller's messenger
+  - `PhishingControllerMessenger` now requires the `PhishingDataService` method actions listed above as allowed actions
+- **BREAKING:** Remove the `urlScanCache`, `tokenScanCache`, and `addressScanCache` properties from `PhishingControllerState`; scan results are now cached (and persisted) by `PhishingDataService`'s query cache
+  - Client state migrations should remove these properties from persisted `PhishingController` state
+- **BREAKING:** Remove the `urlScanCacheTTL`, `urlScanCacheMaxSize`, `tokenScanCacheTTL`, `tokenScanCacheMaxSize`, `addressScanCacheTTL`, and `addressScanCacheMaxSize` options from `PhishingControllerOptions`; scan result freshness is now controlled by `SCAN_RESULT_STALE_TIME` in `PhishingDataService`
+- Tokens for which the bulk scanning API returns no result are now negatively cached for `SCAN_RESULT_STALE_TIME` instead of being re-requested on every call
+- `scanUrl` now reports the underlying error message in `fetchError` for network errors instead of `'timeout of 8000ms exceeded'`
+- Malformed API responses (e.g. a stalelist without a numeric `lastUpdated`, or scan results without a `recommendedAction`/`result_type`) are now rejected and treated as request failures instead of being passed through
 - Bump `@metamask/transaction-controller` from `^69.4.0` to `^69.5.2` ([#9780](https://github.com/MetaMask/core/pull/9780), [#9798](https://github.com/MetaMask/core/pull/9798), [#9823](https://github.com/MetaMask/core/pull/9823))
 
+### Removed
+
+- **BREAKING:** Remove the `CacheEntry` type; the custom cache manager has been replaced by `PhishingDataService`'s query cache
+- **BREAKING:** Remove the `DEFAULT_URL_SCAN_CACHE_TTL`, `DEFAULT_URL_SCAN_CACHE_MAX_SIZE`, `DEFAULT_TOKEN_SCAN_CACHE_TTL`, `DEFAULT_TOKEN_SCAN_CACHE_MAX_SIZE`, `DEFAULT_ADDRESS_SCAN_CACHE_TTL`, and `DEFAULT_ADDRESS_SCAN_CACHE_MAX_SIZE` constants
+
 ## [17.3.1]
 
 ### Changed

packages/phishing-controller/package.json

@@ -57,10 +57,15 @@
   "dependencies": {
     "@metamask/address-book-controller": "^7.1.2",
     "@metamask/base-controller": "^9.1.0",
+    "@metamask/base-data-service": "^0.1.3",
     "@metamask/controller-utils": "^12.3.0",
     "@metamask/messenger": "^2.0.0",
+    "@metamask/storage-service": "^1.0.2",
+    "@metamask/superstruct": "^3.4.1",
     "@metamask/transaction-controller": "^69.5.2",
+    "@metamask/utils": "^11.11.0",
     "@noble/hashes": "^1.8.0",
+    "@tanstack/query-core": "^4.43.0",
     "@types/punycode": "^2.1.0",
     "ethereum-cryptography": "^2.1.2",
     "fastest-levenshtein": "^1.0.16",

packages/phishing-controller/src/BulkTokenScan.test.ts

@@ -1,4 +1,3 @@
-import { safelyExecuteWithTimeout } from '@metamask/controller-utils';
 import { Messenger, MOCK_ANY_NAMESPACE } from '@metamask/messenger';
 import type {
   MessengerActions,
@@ -16,19 +15,11 @@ import type {
   PhishingControllerMessenger,
   PhishingControllerOptions,
 } from './PhishingController.js';
+import { PhishingDataService } from './PhishingDataService.js';
+import type { PhishingDataServiceMessenger } from './PhishingDataService.js';
 import { TokenScanResultType } from './types.js';
 import type { BulkTokenScanRequest, TokenScanApiResponse } from './types.js';
 
-jest.mock('@metamask/controller-utils', () => ({
-  ...jest.requireActual('@metamask/controller-utils'),
-  safelyExecuteWithTimeout: jest.fn(),
-}));
-
-const mockSafelyExecuteWithTimeout =
-  safelyExecuteWithTimeout as jest.MockedFunction<
-    typeof safelyExecuteWithTimeout
-  >;
-
 const controllerName = 'PhishingController';
 
 type AllPhishingControllerActions =
@@ -38,11 +29,13 @@ type AllPhishingControllerEvents = MessengerEvents<PhishingControllerMessenger>;
 
 type RootMessenger = Messenger<
   MockAnyNamespace,
-  AllPhishingControllerActions,
-  AllPhishingControllerEvents,
+  AllPhishingControllerActions | MessengerActions<PhishingDataServiceMessenger>,
+  AllPhishingControllerEvents | MessengerEvents<PhishingDataServiceMessenger>,
   RootMessenger
 >;
 
+const createdDataServices: PhishingDataService[] = [];
+
 /**
  * Creates and returns a root messenger for testing
  *
@@ -55,7 +48,8 @@ function getRootMessenger(): RootMessenger {
 }
 
 /**
- * Constructs a messenger with transaction events enabled.
+ * Constructs a messenger with transaction events enabled, plus a real
+ * PhishingDataService so that tests exercise the full request path via nock.
  *
  * @returns A restricted messenger that can listen to TransactionController events.
  */
@@ -72,8 +66,34 @@ function getMessengerWithTransactionEvents() {
     parent: rootMessenger,
   });
 
+  const dataServiceMessenger = new Messenger<
+    'PhishingDataService',
+    MessengerActions<PhishingDataServiceMessenger>,
+    MessengerEvents<PhishingDataServiceMessenger>,
+    RootMessenger
+  >({
+    namespace: 'PhishingDataService',
+    parent: rootMessenger,
+  });
+  createdDataServices.push(
+    new PhishingDataService({
+      messenger: dataServiceMessenger,
+      policyOptions: { maxRetries: 0 },
+      persistenceConfig: null,
+    }),
+  );
+
   rootMessenger.delegate({
-    actions: [],
+    actions: [
+      'PhishingDataService:getStalelist',
+      'PhishingDataService:getHotlistDiffs',
+      'PhishingDataService:getC2DomainBlocklist',
+      'PhishingDataService:scanUrl',
+      'PhishingDataService:bulkScanUrls',
+      'PhishingDataService:bulkScanTokens',
+      'PhishingDataService:scanAddress',
+      'PhishingDataService:getApprovals',
+    ],
     events: ['TransactionController:stateChange'],
     messenger,
   });
@@ -105,21 +125,15 @@ describe('PhishingController - Bulk Token Scanning', () => {
     controller = getPhishingController();
     consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
     consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
-
-    // Reset the mock to its default behavior (pass through to real implementation)
-    mockSafelyExecuteWithTimeout.mockImplementation(
-      (fn, throwOnTimeout, timeout) => {
-        return jest
-          .requireActual('@metamask/controller-utils')
-          .safelyExecuteWithTimeout(fn, throwOnTimeout, timeout);
-      },
-    );
   });
 
   afterEach(() => {
     cleanAll();
     consoleErrorSpy.mockRestore();
     consoleWarnSpy.mockRestore();
+    while (createdDataServices.length > 0) {
+      createdDataServices.pop()?.destroy();
+    }
   });
 
   describe('bulkScanTokens', () => {
@@ -420,22 +434,31 @@ describe('PhishingController - Bulk Token Scanning', () => {
       });
 
       it('should handle API timeout and return empty results', async () => {
+        jest.useFakeTimers({
+          doNotFake: ['nextTick', 'queueMicrotask'],
+          now: 1_000_000,
+        });
         const tokens = ['0x1234567890123456789012345678901234567890'];
 
-        // Mock safelyExecuteWithTimeout to return null (simulating a timeout)
-        mockSafelyExecuteWithTimeout.mockResolvedValueOnce(null);
+        nock(SECURITY_ALERTS_BASE_URL)
+          .post(TOKEN_BULK_SCANNING_ENDPOINT)
+          .delayConnection(10000)
+          .reply(200, { results: {} });
 
         const request: BulkTokenScanRequest = {
           chainId: '0x1',
           tokens,
         };
 
-        const result = await controller.bulkScanTokens(request);
+        const promise = controller.bulkScanTokens(request);
+        jest.advanceTimersByTime(8000);
+        const result = await promise;
 
         expect(result).toStrictEqual({});
         expect(consoleErrorSpy).toHaveBeenCalledWith(
           'Error scanning tokens: timeout of 8000ms exceeded',
         );
+        jest.useRealTimers();
       });
     });