refactor: migrate PhishingController data fetching to PhishingDataService - #9914
Open
0xOhm wants to merge 34 commits into
Open
refactor: migrate PhishingController data fetching to PhishingDataService#99140xOhm wants to merge 34 commits into
0xOhm wants to merge 34 commits into
Conversation
…vice Extract all remote data fetching from PhishingController into a new PhishingDataService built on the BaseDataService pattern. Removes CacheManager in favor of the service's built-in caching, persistence, and request coalescing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Aug 20, 2026
Author
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
Resolve conflicts from base-data-service 1.0.0 and phishing-controller 17.4.0. - Bump `@metamask/base-data-service` to ^1.0.0 and `@tanstack/query-core` to ^5.62.16 to match main. query-core v4 had no lockfile resolution on main, which broke `yarn install --immutable` in CI. - Rename `cacheTime` to `gcTime` in `getApprovals` for the query-core v5 API. - Keep main's `getAddressScanSupportedChain` and `isAddressScanSupportedChainId`; drop `splitCacheHits`, whose `buildCacheKey` dependency this refactor removes and which was never exported or used. - Take main's `@metamask/transaction-controller` ^69.6.1 and move the ^69.5.2 bump entry under the released 17.4.0 changelog section.
Author
|
@metamaskbot publish-previews |
0xOhm
added a commit
to MetaMask/metamask-extension
that referenced
this pull request
Aug 26, 2026
…tion Resolve the migration-number collision and data-service registry conflict. - Renumber our PhishingController cache-removal migration from 222 to 224. Main shipped 222 (delete persisted EnsController state) and 223 (move StorageService data to IndexedDB) in the meantime, so 222 was taken. `oldVersion` in the test moves from 221 to 223 to match. - Register 223 and 224 in `migrations/index.js`. - `DATA_SERVICES` keeps main's `MoneyAccountBalanceService` and `MoneyAccountApiDataService` plus its `createUIQueryClient` comment, and adds `PhishingDataService`. Still pending, tracked separately: `@metamask/phishing-controller` is not bumped yet, so this cannot pass typecheck against the published package until MetaMask/core#9914 merges and releases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Aug 28, 2026
adonesky1
reviewed
Sep 1, 2026
adonesky1
left a comment
adonesky1
left a comment
Contributor
There was a problem hiding this comment.
I merged main into this branch to clear the conflicts (CHANGELOG, package.json, yarn.lock, all union resolutions). Worth knowing that #10007 landed on main while this was open and removed fetchQuery from every KycService POST for three reasons that all apply here: deduped in-flight writes, responses published on the messenger as cacheUpdated payloads, and the service policy retrying non-idempotent POSTs. Those are the same root cause behind the bulk dedupe and retry fan-out comments I left earlier.
Could we reconcile with that precedent, keeping fetchQuery for the GET list endpoints and scanUrl but calling #postJson directly for scanAddress and getApprovals? The bulk endpoints genuinely need per-item caching so they are a real exception, but it would help to say so explicitly in a comment.
Also note #6388 (C2 blocklist Array to Set) is now in the base. It only touched PhishingDetector.ts, which this PR does not modify, so the merge is clean there.
Bulk URL scanning: - Key bulk queries by the scan URL parameter rather than the bare hostname. Path-sensitive hosts (ipfs.io, github.io, and the other entries in PHISHING_DETECTION_PATH_BASED_ROOT_DOMAINS) collapsed into one cache entry per host, so only the first path in a batch was scanned and the rest inherited its verdict. - Return the results that did resolve when some lookups fail, reporting the failures per URL, instead of discarding the whole batch. A single failed lookup previously threw away fresh cached BLOCK verdicts for unrelated URLs. The call still rejects when nothing could be resolved at all. - Stop caching a "no result" verdict for URLs the API reported an error for. Those URLs were silently skipped for the following minute. - Report invalid URLs instead of collapsing them onto a shared empty key. Request policy: - Disable retries by default. The previous in-controller implementation made a single request per call and the controller's timeouts are sized for one attempt, so inheriting maxRetries: 3 meant a timeout could fire mid-retry. Retries also amplified badly through the batch loaders: a failed batch rejects every item query in it, and each retried on its own, turning one failed request into many single-item requests against a failing host. Caching: - Validate responses inside fetchQuery via responseStruct. Validating after the fact meant a malformed 200 was committed to the cache, and persisted, before it was rejected, so every caller for the next minute got the same error with no request made. - Set gcTime explicitly on scan queries. TanStack Query defaults gcTime to Infinity when it detects a server environment, which includes the MV3 service worker, so the cache would grow unbounded; the cache this replaces was explicitly size-bounded. - Do not retain fetched lists in the query cache. The controller keeps its own copy, and retaining them meant the multi-megabyte stalelist was rewritten to disk on every scan-triggered persist. - Do not route getApprovals through the query cache. It is never cached (staleTime and gcTime are both 0), so the cache only served to publish account-specific approval data on the messenger, matching #10007. Also corrects two changelog claims: PhishingDataService is not yet one of @metamask/wallet's default instances, so init is not automatic.
`responseStruct` requires a `Struct<Json>`, which these structs are not (`HotlistDiffsResponseStruct` infers `unknown[]`), so the build failed. Validating inside the query function achieves the same result: a malformed response throws before TanStack Query commits it, so it is never cached or persisted, and the existing error messages are preserved.
Contributor
|
@metamaskbot publish-previews |
Contributor
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
…ishing-data-service # Conflicts: # packages/phishing-controller/CHANGELOG.md # packages/phishing-controller/package.json # yarn.lock
adonesky1
marked this pull request as ready for review
September 9, 2026 20:36
adonesky1
enabled auto-merge
September 9, 2026 20:38
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a905f5c. Configure here.
mcmire
reviewed
Sep 10, 2026
mcmire
left a comment
mcmire
left a comment
Collaborator
There was a problem hiding this comment.
I have just started looking at this PR. Here are some comments for now. I will do another pass later.
| export const METAMASK_STALELIST_FILE = '/v1/stalelist'; | ||
| export const METAMASK_HOTLIST_DIFF_FILE = '/v2/diffsSince'; | ||
|
|
||
| export const CLIENT_SIDE_DETECION_BASE_URL = |
Collaborator
There was a problem hiding this comment.
Typo
Suggested change
| export const CLIENT_SIDE_DETECION_BASE_URL = | |
| export const CLIENT_SIDE_DETECTION_BASE_URL = |
| gcTime: LIST_GC_TIME, | ||
| }); | ||
|
|
||
| return jsonResponse as DataResultWrapper<PhishingStalelist>; |
Collaborator
There was a problem hiding this comment.
Why the typecast? Ideally this.#validate should return the correct type (as it's the one ensuring that the data matches a known type).
(edit: I'll try to come back to this and make a better suggestion here)
| - **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` ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - 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 ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `scanUrl` now reports the underlying error message in `fetchError` for network errors instead of `'timeout of 8000ms exceeded'` ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - Requests are no longer retried by default; the previous in-controller implementation made a single request per call, and the controller's timeouts are sized for one attempt. Pass `policyOptions.maxRetries` to opt back in ([#9914](https://github.com/MetaMask/core/pull/9914)) |
Collaborator
There was a problem hiding this comment.
Pass policyOptions.maxRetries to what? It would be helpful to more clear about which class is being changed here.
| - 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` ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - 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 ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `scanUrl` now reports the underlying error message in `fetchError` for network errors instead of `'timeout of 8000ms exceeded'` ([#9914](https://github.com/MetaMask/core/pull/9914)) |
Collaborator
There was a problem hiding this comment.
Should we clarify that it's scanUrl in PhishingController that's being changed?
| - `scanUrl` now reports the underlying error message in `fetchError` for network errors instead of `'timeout of 8000ms exceeded'` ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - Requests are no longer retried by default; the previous in-controller implementation made a single request per call, and the controller's timeouts are sized for one attempt. Pass `policyOptions.maxRetries` to opt back in ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - 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, and are not cached ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `bulkScanUrls` now returns the results it was able to resolve even if some lookups fail, reporting the failures per URL in `errors`; it only rejects when no result could be resolved at all. Previously a single failed lookup discarded every result in the batch, including cached `BLOCK` verdicts for unrelated URLs ([#9914](https://github.com/MetaMask/core/pull/9914)) |
Collaborator
There was a problem hiding this comment.
Using multiple sentences for a changelog entry is a sign that it's too long, can more details be given in a sub-bullet?
| - Requests are no longer retried by default; the previous in-controller implementation made a single request per call, and the controller's timeouts are sized for one attempt. Pass `policyOptions.maxRetries` to opt back in ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - 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, and are not cached ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `bulkScanUrls` now returns the results it was able to resolve even if some lookups fail, reporting the failures per URL in `errors`; it only rejects when no result could be resolved at all. Previously a single failed lookup discarded every result in the batch, including cached `BLOCK` verdicts for unrelated URLs ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `bulkScanUrls` no longer caches a "no result" verdict for URLs the API reported an error for, so those URLs are retried on the next call instead of being silently skipped for a minute ([#9914](https://github.com/MetaMask/core/pull/9914)) |
Collaborator
There was a problem hiding this comment.
Is a bug being fixed? If so should this be in the "Fixed" category?
| - Requests are no longer retried by default; the previous in-controller implementation made a single request per call, and the controller's timeouts are sized for one attempt. Pass `policyOptions.maxRetries` to opt back in ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - 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, and are not cached ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `bulkScanUrls` now returns the results it was able to resolve even if some lookups fail, reporting the failures per URL in `errors`; it only rejects when no result could be resolved at all. Previously a single failed lookup discarded every result in the batch, including cached `BLOCK` verdicts for unrelated URLs ([#9914](https://github.com/MetaMask/core/pull/9914)) | ||
| - `bulkScanUrls` no longer caches a "no result" verdict for URLs the API reported an error for, so those URLs are retried on the next call instead of being silently skipped for a minute ([#9914](https://github.com/MetaMask/core/pull/9914)) |
Collaborator
There was a problem hiding this comment.
Nit: Usually we note in the changelog when dependencies are added or updated. Can we do this for @metamask/base-data-service and @metamask/storage-service?
Suggested change
| - `bulkScanUrls` no longer caches a "no result" verdict for URLs the API reported an error for, so those URLs are retried on the next call instead of being silently skipped for a minute ([#9914](https://github.com/MetaMask/core/pull/9914)) | |
| - `bulkScanUrls` no longer caches a "no result" verdict for URLs the API reported an error for, so those URLs are retried on the next call instead of being silently skipped for a minute ([#9914](https://github.com/MetaMask/core/pull/9914)) | |
| - Add `@metamask/base-data-service` (`^2.0.0`) as a dependency ([#9914](https://github.com/MetaMask/core/pull/9914)) | |
| - Add `@metamask/base-data-service` (`^2.0.0`) as a dependency ([#9914](https://github.com/MetaMask/core/pull/9914)) |
| // Deliberately not routed through `fetchQuery`. Approvals reflect live, | ||
| // account-specific state that is never cached, so the query cache would | ||
| // provide no benefit while publishing the response on the messenger as a | ||
| // `cacheUpdated` payload. This matches the handling of non-cached POSTs |
Collaborator
There was a problem hiding this comment.
fetchQuery provides some other benefits, for instance, you can pass a responseStruct option to automatically validate the response. Could you pass staleTime: 0 to bypass the caching? I think this is usually the preferred approach in other packages.
Queries no longer wait indefinitely for StorageService:getItem after init(); the wait is bounded by a new hydrationTimeout (default 1s). Persisted caches are shape-validated before hydration and a shouldHydrateQuery filter lets services validate individual persisted queries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
applyDiffs previously indexed listSets with the diff's list type and threw a TypeError for any type it did not know, and the stricter hotlist struct in this branch turned the same case into a rejected hotlist response that left phishingLists empty. Unknown list types are now skipped per diff. Also adds normalizeScanAddress for lowercasing EVM addresses in scan keys. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A single unrecognized verdict or malformed nested field previously rejected the whole bulk URL, bulk token, or approvals response, dropping every BLOCK and Malicious verdict in the batch. Entries are now validated individually: malformed URL results are reported per URL, malformed token results and approvals are omitted. Hotlist diffs may target unknown list types and the C2 blocklist no longer requires lastFetchedAt, which the controller never reads. Bulk-seeded cache entries now carry a hostname so scanUrl always returns one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bounded rehydration wait added an asynchronous hop before every query even for services that never call init(). That hop delays request timers relative to callers' own timeouts; in the phishing controller's timeout tests the request outlived the test, was aborted during teardown, and TanStack then scheduled a real five-minute gcTime timer on the destroyed query, keeping the Jest process alive. Only await rehydration when an initialization promise exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Address lowercasing lived only in PhishingController, while the service's query keys became public API for UI consumers. A mixed-case token or address passed directly to the service missed the API's lowercase response keys and negatively cached a null verdict. The service now lowercases EVM addresses for both the request and the cache key; non-EVM addresses are unchanged. Also corrects the scanToken JSDoc, which claimed concurrent calls coalesce. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ed scan results With policyOptions.maxRetries enabled, each item query backed by a failed batch retried on its own and de-batched into single-item requests (10 URLs against a failing host produced 31 POSTs). The batch POST now runs under the service policy and item-level batch errors are excluded from retries, so a failed batch is retried intact. Persisted scan results are validated against their endpoint shapes before hydration; entries that fail, and any query other than a scan result, are discarded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mcmire
reviewed
Sep 10, 2026
mcmire
left a comment
mcmire
left a comment
Collaborator
There was a problem hiding this comment.
Still working through this. Left some more comments.
| }, | ||
| }, | ||
| }, | ||
| // Circuit breaking is disabled by default: this service talks to four |
Collaborator
There was a problem hiding this comment.
Have we considered creating four data services, one for each API? Would that it easier to finetune the behavior? Or is our goal here to mimic what the controller was doing before?
| * @param operation - The asynchronous operation to execute. | ||
| * @returns The operation result. | ||
| */ | ||
| protected async executeWithPolicy<Result>( |
Collaborator
There was a problem hiding this comment.
I wonder if it makes more sense to expose the policy property rather than wrapping policy.execute()? I recall data services from other teams where it seems like it would have been nice to have access to the policy object.
| init: RequestInit, | ||
| timeout?: number, | ||
| ): Promise<Json> { | ||
| const controller = new AbortController(); |
Collaborator
There was a problem hiding this comment.
We already have an AbortController kept under this.#abortController. Why are we creating another one?
| async load(key: string): Promise<Json | null> { | ||
| return new Promise((resolve, reject) => { | ||
| pending.push({ key, resolve, reject }); | ||
| // Items registered outside an explicit flush (e.g. by the retry |
Collaborator
There was a problem hiding this comment.
I don't understand why this is necessary, can you shine some light here?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Extracts all remote data fetching from
PhishingControllerinto a newPhishingDataServicebuilt on theBaseDataServicepattern (PSAFE-593).CacheManageris removed in favor of the service's built-in caching, persistence, and request coalescing.Changes
PhishingDataServiceowning all four remote APIs (stalelist/hotlist, request-blocklist/C2, dapp scanning, token/address scanning) with generated method action typesPhishingControllerslimmed to detection/state logic; queries data through the service via the messengerCacheManagerremoved — caching, persistence (StorageService), and request coalescing now come fromBaseDataServiceresult_typeasstring, not enum) to match pre-refactor behavior — production already returns values outside the documented enum (Verified)Verification
maxAge-expired verdicts discardedClient integration PRs
🤖 Generated with Claude Code
Note
High Risk
Breaking changes to phishing/security fetch and caching paths require client wiring and state migrations; incorrect integration could weaken or delay detection until lists and scans refresh.
Overview
Moves all phishing-related remote I/O out of
PhishingControllerinto a newPhishingDataServicebuilt onBaseDataService, so scans and list updates go through messenger actions, TanStack query caching, optionalStorageServicepersistence, and shared retry/circuit-breaker policy instead of in-controllerfetchandCacheManager.Breaking integration: clients must register
PhishingDataService, delegate its method actions on the controller messenger, callinitwhen persistence is enabled, and migrate persisted state by droppingurlScanCache,tokenScanCache, andaddressScanCacheplus the old scan-cache TTL/size options. Scan freshness is now driven by the service’s stale/GC times; list payloads still live in controller state.BaseDataServicegainsexecuteWithPolicy,hydrationTimeout/DEFAULT_HYDRATION_TIMEOUT, andshouldHydrateQuery, and queries afterinitwait for rehydration (with a timeout) while malformed persisted caches are removed instead of hydrated.Behavior tweaks include stricter response validation with per-item bulk error handling, partial bulk results on mixed success/failure, address normalization for cache keys, abort/timeouts on hung requests, and hotlist diffs for unknown list types are skipped rather than failing the whole update.
Reviewed by Cursor Bugbot for commit a9d3e6d. Bugbot is set up for automated code reviews on this repo. Configure here.