From d5266b0d7eb151f5728ee381f2d8fe91dc737a4a Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Fri, 21 Jun 2024 12:29:51 +0200 Subject: [PATCH 1/8] feat: add call to getCollection to get more collection data --- .../src/NftController.test.ts | 240 ++++++- .../assets-controllers/src/NftController.ts | 55 +- .../src/NftDetectionController.test.ts | 595 +++++++++++++++--- .../src/NftDetectionController.ts | 86 ++- 4 files changed, 859 insertions(+), 117 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 05e2961643..72cf21eeea 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -1487,6 +1487,18 @@ describe('NftController', () => { defaultSelectedAccount: OWNER_ACCOUNT, }); + nock(NFT_API_BASE_URL) + .get(`/collections?chainIds=1&contract=0x01`) + .reply(200, { + collections: [ + { + contractDeployedAt: 'timestampTest', + ownerCount: '989', + openseaVerificationStatus: 'verified', + }, + ], + }); + await nftController.addNft('0x01', '1'); expect( nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0], @@ -1501,11 +1513,87 @@ describe('NftController', () => { isCurrentlyOwned: true, tokenURI: '', creator: 'Oxaddress', - collection: { creator: 'Oxaddress', tokenCount: 0 }, + collection: { + creator: 'Oxaddress', + tokenCount: 0, + contractDeployedAt: 'timestampTest', + ownerCount: '989', + openseaVerificationStatus: 'verified', + }, }); }); - it('should add NFT erc721 and aggregate NFT data from both contract and NFT-API', async () => { + it('should add NFT erc721 and aggregate NFT data from both contract and NFT-API even if call to Get Collections fails', async () => { + const { nftController } = setupController({ + options: { + getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), + getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), + getERC721TokenURI: jest + .fn() + .mockResolvedValue( + 'https://ipfs.gitcoin.co:443/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov', + ), + }, + defaultSelectedAccount: OWNER_ACCOUNT, + }); + nock(NFT_API_BASE_URL) + .get( + `/tokens?chainIds=1&tokens=${ERC721_KUDOSADDRESS}%3A${ERC721_KUDOS_TOKEN_ID}&includeTopBid=true&includeAttributes=true&includeLastSale=true`, + ) + .reply(200, { + tokens: [ + { + token: { + kind: 'erc721', + name: 'Kudos Name', + description: 'Kudos Description', + image: 'url', + }, + }, + ], + }); + + nock(NFT_API_BASE_URL) + .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .replyWithError(new Error('Failed to fetch')); + + nock('https://ipfs.gitcoin.co:443') + .get('/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov') + .reply(200, { + image: 'Kudos Image (directly from tokenURI)', + name: 'Kudos Name (directly from tokenURI)', + description: 'Kudos Description (directly from tokenURI)', + }); + + await nftController.addNft(ERC721_KUDOSADDRESS, ERC721_KUDOS_TOKEN_ID); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0], + ).toStrictEqual({ + address: ERC721_KUDOSADDRESS, + image: 'url', + name: 'Kudos Name (directly from tokenURI)', + description: 'Kudos Description (directly from tokenURI)', + tokenId: ERC721_KUDOS_TOKEN_ID, + standard: ERC721, + favorite: false, + isCurrentlyOwned: true, + tokenURI: + 'https://ipfs.gitcoin.co:443/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov', + }); + + expect( + nftController.state.allNftContracts[OWNER_ACCOUNT.address][ + ChainId.mainnet + ][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 () => { const { nftController } = setupController({ options: { getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), @@ -1522,6 +1610,7 @@ describe('NftController', () => { .get( `/tokens?chainIds=1&tokens=${ERC721_KUDOSADDRESS}%3A${ERC721_KUDOS_TOKEN_ID}&includeTopBid=true&includeAttributes=true&includeLastSale=true`, ) + .reply(200, { tokens: [ { @@ -1535,6 +1624,19 @@ describe('NftController', () => { ], }); + nock(NFT_API_BASE_URL) + .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .reply(200, { + collections: [ + { + contractDeployedAt: 'timestampTest', + ownerCount: '989', + openseaVerificationStatus: 'verified', + creator: '0xcreator', + }, + ], + }); + nock('https://ipfs.gitcoin.co:443') .get('/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov') .reply(200, { @@ -1558,6 +1660,12 @@ describe('NftController', () => { isCurrentlyOwned: true, tokenURI: 'https://ipfs.gitcoin.co:443/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov', + collection: { + creator: '0xcreator', + contractDeployedAt: 'timestampTest', + ownerCount: '989', + openseaVerificationStatus: 'verified', + }, }); expect( @@ -1848,7 +1956,7 @@ describe('NftController', () => { }); }); - it('should add an nft and nftContract when there is valid contract information and source is "detected"', async () => { + it('should add an nft and nftContract when there is valid contract information and source is "detected" when call to getCollections fails', async () => { const mockOnNftAdded = jest.fn(); const { nftController } = setupController({ options: { @@ -1884,6 +1992,128 @@ describe('NftController', () => { ], }); + nock(NFT_API_BASE_URL) + .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .replyWithError(new Error('Failed to fetch')); + + await nftController.addNft( + '0x6EbeAf8e8E946F0716E6533A6f2cefc83f60e8Ab', + '123', + { + userAddress: OWNER_ACCOUNT.address, + source: Source.Detected, + }, + ); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address]?.[ChainId.mainnet], + ).toBeUndefined(); + + expect( + nftController.state.allNftContracts[OWNER_ACCOUNT.address]?.[ + ChainId.mainnet + ], + ).toBeUndefined(); + + await nftController.addNft(ERC721_KUDOSADDRESS, ERC721_KUDOS_TOKEN_ID, { + userAddress: OWNER_ACCOUNT.address, + source: Source.Detected, + }); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet], + ).toStrictEqual([ + { + address: ERC721_KUDOSADDRESS, + description: 'Kudos Description', + image: 'Kudos image (from proxy API)', + name: 'Kudos Name', + standard: ERC721, + tokenId: ERC721_KUDOS_TOKEN_ID, + favorite: false, + isCurrentlyOwned: true, + tokenURI: null, + collection: { + tokenCount: '10', + image: 'Kudos logo (from proxy API)', + name: 'Kudos', + creator: undefined, + openseaVerificationStatus: undefined, + ownerCount: undefined, + contractDeployedAt: undefined, + }, + }, + ]); + + expect( + nftController.state.allNftContracts[OWNER_ACCOUNT.address][ + ChainId.mainnet + ], + ).toStrictEqual([ + { + address: ERC721_KUDOSADDRESS, + logo: 'Kudos logo (from proxy API)', + name: 'Kudos', + totalSupply: '10', + schemaName: ERC721, + }, + ]); + + expect(mockOnNftAdded).toHaveBeenCalledWith({ + address: ERC721_KUDOSADDRESS, + tokenId: ERC721_KUDOS_TOKEN_ID, + standard: ERC721, + source: Source.Detected, + }); + }); + + it('should add an nft and nftContract when there is valid contract information and source is "detected" when call to get collections succeeds', async () => { + const mockOnNftAdded = jest.fn(); + const { nftController } = setupController({ + options: { + onNftAdded: mockOnNftAdded, + getERC721AssetName: jest + .fn() + .mockRejectedValue(new Error('Failed to fetch')), + getERC721AssetSymbol: jest + .fn() + .mockRejectedValue(new Error('Failed to fetch')), + }, + defaultSelectedAccount: OWNER_ACCOUNT, + }); + nock(NFT_API_BASE_URL) + .get( + `/tokens?chainIds=1&tokens=${ERC721_KUDOSADDRESS}%3A${ERC721_KUDOS_TOKEN_ID}&includeTopBid=true&includeAttributes=true&includeLastSale=true`, + ) + .reply(200, { + tokens: [ + { + token: { + kind: 'erc721', + name: 'Kudos Name', + description: 'Kudos Description', + image: 'Kudos image (from proxy API)', + collection: { + name: 'Kudos', + tokenCount: '10', + image: 'Kudos logo (from proxy API)', + }, + }, + }, + ], + }); + + nock(NFT_API_BASE_URL) + .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .reply(200, { + collections: [ + { + creator: '0xcreator', + openseaVerificationStatus: 'verified', + }, + ], + }); + await nftController.addNft( '0x6EbeAf8e8E946F0716E6533A6f2cefc83f60e8Ab', '123', @@ -1925,6 +2155,10 @@ describe('NftController', () => { tokenCount: '10', image: 'Kudos logo (from proxy API)', name: 'Kudos', + creator: '0xcreator', + openseaVerificationStatus: 'verified', + ownerCount: undefined, + contractDeployedAt: undefined, }, }, ]); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 8b79b778d8..0af5d05b41 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -52,6 +52,7 @@ import type { Collection, Attributes, LastSale, + GetCollectionsResponse, } from './NftDetectionController'; type NFTStandardType = 'ERC721' | 'ERC1155'; @@ -532,15 +533,39 @@ export class NftController extends BaseController< includeAttributes: 'true', includeLastSale: 'true', }).toString(); - const nftInformation: ReservoirResponse | undefined = - await fetchWithErrorHandling({ - url: `${this.getNftApi()}?${urlParams}`, - options: { - headers: { - Version: '1', + // Params for getCollections API call + const getCollectionParams = new URLSearchParams({ + chainIds: '1', + contract: `${contractAddress}`, + }).toString(); + + const [nftInformation, collectionInformation]: [ + ReservoirResponse | undefined, + GetCollectionsResponse, + ] = await Promise.all([ + safelyExecute(() => + fetchWithErrorHandling({ + url: `${this.getNftApi()}?${urlParams}`, + options: { + headers: { + Version: '1', + }, }, - }, - }); + }), + ), + safelyExecute(() => + fetchWithErrorHandling({ + url: `${ + NFT_API_BASE_URL as string + }/collections?${getCollectionParams}`, + options: { + headers: { + Version: '1', + }, + }, + }), + ), + ]); // if we were still unable to fetch the data we return out the default/null of `NftMetadata` if (!nftInformation?.tokens?.[0]?.token) { return { @@ -585,7 +610,19 @@ export class NftController extends BaseController< }, rarityRank && { rarityRank }, rarity && { rarity }, - collection && { collection }, + (collection || collectionInformation) && { + collection: { + ...(collection || {}), + creator: + collection?.creator || + collectionInformation?.collections[0].creator, + openseaVerificationStatus: + collectionInformation?.collections[0].openseaVerificationStatus, + contractDeployedAt: + collectionInformation?.collections[0].contractDeployedAt, + ownerCount: collectionInformation?.collections[0].ownerCount, + }, + }, ); return nftMetadata; diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index 38b31b0d0d..e8cce6b060 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -28,9 +28,11 @@ import { getDefaultNftControllerState } from './NftController'; import { NftDetectionController, BlockaidResultType, + MAX_GET_COLLECTION_BATCH_SIZE, type AllowedActions, type AllowedEvents, } from './NftDetectionController'; +import * as constants from './NftDetectionController'; const controllerName = 'NftDetectionController' as const; @@ -447,6 +449,15 @@ describe('NftDetectionController', () => { ...getDefaultPreferencesState(), useNftDetection: true, }); + + // Mock /getCollections call + + nock(NFT_API_BASE_URL) + .get( + `/collections?contract=0xCE7ec4B2DfB30eB6c0BB5656D33aAd6BFb4001Fc&contract=0x0B0fa4fF58D28A88d63235bd0756EDca69e49e6d&contract=0xebE4e5E773AFD2bAc25De0cFafa084CFb3cBf1eD&chainId=1`, + ) + .replyWithError(new Error('Failed to fetch')); + // Wait for detect call triggered by preferences state change to settle await advanceTime({ clock, @@ -476,128 +487,481 @@ describe('NftDetectionController', () => { ); }); - it('should detect and add NFTs correctly when blockaid result is in response', async () => { - const mockAddNft = jest.fn(); - const selectedAddress = '0x123'; - const selectedAccount = createMockInternalAccount({ - address: selectedAddress, - }); - const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); - await withController( - { - options: { addNft: mockAddNft }, - mockPreferencesState: {}, - mockGetSelectedAccount, - }, - async ({ controller, controllerEvents }) => { - controllerEvents.triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - useNftDetection: true, - }); - // Wait for detect call triggered by preferences state change to settle - await advanceTime({ - clock, - duration: 1, - }); - mockAddNft.mockReset(); + describe('getCollections', () => { + it('should detect and add NFTs correctly when blockaid result is in response with unsuccessful getCollections', async () => { + const mockAddNft = jest.fn(); + const selectedAddress = '0x123'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + await withController( + { + options: { addNft: mockAddNft }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + mockAddNft.mockReset(); - await controller.detectNfts(); + nock(NFT_API_BASE_URL) + .get(`/collections?contract=0xtest1&contract=0xtest2&chainId=1`) + .replyWithError(new Error('Failed to fetch')); - // Expect to be called twice - expect(mockAddNft).toHaveBeenNthCalledWith(1, '0xtest1', '2574', { - nftMetadata: { - description: 'Description 2574', - image: 'image/2574.png', - name: 'ID 2574', - standard: 'ERC721', - imageOriginal: 'imageOriginal/2574.png', - }, - userAddress: selectedAccount.address, - source: Source.Detected, - networkClientId: undefined, - }); - expect(mockAddNft).toHaveBeenNthCalledWith(2, '0xtest2', '2575', { - nftMetadata: { - description: 'Description 2575', - image: 'image/2575.png', - name: 'ID 2575', - standard: 'ERC721', - imageOriginal: 'imageOriginal/2575.png', - }, - userAddress: selectedAccount.address, - source: Source.Detected, - networkClientId: undefined, - }); - }, - ); - }); + await controller.detectNfts(); - it('should detect and add NFTs and filter them correctly', async () => { - const mockAddNft = jest.fn(); - const selectedAddress = '0x12345'; - const selectedAccount = createMockInternalAccount({ - address: selectedAddress, + // Expect to be called twice + expect(mockAddNft).toHaveBeenNthCalledWith(1, '0xtest1', '2574', { + nftMetadata: { + description: 'Description 2574', + image: 'image/2574.png', + name: 'ID 2574', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2574.png', + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }); + expect(mockAddNft).toHaveBeenNthCalledWith(2, '0xtest2', '2575', { + nftMetadata: { + description: 'Description 2575', + image: 'image/2575.png', + name: 'ID 2575', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2575.png', + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }); + }, + ); + }); + it('should detect and add NFTs correctly when blockaid result is in response with successful getCollections', async () => { + const mockAddNft = jest.fn(); + const selectedAddress = '0x123'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + await withController( + { + options: { addNft: mockAddNft }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + mockAddNft.mockReset(); + + nock(NFT_API_BASE_URL) + .get(`/collections?contract=0xtest1&contract=0xtest2&chainId=1`) + .reply(200, { + collections: [ + { + id: '0xtest1', + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + }, + { + id: '0xtest2', + creator: '0xcreator2', + openseaVerificationStatus: 'verified', + }, + ], + }); + + await controller.detectNfts(); + + // Expect to be called twice + expect(mockAddNft).toHaveBeenNthCalledWith(1, '0xtest1', '2574', { + nftMetadata: { + description: 'Description 2574', + image: 'image/2574.png', + name: 'ID 2574', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2574.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + ownerCount: undefined, + tokenCount: undefined, + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }); + expect(mockAddNft).toHaveBeenNthCalledWith(2, '0xtest2', '2575', { + nftMetadata: { + description: 'Description 2575', + image: 'image/2575.png', + name: 'ID 2575', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2575.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator2', + openseaVerificationStatus: 'verified', + ownerCount: undefined, + tokenCount: undefined, + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }); + }, + ); + }); + it('should detect and add NFTs and filter them correctly', async () => { + const mockAddNft = jest.fn(); + const selectedAddress = '0x12345'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + await withController( + { + options: { addNft: mockAddNft }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + mockAddNft.mockReset(); + + nock(NFT_API_BASE_URL) + .get( + `/collections?contract=0xtestCollection1&contract=0xtestCollection2&chainId=1`, + ) + .reply(200, { + collections: [ + { + id: '0xtestCollection1', + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + }, + { + id: '0xtestCollection2', + creator: '0xcreator2', + openseaVerificationStatus: 'verified', + }, + ], + }); + + await controller.detectNfts(); + + expect(mockAddNft).toHaveBeenCalledTimes(2); + // In this test we mocked that reservoir returned 5 NFTs + // the only NFTs we want to add are when isSpam=== false and (either no blockaid result returned or blockaid says "Benign") + expect(mockAddNft).toHaveBeenNthCalledWith( + 1, + '0xtestCollection1', + '1', + { + nftMetadata: { + description: 'Description 1', + image: 'image/1.png', + name: 'ID 1', + standard: 'ERC721', + imageOriginal: 'imageOriginal/1.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + ownerCount: undefined, + tokenCount: undefined, + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }, + ); + expect(mockAddNft).toHaveBeenNthCalledWith( + 2, + '0xtestCollection2', + '2', + { + nftMetadata: { + description: 'Description 2', + image: 'image/2.png', + name: 'ID 2', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator2', + openseaVerificationStatus: 'verified', + ownerCount: undefined, + tokenCount: undefined, + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }, + ); + }, + ); }); - const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); - await withController( - { - options: { addNft: mockAddNft }, - mockPreferencesState: {}, - mockGetSelectedAccount, - }, - async ({ controller, controllerEvents }) => { - controllerEvents.triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - useNftDetection: true, - }); - // Wait for detect call triggered by preferences state change to settle - await advanceTime({ - clock, - duration: 1, - }); - mockAddNft.mockReset(); - await controller.detectNfts(); + it('should detect and add NFTs from a single collection', async () => { + const mockAddNft = jest.fn(); + const selectedAddress = 'Oxuser'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + await withController( + { + options: { addNft: mockAddNft }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + mockAddNft.mockReset(); + nock(NFT_API_BASE_URL) + .get( + `/users/${selectedAddress}/tokens?chainIds=1&limit=50&includeTopBid=true&continuation=`, + ) + .reply(200, { + tokens: [ + { + token: { + contract: '0xtestCollection1', + kind: 'erc721', + name: 'ID 1', + description: 'Description 1', + image: 'image/1.png', + tokenId: '1', + metadata: { + imageOriginal: 'imageOriginal/1.png', + imageMimeType: 'image/png', + tokenURI: 'tokenURITest', + }, + isSpam: false, + }, + blockaidResult: { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BlockaidResultType.Benign, + }, + }, + { + token: { + contract: '0xtestCollection1', + kind: 'erc721', + name: 'ID 2', + description: 'Description 2', + image: 'image/2.png', + tokenId: '2', + metadata: { + imageOriginal: 'imageOriginal/2.png', + imageMimeType: 'image/png', + tokenURI: 'tokenURITest', + }, + isSpam: false, + }, + }, + ], + }); + + nock(NFT_API_BASE_URL) + .get(`/collections?contract=0xtestCollection1&chainId=1`) + .reply(200, { + collections: [ + { + id: '0xtestCollection1', + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + ownerCount: '555', + }, + ], + }); + + await controller.detectNfts(); + + expect(mockAddNft).toHaveBeenCalledTimes(2); + // In this test we mocked that reservoir returned 5 NFTs + // the only NFTs we want to add are when isSpam=== false and (either no blockaid result returned or blockaid says "Benign") + expect(mockAddNft).toHaveBeenNthCalledWith( + 1, + '0xtestCollection1', + '1', + { + nftMetadata: { + description: 'Description 1', + image: 'image/1.png', + name: 'ID 1', + standard: 'ERC721', + imageOriginal: 'imageOriginal/1.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + ownerCount: '555', + tokenCount: undefined, + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }, + ); + expect(mockAddNft).toHaveBeenNthCalledWith( + 2, + '0xtestCollection1', + '2', + { + nftMetadata: { + description: 'Description 2', + image: 'image/2.png', + name: 'ID 2', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + ownerCount: '555', + tokenCount: undefined, + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }, + ); + }, + ); + }); - expect(mockAddNft).toHaveBeenCalledTimes(2); - // In this test we mocked that reservoir returned 5 NFTs - // the only NFTs we want to add are when isSpam=== false and (either no blockaid result returned or blockaid says "Benign") - expect(mockAddNft).toHaveBeenNthCalledWith( - 1, - '0xtestCollection1', - '1', - { + it('should add collection information correctly when a single batch fails to get collection informations', async () => { + // Mock that MAX_GET_COLLECTION_BATCH_SIZE is equal 1 instead of 20 + Object.defineProperty(constants, 'MAX_GET_COLLECTION_BATCH_SIZE', { + value: 1, + }); + expect(MAX_GET_COLLECTION_BATCH_SIZE).toBe(1); + const mockAddNft = jest.fn(); + const selectedAddress = '0x123'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + await withController( + { + options: { addNft: mockAddNft }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + mockAddNft.mockReset(); + + nock(NFT_API_BASE_URL) + .get(`/collections?contract=0xtest1&chainId=1`) + .reply(200, { + collections: [ + { + id: '0xtest1', + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + }, + ], + }); + + nock(NFT_API_BASE_URL) + .get(`/collections?contract=0xtest2&chainId=1`) + .replyWithError(new Error('Failed to fetch')); + + await controller.detectNfts(); + + // Expect to be called twice + expect(mockAddNft).toHaveBeenNthCalledWith(1, '0xtest1', '2574', { nftMetadata: { - description: 'Description 1', - image: 'image/1.png', - name: 'ID 1', + description: 'Description 2574', + image: 'image/2574.png', + name: 'ID 2574', standard: 'ERC721', - imageOriginal: 'imageOriginal/1.png', + imageOriginal: 'imageOriginal/2574.png', + collection: { + contractDeployedAt: undefined, + creator: '0xcreator1', + openseaVerificationStatus: 'verified', + ownerCount: undefined, + tokenCount: undefined, + }, }, userAddress: selectedAccount.address, source: Source.Detected, networkClientId: undefined, - }, - ); - expect(mockAddNft).toHaveBeenNthCalledWith( - 2, - '0xtestCollection2', - '2', - { + }); + expect(mockAddNft).toHaveBeenNthCalledWith(2, '0xtest2', '2575', { nftMetadata: { - description: 'Description 2', - image: 'image/2.png', - name: 'ID 2', + description: 'Description 2575', + image: 'image/2575.png', + name: 'ID 2575', standard: 'ERC721', - imageOriginal: 'imageOriginal/2.png', + imageOriginal: 'imageOriginal/2575.png', }, userAddress: selectedAccount.address, source: Source.Detected, networkClientId: undefined, - }, - ); - }, - ); + }); + + Object.defineProperty(constants, 'MAX_GET_COLLECTION_BATCH_SIZE', { + value: 20, + }); + expect(MAX_GET_COLLECTION_BATCH_SIZE).toBe(20); + }, + ); + }); }); it('should detect and add NFTs by networkClientId correctly', async () => { @@ -626,6 +990,11 @@ describe('NftDetectionController', () => { duration: 1, }); mockAddNft.mockReset(); + nock(NFT_API_BASE_URL) + .get( + `/collections?contract=0xebE4e5E773AFD2bAc25De0cFafa084CFb3cBf1eD&chainId=1`, + ) + .replyWithError(new Error('Failed to fetch')); await controller.detectNfts({ networkClientId: 'mainnet', @@ -692,6 +1061,12 @@ describe('NftDetectionController', () => { }); mockAddNft.mockReset(); + nock(NFT_API_BASE_URL) + .get( + `/collections?contract=0xebE4e5E773AFD2bAc25De0cFafa084CFb3cBf1eD&chainId=1`, + ) + .replyWithError(new Error('Failed to fetch')); + await controller.detectNfts(); expect(mockAddNft).not.toHaveBeenCalled(); @@ -904,6 +1279,12 @@ describe('NftDetectionController', () => { mockAddNft.mockReset(); mockAddNft.mockRejectedValueOnce(new Error('UNEXPECTED ERROR')); + nock(NFT_API_BASE_URL) + .get( + `/collections?contract=0xCE7ec4B2DfB30eB6c0BB5656D33aAd6BFb4001Fc&contract=0x0B0fa4fF58D28A88d63235bd0756EDca69e49e6d&contract=0xebE4e5E773AFD2bAc25De0cFafa084CFb3cBf1eD&chainId=1`, + ) + .replyWithError(new Error('Failed to fetch')); + await expect(async () => await controller.detectNfts()).rejects.toThrow( 'UNEXPECTED ERROR', ); @@ -965,6 +1346,12 @@ describe('NftDetectionController', () => { ...getDefaultPreferencesState(), useNftDetection: true, }); + + nock(NFT_API_BASE_URL) + .get( + `/collections?contract=0xebE4e5E773AFD2bAc25De0cFafa084CFb3cBf1eD&chainId=1`, + ) + .replyWithError(new Error('Failed to fetch')); await Promise.all([controller.detectNfts(), controller.detectNfts()]); expect(mockAddNft).toHaveBeenCalledTimes(1); diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 63088a597f..b18217cc33 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -9,6 +9,7 @@ import { NFT_API_VERSION, convertHexToDecimal, handleFetch, + fetchWithErrorHandling, } from '@metamask/controller-utils'; import type { NetworkClientId, @@ -24,6 +25,7 @@ import type { } from '@metamask/preferences-controller'; import { createDeferredPromise, type Hex } from '@metamask/utils'; +import { reduceInBatchesSerially } from './assetsUtil'; import { Source } from './constants'; import { type NftController, @@ -330,7 +332,20 @@ export type Attributes = { createdAt?: string; }; -export type Collection = { +export type GetCollectionsResponse = { + collections: CollectionResponse[]; +}; + +export type CollectionResponse = { + id?: string; + openseaVerificationStatus?: string; + contractDeployedAt?: string; + creator?: string; + tokenCount?: string; + ownerCount?: string; +}; + +export type TokenCollection = { id?: string; name?: string; slug?: string; @@ -348,6 +363,8 @@ export type Collection = { royalties?: Royalties[]; }; +export type Collection = TokenCollection & CollectionResponse; + export type Royalties = { bps?: number; recipient?: string; @@ -399,6 +416,8 @@ export type Metadata = { tokenURI?: string; }; +export const MAX_GET_COLLECTION_BATCH_SIZE = 20; + /** * Controller that passively detects nfts for a user address */ @@ -588,6 +607,71 @@ export class NftDetectionController extends BaseController< ? elm.blockaidResult?.result_type === BlockaidResultType.Benign : true), ); + // Retrieve collections from apiNfts + const collections = apiNfts.reduce((acc, currValue) => { + if (!acc.includes(currValue.token.contract)) { + acc.push(currValue.token.contract); + } + return acc; + }, []); + + // Call API to retrive collections infos + // The api accept a max of 20 contracts + const collectionResponse: GetCollectionsResponse = + await reduceInBatchesSerially({ + values: collections, + batchSize: MAX_GET_COLLECTION_BATCH_SIZE, + eachBatch: async (allResponses, batch) => { + const params = new URLSearchParams( + batch.map((s) => ['contract', s]), + ); + params.append('chainId', '1'); // Adding chainId 1 because we are only detecting for mainnet + const collectionResponseForBatch = await fetchWithErrorHandling({ + url: `${ + NFT_API_BASE_URL as string + }/collections?${params.toString()}`, + options: { + headers: { + Version: '1', + }, + }, + timeout: 15000, + }); + + return { + ...allResponses, + ...collectionResponseForBatch, + }; + }, + initialResult: {}, + }); + + // Add collections response fields to newnfts + if (collectionResponse.collections?.length) { + apiNfts.forEach((singleNFT) => { + const found = collectionResponse.collections.find( + (elm) => + elm.id?.toLowerCase() === + singleNFT.token.contract.toLowerCase(), + ); + if (found) { + singleNFT.token = { + ...singleNFT.token, + collection: { + ...(singleNFT.token.collection + ? singleNFT.token.collection + : {}), + creator: found?.creator, + openseaVerificationStatus: found?.openseaVerificationStatus, + contractDeployedAt: found.contractDeployedAt, + ownerCount: found.ownerCount, + tokenCount: found.tokenCount, + }, + }; + } + }); + } + // Proceed to add NFTs const addNftPromises = apiNfts.map(async (nft) => { const { tokenId, From d0649e6e296c4326c7f9c0adb70b6ca20feb8c71 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Fri, 21 Jun 2024 15:59:55 +0200 Subject: [PATCH 2/8] fix: compare existing collection fields with new metadata and update if necessary --- .../src/NftController.test.ts | 143 ++++++++++++++++ .../assets-controllers/src/NftController.ts | 14 +- .../assets-controllers/src/assetsUtil.test.ts | 157 ++++++++++++++++++ packages/assets-controllers/src/assetsUtil.ts | 26 +++ 4 files changed, 338 insertions(+), 2 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 72cf21eeea..9393b608dc 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -1436,6 +1436,149 @@ describe('NftController', () => { isCurrentlyOwned: true, }); }); + it('should update NFT collection field if new nft metadata has new keys', async () => { + const { nftController } = setupController({ + options: {}, + defaultSelectedAccount: OWNER_ACCOUNT, + }); + + await nftController.addNft('0x01', '1', { + nftMetadata: { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + favorite: false, + }, + }); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0], + ).toStrictEqual({ + address: '0x01', + description: 'description', + image: 'image', + name: 'name', + standard: 'standard', + tokenId: '1', + favorite: false, + isCurrentlyOwned: true, + }); + + await nftController.addNft('0x01', '1', { + nftMetadata: { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + favorite: false, + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + contractDeployedAt: 'timestamp', + }, + }, + }); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0], + ).toStrictEqual({ + address: '0x01', + description: 'description', + image: 'image', + name: 'name', + tokenId: '1', + standard: 'standard', + favorite: false, + isCurrentlyOwned: true, + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + contractDeployedAt: 'timestamp', + }, + }); + }); + + it('should not update NFT collection field if new nft metadata does not have new keys', async () => { + const mockOnNftAdded = jest.fn(); + const { nftController } = setupController({ + options: { + onNftAdded: mockOnNftAdded, + }, + defaultSelectedAccount: OWNER_ACCOUNT, + }); + + await nftController.addNft('0x01', '1', { + nftMetadata: { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + favorite: false, + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + contractDeployedAt: 'timestamp', + }, + }, + }); + expect(mockOnNftAdded).toHaveBeenCalled(); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0], + ).toStrictEqual({ + address: '0x01', + description: 'description', + image: 'image', + name: 'name', + standard: 'standard', + tokenId: '1', + favorite: false, + isCurrentlyOwned: true, + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + contractDeployedAt: 'timestamp', + }, + }); + + mockOnNftAdded.mockReset(); + + await nftController.addNft('0x01', '1', { + nftMetadata: { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + favorite: false, + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + contractDeployedAt: 'timestamp', + }, + }, + }); + + expect(mockOnNftAdded).not.toHaveBeenCalled(); + + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0], + ).toStrictEqual({ + address: '0x01', + description: 'description', + image: 'image', + name: 'name', + tokenId: '1', + standard: 'standard', + favorite: false, + isCurrentlyOwned: true, + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + contractDeployedAt: 'timestamp', + }, + }); + }); it('should not duplicate NFT nor NFT contract if already added', async () => { const { nftController } = setupController({ diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 0af5d05b41..5cfd8c5669 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -44,7 +44,11 @@ import BN from 'bn.js'; import { v4 as random } from 'uuid'; import type { AssetsContractController } from './AssetsContractController'; -import { compareNftMetadata, getFormattedIpfsUrl } from './assetsUtil'; +import { + compareNftMetadata, + getFormattedIpfsUrl, + hasNewCollectionFields, +} from './assetsUtil'; import { Source } from './constants'; import type { ApiNftContract, @@ -947,7 +951,13 @@ export class NftController extends BaseController< existingEntry, ); - if (!differentMetadata && existingEntry.isCurrentlyOwned) { + const hasNewFields = hasNewCollectionFields(nftMetadata, existingEntry); + + if ( + !differentMetadata && + existingEntry.isCurrentlyOwned && + !hasNewFields + ) { return; } diff --git a/packages/assets-controllers/src/assetsUtil.test.ts b/packages/assets-controllers/src/assetsUtil.test.ts index 26b8af1619..41dfb32368 100644 --- a/packages/assets-controllers/src/assetsUtil.test.ts +++ b/packages/assets-controllers/src/assetsUtil.test.ts @@ -161,6 +161,163 @@ describe('assetsUtil', () => { }); }); + describe('hasNewCollectionFields', () => { + it('should return false if both objects do not have collection', () => { + const nftMetadata: NftMetadata = { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + }; + const nft: Nft = { + address: 'address', + tokenId: '123', + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + }; + const different = assetsUtil.hasNewCollectionFields(nftMetadata, nft); + expect(different).toBe(false); + }); + + it('should return false if existing object has collection and new nft metadata object does not', () => { + const nftMetadata: NftMetadata = { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + }; + const nft: Nft = { + address: 'address', + tokenId: '123', + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + }, + }; + const different = assetsUtil.hasNewCollectionFields(nftMetadata, nft); + expect(different).toBe(false); + }); + + it('should return false if both objects has the same keys', () => { + const nftMetadata: NftMetadata = { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + }, + }; + const nft: Nft = { + address: 'address', + tokenId: '123', + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + }, + }; + const different = assetsUtil.hasNewCollectionFields(nftMetadata, nft); + expect(different).toBe(false); + }); + + it('should return true if new nft metadata object has keys that do not exist in the existing NFT', () => { + const nftMetadata: NftMetadata = { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + tokenCount: '5555', + ownerCount: '555', + contractDeployedAt: 'timestamp', + }, + }; + const nft: Nft = { + address: 'address', + tokenId: '123', + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + backgroundColor: 'backgroundColor', + imagePreview: 'imagePreview', + imageThumbnail: 'imageThumbnail', + imageOriginal: 'imageOriginal', + animation: 'animation', + animationOriginal: 'animationOriginal', + externalLink: 'externalLink', + collection: { + id: 'address', + openseaVerificationStatus: 'verified', + }, + }; + const different = assetsUtil.hasNewCollectionFields(nftMetadata, nft); + expect(different).toBe(true); + }); + }); + describe('isTokenDetectionSupportedForNetwork', () => { it('returns true for Mainnet', () => { expect( diff --git a/packages/assets-controllers/src/assetsUtil.ts b/packages/assets-controllers/src/assetsUtil.ts index fde0247040..74633130a2 100644 --- a/packages/assets-controllers/src/assetsUtil.ts +++ b/packages/assets-controllers/src/assetsUtil.ts @@ -48,6 +48,32 @@ export function compareNftMetadata(newNftMetadata: NftMetadata, nft: Nft) { return differentValues > 0; } +/** + * Checks whether the existing nft object has all the keys of the new incoming nft metadata object + * @param newNftMetadata - New nft metadata object + * @param nft - Existing nft object to compare with + * @returns Whether the existing nft object has all the new keys from the new Nft metadata object + */ +export function hasNewCollectionFields( + newNftMetadata: NftMetadata, + nft: Nft, +): boolean { + const keysNewNftMetadata = new Set( + Object.keys(newNftMetadata.collection || {}), + ); + const keysExistingNFt = new Set(Object.keys(nft.collection || {})); + + const newFields: string[] = []; + + for (const key of keysNewNftMetadata) { + if (!keysExistingNFt.has(key)) { + newFields.push(key); + } + } + + return newFields.length > 0; +} + const aggregatorNameByKey: Record = { aave: 'Aave', bancor: 'Bancor', From 45b07af5970d2189795990dcd7ff1ee513e80de9 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 24 Jun 2024 14:11:20 +0200 Subject: [PATCH 3/8] fix: add topBid to collectionResponse --- .../src/NftController.test.ts | 34 ++++++++++++++++++- .../assets-controllers/src/NftController.ts | 1 + .../src/NftDetectionController.test.ts | 30 ++++++++++++++++ .../src/NftDetectionController.ts | 9 +++-- 4 files changed, 71 insertions(+), 3 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 9393b608dc..1859b80c91 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -1630,6 +1630,34 @@ describe('NftController', () => { defaultSelectedAccount: OWNER_ACCOUNT, }); + const testTopBid = { + id: 'id', + sourceDomain: 'opensea.io', + price: { + currency: { + contract: '0x01', + name: 'Wrapped Ether', + symbol: 'WETH', + decimals: 18, + }, + amount: { + raw: '201300000000000000', + decimal: 0.2013, + usd: 716.46131, + native: 0.2013, + }, + netAmount: { + raw: '196267500000000000', + decimal: 0.19627, + usd: 698.54978, + native: 0.19627, + }, + }, + maker: 'testMaker', + validFrom: 1719228327, + validUntil: 1719228927, + }; + nock(NFT_API_BASE_URL) .get(`/collections?chainIds=1&contract=0x01`) .reply(200, { @@ -1638,6 +1666,7 @@ describe('NftController', () => { contractDeployedAt: 'timestampTest', ownerCount: '989', openseaVerificationStatus: 'verified', + topBid: testTopBid, }, ], }); @@ -1662,6 +1691,7 @@ describe('NftController', () => { contractDeployedAt: 'timestampTest', ownerCount: '989', openseaVerificationStatus: 'verified', + topBid: testTopBid, }, }); }); @@ -1753,7 +1783,6 @@ describe('NftController', () => { .get( `/tokens?chainIds=1&tokens=${ERC721_KUDOSADDRESS}%3A${ERC721_KUDOS_TOKEN_ID}&includeTopBid=true&includeAttributes=true&includeLastSale=true`, ) - .reply(200, { tokens: [ { @@ -1808,6 +1837,7 @@ describe('NftController', () => { contractDeployedAt: 'timestampTest', ownerCount: '989', openseaVerificationStatus: 'verified', + topBid: undefined, }, }); @@ -2184,6 +2214,7 @@ describe('NftController', () => { openseaVerificationStatus: undefined, ownerCount: undefined, contractDeployedAt: undefined, + topBid: undefined, }, }, ]); @@ -2302,6 +2333,7 @@ describe('NftController', () => { openseaVerificationStatus: 'verified', ownerCount: undefined, contractDeployedAt: undefined, + topBid: undefined, }, }, ]); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 5cfd8c5669..29b9bc792d 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -625,6 +625,7 @@ export class NftController extends BaseController< contractDeployedAt: collectionInformation?.collections[0].contractDeployedAt, ownerCount: collectionInformation?.collections[0].ownerCount, + topBid: collectionInformation?.collections[0].topBid, }, }, ); diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index e8cce6b060..9d104a7e11 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -572,6 +572,34 @@ describe('NftDetectionController', () => { }); mockAddNft.mockReset(); + const testTopBid = { + id: 'id', + sourceDomain: 'opensea.io', + price: { + currency: { + contract: '0x01', + name: 'Wrapped Ether', + symbol: 'WETH', + decimals: 18, + }, + amount: { + raw: '201300000000000000', + decimal: 0.2013, + usd: 716.46131, + native: 0.2013, + }, + netAmount: { + raw: '196267500000000000', + decimal: 0.19627, + usd: 698.54978, + native: 0.19627, + }, + }, + maker: 'testMaker', + validFrom: 1719228327, + validUntil: 1719228927, + }; + nock(NFT_API_BASE_URL) .get(`/collections?contract=0xtest1&contract=0xtest2&chainId=1`) .reply(200, { @@ -580,6 +608,7 @@ describe('NftDetectionController', () => { id: '0xtest1', creator: '0xcreator1', openseaVerificationStatus: 'verified', + topBid: testTopBid, }, { id: '0xtest2', @@ -605,6 +634,7 @@ describe('NftDetectionController', () => { openseaVerificationStatus: 'verified', ownerCount: undefined, tokenCount: undefined, + topBid: testTopBid, }, }, userAddress: selectedAccount.address, diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index b18217cc33..0222ba95c4 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -10,6 +10,7 @@ import { convertHexToDecimal, handleFetch, fetchWithErrorHandling, + NFT_API_TIMEOUT, } from '@metamask/controller-utils'; import type { NetworkClientId, @@ -343,6 +344,9 @@ export type CollectionResponse = { creator?: string; tokenCount?: string; ownerCount?: string; + topBid?: TopBid & { + sourceDomain?: string; + }; }; export type TokenCollection = { @@ -632,10 +636,10 @@ export class NftDetectionController extends BaseController< }/collections?${params.toString()}`, options: { headers: { - Version: '1', + Version: NFT_API_VERSION, }, }, - timeout: 15000, + timeout: NFT_API_TIMEOUT, }); return { @@ -666,6 +670,7 @@ export class NftDetectionController extends BaseController< contractDeployedAt: found.contractDeployedAt, ownerCount: found.ownerCount, tokenCount: found.tokenCount, + topBid: found.topBid, }, }; } From 36522e01572fe54651b03f62d49eaf75c8214a8d Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 27 Jun 2024 16:45:55 +0200 Subject: [PATCH 4/8] fix: rm tokenCount field --- packages/assets-controllers/src/NftDetectionController.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 0222ba95c4..4b1dad8231 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -342,7 +342,6 @@ export type CollectionResponse = { openseaVerificationStatus?: string; contractDeployedAt?: string; creator?: string; - tokenCount?: string; ownerCount?: string; topBid?: TopBid & { sourceDomain?: string; @@ -669,7 +668,6 @@ export class NftDetectionController extends BaseController< openseaVerificationStatus: found?.openseaVerificationStatus, contractDeployedAt: found.contractDeployedAt, ownerCount: found.ownerCount, - tokenCount: found.tokenCount, topBid: found.topBid, }, }; From f1822c99c4907c365fe53c0782368589ec7b54e7 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Fri, 28 Jun 2024 11:46:16 +0200 Subject: [PATCH 5/8] fix: refactor test --- .../assets-controllers/src/assetsUtil.test.ts | 113 ++++-------------- 1 file changed, 21 insertions(+), 92 deletions(-) diff --git a/packages/assets-controllers/src/assetsUtil.test.ts b/packages/assets-controllers/src/assetsUtil.test.ts index 41dfb32368..b087209e73 100644 --- a/packages/assets-controllers/src/assetsUtil.test.ts +++ b/packages/assets-controllers/src/assetsUtil.test.ts @@ -162,8 +162,11 @@ describe('assetsUtil', () => { }); describe('hasNewCollectionFields', () => { - it('should return false if both objects do not have collection', () => { - const nftMetadata: NftMetadata = { + let baseNftMetadata: NftMetadata; + let baseNft: Nft; + + beforeEach(() => { + baseNftMetadata = { name: 'name', image: 'image', description: 'description', @@ -176,94 +179,42 @@ describe('assetsUtil', () => { animationOriginal: 'animationOriginal', externalLink: 'externalLink', }; - const nft: Nft = { + + baseNft = { + ...baseNftMetadata, address: 'address', tokenId: '123', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', }; - const different = assetsUtil.hasNewCollectionFields(nftMetadata, nft); + }); + it('should return false if both objects do not have collection', () => { + const different = assetsUtil.hasNewCollectionFields( + baseNftMetadata, + baseNft, + ); expect(different).toBe(false); }); it('should return false if existing object has collection and new nft metadata object does not', () => { - const nftMetadata: NftMetadata = { - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', - }; - const nft: Nft = { - address: 'address', - tokenId: '123', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', + const different = assetsUtil.hasNewCollectionFields(baseNftMetadata, { + ...baseNft, collection: { id: 'address', openseaVerificationStatus: 'verified', }, - }; - const different = assetsUtil.hasNewCollectionFields(nftMetadata, nft); + }); expect(different).toBe(false); }); it('should return false if both objects has the same keys', () => { const nftMetadata: NftMetadata = { - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', + ...baseNftMetadata, collection: { id: 'address', openseaVerificationStatus: 'verified', }, }; const nft: Nft = { - address: 'address', - tokenId: '123', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', + ...baseNft, collection: { id: 'address', openseaVerificationStatus: 'verified', @@ -275,17 +226,7 @@ describe('assetsUtil', () => { it('should return true if new nft metadata object has keys that do not exist in the existing NFT', () => { const nftMetadata: NftMetadata = { - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', + ...baseNftMetadata, collection: { id: 'address', openseaVerificationStatus: 'verified', @@ -295,19 +236,7 @@ describe('assetsUtil', () => { }, }; const nft: Nft = { - address: 'address', - tokenId: '123', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - backgroundColor: 'backgroundColor', - imagePreview: 'imagePreview', - imageThumbnail: 'imageThumbnail', - imageOriginal: 'imageOriginal', - animation: 'animation', - animationOriginal: 'animationOriginal', - externalLink: 'externalLink', + ...baseNft, collection: { id: 'address', openseaVerificationStatus: 'verified', From 7a200162c1528a5bccbdeb53a9d1965370a44f1d Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Fri, 28 Jun 2024 12:16:00 +0200 Subject: [PATCH 6/8] fix: refactor fct --- packages/assets-controllers/src/assetsUtil.ts | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/packages/assets-controllers/src/assetsUtil.ts b/packages/assets-controllers/src/assetsUtil.ts index 74633130a2..55a0828570 100644 --- a/packages/assets-controllers/src/assetsUtil.ts +++ b/packages/assets-controllers/src/assetsUtil.ts @@ -58,20 +58,10 @@ export function hasNewCollectionFields( newNftMetadata: NftMetadata, nft: Nft, ): boolean { - const keysNewNftMetadata = new Set( - Object.keys(newNftMetadata.collection || {}), - ); - const keysExistingNFt = new Set(Object.keys(nft.collection || {})); - - const newFields: string[] = []; - - for (const key of keysNewNftMetadata) { - if (!keysExistingNFt.has(key)) { - newFields.push(key); - } - } + const keysNewNftMetadata = Object.keys(newNftMetadata.collection || {}); + const keysExistingNft = new Set(Object.keys(nft.collection || {})); - return newFields.length > 0; + return keysNewNftMetadata.some((key) => !keysExistingNft.has(key)); } const aggregatorNameByKey: Record = { From 5eb7be980a4f81f86ff2244cac3655966c7627cd Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 2 Jul 2024 12:03:15 +0200 Subject: [PATCH 7/8] fix: exclude shared contracts from fetching contract information --- .../src/NftController.test.ts | 36 +++- .../assets-controllers/src/NftController.ts | 52 +++--- .../src/NftDetectionController.test.ts | 158 ++++++++++++++++++ .../src/NftDetectionController.ts | 118 +++++++------ 4 files changed, 275 insertions(+), 89 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 1859b80c91..88b0543b85 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -305,11 +305,13 @@ describe('NftController', () => { tokens: [ { token: { + contract: '0x1', kind: 'erc1155', name: 'Name', description: 'Description', image: 'url', collection: { + id: '0x1', creator: 'Oxaddress', tokenCount: 0, }, @@ -1659,7 +1661,7 @@ describe('NftController', () => { }; nock(NFT_API_BASE_URL) - .get(`/collections?chainIds=1&contract=0x01`) + .get(`/collections?chainId=1&id=0x1`) .reply(200, { collections: [ { @@ -1686,6 +1688,7 @@ describe('NftController', () => { tokenURI: '', creator: 'Oxaddress', collection: { + id: '0x1', creator: 'Oxaddress', tokenCount: 0, contractDeployedAt: 'timestampTest', @@ -1717,17 +1720,21 @@ describe('NftController', () => { tokens: [ { token: { + contract: `${ERC721_KUDOSADDRESS}`, kind: 'erc721', name: 'Kudos Name', description: 'Kudos Description', image: 'url', + collection: { + id: `${ERC721_KUDOSADDRESS}`, + }, }, }, ], }); nock(NFT_API_BASE_URL) - .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .get(`/collections?chainId=1&id=${ERC721_KUDOSADDRESS}`) .replyWithError(new Error('Failed to fetch')); nock('https://ipfs.gitcoin.co:443') @@ -1753,6 +1760,14 @@ describe('NftController', () => { isCurrentlyOwned: true, tokenURI: 'https://ipfs.gitcoin.co:443/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov', + collection: { + contractDeployedAt: undefined, + creator: undefined, + id: ERC721_KUDOSADDRESS, + openseaVerificationStatus: undefined, + ownerCount: undefined, + topBid: undefined, + }, }); expect( @@ -1787,17 +1802,21 @@ describe('NftController', () => { tokens: [ { token: { + contract: ERC721_KUDOSADDRESS, kind: 'erc721', name: 'Kudos Name', description: 'Kudos Description', image: 'url', + collection: { + id: ERC721_KUDOSADDRESS, + }, }, }, ], }); nock(NFT_API_BASE_URL) - .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .get(`/collections?chainId=1&id=${ERC721_KUDOSADDRESS}`) .reply(200, { collections: [ { @@ -1833,6 +1852,7 @@ describe('NftController', () => { tokenURI: 'https://ipfs.gitcoin.co:443/api/v0/cat/QmPmt6EAaioN78ECnW5oCL8v2YvVSpoBjLCjrXhhsAvoov', collection: { + id: ERC721_KUDOSADDRESS, creator: '0xcreator', contractDeployedAt: 'timestampTest', ownerCount: '989', @@ -2151,11 +2171,13 @@ describe('NftController', () => { tokens: [ { token: { + contract: ERC721_KUDOSADDRESS, kind: 'erc721', name: 'Kudos Name', description: 'Kudos Description', image: 'Kudos image (from proxy API)', collection: { + id: ERC721_KUDOSADDRESS, name: 'Kudos', tokenCount: '10', image: 'Kudos logo (from proxy API)', @@ -2166,7 +2188,7 @@ describe('NftController', () => { }); nock(NFT_API_BASE_URL) - .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .get(`/collections?chainId=1&id=${ERC721_KUDOSADDRESS}`) .replyWithError(new Error('Failed to fetch')); await nftController.addNft( @@ -2207,6 +2229,7 @@ describe('NftController', () => { isCurrentlyOwned: true, tokenURI: null, collection: { + id: ERC721_KUDOSADDRESS, tokenCount: '10', image: 'Kudos logo (from proxy API)', name: 'Kudos', @@ -2263,11 +2286,13 @@ describe('NftController', () => { tokens: [ { token: { + contract: ERC721_KUDOSADDRESS, kind: 'erc721', name: 'Kudos Name', description: 'Kudos Description', image: 'Kudos image (from proxy API)', collection: { + id: ERC721_KUDOSADDRESS, name: 'Kudos', tokenCount: '10', image: 'Kudos logo (from proxy API)', @@ -2278,7 +2303,7 @@ describe('NftController', () => { }); nock(NFT_API_BASE_URL) - .get(`/collections?chainIds=1&contract=${ERC721_KUDOSADDRESS}`) + .get(`/collections?chainId=1&id=${ERC721_KUDOSADDRESS}`) .reply(200, { collections: [ { @@ -2326,6 +2351,7 @@ describe('NftController', () => { isCurrentlyOwned: true, tokenURI: null, collection: { + id: ERC721_KUDOSADDRESS, tokenCount: '10', image: 'Kudos logo (from proxy API)', name: 'Kudos', diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 29b9bc792d..f85a2736c8 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -24,6 +24,7 @@ import { ERC1155, ApprovalType, NFT_API_BASE_URL, + NFT_API_VERSION, } from '@metamask/controller-utils'; import { type InternalAccount } from '@metamask/keyring-api'; import type { @@ -537,39 +538,32 @@ export class NftController extends BaseController< includeAttributes: 'true', includeLastSale: 'true', }).toString(); + + // First fetch token information + const nftInformation: ReservoirResponse | undefined = + await fetchWithErrorHandling({ + url: `${this.getNftApi()}?${urlParams}`, + options: { + headers: { + Version: NFT_API_VERSION, + }, + }, + }); // Params for getCollections API call const getCollectionParams = new URLSearchParams({ - chainIds: '1', - contract: `${contractAddress}`, + chainId: '1', + id: `${nftInformation?.tokens[0]?.token?.collection?.id as string}`, }).toString(); - - const [nftInformation, collectionInformation]: [ - ReservoirResponse | undefined, - GetCollectionsResponse, - ] = await Promise.all([ - safelyExecute(() => - fetchWithErrorHandling({ - url: `${this.getNftApi()}?${urlParams}`, - options: { - headers: { - Version: '1', - }, - }, - }), - ), - safelyExecute(() => - fetchWithErrorHandling({ - url: `${ - NFT_API_BASE_URL as string - }/collections?${getCollectionParams}`, - options: { - headers: { - Version: '1', - }, + // Fetch collection information using collectionId + const collectionInformation: GetCollectionsResponse | undefined = + await fetchWithErrorHandling({ + url: `${NFT_API_BASE_URL as string}/collections?${getCollectionParams}`, + options: { + headers: { + Version: NFT_API_VERSION, }, - }), - ), - ]); + }, + }); // if we were still unable to fetch the data we return out the default/null of `NftMetadata` if (!nftInformation?.tokens?.[0]?.token) { return { diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index 9d104a7e11..e77016a144 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -156,6 +156,9 @@ describe('NftDetectionController', () => { tokenURI: 'tokenURITest', }, isSpam: false, + collection: { + id: '0xtest1', + }, }, blockaidResult: { // TODO: Either fix this lint violation or explain why it's necessary to ignore. @@ -177,6 +180,9 @@ describe('NftDetectionController', () => { tokenURI: 'tokenURITest', }, isSpam: false, + collection: { + id: '0xtest2', + }, }, blockaidResult: { // TODO: Either fix this lint violation or explain why it's necessary to ignore. @@ -205,6 +211,9 @@ describe('NftDetectionController', () => { tokenURI: 'tokenURITest', }, isSpam: false, + collection: { + id: '0xtestCollection1', + }, }, blockaidResult: { // TODO: Either fix this lint violation or explain why it's necessary to ignore. @@ -226,6 +235,9 @@ describe('NftDetectionController', () => { tokenURI: 'tokenURITest', }, isSpam: false, + collection: { + id: '0xtestCollection2', + }, }, }, { @@ -488,6 +500,130 @@ describe('NftDetectionController', () => { }); describe('getCollections', () => { + it('should not call getCollections api when collection ids do not match contract address', async () => { + const mockAddNft = jest.fn(); + const selectedAddress = 'Oxuser'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); + await withController( + { + options: { addNft: mockAddNft }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + mockAddNft.mockReset(); + nock(NFT_API_BASE_URL) + .get( + `/users/${selectedAddress}/tokens?chainIds=1&limit=50&includeTopBid=true&continuation=`, + ) + .reply(200, { + tokens: [ + { + token: { + contract: '0xtestCollection1', + kind: 'erc721', + name: 'ID 1', + description: 'Description 1', + image: 'image/1.png', + tokenId: '1', + metadata: { + imageOriginal: 'imageOriginal/1.png', + imageMimeType: 'image/png', + tokenURI: 'tokenURITest', + }, + isSpam: false, + collection: { + id: '0xtestCollection1:1223', + }, + }, + blockaidResult: { + // TODO: Either fix this lint violation or explain why it's necessary to ignore. + // eslint-disable-next-line @typescript-eslint/naming-convention + result_type: BlockaidResultType.Benign, + }, + }, + { + token: { + contract: '0xtestCollection1', + kind: 'erc721', + name: 'ID 2', + description: 'Description 2', + image: 'image/2.png', + tokenId: '2', + metadata: { + imageOriginal: 'imageOriginal/2.png', + imageMimeType: 'image/png', + tokenURI: 'tokenURITest', + }, + isSpam: false, + collection: { + id: '0xtestCollection1:34567', + }, + }, + }, + ], + }); + + await controller.detectNfts(); + + expect(mockAddNft).toHaveBeenCalledTimes(2); + // In this test we mocked that reservoir returned 5 NFTs + // the only NFTs we want to add are when isSpam=== false and (either no blockaid result returned or blockaid says "Benign") + expect(mockAddNft).toHaveBeenNthCalledWith( + 1, + '0xtestCollection1', + '1', + { + nftMetadata: { + description: 'Description 1', + image: 'image/1.png', + name: 'ID 1', + standard: 'ERC721', + imageOriginal: 'imageOriginal/1.png', + collection: { + id: '0xtestCollection1:1223', + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }, + ); + expect(mockAddNft).toHaveBeenNthCalledWith( + 2, + '0xtestCollection1', + '2', + { + nftMetadata: { + description: 'Description 2', + image: 'image/2.png', + name: 'ID 2', + standard: 'ERC721', + imageOriginal: 'imageOriginal/2.png', + collection: { + id: '0xtestCollection1:34567', + }, + }, + userAddress: selectedAccount.address, + source: Source.Detected, + networkClientId: undefined, + }, + ); + }, + ); + }); it('should detect and add NFTs correctly when blockaid result is in response with unsuccessful getCollections', async () => { const mockAddNft = jest.fn(); const selectedAddress = '0x123'; @@ -527,6 +663,9 @@ describe('NftDetectionController', () => { name: 'ID 2574', standard: 'ERC721', imageOriginal: 'imageOriginal/2574.png', + collection: { + id: '0xtest1', + }, }, userAddress: selectedAccount.address, source: Source.Detected, @@ -539,6 +678,9 @@ describe('NftDetectionController', () => { name: 'ID 2575', standard: 'ERC721', imageOriginal: 'imageOriginal/2575.png', + collection: { + id: '0xtest2', + }, }, userAddress: selectedAccount.address, source: Source.Detected, @@ -629,6 +771,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/2574.png', collection: { + id: '0xtest1', contractDeployedAt: undefined, creator: '0xcreator1', openseaVerificationStatus: 'verified', @@ -649,6 +792,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/2575.png', collection: { + id: '0xtest2', contractDeployedAt: undefined, creator: '0xcreator2', openseaVerificationStatus: 'verified', @@ -724,6 +868,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/1.png', collection: { + id: '0xtestCollection1', contractDeployedAt: undefined, creator: '0xcreator1', openseaVerificationStatus: 'verified', @@ -748,6 +893,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/2.png', collection: { + id: '0xtestCollection2', contractDeployedAt: undefined, creator: '0xcreator2', openseaVerificationStatus: 'verified', @@ -808,6 +954,9 @@ describe('NftDetectionController', () => { tokenURI: 'tokenURITest', }, isSpam: false, + collection: { + id: '0xtestCollection1', + }, }, blockaidResult: { // TODO: Either fix this lint violation or explain why it's necessary to ignore. @@ -829,6 +978,9 @@ describe('NftDetectionController', () => { tokenURI: 'tokenURITest', }, isSpam: false, + collection: { + id: '0xtestCollection1', + }, }, }, ], @@ -864,6 +1016,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/1.png', collection: { + id: '0xtestCollection1', contractDeployedAt: undefined, creator: '0xcreator1', openseaVerificationStatus: 'verified', @@ -888,6 +1041,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/2.png', collection: { + id: '0xtestCollection1', contractDeployedAt: undefined, creator: '0xcreator1', openseaVerificationStatus: 'verified', @@ -961,6 +1115,7 @@ describe('NftDetectionController', () => { standard: 'ERC721', imageOriginal: 'imageOriginal/2574.png', collection: { + id: '0xtest1', contractDeployedAt: undefined, creator: '0xcreator1', openseaVerificationStatus: 'verified', @@ -979,6 +1134,9 @@ describe('NftDetectionController', () => { name: 'ID 2575', standard: 'ERC721', imageOriginal: 'imageOriginal/2575.png', + collection: { + id: '0xtest2', + }, }, userAddress: selectedAccount.address, source: Source.Detected, diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 4b1dad8231..ab92a3094c 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -611,69 +611,78 @@ export class NftDetectionController extends BaseController< : true), ); // Retrieve collections from apiNfts + // contract and collection.id are equal for simple contract addresses; this is to exclude cases for shared contracts const collections = apiNfts.reduce((acc, currValue) => { - if (!acc.includes(currValue.token.contract)) { + if ( + !acc.includes(currValue.token.contract) && + currValue.token.contract === currValue?.token?.collection?.id + ) { acc.push(currValue.token.contract); } return acc; }, []); - // Call API to retrive collections infos - // The api accept a max of 20 contracts - const collectionResponse: GetCollectionsResponse = - await reduceInBatchesSerially({ - values: collections, - batchSize: MAX_GET_COLLECTION_BATCH_SIZE, - eachBatch: async (allResponses, batch) => { - const params = new URLSearchParams( - batch.map((s) => ['contract', s]), + if (collections.length !== 0) { + // Call API to retrive collections infos + // The api accept a max of 20 contracts + const collectionResponse: GetCollectionsResponse = + await reduceInBatchesSerially({ + values: collections, + batchSize: MAX_GET_COLLECTION_BATCH_SIZE, + eachBatch: async (allResponses, batch) => { + const params = new URLSearchParams( + batch.map((s) => ['contract', s]), + ); + params.append('chainId', '1'); // Adding chainId 1 because we are only detecting for mainnet + const collectionResponseForBatch = await fetchWithErrorHandling( + { + url: `${ + NFT_API_BASE_URL as string + }/collections?${params.toString()}`, + options: { + headers: { + Version: NFT_API_VERSION, + }, + }, + timeout: NFT_API_TIMEOUT, + }, + ); + + return { + ...allResponses, + ...collectionResponseForBatch, + }; + }, + initialResult: {}, + }); + + // Add collections response fields to newnfts + if (collectionResponse.collections?.length) { + apiNfts.forEach((singleNFT) => { + const found = collectionResponse.collections.find( + (elm) => + elm.id?.toLowerCase() === + singleNFT.token.contract.toLowerCase(), ); - params.append('chainId', '1'); // Adding chainId 1 because we are only detecting for mainnet - const collectionResponseForBatch = await fetchWithErrorHandling({ - url: `${ - NFT_API_BASE_URL as string - }/collections?${params.toString()}`, - options: { - headers: { - Version: NFT_API_VERSION, + if (found) { + singleNFT.token = { + ...singleNFT.token, + collection: { + ...(singleNFT.token.collection + ? singleNFT.token.collection + : {}), + creator: found?.creator, + openseaVerificationStatus: found?.openseaVerificationStatus, + contractDeployedAt: found.contractDeployedAt, + ownerCount: found.ownerCount, + topBid: found.topBid, }, - }, - timeout: NFT_API_TIMEOUT, - }); - - return { - ...allResponses, - ...collectionResponseForBatch, - }; - }, - initialResult: {}, - }); - - // Add collections response fields to newnfts - if (collectionResponse.collections?.length) { - apiNfts.forEach((singleNFT) => { - const found = collectionResponse.collections.find( - (elm) => - elm.id?.toLowerCase() === - singleNFT.token.contract.toLowerCase(), - ); - if (found) { - singleNFT.token = { - ...singleNFT.token, - collection: { - ...(singleNFT.token.collection - ? singleNFT.token.collection - : {}), - creator: found?.creator, - openseaVerificationStatus: found?.openseaVerificationStatus, - contractDeployedAt: found.contractDeployedAt, - ownerCount: found.ownerCount, - topBid: found.topBid, - }, - }; - } - }); + }; + } + }); + } } + // Proceed to add NFTs const addNftPromises = apiNfts.map(async (nft) => { const { @@ -724,7 +733,6 @@ export class NftDetectionController extends BaseController< rarityScore && { rarityScore }, collection && { collection }, ); - await this.#addNft(contract, tokenId, { nftMetadata, userAddress, From e84aa9303f00c56f0a8d2261d09450cb6f4f7b3b Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 10 Jul 2024 18:48:58 +0200 Subject: [PATCH 8/8] fix: minor refactor changes --- packages/assets-controllers/src/NftDetectionController.ts | 4 +--- packages/assets-controllers/src/assetsUtil.ts | 4 ++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index ab92a3094c..f79dd52d8f 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -668,9 +668,7 @@ export class NftDetectionController extends BaseController< singleNFT.token = { ...singleNFT.token, collection: { - ...(singleNFT.token.collection - ? singleNFT.token.collection - : {}), + ...(singleNFT.token.collection ?? {}), creator: found?.creator, openseaVerificationStatus: found?.openseaVerificationStatus, contractDeployedAt: found.contractDeployedAt, diff --git a/packages/assets-controllers/src/assetsUtil.ts b/packages/assets-controllers/src/assetsUtil.ts index 55a0828570..6105a18095 100644 --- a/packages/assets-controllers/src/assetsUtil.ts +++ b/packages/assets-controllers/src/assetsUtil.ts @@ -58,8 +58,8 @@ export function hasNewCollectionFields( newNftMetadata: NftMetadata, nft: Nft, ): boolean { - const keysNewNftMetadata = Object.keys(newNftMetadata.collection || {}); - const keysExistingNft = new Set(Object.keys(nft.collection || {})); + const keysNewNftMetadata = Object.keys(newNftMetadata.collection ?? {}); + const keysExistingNft = new Set(Object.keys(nft.collection ?? {})); return keysNewNftMetadata.some((key) => !keysExistingNft.has(key)); }