From 9db40dcd035a865847e552217f941ce644ae3881 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 12:01:59 +0100 Subject: [PATCH 01/17] chore: reduce state writes from O(n) to O(1) for NFT detection --- .../assets-controllers/src/NftController.ts | 471 ++++++++++-------- 1 file changed, 275 insertions(+), 196 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 10f5e28bc83..14edb501410 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -266,6 +266,22 @@ type NftAsset = { tokenId: string; }; +type NftContractToAdd = { + networkClientId: NetworkClientId; + tokenAddress: string; + nftMetadata: NftMetadata; + source: Source; +}; + +type NftToAdd = { + tokenAddress: string; + tokenId: string; + nftMetadata: NftMetadata; + nftContract: NftContract; + chainId: Hex; + source: Source; +}; + /** * The name of the {@link NftController}. */ @@ -898,92 +914,112 @@ export class NftController extends BaseController< } /** - * Adds an individual NFT to the stored NFT list. + * Adds multiple NFTs to the stored NFT list for a given user. * - * @param tokenAddress - Hex address of the NFT contract. - * @param tokenId - The NFT identifier. - * @param nftMetadata - NFT optional information (name, image and description). - * @param nftContract - An object containing contract data of the NFT being added. - * @param chainId - The chainId of the network where the NFT is being added. - * @param userAddress - The address of the account where the NFT is being added. - * @param source - Whether the NFT was detected, added manually or suggested by a dapp. - * @returns A promise resolving to `undefined`. + * @param userAddress - The address of the account where the NFTs are being added. + * @param nfts - Array of NFT objects to add. + * @param nfts[].tokenAddress - Hex address of the NFT contract. + * @param nfts[].tokenId - The NFT identifier. + * @param nfts[].nftMetadata - NFT optional information (name, image and description). + * @param nfts[].nftContract - An object containing contract data of the NFT being added. + * @param nfts[].chainId - The chainId of the network where the NFT is being added. + * @param nfts[].source - Whether the NFT was detected, added manually or suggested by a dapp. + * @returns A promise that resolves when all NFTs have been added or updated. */ - async #addIndividualNft( - tokenAddress: string, - tokenId: string, - nftMetadata: NftMetadata, - nftContract: NftContract, - chainId: Hex, - userAddress: string, - source: Source, - ): Promise { + async #addMultipleNfts(userAddress: string, nfts: NftToAdd[]): Promise { const releaseLock = await this.#mutex.acquire(); try { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); const { allNfts } = this.state; + const allNftsForUser = allNfts[userAddress] || {}; + const allNftsForUserPerChain: { + [chainId: `0x${string}`]: Nft[]; + } = {}; + const modifiedChainIds = new Set(); + + for (const { + tokenAddress, + tokenId, + nftMetadata, + nftContract, + chainId, + source, + } of nfts) { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); - const nfts = [...(allNfts[userAddress]?.[chainId] ?? [])]; - - const existingEntry = nfts.find( - (nft) => - nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && - nft.tokenId === tokenId, - ); - - if (existingEntry) { - const differentMetadata = compareNftMetadata( - nftMetadata, - existingEntry, - ); - - const hasNewFields = hasNewCollectionFields(nftMetadata, existingEntry); - - if ( - !differentMetadata && - existingEntry.isCurrentlyOwned && - !hasNewFields - ) { - return; + if (!allNftsForUserPerChain[chainId]) { + allNftsForUserPerChain[chainId] = [ + ...(allNftsForUser?.[chainId] ?? []), + ]; } - const indexToUpdate = nfts.findIndex( + const existingEntry = allNftsForUserPerChain[chainId].find( (nft) => nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, ); - if (indexToUpdate !== -1) { - nfts[indexToUpdate] = { - ...existingEntry, + if (existingEntry) { + const differentMetadata = compareNftMetadata( + nftMetadata, + existingEntry, + ); + + const hasNewFields = hasNewCollectionFields( + nftMetadata, + existingEntry, + ); + + if ( + !differentMetadata && + existingEntry.isCurrentlyOwned && + !hasNewFields + ) { + continue; + } + + const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( + (nft) => + nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && + nft.tokenId === tokenId, + ); + + if (indexToUpdate !== -1) { + allNftsForUserPerChain[chainId][indexToUpdate] = { + ...existingEntry, + ...nftMetadata, + }; + } + } else { + const newEntry: Nft = { + address: checksumHexAddress, + tokenId, + favorite: false, + isCurrentlyOwned: true, ...nftMetadata, }; + + allNftsForUserPerChain[chainId].push(newEntry); } - } else { - const newEntry: Nft = { - address: checksumHexAddress, - tokenId, - favorite: false, - isCurrentlyOwned: true, - ...nftMetadata, - }; - nfts.push(newEntry); - } + modifiedChainIds.add(chainId); - this.#updateNestedNftState(nfts, ALL_NFTS_STATE_KEY, { - chainId, - userAddress, - }); + if (this.#onNftAdded) { + this.#onNftAdded({ + address: checksumHexAddress, + symbol: nftContract.symbol, + tokenId: tokenId.toString(), + standard: nftMetadata.standard, + source, + }); + } + } - if (this.#onNftAdded) { - this.#onNftAdded({ - address: checksumHexAddress, - symbol: nftContract.symbol, - tokenId: tokenId.toString(), - standard: nftMetadata.standard, - source, - }); + for (const chainId of modifiedChainIds) { + this.#updateNestedNftState( + allNftsForUserPerChain[chainId], + ALL_NFTS_STATE_KEY, + { chainId, userAddress }, + ); } } finally { releaseLock(); @@ -991,112 +1027,133 @@ export class NftController extends BaseController< } /** - * Adds an NFT contract to the stored NFT contracts list. + * Adds multiple NFT contracts to the stored NFT contracts list for a given user. * - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @param options - options. - * @param options.tokenAddress - Hex address of the NFT contract. - * @param options.userAddress - The address of the account where the NFT is being added. - * @param options.nftMetadata - The retrieved NFTMetadata from API. - * @param options.source - Whether the NFT was detected, added manually or suggested by a dapp. - * @returns Promise resolving to the current NFT contracts list. + * @param userAddress - The address of the account where the NFT contracts are being added. + * @param contracts - Array of contract objects to add. + * @param contracts[].networkClientId - The networkClientId used to identify the network client for the request. + * @param contracts[].tokenAddress - Hex address of the NFT contract. + * @param contracts[].nftMetadata - The retrieved NFT metadata from the API. + * @param contracts[].source - Whether the NFT was detected, added manually or suggested by a dapp. + * @returns Promise resolving to an object mapping chainIds to their updated NFT contract arrays. */ - async #addNftContract( - networkClientId: NetworkClientId, - { - tokenAddress, - userAddress, - source, - nftMetadata, - }: { - tokenAddress: string; - userAddress: string; - nftMetadata: NftMetadata; - source?: Source; - }, - ): Promise { + async #addNftContracts( + userAddress: string, + contracts: NftContractToAdd[], + ): Promise<{ [chainId: `0x${string}`]: NftContract[] }> { const releaseLock = await this.#mutex.acquire(); try { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); const { allNftContracts } = this.state; - const { - configuration: { chainId }, - } = this.messenger.call( - 'NetworkController:getNetworkClientById', + const allNftContractsForUser = allNftContracts[userAddress] || {}; + const nftContractsForUserPerChain: { + [chainId: `0x${string}`]: NftContract[]; + } = {}; + const modifiedChainIds = new Set(); + + for (const { networkClientId, - ); + tokenAddress, + source, + nftMetadata, + } of contracts) { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); + const { + configuration: { chainId }, + } = this.messenger.call( + 'NetworkController:getNetworkClientById', + networkClientId, + ); - const nftContracts = allNftContracts[userAddress]?.[chainId] || []; + if (!nftContractsForUserPerChain[chainId]) { + nftContractsForUserPerChain[chainId] = [ + ...(allNftContractsForUser?.[chainId] ?? []), + ]; + } - const existingEntry = nftContracts.find( - (nftContract) => - nftContract.address.toLowerCase() === - checksumHexAddress.toLowerCase(), - ); - if (existingEntry) { - return nftContracts; - } + const existingEntry = nftContractsForUserPerChain[chainId].find( + (nftContract) => + nftContract.address.toLowerCase() === + checksumHexAddress.toLowerCase(), + ); - // this doesn't work currently for detection if the user switches networks while the detection is processing - // will be fixed once detection uses networkClientIds - // get name and symbol if ERC721 then put together the metadata - const contractInformation = await this.#getNftContractInformation( - checksumHexAddress, - nftMetadata, - networkClientId, - ); - const { - asset_contract_type, - created_date, - symbol, - description, - external_link, - schema_name, - collection: { name, image_url, tokenCount }, - } = contractInformation; - - // If the nft is auto-detected we want some valid metadata to be present - if ( - source === Source.Detected && - 'address' in contractInformation && - typeof contractInformation.address === 'string' && - 'collection' in contractInformation && - contractInformation.collection.name === null && - 'image_url' in contractInformation.collection && - contractInformation.collection.image_url === null && - Object.entries(contractInformation).every(([key, value]) => { - return key === 'address' || key === 'collection' || !value; - }) - ) { - return nftContracts; + if (existingEntry) { + continue; // TODO juan check if we need to update the contract + } + + let contractInformation; + try { + // this doesn't work currently for detection if the user switches networks while the detection is processing + // will be fixed once detection uses networkClientIds + // get name and symbol if ERC721 then put together the metadata + contractInformation = await this.#getNftContractInformation( + checksumHexAddress, + nftMetadata, + networkClientId, + ); + } catch (error) { + console.error( + `NftController: Failed to fetch contract information for ${checksumHexAddress}`, + error, + ); + continue; + } + + // If the nft is auto-detected we want some valid metadata to be present + if ( + source === Source.Detected && + 'address' in contractInformation && + typeof contractInformation.address === 'string' && + 'collection' in contractInformation && + contractInformation.collection.name === null && + 'image_url' in contractInformation.collection && + contractInformation.collection.image_url === null && + Object.entries(contractInformation).every(([key, value]) => { + return key === 'address' || key === 'collection' || !value; + }) + ) { + continue; + } + + const { + asset_contract_type, + created_date, + symbol, + description, + external_link, + schema_name, + collection: { name, image_url, tokenCount }, + } = contractInformation; + + /* istanbul ignore next */ + const newEntry: NftContract = Object.assign( + {}, + { address: checksumHexAddress }, + description && { description }, + name && { name }, + image_url && { logo: image_url }, + symbol && { symbol }, + tokenCount !== null && + typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, + asset_contract_type && { assetContractType: asset_contract_type }, + created_date && { createdDate: created_date }, + schema_name && { schemaName: schema_name }, + external_link && { externalLink: external_link }, + ); + + nftContractsForUserPerChain[chainId].push(newEntry); + modifiedChainIds.add(chainId); } - /* istanbul ignore next */ - const newEntry: NftContract = Object.assign( - {}, - { address: checksumHexAddress }, - description && { description }, - name && { name }, - image_url && { logo: image_url }, - symbol && { symbol }, - tokenCount !== null && - typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, - asset_contract_type && { assetContractType: asset_contract_type }, - created_date && { createdDate: created_date }, - schema_name && { schemaName: schema_name }, - external_link && { externalLink: external_link }, - ); - const newNftContracts = [...nftContracts, newEntry]; - this.#updateNestedNftState( - newNftContracts, - ALL_NFTS_CONTRACTS_STATE_KEY, - { - chainId, - userAddress, - }, - ); + // Loops once per chain (not once per NFT contract) + for (const chainId of modifiedChainIds) { + this.#updateNestedNftState( + nftContractsForUserPerChain[chainId], + ALL_NFTS_CONTRACTS_STATE_KEY, + { chainId, userAddress }, + ); + } - return newNftContracts; + return nftContractsForUserPerChain; } finally { releaseLock(); } @@ -1472,24 +1529,30 @@ export class NftController extends BaseController< nftMetadata = await this.#sanitizeNftMetadata(nftMetadata); } - const newNftContracts = await this.#addNftContract(networkClientId, { - tokenAddress: checksumHexAddress, - userAddress: addressToSearch, - source, - nftMetadata, - }); + const newNftContracts = await this.#addNftContracts(addressToSearch, [ + { + tokenAddress: checksumHexAddress, + networkClientId, + source, + nftMetadata, + }, + ]); // If NFT contract was not added, do not add individual NFT - const nftContract = newNftContracts.find( - (contract) => - contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), - ); + const nftContract = Object.values(newNftContracts) + .flat() + .find( + (contract) => + contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), + ); + const { configuration: { chainId }, } = this.messenger.call( 'NetworkController:getNetworkClientById', networkClientId, ); + // This is the case when the NFT is added manually and not detected automatically // TODO: An improvement would be to make the chainId a required field and return it when getting the NFT information if (!nftMetadata.chainId) { @@ -1498,15 +1561,16 @@ export class NftController extends BaseController< // If NFT contract information, add individual NFT if (nftContract) { - await this.#addIndividualNft( - checksumHexAddress, - tokenId, - nftMetadata, - nftContract, - chainId, - addressToSearch, - source, - ); + await this.#addMultipleNfts(addressToSearch, [ + { + tokenAddress: checksumHexAddress, + tokenId, + nftMetadata, + nftContract, + chainId, + source, + }, + ]); } } @@ -1530,6 +1594,7 @@ export class NftController extends BaseController< userAddress: string, source: Source = Source.Custom, ): Promise { + // Make sure the user address is valid const addressToSearch = this.#getAddressOrSelectedAddress(userAddress); if (!addressToSearch) { return; @@ -1540,6 +1605,12 @@ export class NftController extends BaseController< nfts.map((nft) => nft.nftMetadata), ); + // Loop through each NFT and add the NFT contract to the list + const nftContractsToAdd: (NftContractToAdd & { + chainId: Hex; + tokenId: string; + })[] = []; + for (const [index, nft] of nfts.entries()) { const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); @@ -1550,32 +1621,40 @@ export class NftController extends BaseController< hexChainId, ); - const newNftContracts = await this.#addNftContract(networkClientId, { + nftContractsToAdd.push({ + networkClientId, tokenAddress: checksumHexAddress, - userAddress: addressToSearch, source, nftMetadata: sanitizedNftMetadata[index], + chainId: hexChainId, + tokenId: nft.tokenId, }); + } + + // Add the NFT contracts to state + const newNftContracts = await this.#addNftContracts( + addressToSearch, + nftContractsToAdd, + ); + + const nftsToAdd: NftToAdd[] = []; - // If NFT contract was not added, do not add individual NFT - const nftContract = newNftContracts.find( + nftContractsToAdd.forEach((nftContractToAdd) => { + const nftContract = newNftContracts[nftContractToAdd.chainId].find( (contract) => - contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), + contract.address.toLowerCase() === + nftContractToAdd.tokenAddress.toLowerCase(), ); - - // If NFT contract information, add individual NFT if (nftContract) { - await this.#addIndividualNft( - checksumHexAddress, - nft.tokenId, - sanitizedNftMetadata[index], + nftsToAdd.push({ + ...nftContractToAdd, nftContract, - hexChainId, - addressToSearch, - source, - ); + }); } - } + }); + + // If NFT contract information, add individual NFT + await this.#addMultipleNfts(addressToSearch, nftsToAdd); } /** From 6030ed9bd0bb5636ab92a8e99920ca85f49eff0e Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 12:14:54 +0100 Subject: [PATCH 02/17] chore: minor changes --- packages/assets-controllers/src/NftController.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 14edb501410..3075bb61f17 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -1559,7 +1559,6 @@ export class NftController extends BaseController< nftMetadata.chainId = convertHexToDecimal(chainId); } - // If NFT contract information, add individual NFT if (nftContract) { await this.#addMultipleNfts(addressToSearch, [ { @@ -1653,8 +1652,9 @@ export class NftController extends BaseController< } }); - // If NFT contract information, add individual NFT - await this.#addMultipleNfts(addressToSearch, nftsToAdd); + if (nftsToAdd.length > 0) { + await this.#addMultipleNfts(addressToSearch, nftsToAdd); + } } /** From aba2797906d4c3a7e6c2d1eefb206deab7dc641e Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 12:23:43 +0100 Subject: [PATCH 03/17] chore: trycatch in loop --- .../assets-controllers/src/NftController.ts | 253 +++++++++--------- 1 file changed, 129 insertions(+), 124 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 3075bb61f17..fae695ed747 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -944,73 +944,80 @@ export class NftController extends BaseController< chainId, source, } of nfts) { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); - - if (!allNftsForUserPerChain[chainId]) { - allNftsForUserPerChain[chainId] = [ - ...(allNftsForUser?.[chainId] ?? []), - ]; - } - - const existingEntry = allNftsForUserPerChain[chainId].find( - (nft) => - nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && - nft.tokenId === tokenId, - ); - - if (existingEntry) { - const differentMetadata = compareNftMetadata( - nftMetadata, - existingEntry, - ); - - const hasNewFields = hasNewCollectionFields( - nftMetadata, - existingEntry, - ); + try { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); - if ( - !differentMetadata && - existingEntry.isCurrentlyOwned && - !hasNewFields - ) { - continue; + if (!allNftsForUserPerChain[chainId]) { + allNftsForUserPerChain[chainId] = [ + ...(allNftsForUser?.[chainId] ?? []), + ]; } - const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( + const existingEntry = allNftsForUserPerChain[chainId].find( (nft) => nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, ); - if (indexToUpdate !== -1) { - allNftsForUserPerChain[chainId][indexToUpdate] = { - ...existingEntry, + if (existingEntry) { + const differentMetadata = compareNftMetadata( + nftMetadata, + existingEntry, + ); + + const hasNewFields = hasNewCollectionFields( + nftMetadata, + existingEntry, + ); + + if ( + !differentMetadata && + existingEntry.isCurrentlyOwned && + !hasNewFields + ) { + continue; + } + + const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( + (nft) => + nft.address.toLowerCase() === + checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, + ); + + if (indexToUpdate !== -1) { + allNftsForUserPerChain[chainId][indexToUpdate] = { + ...existingEntry, + ...nftMetadata, + }; + } + } else { + const newEntry: Nft = { + address: checksumHexAddress, + tokenId, + favorite: false, + isCurrentlyOwned: true, ...nftMetadata, }; - } - } else { - const newEntry: Nft = { - address: checksumHexAddress, - tokenId, - favorite: false, - isCurrentlyOwned: true, - ...nftMetadata, - }; - allNftsForUserPerChain[chainId].push(newEntry); - } + allNftsForUserPerChain[chainId].push(newEntry); + } - modifiedChainIds.add(chainId); + modifiedChainIds.add(chainId); - if (this.#onNftAdded) { - this.#onNftAdded({ - address: checksumHexAddress, - symbol: nftContract.symbol, - tokenId: tokenId.toString(), - standard: nftMetadata.standard, - source, - }); + if (this.#onNftAdded) { + this.#onNftAdded({ + address: checksumHexAddress, + symbol: nftContract.symbol, + tokenId: tokenId.toString(), + standard: nftMetadata.standard, + source, + }); + } + } catch (error) { + console.error( + `NftController: Failed to add NFT ${tokenAddress} #${tokenId}`, + error, + ); } } @@ -1056,92 +1063,90 @@ export class NftController extends BaseController< source, nftMetadata, } of contracts) { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); - const { - configuration: { chainId }, - } = this.messenger.call( - 'NetworkController:getNetworkClientById', - networkClientId, - ); + try { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); + const { + configuration: { chainId }, + } = this.messenger.call( + 'NetworkController:getNetworkClientById', + networkClientId, + ); - if (!nftContractsForUserPerChain[chainId]) { - nftContractsForUserPerChain[chainId] = [ - ...(allNftContractsForUser?.[chainId] ?? []), - ]; - } + if (!nftContractsForUserPerChain[chainId]) { + nftContractsForUserPerChain[chainId] = [ + ...(allNftContractsForUser?.[chainId] ?? []), + ]; + } - const existingEntry = nftContractsForUserPerChain[chainId].find( - (nftContract) => - nftContract.address.toLowerCase() === - checksumHexAddress.toLowerCase(), - ); + const existingEntry = nftContractsForUserPerChain[chainId].find( + (nftContract) => + nftContract.address.toLowerCase() === + checksumHexAddress.toLowerCase(), + ); - if (existingEntry) { - continue; // TODO juan check if we need to update the contract - } + if (existingEntry) { + continue; // TODO juan check if we need to update the contract + } - let contractInformation; - try { // this doesn't work currently for detection if the user switches networks while the detection is processing // will be fixed once detection uses networkClientIds // get name and symbol if ERC721 then put together the metadata - contractInformation = await this.#getNftContractInformation( + const contractInformation = await this.#getNftContractInformation( checksumHexAddress, nftMetadata, networkClientId, ); + + // If the nft is auto-detected we want some valid metadata to be present + if ( + source === Source.Detected && + 'address' in contractInformation && + typeof contractInformation.address === 'string' && + 'collection' in contractInformation && + contractInformation.collection.name === null && + 'image_url' in contractInformation.collection && + contractInformation.collection.image_url === null && + Object.entries(contractInformation).every(([key, value]) => { + return key === 'address' || key === 'collection' || !value; + }) + ) { + continue; + } + + const { + asset_contract_type, + created_date, + symbol, + description, + external_link, + schema_name, + collection: { name, image_url, tokenCount }, + } = contractInformation; + + /* istanbul ignore next */ + const newEntry: NftContract = Object.assign( + {}, + { address: checksumHexAddress }, + description && { description }, + name && { name }, + image_url && { logo: image_url }, + symbol && { symbol }, + tokenCount !== null && + typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, + asset_contract_type && { assetContractType: asset_contract_type }, + created_date && { createdDate: created_date }, + schema_name && { schemaName: schema_name }, + external_link && { externalLink: external_link }, + ); + + nftContractsForUserPerChain[chainId].push(newEntry); + modifiedChainIds.add(chainId); } catch (error) { console.error( - `NftController: Failed to fetch contract information for ${checksumHexAddress}`, + `NftController: Failed to add NFT contract for ${tokenAddress}`, error, ); - continue; } - - // If the nft is auto-detected we want some valid metadata to be present - if ( - source === Source.Detected && - 'address' in contractInformation && - typeof contractInformation.address === 'string' && - 'collection' in contractInformation && - contractInformation.collection.name === null && - 'image_url' in contractInformation.collection && - contractInformation.collection.image_url === null && - Object.entries(contractInformation).every(([key, value]) => { - return key === 'address' || key === 'collection' || !value; - }) - ) { - continue; - } - - const { - asset_contract_type, - created_date, - symbol, - description, - external_link, - schema_name, - collection: { name, image_url, tokenCount }, - } = contractInformation; - - /* istanbul ignore next */ - const newEntry: NftContract = Object.assign( - {}, - { address: checksumHexAddress }, - description && { description }, - name && { name }, - image_url && { logo: image_url }, - symbol && { symbol }, - tokenCount !== null && - typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, - asset_contract_type && { assetContractType: asset_contract_type }, - created_date && { createdDate: created_date }, - schema_name && { schemaName: schema_name }, - external_link && { externalLink: external_link }, - ); - - nftContractsForUserPerChain[chainId].push(newEntry); - modifiedChainIds.add(chainId); } // Loops once per chain (not once per NFT contract) From 6c0d2338859df077c5d5ff95527d071ea32d2f0a Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 12:39:47 +0100 Subject: [PATCH 04/17] fix: minor bug --- packages/assets-controllers/src/NftController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index fae695ed747..c538447a112 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -1644,7 +1644,7 @@ export class NftController extends BaseController< const nftsToAdd: NftToAdd[] = []; nftContractsToAdd.forEach((nftContractToAdd) => { - const nftContract = newNftContracts[nftContractToAdd.chainId].find( + const nftContract = newNftContracts[nftContractToAdd.chainId]?.find( (contract) => contract.address.toLowerCase() === nftContractToAdd.tokenAddress.toLowerCase(), From ab57c024ef9786b1d576bcd85fd42fd30520e0a9 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 2 Mar 2026 12:05:42 +0000 Subject: [PATCH 05/17] chore: update assets-controllers changelog for NFT detection optimization Co-authored-by: Juanmi --- packages/assets-controllers/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index de936af7df4..e4f23e3450a 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Optimize NFT Controller state writes from O(n) to O(1) for NFT detection by batching contract and NFT additions ([#8079](https://github.com/MetaMask/core/pull/8079)) - Bump `@metamask/transaction-controller` from `^62.18.0` to `^62.19.0` ([#8031](https://github.com/MetaMask/core/pull/8031)) ### Fixed From c443bd7820c852098c27822d572cc02fe897f6cb Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 15:56:11 +0100 Subject: [PATCH 06/17] chore: minor change --- packages/assets-controllers/src/NftController.ts | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index c538447a112..ba2bfae163c 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -935,6 +935,13 @@ export class NftController extends BaseController< [chainId: `0x${string}`]: Nft[]; } = {}; const modifiedChainIds = new Set(); + const pendingCallbacks: { + address: string; + symbol: string | undefined; + tokenId: string; + standard: string | null; + source: Source; + }[] = []; for (const { tokenAddress, @@ -1005,7 +1012,7 @@ export class NftController extends BaseController< modifiedChainIds.add(chainId); if (this.#onNftAdded) { - this.#onNftAdded({ + pendingCallbacks.push({ address: checksumHexAddress, symbol: nftContract.symbol, tokenId: tokenId.toString(), @@ -1028,6 +1035,10 @@ export class NftController extends BaseController< { chainId, userAddress }, ); } + + for (const callbackData of pendingCallbacks) { + this.#onNftAdded?.(callbackData); + } } finally { releaseLock(); } From 97ea630699edee31a73006f59c06cd7d34ed165f Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 15:57:44 +0100 Subject: [PATCH 07/17] chore: update changelog --- packages/assets-controllers/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index e4f23e3450a..b2a7566c122 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- Optimize NFT Controller state writes from O(n) to O(1) for NFT detection by batching contract and NFT additions ([#8079](https://github.com/MetaMask/core/pull/8079)) +- Optimize NFT Controller state writes from O(2n) to O(2) for NFT detection by batching contract and NFT additions ([#8079](https://github.com/MetaMask/core/pull/8079)) - Bump `@metamask/transaction-controller` from `^62.18.0` to `^62.19.0` ([#8031](https://github.com/MetaMask/core/pull/8031)) ### Fixed From 78d4cb2e9b44168ac14a5f7e80b7bebb69c49c6f Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 2 Mar 2026 16:54:34 +0100 Subject: [PATCH 08/17] chore: throw errro when there is a real error --- .../assets-controllers/src/NftController.ts | 74 +++++++++++++------ 1 file changed, 51 insertions(+), 23 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index ba2bfae163c..2c19b65121a 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -926,7 +926,10 @@ export class NftController extends BaseController< * @param nfts[].source - Whether the NFT was detected, added manually or suggested by a dapp. * @returns A promise that resolves when all NFTs have been added or updated. */ - async #addMultipleNfts(userAddress: string, nfts: NftToAdd[]): Promise { + async #addMultipleNfts( + userAddress: string, + nfts: NftToAdd[], + ): Promise<{ errors: Error[] }> { const releaseLock = await this.#mutex.acquire(); try { const { allNfts } = this.state; @@ -935,6 +938,7 @@ export class NftController extends BaseController< [chainId: `0x${string}`]: Nft[]; } = {}; const modifiedChainIds = new Set(); + const errors: Error[] = []; const pendingCallbacks: { address: string; symbol: string | undefined; @@ -1025,6 +1029,9 @@ export class NftController extends BaseController< `NftController: Failed to add NFT ${tokenAddress} #${tokenId}`, error, ); + errors.push( + error instanceof Error ? error : new Error(String(error)), + ); } } @@ -1039,6 +1046,8 @@ export class NftController extends BaseController< for (const callbackData of pendingCallbacks) { this.#onNftAdded?.(callbackData); } + + return { errors }; } finally { releaseLock(); } @@ -1058,7 +1067,10 @@ export class NftController extends BaseController< async #addNftContracts( userAddress: string, contracts: NftContractToAdd[], - ): Promise<{ [chainId: `0x${string}`]: NftContract[] }> { + ): Promise<{ + contracts: { [chainId: `0x${string}`]: NftContract[] }; + errors: Error[]; + }> { const releaseLock = await this.#mutex.acquire(); try { const { allNftContracts } = this.state; @@ -1067,6 +1079,7 @@ export class NftController extends BaseController< [chainId: `0x${string}`]: NftContract[]; } = {}; const modifiedChainIds = new Set(); + const errors: Error[] = []; for (const { networkClientId, @@ -1096,7 +1109,7 @@ export class NftController extends BaseController< ); if (existingEntry) { - continue; // TODO juan check if we need to update the contract + continue; } // this doesn't work currently for detection if the user switches networks while the detection is processing @@ -1157,6 +1170,9 @@ export class NftController extends BaseController< `NftController: Failed to add NFT contract for ${tokenAddress}`, error, ); + errors.push( + error instanceof Error ? error : new Error(String(error)), + ); } } @@ -1169,7 +1185,7 @@ export class NftController extends BaseController< ); } - return nftContractsForUserPerChain; + return { contracts: nftContractsForUserPerChain, errors }; } finally { releaseLock(); } @@ -1545,14 +1561,19 @@ export class NftController extends BaseController< nftMetadata = await this.#sanitizeNftMetadata(nftMetadata); } - const newNftContracts = await this.#addNftContracts(addressToSearch, [ - { - tokenAddress: checksumHexAddress, - networkClientId, - source, - nftMetadata, - }, - ]); + const { contracts: newNftContracts, errors: contractErrors } = + await this.#addNftContracts(addressToSearch, [ + { + tokenAddress: checksumHexAddress, + networkClientId, + source, + nftMetadata, + }, + ]); + + if (contractErrors.length > 0) { + throw contractErrors[0]; + } // If NFT contract was not added, do not add individual NFT const nftContract = Object.values(newNftContracts) @@ -1576,16 +1597,23 @@ export class NftController extends BaseController< } if (nftContract) { - await this.#addMultipleNfts(addressToSearch, [ - { - tokenAddress: checksumHexAddress, - tokenId, - nftMetadata, - nftContract, - chainId, - source, - }, - ]); + const { errors: nftErrors } = await this.#addMultipleNfts( + addressToSearch, + [ + { + tokenAddress: checksumHexAddress, + tokenId, + nftMetadata, + nftContract, + chainId, + source, + }, + ], + ); + + if (nftErrors.length > 0) { + throw nftErrors[0]; + } } } @@ -1647,7 +1675,7 @@ export class NftController extends BaseController< } // Add the NFT contracts to state - const newNftContracts = await this.#addNftContracts( + const { contracts: newNftContracts } = await this.#addNftContracts( addressToSearch, nftContractsToAdd, ); From 700a43e27b89e2bc4da867009ad13bd8188d7062 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Tue, 3 Mar 2026 12:28:52 +0100 Subject: [PATCH 09/17] chore: avoid making unecessary api calls to RPC for name and symbol if it has already been returned by API provider --- .../assets-controllers/src/NftController.ts | 91 +++++++++---------- 1 file changed, 43 insertions(+), 48 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 2c19b65121a..3e398df6488 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -819,48 +819,17 @@ export class NftController extends BaseController< } /** - * Request NFT contract information from the contract itself. + * Request NFT contract information, merging on-chain data with metadata + * already received from the NFT API. * - * @param contractAddress - Hex address of the NFT contract. - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @returns Promise resolving to the current NFT name and image. - */ - async #getNftContractInformationFromContract( - // TODO for calls to blockchain we need to explicitly pass the currentNetworkClientId since its relying on the provider - contractAddress: string, - networkClientId: NetworkClientId, - ): Promise< - Partial & - Pick & - Pick - > { - const [name, symbol] = await Promise.all([ - this.messenger.call( - 'AssetsContractController:getERC721AssetName', - contractAddress, - networkClientId, - ), - this.messenger.call( - 'AssetsContractController:getERC721AssetSymbol', - contractAddress, - networkClientId, - ), - ]); - - return { - collection: { name }, - symbol, - address: contractAddress, - }; - } - - /** - * Request NFT contract information from Blockchain and aggregate with received data from NFTMetadata. + * Each field (name, symbol) is fetched from the chain independently: if the + * API already supplied a value for a field, the RPC call for that field is + * skipped. Both calls are issued in parallel when both are needed. * * @param contractAddress - Hex address of the NFT contract. - * @param nftMetadataFromApi - Received NFT information to be aggregated with blockchain contract information. - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @returns Promise resolving to the NFT contract name, image and description. + * @param nftMetadataFromApi - NFT information received from the API. + * @param networkClientId - Network client to use for any on-chain calls. + * @returns Promise resolving to the aggregated NFT contract information. */ async #getNftContractInformation( contractAddress: string, @@ -871,21 +840,47 @@ export class NftController extends BaseController< Pick & Pick > { - const blockchainContractData = await safelyExecute(() => - this.#getNftContractInformationFromContract( - contractAddress, - networkClientId, - ), - ); + const apiName = nftMetadataFromApi.collection?.name; + const apiSymbol = nftMetadataFromApi.collection?.symbol; + + const needsOnChainName = apiName === null || apiName === undefined; + const needsOnChainSymbol = apiSymbol === null || apiSymbol === undefined; + + // TODO for calls to blockchain we need to explicitly pass the + // currentNetworkClientId since its relying on the provider + const [onChainName, onChainSymbol] = await Promise.all([ + needsOnChainName + ? safelyExecute(() => + this.messenger.call( + 'AssetsContractController:getERC721AssetName', + contractAddress, + networkClientId, + ), + ) + : Promise.resolve(undefined), + needsOnChainSymbol + ? safelyExecute(() => + this.messenger.call( + 'AssetsContractController:getERC721AssetSymbol', + contractAddress, + networkClientId, + ), + ) + : Promise.resolve(undefined), + ]); + + const name = apiName ?? onChainName; + const symbol = apiSymbol ?? onChainSymbol; if ( - blockchainContractData || + name !== undefined || + symbol !== undefined || !Object.values(nftMetadataFromApi).every((value) => value === null) ) { return { address: contractAddress, - ...blockchainContractData, schema_name: nftMetadataFromApi?.standard ?? null, + ...(symbol !== undefined && { symbol }), collection: { name: null, image_url: @@ -894,7 +889,7 @@ export class NftController extends BaseController< null, tokenCount: nftMetadataFromApi?.collection?.tokenCount ?? null, ...nftMetadataFromApi?.collection, - ...blockchainContractData?.collection, + ...(name !== undefined && { name }), }, }; } From b79a09df64397fa621caf28d021f3204e7601dfc Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 12:32:37 +0100 Subject: [PATCH 10/17] chore: avoid getting nftcontractinfo from rpc --- .../assets-controllers/src/NftController.ts | 48 +++---------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 3e398df6488..e39a009eaad 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -828,49 +828,16 @@ export class NftController extends BaseController< * * @param contractAddress - Hex address of the NFT contract. * @param nftMetadataFromApi - NFT information received from the API. - * @param networkClientId - Network client to use for any on-chain calls. * @returns Promise resolving to the aggregated NFT contract information. */ - async #getNftContractInformation( + #getNftContractInformation( contractAddress: string, nftMetadataFromApi: NftMetadata, - networkClientId: NetworkClientId, - ): Promise< - Partial & - Pick & - Pick - > { - const apiName = nftMetadataFromApi.collection?.name; - const apiSymbol = nftMetadataFromApi.collection?.symbol; - - const needsOnChainName = apiName === null || apiName === undefined; - const needsOnChainSymbol = apiSymbol === null || apiSymbol === undefined; - - // TODO for calls to blockchain we need to explicitly pass the - // currentNetworkClientId since its relying on the provider - const [onChainName, onChainSymbol] = await Promise.all([ - needsOnChainName - ? safelyExecute(() => - this.messenger.call( - 'AssetsContractController:getERC721AssetName', - contractAddress, - networkClientId, - ), - ) - : Promise.resolve(undefined), - needsOnChainSymbol - ? safelyExecute(() => - this.messenger.call( - 'AssetsContractController:getERC721AssetSymbol', - contractAddress, - networkClientId, - ), - ) - : Promise.resolve(undefined), - ]); - - const name = apiName ?? onChainName; - const symbol = apiSymbol ?? onChainSymbol; + ): Partial & + Pick & + Pick { + const name = nftMetadataFromApi.collection?.name; + const symbol = nftMetadataFromApi.collection?.symbol; if ( name !== undefined || @@ -1110,10 +1077,9 @@ export class NftController extends BaseController< // this doesn't work currently for detection if the user switches networks while the detection is processing // will be fixed once detection uses networkClientIds // get name and symbol if ERC721 then put together the metadata - const contractInformation = await this.#getNftContractInformation( + const contractInformation = this.#getNftContractInformation( checksumHexAddress, nftMetadata, - networkClientId, ); // If the nft is auto-detected we want some valid metadata to be present From 1990baa7b62b2f0119006a866c5864210d204611 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 12:40:38 +0100 Subject: [PATCH 11/17] chore: update changelog --- packages/assets-controllers/CHANGELOG.md | 1 + packages/assets-controllers/src/NftController.ts | 13 +++---------- 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index 4008c099c05..e682251d06a 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Optimize NFT Controller state writes from O(2n) to O(2) for NFT detection by batching contract and NFT additions ([#8079](https://github.com/MetaMask/core/pull/8079)) +- Remove on-chain ERC-721 RPC fallback for contract name and symbol in `NftController`; contract metadata is now sourced exclusively from the NFT API, eliminating `getERC721AssetName`/`getERC721AssetSymbol` calls during NFT detection ([#8079](https://github.com/MetaMask/core/pull/8079)) - When `isHomepageSectionsV1Enabled` is true, `AccountTrackerController` now uses all popular EVM networks (via `NetworkEnablementController:listPopularEvmNetworks`) for balance refresh on account change and keyring unlock, instead of only the enabled networks from `NetworkEnablementController` state ([#8117](https://github.com/MetaMask/core/pull/8117)) ### Fixed diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index e39a009eaad..c5df09ac73b 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -819,16 +819,12 @@ export class NftController extends BaseController< } /** - * Request NFT contract information, merging on-chain data with metadata - * already received from the NFT API. - * - * Each field (name, symbol) is fetched from the chain independently: if the - * API already supplied a value for a field, the RPC call for that field is - * skipped. Both calls are issued in parallel when both are needed. + * Builds NFT contract information from metadata already received from the + * NFT API. No on-chain RPC calls are made. * * @param contractAddress - Hex address of the NFT contract. * @param nftMetadataFromApi - NFT information received from the API. - * @returns Promise resolving to the aggregated NFT contract information. + * @returns The aggregated NFT contract information. */ #getNftContractInformation( contractAddress: string, @@ -1598,7 +1594,6 @@ export class NftController extends BaseController< userAddress: string, source: Source = Source.Custom, ): Promise { - // Make sure the user address is valid const addressToSearch = this.#getAddressOrSelectedAddress(userAddress); if (!addressToSearch) { return; @@ -1609,7 +1604,6 @@ export class NftController extends BaseController< nfts.map((nft) => nft.nftMetadata), ); - // Loop through each NFT and add the NFT contract to the list const nftContractsToAdd: (NftContractToAdd & { chainId: Hex; tokenId: string; @@ -1635,7 +1629,6 @@ export class NftController extends BaseController< }); } - // Add the NFT contracts to state const { contracts: newNftContracts } = await this.#addNftContracts( addressToSearch, nftContractsToAdd, From 4a80e58ec46c49c74628ff3764c552605dfaa884 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 13:26:52 +0100 Subject: [PATCH 12/17] chore: minor changes impementation --- packages/assets-controllers/CHANGELOG.md | 2 +- .../src/NftController.test.ts | 80 +--- .../assets-controllers/src/NftController.ts | 352 ++++++++---------- 3 files changed, 174 insertions(+), 260 deletions(-) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index e682251d06a..536efc7f74f 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -14,7 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Optimize NFT Controller state writes from O(2n) to O(2) for NFT detection by batching contract and NFT additions ([#8079](https://github.com/MetaMask/core/pull/8079)) -- Remove on-chain ERC-721 RPC fallback for contract name and symbol in `NftController`; contract metadata is now sourced exclusively from the NFT API, eliminating `getERC721AssetName`/`getERC721AssetSymbol` calls during NFT detection ([#8079](https://github.com/MetaMask/core/pull/8079)) +- Remove on-chain ERC-721 RPC fallback for contract name and symbol in `NftController`; contract metadata is now sourced exclusively from the NFT API, eliminating `getERC721AssetName`/`getERC721AssetSymbol` calls during NFT detection. NFT contracts that previously had `name` or `symbol` populated via on-chain calls will no longer have those fields if the NFT API does not supply them ([#8079](https://github.com/MetaMask/core/pull/8079)) - When `isHomepageSectionsV1Enabled` is true, `AccountTrackerController` now uses all popular EVM networks (via `NetworkEnablementController:listPopularEvmNetworks`) for balance refresh on account change and keyring unlock, instead of only the enabled networks from `NetworkEnablementController` state ([#8117](https://github.com/MetaMask/core/pull/8117)) ### Fixed diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index d78ec771e4c..fdedbebaccf 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -772,7 +772,7 @@ describe('NftController', () => { .mockResolvedValueOnce({}) // 5. `AccountsController:getAccount` .mockReturnValueOnce(OWNER_ACCOUNT) - // 3. `NetworkClientController:getNetworkClientById` + // 6. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -785,11 +785,7 @@ describe('NftController', () => { }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any) - // 6. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 7. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 3. `NetworkClientController:getNetworkClientById` + // 7. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -809,7 +805,7 @@ describe('NftController', () => { 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); + expect(callActionSpy).toHaveBeenCalledTimes(8); expect(callActionSpy).toHaveBeenNthCalledWith( 5, 'ApprovalController:addRequest', @@ -911,11 +907,7 @@ describe('NftController', () => { }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 10. `NetworkClientController:getNetworkClientById` + // 8. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -935,7 +927,7 @@ describe('NftController', () => { 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); + expect(callActionSpy).toHaveBeenCalledTimes(8); expect(callActionSpy).toHaveBeenNthCalledWith( 5, 'ApprovalController:addRequest', @@ -1037,11 +1029,7 @@ describe('NftController', () => { }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 10. `NetworkClientController:getNetworkClientById` + // 8. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -1061,7 +1049,7 @@ describe('NftController', () => { 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); + expect(callActionSpy).toHaveBeenCalledTimes(8); expect(callActionSpy).toHaveBeenNthCalledWith( 5, 'ApprovalController:addRequest', @@ -1164,11 +1152,7 @@ describe('NftController', () => { }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 10. `NetworkClientController:getNetworkClientById` + // 8. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -1188,7 +1172,7 @@ describe('NftController', () => { 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); + expect(callActionSpy).toHaveBeenCalledTimes(8); expect(callActionSpy).toHaveBeenNthCalledWith( 5, 'ApprovalController:addRequest', @@ -1299,11 +1283,7 @@ describe('NftController', () => { }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any) - // 10. `AssetsContractController:getERC721AssetName` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 11. `AssetsContractController:getERC721AssetSymbol` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 12. `NetworkClientController:getNetworkClientById` + // 10. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -1323,7 +1303,7 @@ describe('NftController', () => { 'https://etherscan.io', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(12); + expect(callActionSpy).toHaveBeenCalledTimes(10); expect(callActionSpy).toHaveBeenNthCalledWith( 7, 'ApprovalController:addRequest', @@ -1431,11 +1411,7 @@ describe('NftController', () => { }, // eslint-disable-next-line @typescript-eslint/no-explicit-any } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 9. `NetworkClientController:getNetworkClientById` + // 8. `NetworkClientController:getNetworkClientById` .mockReturnValueOnce({ configuration: { type: 'infura', @@ -1456,7 +1432,7 @@ describe('NftController', () => { 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(12); + expect(callActionSpy).toHaveBeenCalledTimes(10); expect(callActionSpy).toHaveBeenNthCalledWith( 7, 'ApprovalController:addRequest', @@ -1726,7 +1702,6 @@ describe('NftController', () => { it('should add the nft contract to the correct chain in state when source is detected', async () => { const { nftController } = setupController({ options: {}, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), }); await nftController.addNft('0x01', '1', 'mainnet', { @@ -1752,7 +1727,6 @@ describe('NftController', () => { ).toStrictEqual({ address: '0x01', logo: 'url', - name: 'Name', schemaName: 'standard', totalSupply: '0', }); @@ -1761,7 +1735,6 @@ describe('NftController', () => { it('should add the nft contract to the correct chain in state when source is custom', async () => { const { nftController } = setupController({ options: {}, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), }); await nftController.addNft('0x01', '1', 'sepolia', { @@ -1785,7 +1758,6 @@ describe('NftController', () => { ).toStrictEqual({ address: '0x01', logo: 'url', - name: 'Name', schemaName: 'standard', totalSupply: '0', }); @@ -1795,7 +1767,6 @@ describe('NftController', () => { options: { // chainId: ChainId.mainnet, }, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), }); await nftController.addNft('0x01', '1', 'mainnet', { @@ -1837,7 +1808,6 @@ describe('NftController', () => { ).toStrictEqual({ address: '0x01', logo: 'url', - name: 'Name', totalSupply: '0', schemaName: 'standard', }); @@ -2231,10 +2201,8 @@ describe('NftController', () => { }); }); - it('should add NFT erc721 and aggregate NFT data from both contract and NFT-API even if call to Get Collections fails', async () => { + it('should add NFT erc721 and aggregate NFT data from NFT-API even if call to Get Collections fails', async () => { const { nftController } = setupController({ - getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), getERC721TokenURI: jest .fn() .mockResolvedValue( @@ -2306,15 +2274,11 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_KUDOSADDRESS, - name: 'KudosToken', - symbol: 'KDO', schemaName: ERC721, }); }); - it('should add NFT erc721 and aggregate NFT data from both contract and NFT-API when call to Get Collections succeeds', async () => { + it('should add NFT erc721 and aggregate NFT data from NFT-API when call to Get Collections succeeds', async () => { const { nftController } = setupController({ - getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), getERC721TokenURI: jest .fn() .mockResolvedValue( @@ -2382,8 +2346,6 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_KUDOSADDRESS, - name: 'KudosToken', - symbol: 'KDO', schemaName: ERC721, }); }); @@ -2435,10 +2397,8 @@ describe('NftController', () => { }); }); - it('should add NFT erc721 and get NFT information only from contract', async () => { + it('should add NFT erc721 and get NFT information from tokenURI when NFT API returns 404', async () => { const { nftController } = setupController({ - getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), getERC721TokenURI: jest.fn().mockImplementation((tokenAddress) => { switch (tokenAddress) { case ERC721_KUDOSADDRESS: @@ -2491,8 +2451,6 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_KUDOSADDRESS, - name: 'KudosToken', - symbol: 'KDO', schemaName: ERC721, }); }); @@ -3019,10 +2977,6 @@ describe('NftController', () => { it('should add NFT with metadata hosted in IPFS', async () => { const { nftController, triggerPreferencesStateChange, mockGetAccount } = setupController({ - getERC721AssetName: jest - .fn() - .mockResolvedValue("Maltjik.jpg's Depressionists"), - getERC721AssetSymbol: jest.fn().mockResolvedValue('DPNS'), getERC721TokenURI: jest.fn().mockImplementation((tokenAddress) => { switch (tokenAddress) { case ERC721_DEPRESSIONIST_ADDRESS: @@ -3053,8 +3007,6 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_DEPRESSIONIST_ADDRESS, - name: "Maltjik.jpg's Depressionists", - symbol: 'DPNS', schemaName: ERC721, }); expect( diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index c5df09ac73b..02d4707aab9 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -882,12 +882,8 @@ export class NftController extends BaseController< * @param nfts[].nftContract - An object containing contract data of the NFT being added. * @param nfts[].chainId - The chainId of the network where the NFT is being added. * @param nfts[].source - Whether the NFT was detected, added manually or suggested by a dapp. - * @returns A promise that resolves when all NFTs have been added or updated. */ - async #addMultipleNfts( - userAddress: string, - nfts: NftToAdd[], - ): Promise<{ errors: Error[] }> { + async #addMultipleNfts(userAddress: string, nfts: NftToAdd[]): Promise { const releaseLock = await this.#mutex.acquire(); try { const { allNfts } = this.state; @@ -896,7 +892,6 @@ export class NftController extends BaseController< [chainId: `0x${string}`]: Nft[]; } = {}; const modifiedChainIds = new Set(); - const errors: Error[] = []; const pendingCallbacks: { address: string; symbol: string | undefined; @@ -913,83 +908,73 @@ export class NftController extends BaseController< chainId, source, } of nfts) { - try { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); + const checksumHexAddress = toChecksumHexAddress(tokenAddress); - if (!allNftsForUserPerChain[chainId]) { - allNftsForUserPerChain[chainId] = [ - ...(allNftsForUser?.[chainId] ?? []), - ]; + if (!allNftsForUserPerChain[chainId]) { + allNftsForUserPerChain[chainId] = [ + ...(allNftsForUser?.[chainId] ?? []), + ]; + } + + const existingEntry = allNftsForUserPerChain[chainId].find( + (nft) => + nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && + nft.tokenId === tokenId, + ); + + if (existingEntry) { + const differentMetadata = compareNftMetadata( + nftMetadata, + existingEntry, + ); + + const hasNewFields = hasNewCollectionFields( + nftMetadata, + existingEntry, + ); + + if ( + !differentMetadata && + existingEntry.isCurrentlyOwned && + !hasNewFields + ) { + continue; } - const existingEntry = allNftsForUserPerChain[chainId].find( + const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( (nft) => nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, ); - if (existingEntry) { - const differentMetadata = compareNftMetadata( - nftMetadata, - existingEntry, - ); - - const hasNewFields = hasNewCollectionFields( - nftMetadata, - existingEntry, - ); - - if ( - !differentMetadata && - existingEntry.isCurrentlyOwned && - !hasNewFields - ) { - continue; - } - - const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( - (nft) => - nft.address.toLowerCase() === - checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, - ); - - if (indexToUpdate !== -1) { - allNftsForUserPerChain[chainId][indexToUpdate] = { - ...existingEntry, - ...nftMetadata, - }; - } - } else { - const newEntry: Nft = { - address: checksumHexAddress, - tokenId, - favorite: false, - isCurrentlyOwned: true, + if (indexToUpdate !== -1) { + allNftsForUserPerChain[chainId][indexToUpdate] = { + ...existingEntry, ...nftMetadata, }; - - allNftsForUserPerChain[chainId].push(newEntry); } + } else { + const newEntry: Nft = { + address: checksumHexAddress, + tokenId, + favorite: false, + isCurrentlyOwned: true, + ...nftMetadata, + }; - modifiedChainIds.add(chainId); + allNftsForUserPerChain[chainId].push(newEntry); + } - if (this.#onNftAdded) { - pendingCallbacks.push({ - address: checksumHexAddress, - symbol: nftContract.symbol, - tokenId: tokenId.toString(), - standard: nftMetadata.standard, - source, - }); - } - } catch (error) { - console.error( - `NftController: Failed to add NFT ${tokenAddress} #${tokenId}`, - error, - ); - errors.push( - error instanceof Error ? error : new Error(String(error)), - ); + modifiedChainIds.add(chainId); + + if (this.#onNftAdded) { + pendingCallbacks.push({ + address: checksumHexAddress, + symbol: nftContract.symbol, + tokenId: tokenId.toString(), + standard: nftMetadata.standard, + source, + }); } } @@ -1004,8 +989,6 @@ export class NftController extends BaseController< for (const callbackData of pendingCallbacks) { this.#onNftAdded?.(callbackData); } - - return { errors }; } finally { releaseLock(); } @@ -1025,10 +1008,7 @@ export class NftController extends BaseController< async #addNftContracts( userAddress: string, contracts: NftContractToAdd[], - ): Promise<{ - contracts: { [chainId: `0x${string}`]: NftContract[] }; - errors: Error[]; - }> { + ): Promise<{ contracts: { [chainId: `0x${string}`]: NftContract[] } }> { const releaseLock = await this.#mutex.acquire(); try { const { allNftContracts } = this.state; @@ -1037,7 +1017,6 @@ export class NftController extends BaseController< [chainId: `0x${string}`]: NftContract[]; } = {}; const modifiedChainIds = new Set(); - const errors: Error[] = []; for (const { networkClientId, @@ -1045,92 +1024,84 @@ export class NftController extends BaseController< source, nftMetadata, } of contracts) { - try { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); - const { - configuration: { chainId }, - } = this.messenger.call( - 'NetworkController:getNetworkClientById', - networkClientId, - ); + const checksumHexAddress = toChecksumHexAddress(tokenAddress); + const { + configuration: { chainId }, + } = this.messenger.call( + 'NetworkController:getNetworkClientById', + networkClientId, + ); - if (!nftContractsForUserPerChain[chainId]) { - nftContractsForUserPerChain[chainId] = [ - ...(allNftContractsForUser?.[chainId] ?? []), - ]; - } + // Initialised before the existingEntry check so pre-existing contracts + // are still present in the returned map for callers to look up. + if (!nftContractsForUserPerChain[chainId]) { + nftContractsForUserPerChain[chainId] = [ + ...(allNftContractsForUser?.[chainId] ?? []), + ]; + } - const existingEntry = nftContractsForUserPerChain[chainId].find( - (nftContract) => - nftContract.address.toLowerCase() === - checksumHexAddress.toLowerCase(), - ); + const existingEntry = nftContractsForUserPerChain[chainId].find( + (nftContract) => + nftContract.address.toLowerCase() === + checksumHexAddress.toLowerCase(), + ); - if (existingEntry) { - continue; - } + if (existingEntry) { + continue; + } - // this doesn't work currently for detection if the user switches networks while the detection is processing - // will be fixed once detection uses networkClientIds - // get name and symbol if ERC721 then put together the metadata - const contractInformation = this.#getNftContractInformation( - checksumHexAddress, - nftMetadata, - ); + // this doesn't work currently for detection if the user switches networks while the detection is processing + // will be fixed once detection uses networkClientIds + // get name and symbol if ERC721 then put together the metadata + const contractInformation = this.#getNftContractInformation( + checksumHexAddress, + nftMetadata, + ); - // If the nft is auto-detected we want some valid metadata to be present - if ( - source === Source.Detected && - 'address' in contractInformation && - typeof contractInformation.address === 'string' && - 'collection' in contractInformation && - contractInformation.collection.name === null && - 'image_url' in contractInformation.collection && - contractInformation.collection.image_url === null && - Object.entries(contractInformation).every(([key, value]) => { - return key === 'address' || key === 'collection' || !value; - }) - ) { - continue; - } + // If the nft is auto-detected we want some valid metadata to be present + if ( + source === Source.Detected && + 'address' in contractInformation && + typeof contractInformation.address === 'string' && + 'collection' in contractInformation && + contractInformation.collection.name === null && + 'image_url' in contractInformation.collection && + contractInformation.collection.image_url === null && + Object.entries(contractInformation).every(([key, value]) => { + return key === 'address' || key === 'collection' || !value; + }) + ) { + continue; + } - const { - asset_contract_type, - created_date, - symbol, - description, - external_link, - schema_name, - collection: { name, image_url, tokenCount }, - } = contractInformation; - - /* istanbul ignore next */ - const newEntry: NftContract = Object.assign( - {}, - { address: checksumHexAddress }, - description && { description }, - name && { name }, - image_url && { logo: image_url }, - symbol && { symbol }, - tokenCount !== null && - typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, - asset_contract_type && { assetContractType: asset_contract_type }, - created_date && { createdDate: created_date }, - schema_name && { schemaName: schema_name }, - external_link && { externalLink: external_link }, - ); + const { + asset_contract_type, + created_date, + symbol, + description, + external_link, + schema_name, + collection: { name, image_url, tokenCount }, + } = contractInformation; + + /* istanbul ignore next */ + const newEntry: NftContract = Object.assign( + {}, + { address: checksumHexAddress }, + description && { description }, + name && { name }, + image_url && { logo: image_url }, + symbol && { symbol }, + tokenCount !== null && + typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, + asset_contract_type && { assetContractType: asset_contract_type }, + created_date && { createdDate: created_date }, + schema_name && { schemaName: schema_name }, + external_link && { externalLink: external_link }, + ); - nftContractsForUserPerChain[chainId].push(newEntry); - modifiedChainIds.add(chainId); - } catch (error) { - console.error( - `NftController: Failed to add NFT contract for ${tokenAddress}`, - error, - ); - errors.push( - error instanceof Error ? error : new Error(String(error)), - ); - } + nftContractsForUserPerChain[chainId].push(newEntry); + modifiedChainIds.add(chainId); } // Loops once per chain (not once per NFT contract) @@ -1142,7 +1113,7 @@ export class NftController extends BaseController< ); } - return { contracts: nftContractsForUserPerChain, errors }; + return { contracts: nftContractsForUserPerChain }; } finally { releaseLock(); } @@ -1518,19 +1489,17 @@ export class NftController extends BaseController< nftMetadata = await this.#sanitizeNftMetadata(nftMetadata); } - const { contracts: newNftContracts, errors: contractErrors } = - await this.#addNftContracts(addressToSearch, [ + const { contracts: newNftContracts } = await this.#addNftContracts( + addressToSearch, + [ { tokenAddress: checksumHexAddress, networkClientId, source, nftMetadata, }, - ]); - - if (contractErrors.length > 0) { - throw contractErrors[0]; - } + ], + ); // If NFT contract was not added, do not add individual NFT const nftContract = Object.values(newNftContracts) @@ -1554,23 +1523,16 @@ export class NftController extends BaseController< } if (nftContract) { - const { errors: nftErrors } = await this.#addMultipleNfts( - addressToSearch, - [ - { - tokenAddress: checksumHexAddress, - tokenId, - nftMetadata, - nftContract, - chainId, - source, - }, - ], - ); - - if (nftErrors.length > 0) { - throw nftErrors[0]; - } + await this.#addMultipleNfts(addressToSearch, [ + { + tokenAddress: checksumHexAddress, + tokenId, + nftMetadata, + nftContract, + chainId, + source, + }, + ]); } } @@ -1604,16 +1566,13 @@ export class NftController extends BaseController< nfts.map((nft) => nft.nftMetadata), ); - const nftContractsToAdd: (NftContractToAdd & { - chainId: Hex; - tokenId: string; - })[] = []; + // Contracts are batched first so we can resolve accepted contracts + // before building the NFT list for the second batch write. + const nftContractsToAdd: NftContractToAdd[] = []; for (const [index, nft] of nfts.entries()) { const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); - const hexChainId = toHex(nft.nftMetadata.chainId); - const networkClientId = this.messenger.call( 'NetworkController:findNetworkClientIdByChainId', hexChainId, @@ -1624,8 +1583,6 @@ export class NftController extends BaseController< tokenAddress: checksumHexAddress, source, nftMetadata: sanitizedNftMetadata[index], - chainId: hexChainId, - tokenId: nft.tokenId, }); } @@ -1636,19 +1593,24 @@ export class NftController extends BaseController< const nftsToAdd: NftToAdd[] = []; - nftContractsToAdd.forEach((nftContractToAdd) => { - const nftContract = newNftContracts[nftContractToAdd.chainId]?.find( + for (const [index, nft] of nfts.entries()) { + const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); + const hexChainId = toHex(nft.nftMetadata.chainId); + const nftContract = newNftContracts[hexChainId]?.find( (contract) => - contract.address.toLowerCase() === - nftContractToAdd.tokenAddress.toLowerCase(), + contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), ); if (nftContract) { nftsToAdd.push({ - ...nftContractToAdd, + tokenAddress: checksumHexAddress, + tokenId: nft.tokenId, + nftMetadata: sanitizedNftMetadata[index], nftContract, + chainId: hexChainId, + source, }); } - }); + } if (nftsToAdd.length > 0) { await this.#addMultipleNfts(addressToSearch, nftsToAdd); From 5652068d5928b664ab23d4e3b0a94b35018c72f8 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 13:57:42 +0100 Subject: [PATCH 13/17] chore: fix regression and add tests for it --- .../src/NftController.test.ts | 126 +++++++++ .../assets-controllers/src/NftController.ts | 258 +++++++++--------- 2 files changed, 259 insertions(+), 125 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index fdedbebaccf..eb6a5460654 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -3468,6 +3468,132 @@ describe('NftController', () => { tokenId: '1', }); }); + + it('should skip a contract that fails getNetworkClientById and still add the remaining NFTs', async () => { + const { nftController, nftControllerMessenger } = setupController({}); + + // Make getNetworkClientById throw for 'sepolia' to simulate a mid-batch + // network lookup failure (e.g., network switched during detection). + const originalCall = nftControllerMessenger.call.bind( + nftControllerMessenger, + ); + jest + .spyOn(nftControllerMessenger, 'call') + // eslint-disable-next-line @typescript-eslint/no-explicit-any + .mockImplementation((action: any, ...args: any[]): any => { + if ( + action === 'NetworkController:getNetworkClientById' && + args[0] === 'sepolia' + ) { + throw new Error('Network unavailable'); + } + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return (originalCall as (...a: any[]) => any)(action, ...args); + }); + + await nftController.addNfts( + [ + { + tokenAddress: '0x01', + tokenId: '1', + nftMetadata: { + name: 'NFT 1', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + { + tokenAddress: '0x02', + tokenId: '2', + nftMetadata: { + name: 'NFT 2', + image: null, + description: null, + standard: ERC721, + chainId: 11155111, // sepolia — getNetworkClientById will throw + }, + }, + { + tokenAddress: '0x03', + tokenId: '3', + nftMetadata: { + name: 'NFT 3', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + ], + OWNER_ACCOUNT.address, + ); + + // NFTs 1 and 3 (mainnet) should be added despite NFT 2 failing + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet], + ).toHaveLength(2); + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet], + ).toMatchObject([ + { address: '0x01', tokenId: '1' }, + { address: '0x03', tokenId: '3' }, + ]); + // NFT 2 (sepolia) should not have been added + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address]?.['0xaa36a7'], + ).toBeUndefined(); + }); + + it('should fire onNftAdded callbacks only after all NFT state has been written', async () => { + let stateWhenFirstCallbackFired: NftControllerState | undefined; + const mockOnNftAdded = jest.fn(); + + const { nftController } = setupController({ + options: { onNftAdded: mockOnNftAdded }, + }); + + mockOnNftAdded.mockImplementationOnce(() => { + stateWhenFirstCallbackFired = nftController.state; + }); + + await nftController.addNfts( + [ + { + tokenAddress: '0x01', + tokenId: '1', + nftMetadata: { + name: 'NFT 1', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + { + tokenAddress: '0x02', + tokenId: '2', + nftMetadata: { + name: 'NFT 2', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + ], + OWNER_ACCOUNT.address, + ); + + expect(mockOnNftAdded).toHaveBeenCalledTimes(2); + // Both NFTs must already be in state when the first callback fires + expect( + stateWhenFirstCallbackFired?.allNfts[OWNER_ACCOUNT.address][ + ChainId.mainnet + ], + ).toHaveLength(2); + }); }); describe('addNftVerifyOwnership', () => { diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 02d4707aab9..5bf086f5b45 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -908,73 +908,77 @@ export class NftController extends BaseController< chainId, source, } of nfts) { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); + try { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); - if (!allNftsForUserPerChain[chainId]) { - allNftsForUserPerChain[chainId] = [ - ...(allNftsForUser?.[chainId] ?? []), - ]; - } - - const existingEntry = allNftsForUserPerChain[chainId].find( - (nft) => - nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && - nft.tokenId === tokenId, - ); - - if (existingEntry) { - const differentMetadata = compareNftMetadata( - nftMetadata, - existingEntry, - ); - - const hasNewFields = hasNewCollectionFields( - nftMetadata, - existingEntry, - ); - - if ( - !differentMetadata && - existingEntry.isCurrentlyOwned && - !hasNewFields - ) { - continue; + if (!allNftsForUserPerChain[chainId]) { + allNftsForUserPerChain[chainId] = [ + ...(allNftsForUser?.[chainId] ?? []), + ]; } - const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( + const existingEntry = allNftsForUserPerChain[chainId].find( (nft) => nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, ); - if (indexToUpdate !== -1) { - allNftsForUserPerChain[chainId][indexToUpdate] = { - ...existingEntry, + if (existingEntry) { + const differentMetadata = compareNftMetadata( + nftMetadata, + existingEntry, + ); + + const hasNewFields = hasNewCollectionFields( + nftMetadata, + existingEntry, + ); + + if ( + !differentMetadata && + existingEntry.isCurrentlyOwned && + !hasNewFields + ) { + continue; + } + + const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( + (nft) => + nft.address.toLowerCase() === + checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, + ); + + if (indexToUpdate !== -1) { + allNftsForUserPerChain[chainId][indexToUpdate] = { + ...existingEntry, + ...nftMetadata, + }; + } + } else { + const newEntry: Nft = { + address: checksumHexAddress, + tokenId, + favorite: false, + isCurrentlyOwned: true, ...nftMetadata, }; - } - } else { - const newEntry: Nft = { - address: checksumHexAddress, - tokenId, - favorite: false, - isCurrentlyOwned: true, - ...nftMetadata, - }; - allNftsForUserPerChain[chainId].push(newEntry); - } + allNftsForUserPerChain[chainId].push(newEntry); + } - modifiedChainIds.add(chainId); + modifiedChainIds.add(chainId); - if (this.#onNftAdded) { - pendingCallbacks.push({ - address: checksumHexAddress, - symbol: nftContract.symbol, - tokenId: tokenId.toString(), - standard: nftMetadata.standard, - source, - }); + if (this.#onNftAdded) { + pendingCallbacks.push({ + address: checksumHexAddress, + symbol: nftContract.symbol, + tokenId: tokenId.toString(), + standard: nftMetadata.standard, + source, + }); + } + } catch (error) { + console.error('Failed to add NFT', tokenAddress, tokenId, error); } } @@ -1024,84 +1028,88 @@ export class NftController extends BaseController< source, nftMetadata, } of contracts) { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); - const { - configuration: { chainId }, - } = this.messenger.call( - 'NetworkController:getNetworkClientById', - networkClientId, - ); + try { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); + const { + configuration: { chainId }, + } = this.messenger.call( + 'NetworkController:getNetworkClientById', + networkClientId, + ); - // Initialised before the existingEntry check so pre-existing contracts - // are still present in the returned map for callers to look up. - if (!nftContractsForUserPerChain[chainId]) { - nftContractsForUserPerChain[chainId] = [ - ...(allNftContractsForUser?.[chainId] ?? []), - ]; - } + // Initialised before the existingEntry check so pre-existing contracts + // are still present in the returned map for callers to look up. + if (!nftContractsForUserPerChain[chainId]) { + nftContractsForUserPerChain[chainId] = [ + ...(allNftContractsForUser?.[chainId] ?? []), + ]; + } - const existingEntry = nftContractsForUserPerChain[chainId].find( - (nftContract) => - nftContract.address.toLowerCase() === - checksumHexAddress.toLowerCase(), - ); + const existingEntry = nftContractsForUserPerChain[chainId].find( + (nftContract) => + nftContract.address.toLowerCase() === + checksumHexAddress.toLowerCase(), + ); - if (existingEntry) { - continue; - } + if (existingEntry) { + continue; + } - // this doesn't work currently for detection if the user switches networks while the detection is processing - // will be fixed once detection uses networkClientIds - // get name and symbol if ERC721 then put together the metadata - const contractInformation = this.#getNftContractInformation( - checksumHexAddress, - nftMetadata, - ); + // this doesn't work currently for detection if the user switches networks while the detection is processing + // will be fixed once detection uses networkClientIds + // get name and symbol if ERC721 then put together the metadata + const contractInformation = this.#getNftContractInformation( + checksumHexAddress, + nftMetadata, + ); - // If the nft is auto-detected we want some valid metadata to be present - if ( - source === Source.Detected && - 'address' in contractInformation && - typeof contractInformation.address === 'string' && - 'collection' in contractInformation && - contractInformation.collection.name === null && - 'image_url' in contractInformation.collection && - contractInformation.collection.image_url === null && - Object.entries(contractInformation).every(([key, value]) => { - return key === 'address' || key === 'collection' || !value; - }) - ) { - continue; - } + // If the nft is auto-detected we want some valid metadata to be present + if ( + source === Source.Detected && + 'address' in contractInformation && + typeof contractInformation.address === 'string' && + 'collection' in contractInformation && + contractInformation.collection.name === null && + 'image_url' in contractInformation.collection && + contractInformation.collection.image_url === null && + Object.entries(contractInformation).every(([key, value]) => { + return key === 'address' || key === 'collection' || !value; + }) + ) { + continue; + } - const { - asset_contract_type, - created_date, - symbol, - description, - external_link, - schema_name, - collection: { name, image_url, tokenCount }, - } = contractInformation; - - /* istanbul ignore next */ - const newEntry: NftContract = Object.assign( - {}, - { address: checksumHexAddress }, - description && { description }, - name && { name }, - image_url && { logo: image_url }, - symbol && { symbol }, - tokenCount !== null && - typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, - asset_contract_type && { assetContractType: asset_contract_type }, - created_date && { createdDate: created_date }, - schema_name && { schemaName: schema_name }, - external_link && { externalLink: external_link }, - ); + const { + asset_contract_type, + created_date, + symbol, + description, + external_link, + schema_name, + collection: { name, image_url, tokenCount }, + } = contractInformation; + + /* istanbul ignore next */ + const newEntry: NftContract = Object.assign( + {}, + { address: checksumHexAddress }, + description && { description }, + name && { name }, + image_url && { logo: image_url }, + symbol && { symbol }, + tokenCount !== null && + typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, + asset_contract_type && { assetContractType: asset_contract_type }, + created_date && { createdDate: created_date }, + schema_name && { schemaName: schema_name }, + external_link && { externalLink: external_link }, + ); - nftContractsForUserPerChain[chainId].push(newEntry); - modifiedChainIds.add(chainId); + nftContractsForUserPerChain[chainId].push(newEntry); + modifiedChainIds.add(chainId); + } catch (error) { + console.error('Failed to add NFT contract', tokenAddress, error); + } } // Loops once per chain (not once per NFT contract) From 2cc953b85357524136c50f6404281578ffd31542 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 14:11:20 +0100 Subject: [PATCH 14/17] chore: minor changes --- .../assets-controllers/src/NftController.ts | 67 +++++++++++++------ 1 file changed, 46 insertions(+), 21 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 5bf086f5b45..8d2bf8a5515 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -1574,36 +1574,61 @@ export class NftController extends BaseController< nfts.map((nft) => nft.nftMetadata), ); - // Contracts are batched first so we can resolve accepted contracts - // before building the NFT list for the second batch write. - const nftContractsToAdd: NftContractToAdd[] = []; + // Resolve network client IDs per item up front. Items that fail (e.g., + // the user removes a network during detection) are skipped individually + // so the rest of the batch is unaffected. Resolved data is bundled into + // one object per NFT to avoid index-alignment issues between the two loops. + const resolvedNfts: { + contractToAdd: NftContractToAdd; + tokenId: string; + checksumHexAddress: string; + hexChainId: Hex; + sanitizedMetadata: NftMetadata; + }[] = []; for (const [index, nft] of nfts.entries()) { - const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); - const hexChainId = toHex(nft.nftMetadata.chainId); - const networkClientId = this.messenger.call( - 'NetworkController:findNetworkClientIdByChainId', - hexChainId, - ); + try { + const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); + const hexChainId = toHex(nft.nftMetadata.chainId); + const networkClientId = this.messenger.call( + 'NetworkController:findNetworkClientIdByChainId', + hexChainId, + ); - nftContractsToAdd.push({ - networkClientId, - tokenAddress: checksumHexAddress, - source, - nftMetadata: sanitizedNftMetadata[index], - }); + resolvedNfts.push({ + contractToAdd: { + networkClientId, + tokenAddress: checksumHexAddress, + source, + nftMetadata: sanitizedNftMetadata[index], + }, + tokenId: nft.tokenId, + checksumHexAddress, + hexChainId, + sanitizedMetadata: sanitizedNftMetadata[index], + }); + } catch (error) { + console.error( + 'Failed to resolve network for NFT', + nft.tokenAddress, + error, + ); + } } const { contracts: newNftContracts } = await this.#addNftContracts( addressToSearch, - nftContractsToAdd, + resolvedNfts.map((item) => item.contractToAdd), ); const nftsToAdd: NftToAdd[] = []; - for (const [index, nft] of nfts.entries()) { - const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); - const hexChainId = toHex(nft.nftMetadata.chainId); + for (const { + checksumHexAddress, + tokenId, + hexChainId, + sanitizedMetadata, + } of resolvedNfts) { const nftContract = newNftContracts[hexChainId]?.find( (contract) => contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), @@ -1611,8 +1636,8 @@ export class NftController extends BaseController< if (nftContract) { nftsToAdd.push({ tokenAddress: checksumHexAddress, - tokenId: nft.tokenId, - nftMetadata: sanitizedNftMetadata[index], + tokenId, + nftMetadata: sanitizedMetadata, nftContract, chainId: hexChainId, source, From dca7a96a06032e85c4b8bc359c61376bbe03061d Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 14:42:01 +0100 Subject: [PATCH 15/17] chore: resolved comments --- .../src/NftController.test.ts | 31 +++++-------------- 1 file changed, 8 insertions(+), 23 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index eb6a5460654..a7f28915c1d 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -3469,27 +3469,12 @@ describe('NftController', () => { }); }); - it('should skip a contract that fails getNetworkClientById and still add the remaining NFTs', async () => { - const { nftController, nftControllerMessenger } = setupController({}); + it('should skip an NFT whose chain ID is not registered and still add the remaining NFTs', async () => { + // Chain ID 0x999 is not registered in any mock network client config, so + // findNetworkClientIdByChainId throws for it naturally — no spy needed. + const UNREGISTERED_CHAIN_ID = 0x999; - // Make getNetworkClientById throw for 'sepolia' to simulate a mid-batch - // network lookup failure (e.g., network switched during detection). - const originalCall = nftControllerMessenger.call.bind( - nftControllerMessenger, - ); - jest - .spyOn(nftControllerMessenger, 'call') - // eslint-disable-next-line @typescript-eslint/no-explicit-any - .mockImplementation((action: any, ...args: any[]): any => { - if ( - action === 'NetworkController:getNetworkClientById' && - args[0] === 'sepolia' - ) { - throw new Error('Network unavailable'); - } - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return (originalCall as (...a: any[]) => any)(action, ...args); - }); + const { nftController } = setupController({}); await nftController.addNfts( [ @@ -3512,7 +3497,7 @@ describe('NftController', () => { image: null, description: null, standard: ERC721, - chainId: 11155111, // sepolia — getNetworkClientById will throw + chainId: UNREGISTERED_CHAIN_ID, }, }, { @@ -3540,9 +3525,9 @@ describe('NftController', () => { { address: '0x01', tokenId: '1' }, { address: '0x03', tokenId: '3' }, ]); - // NFT 2 (sepolia) should not have been added + // NFT 2 (unknown chain) should not have been added expect( - nftController.state.allNfts[OWNER_ACCOUNT.address]?.['0xaa36a7'], + nftController.state.allNfts[OWNER_ACCOUNT.address]?.['0x999'], ).toBeUndefined(); }); From 5358fb54e75bf13132e816094b1c0f4d7d66ba96 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 15:25:28 +0100 Subject: [PATCH 16/17] chore: updated tests --- .../src/NftController.test.ts | 504 ++++-------------- 1 file changed, 93 insertions(+), 411 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index a7f28915c1d..5ee3b7958e5 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -181,6 +181,7 @@ function setupController({ getAccount, getSelectedAccount, bulkScanUrlsMock, + approvalAddRequest, mockNetworkClientConfigurationsByNetworkClientId = {}, defaultSelectedAccount = OWNER_ACCOUNT, mockGetNetworkClientIdByChainId = {}, @@ -223,6 +224,7 @@ function setupController({ Promise, [string[]] >; + approvalAddRequest?: jest.Mock; mockNetworkClientConfigurationsByNetworkClientId?: Record< NetworkClientId, NetworkClientConfiguration @@ -330,20 +332,32 @@ function setupController({ mockGetERC1155TokenURI, ); - const approvalControllerMessenger = new Messenger< - 'ApprovalController', - MessengerActions, - MessengerEvents, - RootMessenger - >({ - namespace: 'ApprovalController', - parent: messenger, - }); + let approvalController: ApprovalController; - const approvalController = new ApprovalController({ - messenger: approvalControllerMessenger, - showApprovalRequest: jest.fn(), - }); + if (approvalAddRequest) { + messenger.registerActionHandler( + 'ApprovalController:addRequest', + approvalAddRequest, + ); + // Provide a stub so callers can still destructure `approvalController` + // without needing to branch. The stub is intentionally minimal. + approvalController = { addRequest: approvalAddRequest } as never; + } else { + const approvalControllerMessenger = new Messenger< + 'ApprovalController', + MessengerActions, + MessengerEvents, + RootMessenger + >({ + namespace: 'ApprovalController', + parent: messenger, + }); + + approvalController = new ApprovalController({ + messenger: approvalControllerMessenger, + showApprovalRequest: jest.fn(), + }); + } // Register the phishing controller mock if provided if (bulkScanUrlsMock) { @@ -647,12 +661,12 @@ describe('NftController', () => { }); it('should error if the user does not own the suggested ERC721 NFT', async function () { - const { nftController, nftControllerMessenger } = setupController({ + const addRequestMock = jest.fn(); + const { nftController } = setupController({ getERC721OwnerOf: jest.fn().mockImplementation(() => '0x12345abcefg'), + approvalAddRequest: addRequestMock, }); - const callActionSpy = jest.spyOn(nftControllerMessenger, 'call'); - await expect(() => nftController.watchNft( ERC721_NFT, @@ -661,12 +675,7 @@ describe('NftController', () => { 'mainnet', ), ).rejects.toThrow('Suggested NFT is not owned by the selected account'); - // First call is getInternalAccount. Second call is the approval request. - expect(callActionSpy).not.toHaveBeenNthCalledWith( - 2, - 'ApprovalController:addRequest', - expect.any(Object), - ); + expect(addRequestMock).not.toHaveBeenCalled(); }); it('should error if the call to isNftOwner fail', async function () { @@ -686,12 +695,12 @@ describe('NftController', () => { }); it('should error if the user does not own the suggested ERC1155 NFT', async function () { - const { nftController, nftControllerMessenger } = setupController({ + const addRequestMock = jest.fn(); + const { nftController, mockGetAccount } = setupController({ getERC1155BalanceOf: jest.fn().mockImplementation(() => new BN(0)), + approvalAddRequest: addRequestMock, }); - const callActionSpy = jest.spyOn(nftControllerMessenger, 'call'); - await expect(() => nftController.watchNft( ERC1155_NFT, @@ -700,12 +709,9 @@ describe('NftController', () => { 'mainnet', ), ).rejects.toThrow('Suggested NFT is not owned by the selected account'); - // First call is to get InternalAccount - expect(callActionSpy).toHaveBeenNthCalledWith( - 1, - 'AccountsController:getAccount', - expect.any(String), - ); + // getAccount must be called to look up the owner to compare against + expect(mockGetAccount).toHaveBeenCalledWith(expect.any(String)); + expect(addRequestMock).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API disabled and IPFS gateway enabled', async function () { @@ -719,19 +725,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); @@ -744,71 +752,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 3. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 4. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 5. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 6. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(8); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -827,8 +781,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API enabled and IPFS gateway enabled', async function () { @@ -842,19 +797,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -866,71 +823,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 5. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 6. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(8); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -949,8 +852,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API disabled and IPFS gateway disabled', async function () { @@ -964,19 +868,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -988,71 +894,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 5. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 6. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(8); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -1071,8 +923,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API enabled and IPFS gateway disabled', async function () { @@ -1086,19 +939,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); @@ -1111,71 +966,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 5. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 6. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(8); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -1194,8 +995,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC1155 type and add to suggestedNfts with the OpenSea API disabled', async function () { @@ -1210,9 +1012,9 @@ describe('NftController', () => { }), ); + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, } = setupController({ @@ -1227,6 +1029,7 @@ describe('NftController', () => { getERC1155TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); @@ -1235,78 +1038,21 @@ describe('NftController', () => { isIpfsGatewayEnabled: true, displayNftMedia: false, }); + const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 3. `AssetsContractController:getERC1155BalanceOf` - .mockResolvedValueOnce(new BN(1)) - // 4. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 5. `AssetsContractController:getERC721TokenURI` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 6. `AssetsContractController:getERC1155TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 7. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 8. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 9. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 10. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC1155_NFT, ERC1155, 'https://etherscan.io', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); - expect(callActionSpy).toHaveBeenNthCalledWith( - 7, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://etherscan.io', @@ -1325,8 +1071,6 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); }); it('should handle ERC1155 type and add to suggestedNfts with the OpenSea API enabled', async function () { @@ -1341,11 +1085,8 @@ describe('NftController', () => { }), ); - const { - nftController, - nftControllerMessenger, - triggerPreferencesStateChange, - } = setupController({ + const addRequestMock = jest.fn().mockResolvedValue(undefined); + const { nftController, triggerPreferencesStateChange } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest .fn() @@ -1357,74 +1098,19 @@ describe('NftController', () => { getERC1155TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), + approvalAddRequest: addRequestMock, }); triggerPreferencesStateChange({ ...getDefaultPreferencesState(), isIpfsGatewayEnabled: true, displayNftMedia: true, }); + const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 3. `AssetsContractController:getERC1155BalanceOf` - .mockResolvedValueOnce(new BN(1)) - // 4. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 5. `AssetsContractController:getERC1155TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 6. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 7. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 9. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC1155_NFT, ERC1155, @@ -1432,10 +1118,8 @@ describe('NftController', () => { 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); - expect(callActionSpy).toHaveBeenNthCalledWith( - 7, - 'ApprovalController:addRequest', + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://etherscan.io', @@ -1454,8 +1138,6 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); }); it('should add the NFT to the correct chainId/selectedAddress in state when passed a userAddress in the options argument', async function () { From c71e0d355b9a130097d92f554642ef1caeda2d51 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Mon, 9 Mar 2026 15:52:13 +0100 Subject: [PATCH 17/17] chore: minor --- packages/assets-controllers/src/NftController.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 5ee3b7958e5..4b60131d371 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -166,6 +166,10 @@ jest.mock('uuid', () => { * `AccountsController:getSelectedAccount` action. * @param args.bulkScanUrlsMock - Used to construct mock versions of the * `PhishingController:bulkScanUrls` action. + * @param args.approvalAddRequest - When provided, registered directly as the + * `ApprovalController:addRequest` action handler instead of creating a real + * ApprovalController. Use this when the test needs to assert on or auto-resolve + * approval requests without the full approval flow. * @param args.defaultSelectedAccount - The default selected account to use in * @param args.displayNftMedia - The default displayNftMedia to use in * @returns A collection of test controllers and mocks.