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
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
| 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)
Contributor
There was a problem hiding this comment.
The casts come from fetchQuery's TQueryFnData extends Json bound: superstruct optional() infers T | undefined, which isn't Json, and responseStruct needs a Struct<Json> for the same reason, so it can't take these structs without casting the raw response first. For the two structs without optionals (stalelist and address scan) the validated type satisfies the bound on its own, so I dropped the casts there in d04bcaa. Removing the rest means relaxing that bound in base-data-service; I'd do that as a follow-up unless you want it here.
Collaborator
There was a problem hiding this comment.
Gotcha. Yeah, we'll have to figure out a way to relax the types in BaseDataService. That's clearly out of scope here so these typecasts are fine.
Member
There was a problem hiding this comment.
exactOptional may be what we are looking for here to be JSON compatible?
Collaborator
There was a problem hiding this comment.
I revisited this and came to a different conclusion as you @adonesky1. I don't think it's due to the types in fetchQuery. I left some more comments here: #9914 (comment)
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.
| 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?
Contributor
There was a problem hiding this comment.
fetch only takes one cancel signal, but a request can be cancelled for up to three reasons: TanStack cancels that query (single-item requests only; batched requests aren't tied to one query), the service is destroyed (that's this.#abortController), or the request's own timeout fires. The per-request controller merges those into the one signal fetch accepts, and keeps the timeout separate so the error can say it was a timeout. Comment added in 8d5a1c9 and cff89ad.
To be clear this isn't shape I think we want long term. AbortSignal.any would be the one-liner but mobile's RN polyfill doesn't have it (its shim.js has to back-fill even AbortSignal.timeout), so it's the portable option for now. The better home is BaseDataService: hand each query function a signal covering both the query and the service's teardown, with an optional timeout built on cockatiel's timeout policy, which already models this and throws a typed TaskCancelledError. That would delete this wiring here and give every subclass the same guarantees. Happy to write that up as a follow-up if you agree with the direction.
Collaborator
There was a problem hiding this comment.
Oh okay. So it sounds like one AbortController is service-level (and gets triggered when the service is destroyed) and the other is request-level (and may get triggered by TanStack Query or manually)? I'm asking because I'm wondering if this is a pattern that we need to port to BaseDataService.
…response casts getApprovals now goes through fetchQuery with staleTime and gcTime of 0, the same pattern KycService, ShieldApiService, and SubscriptionService use for uncached reads. The precedent in #10007 bypasses fetchQuery for writes, not for POST-shaped reads, so the direct executeWithPolicy path was a special case with no benefit. getStalelist and scanAddress no longer cast through Json: their structs have no optional fields, so the validated type satisfies fetchQuery's Json bound and flows through as the return type. The remaining casts are forced by optional() fields inferring T | undefined, which fetchQuery's TQueryFnData bound rejects; relaxing that lives in base-data-service. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fold new-service behavior into the PhishingDataService entry, move fixes of released behavior to Fixed, list the added dependencies, name the class and method in ambiguous entries, and drop entries that only fixed iterations of this branch that never shipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # packages/base-data-service/CHANGELOG.md # packages/phishing-controller/CHANGELOG.md
Replace the protected executeWithPolicy helper (introduced on this branch) with a protected policy getter. Subclasses can still run uncached requests under the shared retry and circuit-breaker policy, and can now also observe its onBreak, onDegraded, and onRetry events, which hand-rolled services elsewhere in the monorepo forward but BaseDataService subclasses could not reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each request combines the query's TanStack signal, the service-wide destroy signal, and its own timeout into the single signal that fetch accepts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The batch loader had two ways to send a batch: an explicit flush() called by each bulk method, and a microtask fallback originally added for retried item queries. Item queries are no longer retried, and once a client has called init() every lookup runs after the explicit flush anyway, so the deferred flush was already doing all the work in that configuration. Remove the explicit flush and its API; every lookup made in the same turn is sent in one batch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each query used to start its own hydration deadline. While storage hung, every query paid the full timeout, and because the per-query timers fire as separate macrotasks the item lookups of one bulk scan resumed in different turns and were sent as single-item requests. One shared deadline per service means all waiters resume together, batches stay intact, and no query waits again once the deadline has passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ishing-data-service
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 dfa5fd2. Configure here.
The new @metamask/utils dependency was missing from the package tsconfigs and README graph, which failed lint:tsconfigs and readme-content:check.
mcmire
reviewed
Sep 14, 2026
| timestamp: number; | ||
| }; | ||
|
|
||
| const PersistedCacheStruct = objectType({ |
Collaborator
There was a problem hiding this comment.
What do you think about grouping this consts with the others and moving it below DEFAULT_HYDRATION_TIMEOUT?
Collaborator
There was a problem hiding this comment.
Alternatively, now that we have this struct, it's a bit confusing to have this + PersistedCache. Usually we derive types from structs, but in this case, if we were to derive a type from PersistedCacheStruct and call it PersistedCache, it would be too broad.
Maybe we don't need this struct after all? (also see below)
| * | ||
| * @param initialization - The pending rehydration. | ||
| */ | ||
| async #waitForInitialization(initialization: Promise<void>): Promise<void> { |
Collaborator
There was a problem hiding this comment.
What are your thoughts on baking this into init itself? That way we only need one property to hold the initialization promise (this.#initializationPromise) and not two.
| return; | ||
| } | ||
|
|
||
| if (!is(untypedCache, PersistedCacheStruct)) { |
Collaborator
There was a problem hiding this comment.
Considering that PersistedCacheStruct and PersistedCache are similarly named, it might be tempting to make use of the struct to more idiomatically assign a type to cache rather than using a type assertion, e.g.:
const [error, cache] = validate(untypedCache, PersistedCacheStruct);
if (error) {
await this.#externalMessenger.call(
'StorageService:removeItem',
this.name,
STORAGE_SERVICE_KEY,
);
return;
}However, this would not work, because PersistedCacheStruct is more loosely defined than PersistedCache.
Maybe using a struct here is too heavy-handed and we can get away with simple checks?
Suggested change
| if (!is(untypedCache, PersistedCacheStruct)) { | |
| if ( | |
| !( | |
| isPlainObject(untypedCache) && | |
| hasProperty(untypedCache, 'timestamp') && | |
| typeof untypedCache.timestamp === 'number' && | |
| hasProperty(untypedCache, 'state') && | |
| isPlainObject(untypedCache.state) && | |
| hasProperty(untypedCache.state, 'queries') && | |
| Array.isArray(untypedCache.state.queries) && | |
| hasProperty(untypedCache.state, 'mutations') && | |
| Array.isArray(untypedCache.state.mutations) | |
| ) | |
| ) { |
We can also move this to an isPersistedCache function if it's too much to see here.
What do you think?
| * | ||
| * @returns The service policy. | ||
| */ | ||
| protected get policy(): ServicePolicy { |
Collaborator
There was a problem hiding this comment.
Is it worth adding a comment explaining why this is a getter?
Suggested change
| protected get policy(): ServicePolicy { | |
| // A getter so that `policy` cannot be overwritten in a JavaScript project. | |
| protected get policy(): ServicePolicy { |
| response: unknown, | ||
| struct: Struct<Type, Schema>, | ||
| endpointName: string, | ||
| ): Infer<Struct<Type, Schema>> { |
Collaborator
There was a problem hiding this comment.
Would this also work?
Suggested change
| ): Infer<Struct<Type, Schema>> { | |
| ): Type { |
Comment on lines
+1128
to
+1136
| } as Json; | ||
| }, | ||
| // Live account state: always refetch and evict as soon as the call | ||
| // settles, as other data services do for uncached reads. | ||
| staleTime: 0, | ||
| gcTime: 0, | ||
| }); | ||
|
|
||
| return jsonResponse as ApprovalsResponse; |
Collaborator
There was a problem hiding this comment.
Similar as above:
Suggested change
| } as Json; | |
| }, | |
| // Live account state: always refetch and evict as soon as the call | |
| // settles, as other data services do for uncached reads. | |
| staleTime: 0, | |
| gcTime: 0, | |
| }); | |
| return jsonResponse as ApprovalsResponse; | |
| }; | |
| }, | |
| // Live account state: always refetch and evict as soon as the call | |
| // settles, as other data services do for uncached reads. | |
| staleTime: 0, | |
| gcTime: 0, | |
| }); | |
| return jsonResponse; |
| init: RequestInit, | ||
| timeout?: number, | ||
| ): Promise<Json> { | ||
| const controller = new AbortController(); |
Collaborator
There was a problem hiding this comment.
Oh okay. So it sounds like one AbortController is service-level (and gets triggered when the service is destroyed) and the other is request-level (and may get triggered by TanStack Query or manually)? I'm asking because I'm wondering if this is a pattern that we need to port to BaseDataService.
| */ | ||
| async #toJson(response: Response): Promise<Json> { | ||
| if (!response.ok) { | ||
| throw new HttpError( |
Collaborator
There was a problem hiding this comment.
Nit: The HTTP status does not seem to correlate to converting the response to JSON. Maybe it's worth it to bake this into #fetchJson instead?
| // cache rehydration) joins the same batch. | ||
| if (!flushScheduled) { | ||
| flushScheduled = true; | ||
| queueMicrotask(flush); |
Collaborator
There was a problem hiding this comment.
This whole function looks similar to reduceInBatchesSerially from assets-controller:
However, this part is a bit different. Do we need to schedule the flush as a microtask because requests go through a query client? assets-controller also uses a query client under the hood, but it doesn't need to do this. So I'm wondering what the differences are between the approach we're taking here and the approach that is being taken in assets-controller to batch requests and whether the complexity is worth it.
You can see where it's being used here for instance:
(Asking because I'm wondering if this is a pattern we need to add to base-data-service, so I want to know the problems we're solving and what the best approach is.)
| // Persisted scan results are validated before they can be served | ||
| // from the cache; a caller-provided filter is applied on top. | ||
| shouldHydrateQuery: (query): boolean => | ||
| isValidPersistedScanQuery(query) && |
Collaborator
There was a problem hiding this comment.
What is this line connected to? Is it the fact we are batching requests or is it the fact that we are validating requests inside of the queryFn?
Comment on lines
+206
to
+208
| #initializationPromise?: Promise<void>; | ||
|
|
||
| #boundedInitialization?: Promise<void>; |
Member
There was a problem hiding this comment.
None of the changes to BaseDataService should be done in this PR, they should be opened as a separate PR IMO.
| return; | ||
| } | ||
|
|
||
| if (!is(untypedCache, PersistedCacheStruct)) { |
Member
There was a problem hiding this comment.
What is this trying to guard against?
| responseStruct?: TDataStruct; | ||
| }): Promise<TData> { | ||
| if (this.#initializationPromise) { | ||
| await this.#waitForInitialization(this.#initializationPromise); |
Member
There was a problem hiding this comment.
What's the use-case here? The thinking originally was that persisted state would be loaded during init anyways before queries start to fire.
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
4 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
Large breaking refactor of security-sensitive phishing and scan networking, new required wiring and state migrations, plus changed caching/persistence semantics for scan verdicts.
Overview
Introduces
PhishingDataServiceon top ofBaseDataServiceso stalelist/hotlist, C2 blocklist, URL/token/address scans, and approvals go through messenger actions with TanStack query caching, optionalStorageServicepersistence, and batched bulk scans.PhishingControllerkeeps list/detection logic only and calls the service; the in-controllerCacheManagerand persistedurlScanCache/tokenScanCache/addressScanCachestate (plus related constructor options) are removed.BaseDataServicegains bounded wait for cache rehydration afterinit(hydrationTimeout,DEFAULT_HYDRATION_TIMEOUT), optionalshouldHydrateQuery, persisted-cache shape validation with discard-on-failure, and a protectedpolicygetter for subclasses.Behavior fixes bundled with the move: partial
bulkScanUrls/ bulk token results instead of failing the whole batch, skip unrecognized hotlist diff list types, negative caching for tokens with no API row, stricter rejection of malformed responses (not cached), and transaction-driven token scans coalesced per chain.Breaking for integrators: register
PhishingDataService, delegate its actions, callinitwhen persistence is enabled, and migrate persisted controller state away from the removed scan-cache fields.Reviewed by Cursor Bugbot for commit c0b3196. Bugbot is set up for automated code reviews on this repo. Configure here.