From 709cca42c5f718296090ce75b936121db4b5d65a Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Wed, 18 Mar 2026 18:46:21 +0100 Subject: [PATCH 1/2] fix: remove SPAM NFTs --- packages/assets-controllers/CHANGELOG.md | 4 + .../src/NftDetectionController.test.ts | 143 ++++++++++++++++++ .../src/NftDetectionController.ts | 40 +++++ 3 files changed, 187 insertions(+) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index fe89887921e..94be5696123 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Added + +- `NftDetectionController` now accepts a `removeNft` callback (matching `NftController['removeNft']`) and automatically removes NFTs from state during detection when the backend marks them as spam (`isSpam: true`) + ## [101.0.1] ### Changed diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index bdc5704c44e..8adc5d4f5bb 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -1135,6 +1135,148 @@ describe('NftDetectionController', () => { ); }); + it('should remove NFTs from state when the API now marks them as spam', async () => { + const mockAddNfts = jest.fn(); + const mockRemoveNft = jest.fn(); + const selectedAddress = '0xSpamDetect'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + + const spamContract = '0xSpamContract'; + const spamTokenId = '42'; + + nock(NFT_API_BASE_URL) + .get( + `/users/${selectedAddress}/tokens?chainIds=1&limit=50&includeTopBid=true&continuation=`, + ) + .reply(200, { + tokens: [ + { + token: { + chainId: 1, + contract: spamContract, + kind: 'erc721', + name: 'Spam NFT', + description: 'This NFT is now spam', + image: 'image/spam.png', + tokenId: spamTokenId, + metadata: { + imageOriginal: 'imageOriginal/spam.png', + imageMimeType: 'image/png', + tokenURI: 'tokenURITest', + }, + isSpam: true, + }, + }, + ], + }); + + const mockGetNftState = jest.fn().mockReturnValue({ + ...getDefaultNftControllerState(), + allNfts: { + [selectedAddress]: { + '0x1': [ + { + address: spamContract, + tokenId: spamTokenId, + name: 'Spam NFT', + favorite: false, + isCurrentlyOwned: true, + }, + ], + }, + }, + }); + + await withController( + { + options: { + addNfts: mockAddNfts, + removeNft: mockRemoveNft, + getNftState: mockGetNftState, + }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + + await jestAdvanceTime({ duration: 1 }); + mockAddNfts.mockReset(); + mockRemoveNft.mockReset(); + + await controller.detectNfts(['0x1']); + + expect(mockRemoveNft).toHaveBeenCalledWith( + spamContract, + spamTokenId, + expect.any(String), + { userAddress: selectedAddress }, + ); + expect(mockAddNfts).toHaveBeenCalledWith([], selectedAddress, Source.Detected); + }, + ); + }); + + it('should not remove NFTs from state when spam NFT does not exist in state', async () => { + const mockAddNfts = jest.fn(); + const mockRemoveNft = jest.fn(); + const selectedAddress = '0xSpamDetectNoState'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + + nock(NFT_API_BASE_URL) + .get( + `/users/${selectedAddress}/tokens?chainIds=1&limit=50&includeTopBid=true&continuation=`, + ) + .reply(200, { + tokens: [ + { + token: { + chainId: 1, + contract: '0xSpamContract', + kind: 'erc721', + name: 'Spam NFT', + tokenId: '42', + isSpam: true, + }, + }, + ], + }); + + await withController( + { + options: { + addNfts: mockAddNfts, + removeNft: mockRemoveNft, + getNftState: getDefaultNftControllerState, + }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + + await jestAdvanceTime({ duration: 1 }); + mockRemoveNft.mockReset(); + + await controller.detectNfts(['0x1']); + + expect(mockRemoveNft).not.toHaveBeenCalled(); + }, + ); + }); + it('should stop pagination when signal is aborted', async () => { const mockAddNfts = jest.fn(); const selectedAddress = '0xAbortSignal'; @@ -1383,6 +1525,7 @@ async function withController( messenger: nftDetectionControllerMessenger, disabled: true, addNfts: jest.fn(), + removeNft: jest.fn(), getNftState: getDefaultNftControllerState, ...options, }); diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 8ce888e501d..8a6f593099a 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -12,6 +12,7 @@ import { NFT_API_VERSION, convertHexToDecimal, handleFetch, + toHex, } from '@metamask/controller-utils'; import type { Messenger } from '@metamask/messenger'; import type { @@ -423,6 +424,8 @@ export class NftDetectionController extends BaseController< readonly #addNfts: NftController['addNfts']; + readonly #removeNft: NftController['removeNft']; + readonly #getNftState: () => NftControllerState; #inProcessNftFetchingUpdates: Record<`${string}:${string}`, Promise>; @@ -434,17 +437,20 @@ export class NftDetectionController extends BaseController< * @param options.messenger - A reference to the messaging system. * @param options.disabled - Represents previous value of useNftDetection. Used to detect changes of useNftDetection. Default value is true. * @param options.addNfts - Add multiple NFTs. + * @param options.removeNft - Remove a single NFT. * @param options.getNftState - Gets the current state of the Assets controller. */ constructor({ messenger, disabled = false, addNfts, + removeNft, getNftState, }: { messenger: NftDetectionControllerMessenger; disabled: boolean; addNfts: NftController['addNfts']; + removeNft: NftController['removeNft']; getNftState: () => NftControllerState; }) { super({ @@ -458,6 +464,7 @@ export class NftDetectionController extends BaseController< this.#getNftState = getNftState; this.#addNfts = addNfts; + this.#removeNft = removeNft; this.messenger.subscribe( 'PreferencesController:stateChange', @@ -615,6 +622,39 @@ export class NftDetectionController extends BaseController< : true), ); + // Remove NFTs that the API now marks as spam but are still in state + const spamNfts = resultNftApi.tokens.filter( + (elm) => elm.token.isSpam === true, + ); + for (const { token } of spamNfts) { + const { contract, tokenId, chainId } = token; + if (!chainId) { + continue; + } + const hexChainId = toHex(chainId); + const { allNfts } = this.#getNftState(); + const nftsForChain = allNfts[userAddress]?.[hexChainId] ?? []; + const checksumAddress = toChecksumHexAddress(contract); + const existsInState = nftsForChain.some( + (nft) => + nft.address.toLowerCase() === checksumAddress.toLowerCase() && + nft.tokenId === tokenId, + ); + if (existsInState) { + try { + const networkClientId = this.messenger.call( + 'NetworkController:findNetworkClientIdByChainId', + hexChainId, + ); + this.#removeNft(contract, tokenId, networkClientId, { + userAddress, + }); + } catch { + // Network may not be configured; skip this NFT + } + } + } + // Proceed to add NFTs const nftsToAdd = apiNfts .map((nft) => { From 707ea934d0bbbc905321544dff9ce9d99a84d98b Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Wed, 18 Mar 2026 18:56:02 +0100 Subject: [PATCH 2/2] chore: minor changes --- packages/assets-controllers/CHANGELOG.md | 5 +- .../src/NftDetectionController.test.ts | 87 ++++++++++++++++--- 2 files changed, 78 insertions(+), 14 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 94be5696123..a4df0c7b8d0 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -7,9 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Added +### Changed -- `NftDetectionController` now accepts a `removeNft` callback (matching `NftController['removeNft']`) and automatically removes NFTs from state during detection when the backend marks them as spam (`isSpam: true`) +- **BREAKING:** `NftDetectionController` constructor now requires a `removeNft` option (type `NftController['removeNft']`); during detection, any NFT already in state that the API now returns as spam (`isSpam: true`) is automatically removed ([#8237](https://github.com/MetaMask/core/pull/8237)) + - Callers must pass `removeNft: nftController.removeNft.bind(nftController)` (or equivalent) when constructing `NftDetectionController` ## [101.0.1] diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index 8adc5d4f5bb..696f3c8fcb9 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -1157,16 +1157,7 @@ describe('NftDetectionController', () => { token: { chainId: 1, contract: spamContract, - kind: 'erc721', - name: 'Spam NFT', - description: 'This NFT is now spam', - image: 'image/spam.png', tokenId: spamTokenId, - metadata: { - imageOriginal: 'imageOriginal/spam.png', - imageMimeType: 'image/png', - tokenURI: 'tokenURITest', - }, isSpam: true, }, }, @@ -1215,7 +1206,7 @@ describe('NftDetectionController', () => { expect(mockRemoveNft).toHaveBeenCalledWith( spamContract, spamTokenId, - expect.any(String), + 'mainnet', { userAddress: selectedAddress }, ); expect(mockAddNfts).toHaveBeenCalledWith([], selectedAddress, Source.Detected); @@ -1224,7 +1215,6 @@ describe('NftDetectionController', () => { }); it('should not remove NFTs from state when spam NFT does not exist in state', async () => { - const mockAddNfts = jest.fn(); const mockRemoveNft = jest.fn(); const selectedAddress = '0xSpamDetectNoState'; const selectedAccount = createMockInternalAccount({ @@ -1254,7 +1244,6 @@ describe('NftDetectionController', () => { await withController( { options: { - addNfts: mockAddNfts, removeNft: mockRemoveNft, getNftState: getDefaultNftControllerState, }, @@ -1277,6 +1266,80 @@ describe('NftDetectionController', () => { ); }); + it('should not remove spam NFT when the network client cannot be resolved for its chain', async () => { + const mockRemoveNft = jest.fn(); + const selectedAddress = '0xSpamUnknownChain'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + + const spamContract = '0xSpamContractUnknownChain'; + const spamTokenId = '99'; + + // chainId 0xdead is not registered in the default mock, so findNetworkClientIdByChainId + // will throw, and the NFT removal should be silently skipped. + nock(NFT_API_BASE_URL) + .get( + `/users/${selectedAddress}/tokens?chainIds=57005&limit=50&includeTopBid=true&continuation=`, + ) + .reply(200, { + tokens: [ + { + token: { + chainId: 57005, + contract: spamContract, + kind: 'erc721', + name: 'Spam NFT on Unknown Chain', + tokenId: spamTokenId, + isSpam: true, + }, + }, + ], + }); + + const mockGetNftState = jest.fn().mockReturnValue({ + ...getDefaultNftControllerState(), + allNfts: { + [selectedAddress]: { + '0xdead': [ + { + address: spamContract, + tokenId: spamTokenId, + name: 'Spam NFT on Unknown Chain', + favorite: false, + isCurrentlyOwned: true, + }, + ], + }, + }, + }); + + await withController( + { + options: { + removeNft: mockRemoveNft, + getNftState: mockGetNftState, + }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + + await jestAdvanceTime({ duration: 1 }); + mockRemoveNft.mockReset(); + + await controller.detectNfts(['0xdead']); + + expect(mockRemoveNft).not.toHaveBeenCalled(); + }, + ); + }); + it('should stop pagination when signal is aborted', async () => { const mockAddNfts = jest.fn(); const selectedAddress = '0xAbortSignal';