Conversation
Wire the new PhishingDataService messenger/init alongside PhishingController and add migration 222 to drop the controller's legacy scan caches now owned by the service. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
…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>
Migrations at or above MIGRATION_V2_START_VERSION (186) must mutate `versionedData` in place, record touched controllers in `changedControllers`, and return nothing. Migration 224 was written in the pre-186 style: it deep-cloned and returned new state. At runtime the migrator would have thrown "migration returned value when none expected", emitted an error, and broken out of the migration loop -- leaving the legacy scan caches in place and stopping every later migration. It also never reported `PhishingController` as changed, so the deletion would not have been persisted under split state. The unit test missed this because it called `migrate()` directly rather than through the Migrator. `app/scripts/lib/migrator/index.test.js`'s "should match the last version in live migrations" catches it, and now passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
0xOhm
changed the title
Contributor
✨ Files requiring CODEOWNER review ✨🫰 @MetaMask/core-platform (1 files, +2 -0)
👨🔧 @MetaMask/extension-platform (1 files, +2 -0)
|
Contributor
Builds ready [5739141]
⚡ Performance Benchmarks (Total: 🟢 16 pass · 🟡 0 warn · 🔴 0 fail)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Contributor
|
The UI read path doesn't look migrated here. Migration 224 deletes With the key gone, Mobile's counterpart already handles this. metamask-mobile#35057 drops the selector and reads the query cache instead: const { data: scanResult } = useQuery<PhishingDetectionScanResult>({
queryKey: ['PhishingDataService:scanUrl', hostname ?? ''],
enabled: Boolean(hostname),
});( Worth flagging that this is easy to miss in manual testing: Is the UI migration planned as a separate PR? |
Author
|
I have read the CLA Document and I hereby sign the CLA |
…ache Migration 224 deletes urlScanCache, tokenScanCache and addressScanCache from PhishingController state, but the UI still read the first two off state.metamask, so every origin and token resolved to Unknown. Point the trust signal hooks at the PhishingDataService query cache instead, matching metamask-mobile#35057: - useOriginTrustSignals now uses `useQuery` on ['PhishingDataService:scanUrl', <scan url param>]. The key uses getPhishingDetectionScanUrlParam rather than the bare hostname, so it is the same entry PhishingController.scanUrl primes from the background and it keeps working for path-based gateway hosts. - useTokenTrustSignalsForAddresses now uses `useQueries` on ['PhishingDataService:scanToken', chain, token], resolving the chain name and preserving address case on non-EVM chains, as the service does. Drop the selectors that read the removed state: getUrlScanCacheResult, getTokenScanCache and getTokenScanResultsForAddresses, plus ui/selectors/token-scan.ts, which had no production consumers. selectNonEvmTransactionsForActivity also read tokenScanCache to hide transactions carrying malicious tokens. Its cache keys never matched what MultichainAssetsController and TokenDataSource wrote (they call bulkScanTokens with chainId: chain.namespace, so entries were keyed `solana:<CasePreservedMint>`, while the selector built `solana:5eykt4un...:<lowercased mint>`), so the filter was already inert before this change. Remove the dead read; rewiring it against the query cache with correct keys is a follow-up.
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
2 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
Integrates the new
PhishingDataServicefrom MetaMask/core#9914 (PSAFE-593) alongsidePhishingController, and adds migration 224 to drop the controller's legacy scan caches now owned by the service.CHANGELOG entry: null
Changes
phishing-data-service-init+ messenger wiring inmessenger-client-initPhishingControllermessenger updated for the service-backed data flowshared/constants/data-services.tsregistersPhishingDataServicealongside the money-account servicesPhishingControllerstate (surgical + idempotent, covered by fixture tests)Dependency note — draft until core publishes
This PR intentionally does not bump
@metamask/phishing-controlleryet.maincurrently ships17.4.0, which does not containPhishingDataService; once MetaMask/core#9914 merges it will publish as a major (18.0.0) because of its breaking changes, and the bump lands here then. Verification below ran against the locally built package viafile:resolutions.Also worth noting for the bump: core's
phishing-controllernow requires@metamask/storage-service@^1.0.2(this repo declares^1.0.0, so the resolution needs to land at ≥ 1.0.2) and@tanstack/query-core@^5.62.16.Migration numbering
Originally authored as migration 222.
mainhas since shipped 222 (delete persistedEnsControllerstate) and 223 (move StorageService data to IndexedDB), so this is now 224.Migration 223 is compatible with this work: it relocates the backing store behind
StorageService, andPhishingDataServicepersistence goes through theStorageService:getItem/setItem/removeItemmessenger actions, so the change is transparent to it.Verification
migrations.test.js+lib/migrator/index.test.js— 13 passingyarn lint:tscclean against the new packageRe-verification pending
The integration/e2e results above predate the merge of
main(252 commits) and the core dependency bump. They need re-running against the published18.0.0once core releases.🤖 Generated with Claude Code