fix: auto-remove NFTs from state when backend reclassifies them as spam

packages/assets-controllers/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Changed
+
+- **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]
 
 ### Changed

packages/assets-controllers/src/NftDetectionController.test.ts

@@ -1135,6 +1135,211 @@
     );
   });
 
+  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,
+              tokenId: spamTokenId,
+              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,
+          'mainnet',
+          { 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 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: {
+          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 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';
@@ -1383,6 +1588,7 @@
     messenger: nftDetectionControllerMessenger,
     disabled: true,
     addNfts: jest.fn(),
+    removeNft: jest.fn(),
     getNftState: getDefaultNftControllerState,
     ...options,
   });

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<void>>;
@@ -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) => {