From eee87cea6fbd866b09fc1be007d875892cf7b27b Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 15 May 2024 20:35:52 +0200 Subject: [PATCH 01/11] feat: remove nft detection polling --- .../assets-controllers/src/NftController.ts | 48 ++++ .../src/NftDetectionController.ts | 238 ++++++++++-------- 2 files changed, 187 insertions(+), 99 deletions(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 83fdeae592..35f0207366 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -92,6 +92,11 @@ type NftUpdate = { newMetadata: NftMetadata; }; +export type NftFetchStatus = { + isFetchingInProgress: boolean; + lastFetchTimestamp: number; +}; + /** * @type NftContract * @@ -169,6 +174,7 @@ export type NftMetadata = { * @property allNftContracts - Object containing NFT contract information * @property allNfts - Object containing NFTs per account and network * @property ignoredNfts - List of NFTs that should be ignored + * @property isNftFetchingInProgress - Object containing boolean param indicating whether core finished fetching nfts per account and network */ export type NftControllerState = { allNftContracts: { @@ -182,6 +188,9 @@ export type NftControllerState = { }; }; ignoredNfts: Nft[]; + isNftFetchingInProgress: { + [key: string]: { [chainId: Hex]: NftFetchStatus }; + }; }; const nftControllerMetadata = { @@ -192,6 +201,7 @@ const nftControllerMetadata = { const ALL_NFTS_STATE_KEY = 'allNfts'; const ALL_NFTS_CONTRACTS_STATE_KEY = 'allNftContracts'; +const IS_NFTS_FETCHING_IN_PROGRESS_KEY = 'isNftFetchingInProgress'; type NftAsset = { address: string; @@ -242,6 +252,7 @@ export const getDefaultNftControllerState = (): NftControllerState => ({ allNftContracts: {}, allNfts: {}, ignoredNfts: [], + isNftFetchingInProgress: {}, }); /** @@ -480,6 +491,27 @@ export class NftController extends BaseController< }); } + #updateNestedNftFetchingProgressStatus( + newStatus: boolean, + { userAddress, chainId }: { userAddress: string; chainId: Hex }, + ) { + this.update((state) => { + const oldState = state[IS_NFTS_FETCHING_IN_PROGRESS_KEY]; + const addressState = oldState[userAddress] || {}; + const newAddressState = { + ...addressState, + [chainId]: { + isFetchingInProgress: newStatus, + lastFetchTimestamp: Date.now(), + }, + }; + state[IS_NFTS_FETCHING_IN_PROGRESS_KEY] = { + ...oldState, + [userAddress]: newAddressState, + }; + }); + } + /** * Request individual NFT information from NFT API. * @@ -1812,6 +1844,22 @@ export class NftController extends BaseController< }); } + updateNftFetchingProgressStatus( + newValue: boolean, + { + userAddress = this.#selectedAddress, + networkClientId, + }: { networkClientId?: NetworkClientId; userAddress?: string } = { + userAddress: this.#selectedAddress, + }, + ) { + const chainId = this.#getCorrectChainId({ networkClientId }); + this.#updateNestedNftFetchingProgressStatus(newValue, { + userAddress, + chainId, + }); + } + /** * Resets the transaction status of an NFT. * diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 25f7fd6ef4..a8eb4b29b0 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -7,6 +7,7 @@ import { NFT_API_BASE_URL, NFT_API_VERSION, NFT_API_TIMEOUT, + convertHexToDecimal, } from '@metamask/controller-utils'; import type { NetworkClientId, @@ -21,6 +22,7 @@ import type { PreferencesControllerStateChangeEvent, PreferencesState, } from '@metamask/preferences-controller'; +import type { Hex } from '@metamask/utils'; import { Source } from './constants'; import { @@ -29,6 +31,7 @@ import { type NftMetadata, } from './NftController'; + const DEFAULT_INTERVAL = 180000; const controllerName = 'NftDetectionController'; @@ -50,6 +53,7 @@ export type NftDetectionControllerMessenger = RestrictedControllerMessenger< AllowedActions['type'], AllowedEvents['type'] >; +const supportedNftDetectionNetworks: Hex[] = [ChainId.mainnet]; /** * @type ApiNft @@ -345,8 +349,11 @@ export type Metadata = { tokenURI?: string; }; +const RATE_LIMIT_NFT_DETECTION_DELAY = 600; // 10 mins +const RATE_LIMIT_NFT_DETECTION_INTERVAL = RATE_LIMIT_NFT_DETECTION_DELAY * 1000; + /** - * Controller that passively polls on a set interval for NFT auto detection + * Controller that passively detects nfts for a user address */ export class NftDetectionController extends StaticIntervalPollingController< typeof controllerName, @@ -361,6 +368,8 @@ export class NftDetectionController extends StaticIntervalPollingController< readonly #addNft: NftController['addNft']; + readonly #updateNftFetchingProgressStatus: NftController['updateNftFetchingProgressStatus']; + readonly #getNftState: () => NftControllerState; /** @@ -371,6 +380,7 @@ export class NftDetectionController extends StaticIntervalPollingController< * @param options.messenger - A reference to the messaging system. * @param options.disabled - Represents previous value of useNftDetection. Used to detect changes of useNftDetection. Default value is true. * @param options.addNft - Add an NFT. + * @param options.updateNftFetchingProgressStatus - updateNftFetchingProgressStatus. * @param options.getNftState - Gets the current state of the Assets controller. */ constructor({ @@ -378,12 +388,14 @@ export class NftDetectionController extends StaticIntervalPollingController< messenger, disabled = false, addNft, + updateNftFetchingProgressStatus, getNftState, }: { interval?: number; messenger: NftDetectionControllerMessenger; disabled: boolean; addNft: NftController['addNft']; + updateNftFetchingProgressStatus: NftController['updateNftFetchingProgressStatus']; getNftState: () => NftControllerState; }) { super({ @@ -397,6 +409,7 @@ export class NftDetectionController extends StaticIntervalPollingController< this.#getNftState = getNftState; this.#addNft = addNft; + this.#updateNftFetchingProgressStatus = updateNftFetchingProgressStatus; this.messagingSystem.subscribe( 'PreferencesController:stateChange', @@ -479,53 +492,44 @@ export class NftDetectionController extends StaticIntervalPollingController< #onPreferencesControllerStateChange({ useNftDetection }: PreferencesState) { if (!useNftDetection !== this.#disabled) { this.#disabled = !useNftDetection; - if (useNftDetection) { - this.start(); - } else { - this.stop(); - } } } - #getOwnerNftApi({ address, next }: { address: string; next?: string }) { - return `${NFT_API_BASE_URL}/users/${address}/tokens?chainIds=1&limit=50&includeTopBid=true&continuation=${ + #getOwnerNftApi({ + chainId, + address, + next, + }: { + chainId: string; + address: string; + next?: string; + }) { + return `${NFT_API_BASE_URL}/users/${address}/tokens?chainIds=${chainId}&limit=50&includeTopBid=true&continuation=${ next ?? '' }`; } - async #getOwnerNfts(address: string) { - let nftApiResponse: ReservoirResponse; - let nfts: TokensResponse[] = []; - let next; - - do { - nftApiResponse = await fetchWithErrorHandling({ - url: this.#getOwnerNftApi({ address, next }), - options: { - headers: { - Version: NFT_API_VERSION, - }, + async #getOwnerNfts( + address: string, + chainId: Hex, + cursor: string | undefined, + ) { + // Convert hex chainId to number + const convertedChainId = convertHexToDecimal(chainId).toString(); + const nftApiResponse: ReservoirResponse = await fetchWithErrorHandling({ + url: this.#getOwnerNftApi({ + chainId: convertedChainId, + address, + next: cursor, + }), + options: { + headers: { + Version: NFT_API_VERSION, }, - timeout: NFT_API_TIMEOUT, - }); - - if (!nftApiResponse) { - return nfts; - } - - const newNfts = - nftApiResponse.tokens?.filter( - (elm) => - elm.token.isSpam === false && - (elm.blockaidResult?.result_type - ? elm.blockaidResult?.result_type === BlockaidResultType.Benign - : true), - ) ?? []; - - nfts = [...nfts, ...newNfts]; - } while ((next = nftApiResponse.continuation)); - - return nfts; + }, + timeout: NFT_API_TIMEOUT, + }); + return nftApiResponse; } /** @@ -544,8 +548,18 @@ export class NftDetectionController extends StaticIntervalPollingController< options?.userAddress ?? this.messagingSystem.call('PreferencesController:getState') .selectedAddress; + + const { selectedNetworkClientId } = this.messagingSystem.call( + 'NetworkController:getState', + ); + const { + configuration: { chainId }, + } = this.messagingSystem.call( + 'NetworkController:getNetworkClientById', + selectedNetworkClientId, + ); /* istanbul ignore if */ - if (!this.isMainnet() || this.#disabled) { + if (!supportedNftDetectionNetworks.includes(chainId) || this.#disabled) { return; } /* istanbul ignore else */ @@ -553,66 +567,92 @@ export class NftDetectionController extends StaticIntervalPollingController< return; } - const apiNfts = await this.#getOwnerNfts(userAddress); - const addNftPromises = apiNfts.map(async (nft) => { - const { - tokenId: token_id, - contract, - kind, - image: image_url, - imageSmall: image_thumbnail_url, - metadata: { imageOriginal: image_original_url } = {}, - name, - description, - attributes, - topBid, - lastSale, - rarityRank, - rarityScore, - collection, - } = nft.token; - - let ignored; - /* istanbul ignore else */ - const { ignoredNfts } = this.#getNftState(); - if (ignoredNfts.length > 0) { - ignored = ignoredNfts.find((c) => { + const { isNftFetchingInProgress } = this.#getNftState(); + const time = Date.now(); + const timeSinceLastRequest = + time - isNftFetchingInProgress[userAddress]?.[chainId].lastFetchTimestamp; + + if (timeSinceLastRequest < RATE_LIMIT_NFT_DETECTION_INTERVAL) { + return; + } + + let next; + let apiNfts: TokensResponse[] = []; + let resultNftApi: ReservoirResponse; + + do { + resultNftApi = await this.#getOwnerNfts(userAddress, chainId, next); + apiNfts = resultNftApi.tokens.filter( + (elm) => + elm.token.isSpam === false && + (elm.blockaidResult?.result_type + ? elm.blockaidResult?.result_type === BlockaidResultType.Benign + : true), + ); + const addNftPromises = apiNfts.map(async (nft) => { + const { + tokenId: token_id, + contract, + kind, + image: image_url, + imageSmall: image_thumbnail_url, + metadata: { imageOriginal: image_original_url } = {}, + name, + description, + attributes, + topBid, + lastSale, + rarityRank, + rarityScore, + collection, + } = nft.token; + + let ignored; + /* istanbul ignore else */ + const { ignoredNfts } = this.#getNftState(); + if (ignoredNfts.length) { + ignored = ignoredNfts.find((c) => { + /* istanbul ignore next */ + return ( + c.address === toChecksumHexAddress(contract) && + c.tokenId === token_id + ); + }); + } + + /* istanbul ignore else */ + if (!ignored) { /* istanbul ignore next */ - return ( - c.address === toChecksumHexAddress(contract) && - c.tokenId === token_id + const nftMetadata: NftMetadata = Object.assign( + {}, + { name }, + description && { description }, + image_url && { image: image_url }, + image_thumbnail_url && { imageThumbnail: image_thumbnail_url }, + image_original_url && { imageOriginal: image_original_url }, + kind && { standard: kind.toUpperCase() }, + lastSale && { lastSale }, + attributes && { attributes }, + topBid && { topBid }, + rarityRank && { rarityRank }, + rarityScore && { rarityScore }, + collection && { collection }, ); - }); - } - - /* istanbul ignore else */ - if (!ignored) { - /* istanbul ignore next */ - const nftMetadata: NftMetadata = Object.assign( - {}, - { name }, - description && { description }, - image_url && { image: image_url }, - image_thumbnail_url && { imageThumbnail: image_thumbnail_url }, - image_original_url && { imageOriginal: image_original_url }, - kind && { standard: kind.toUpperCase() }, - lastSale && { lastSale }, - attributes && { attributes }, - topBid && { topBid }, - rarityRank && { rarityRank }, - rarityScore && { rarityScore }, - collection && { collection }, - ); - - await this.#addNft(contract, token_id, { - nftMetadata, - userAddress, - source: Source.Detected, - networkClientId: options?.networkClientId, - }); - } - }); - await Promise.all(addNftPromises); + + await this.#addNft(contract, token_id, { + nftMetadata, + userAddress, + source: Source.Detected, + networkClientId: options?.networkClientId, + }); + } + }); + await Promise.all(addNftPromises); + // update status + const isStillFetching = Boolean(resultNftApi.continuation); + + this.#updateNftFetchingProgressStatus(isStillFetching); + } while ((next = resultNftApi.continuation)); } } From c58add778d681ebdfd77e10892caf09de3f60d5b Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 3 Jun 2024 10:36:03 +0200 Subject: [PATCH 02/11] fix: fix conflicts --- packages/assets-controllers/src/NftController.ts | 1 + packages/assets-controllers/src/NftDetectionController.ts | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 35f0207366..2a99110b53 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -197,6 +197,7 @@ const nftControllerMetadata = { allNftContracts: { persist: true, anonymous: false }, allNfts: { persist: true, anonymous: false }, ignoredNfts: { persist: true, anonymous: false }, + isNftFetchingInProgress: { persist: true, anonymous: false }, }; const ALL_NFTS_STATE_KEY = 'allNfts'; diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index a8eb4b29b0..26654896a0 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -31,7 +31,6 @@ import { type NftMetadata, } from './NftController'; - const DEFAULT_INTERVAL = 180000; const controllerName = 'NftDetectionController'; From de0943de9318aa09bba99a3ab291d53b29a0aa5a Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 3 Jun 2024 15:15:10 +0200 Subject: [PATCH 03/11] fix: make nftDetectionController not a polling controller and fix tests --- .../src/NftController.test.ts | 2 + .../src/NftDetectionController.test.ts | 301 +++--------------- .../src/NftDetectionController.ts | 63 +--- 3 files changed, 52 insertions(+), 314 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 2c5004e1c3..9188d3ead6 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -251,11 +251,13 @@ describe('NftController', () => { it('should set default state', () => { const { nftController } = setupController(); + console.log('here', nftController.state); expect(nftController.state).toStrictEqual({ allNftContracts: {}, allNfts: {}, ignoredNfts: [], + isNftFetchingInProgress: {}, }); }); diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index 8984134a0e..c2b70297b8 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -20,10 +20,7 @@ import * as sinon from 'sinon'; import { FakeBlockTracker } from '../../../tests/fake-block-tracker'; import { FakeProvider } from '../../../tests/fake-provider'; import { advanceTime } from '../../../tests/helpers'; -import { - buildCustomNetworkClientConfiguration, - buildMockGetNetworkClientById, -} from '../../network-controller/tests/helpers'; +import { buildMockGetNetworkClientById } from '../../network-controller/tests/helpers'; import { Source } from './constants'; import { getDefaultNftControllerState } from './NftController'; import { @@ -33,8 +30,6 @@ import { type AllowedEvents, } from './NftDetectionController'; -const DEFAULT_INTERVAL = 180000; - const controllerName = 'NftDetectionController' as const; describe('NftDetectionController', () => { @@ -287,9 +282,9 @@ describe('NftDetectionController', () => { sinon.restore(); }); - it('should poll and detect NFTs on interval while on mainnet', async () => { + it('should call detect NFTs on mainnet', async () => { await withController( - { options: { interval: 10 } }, + { options: {} }, async ({ controller, controllerEvents }) => { const mockNfts = sinon .stub(controller, 'detectNfts') @@ -298,12 +293,9 @@ describe('NftDetectionController', () => { ...getDefaultPreferencesState(), useNftDetection: true, }); - // Wait for detect call triggered by preferences state change to settle - await advanceTime({ - clock, - duration: 1, - }); + // call detectNfts + await controller.detectNfts(); expect(mockNfts.calledOnce).toBe(true); await advanceTime({ @@ -311,12 +303,12 @@ describe('NftDetectionController', () => { duration: 10, }); - expect(mockNfts.calledTwice).toBe(true); + expect(mockNfts.calledTwice).toBe(false); }, ); }); - it('should poll and detect NFTs by networkClientId on interval while on mainnet', async () => { + it('should call detect NFTs by networkClientId on mainnet', async () => { await withController(async ({ controller }) => { const spy = jest .spyOn(controller, 'detectNfts') @@ -324,23 +316,12 @@ describe('NftDetectionController', () => { return Promise.resolve(); }); - controller.startPollingByNetworkClientId('mainnet', { - address: '0x1', + // call detectNfts + await controller.detectNfts({ + networkClientId: 'mainnet', + userAddress: '0x1', }); - await advanceTime({ clock, duration: 0 }); - expect(spy.mock.calls).toHaveLength(1); - await advanceTime({ - clock, - duration: DEFAULT_INTERVAL / 2, - }); - expect(spy.mock.calls).toHaveLength(1); - await advanceTime({ - clock, - duration: DEFAULT_INTERVAL / 2, - }); - expect(spy.mock.calls).toHaveLength(2); - await advanceTime({ clock, duration: DEFAULT_INTERVAL }); expect(spy.mock.calls).toMatchObject([ [ { @@ -348,111 +329,10 @@ describe('NftDetectionController', () => { userAddress: '0x1', }, ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], ]); }); }); - it('should not rely on the currently selected chain to poll for NFTs when a specific chain is being targeted for polling', async () => { - await withController( - { - mockNetworkClientConfigurationsByNetworkClientId: { - 'AAAA-AAAA-AAAA-AAAA': buildCustomNetworkClientConfiguration({ - chainId: '0x1337', - }), - }, - }, - async ({ controller, controllerEvents }) => { - const spy = jest - .spyOn(controller, 'detectNfts') - .mockImplementation(() => { - return Promise.resolve(); - }); - - controller.startPollingByNetworkClientId('mainnet', { - address: '0x1', - }); - - await advanceTime({ clock, duration: 0 }); - expect(spy.mock.calls).toHaveLength(1); - await advanceTime({ - clock, - duration: DEFAULT_INTERVAL / 2, - }); - expect(spy.mock.calls).toHaveLength(1); - await advanceTime({ - clock, - duration: DEFAULT_INTERVAL / 2, - }); - expect(spy.mock.calls).toHaveLength(2); - await advanceTime({ clock, duration: DEFAULT_INTERVAL }); - expect(spy.mock.calls).toMatchObject([ - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - ]); - - controllerEvents.triggerNetworkStateChange({ - ...defaultNetworkState, - selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA', - }); - await advanceTime({ clock, duration: DEFAULT_INTERVAL }); - expect(spy.mock.calls).toMatchObject([ - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - [ - { - networkClientId: 'mainnet', - userAddress: '0x1', - }, - ], - ]); - }, - ); - }); - it('should detect mainnet truthy', async () => { await withController( { @@ -485,109 +365,41 @@ describe('NftDetectionController', () => { ); }); - it('should not autodetect while not on mainnet', async () => { - await withController(async ({ controller }) => { - const mockNfts = sinon.stub(controller, 'detectNfts'); - - await controller.start(); - await advanceTime({ clock, duration: DEFAULT_INTERVAL }); - - expect(mockNfts.called).toBe(false); - }); - }); - - it('should respond to chain ID changing when using legacy polling', async () => { - const mockAddNft = jest.fn(); - const pollingInterval = 100; - + it('should return when detectNfts is called on a not supported network for detection', async () => { + const selectedAddress = '0x1'; await withController( { - options: { - interval: pollingInterval, - addNft: mockAddNft, - disabled: false, - }, - mockNetworkClientConfigurationsByNetworkClientId: { - 'AAAA-AAAA-AAAA-AAAA': buildCustomNetworkClientConfiguration({ - chainId: '0x123', - }), - }, mockNetworkState: { - selectedNetworkClientId: 'mainnet', + selectedNetworkClientId: 'goerli', }, mockPreferencesState: { - selectedAddress: '0x1', + selectedAddress, }, }, - async ({ controller, controllerEvents }) => { - await controller.start(); - // await clock.tickAsync(pollingInterval); + async ({ controller }) => { + const mockNfts = sinon.stub(controller, 'detectNfts'); - expect(mockAddNft).toHaveBeenNthCalledWith( - 1, - '0xCE7ec4B2DfB30eB6c0BB5656D33aAd6BFb4001Fc', - '2577', - { - nftMetadata: { - description: - "Redacted Remilio Babies is a collection of 10,000 neochibi pfpNFT's expanding the Milady Maker paradigm with the introduction of young J.I.T. energy and schizophrenic reactionary aesthetics. We are #REMILIONAIREs.", - image: 'https://imgtest', - imageOriginal: 'https://remilio.org/remilio/632.png', - imageThumbnail: 'https://imgSmall', - name: 'Remilio 632', - rarityRank: 8872, - rarityScore: 343.443, - standard: 'ERC721', - }, - userAddress: '0x1', - source: Source.Detected, - }, - ); - expect(mockAddNft).toHaveBeenNthCalledWith( - 2, - '0x0B0fa4fF58D28A88d63235bd0756EDca69e49e6d', - '2578', - { - nftMetadata: { - description: 'Description 2578', - image: 'https://imgtest', - imageOriginal: 'https://remilio.org/remilio/632.png', - imageThumbnail: 'https://imgSmall', - name: 'ID 2578', - rarityRank: 8872, - rarityScore: 343.443, - standard: 'ERC721', - }, - userAddress: '0x1', - source: Source.Detected, - }, - ); - expect(mockAddNft).toHaveBeenNthCalledWith( - 3, - '0xebE4e5E773AFD2bAc25De0cFafa084CFb3cBf1eD', - '2574', - { - nftMetadata: { - description: 'Description 2574', - image: 'image/2574.png', - imageOriginal: 'imageOriginal/2574.png', - name: 'ID 2574', - standard: 'ERC721', - }, - userAddress: '0x1', - source: Source.Detected, - }, - ); + // nock + const mockApiCall = nock(NFT_API_BASE_URL) + .get(`/users/${selectedAddress}/tokens`) + .query({ + continuation: '', + limit: '50', + chainIds: '1', + includeTopBid: true, + }) + .reply(200, { + tokens: [], + }); - controllerEvents.triggerNetworkStateChange({ - ...defaultNetworkState, - selectedNetworkClientId: 'AAAA-AAAA-AAAA-AAAA', + // call detectNfts + await controller.detectNfts({ + networkClientId: 'goerli', + userAddress: selectedAddress, }); - await clock.tickAsync(pollingInterval); - // Not 6 times, which is what would happen if detectNfts were called - // again - expect(mockAddNft).toHaveBeenCalledTimes(3); + expect(mockNfts.called).toBe(true); + expect(mockApiCall.isDone()).toBe(false); }, ); }); @@ -796,7 +608,7 @@ describe('NftDetectionController', () => { ); }); - it('should not autodetect NFTs that exist in the ignoreList', async () => { + it('should not detect NFTs that exist in the ignoreList', async () => { const mockAddNft = jest.fn(); const mockGetNftState = jest.fn().mockImplementation(() => { return { @@ -886,7 +698,7 @@ describe('NftDetectionController', () => { it('should not detectNfts when disabled is false and useNftDetection is true', async () => { await withController( - { options: { disabled: false, interval: 10 } }, + { options: { disabled: false } }, async ({ controller, controllerEvents }) => { const mockNfts = sinon.stub(controller, 'detectNfts'); controllerEvents.triggerPreferencesStateChange({ @@ -900,13 +712,6 @@ describe('NftDetectionController', () => { }); expect(mockNfts.calledOnce).toBe(false); - - await advanceTime({ - clock, - duration: 10, - }); - - expect(mockNfts.calledTwice).toBe(false); }, ); }); @@ -979,18 +784,6 @@ describe('NftDetectionController', () => { await withController( { mockPreferencesState: { selectedAddress } }, async ({ controller, controllerEvents }) => { - // This mock is for the initial detect call after preferences change - nock(NFT_API_BASE_URL) - .get(`/users/${selectedAddress}/tokens`) - .query({ - continuation: '', - limit: '50', - chainIds: '1', - includeTopBid: true, - }) - .reply(200, { - tokens: [], - }); controllerEvents.triggerPreferencesStateChange({ ...getDefaultPreferencesState(), selectedAddress, @@ -1048,7 +841,7 @@ describe('NftDetectionController', () => { ); }); - it('should only re-detect when relevant settings change', async () => { + it('should not call detectNfts when settings change', async () => { await withController({}, async ({ controller, controllerEvents }) => { const detectNfts = sinon.stub(controller, 'detectNfts'); @@ -1060,7 +853,7 @@ describe('NftDetectionController', () => { }); } await advanceTime({ clock, duration: 1 }); - expect(detectNfts.callCount).toBe(1); + expect(detectNfts.callCount).toBe(0); // Irrelevant preference changes shouldn't trigger a detection controllerEvents.triggerPreferencesStateChange({ @@ -1069,7 +862,7 @@ describe('NftDetectionController', () => { securityAlertsEnabled: true, }); await advanceTime({ clock, duration: 1 }); - expect(detectNfts.callCount).toBe(1); + expect(detectNfts.callCount).toBe(0); }); }); }); @@ -1168,6 +961,7 @@ async function withController( messenger: controllerMessenger, disabled: true, addNft: jest.fn(), + updateNftFetchingProgressStatus: jest.fn(), getNftState: getDefaultNftControllerState, ...options, }); @@ -1181,13 +975,8 @@ async function withController( }, }; - try { - return await testFunction({ - controller, - controllerEvents, - }); - } finally { - controller.stop(); - controller.stopAllPolling(); - } + return await testFunction({ + controller, + controllerEvents, + }); } diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 26654896a0..0dfa2734c4 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -1,5 +1,6 @@ import type { AddApprovalRequest } from '@metamask/approval-controller'; import type { RestrictedControllerMessenger } from '@metamask/base-controller'; +import { BaseController } from '@metamask/base-controller'; import { fetchWithErrorHandling, toChecksumHexAddress, @@ -16,7 +17,6 @@ import type { NetworkControllerStateChangeEvent, NetworkControllerGetStateAction, } from '@metamask/network-controller'; -import { StaticIntervalPollingController } from '@metamask/polling-controller'; import type { PreferencesControllerGetStateAction, PreferencesControllerStateChangeEvent, @@ -31,10 +31,10 @@ import { type NftMetadata, } from './NftController'; -const DEFAULT_INTERVAL = 180000; - const controllerName = 'NftDetectionController'; +export type NFTDetectionControllerState = Record; + export type AllowedActions = | AddApprovalRequest | NetworkControllerGetStateAction @@ -354,15 +354,11 @@ const RATE_LIMIT_NFT_DETECTION_INTERVAL = RATE_LIMIT_NFT_DETECTION_DELAY * 1000; /** * Controller that passively detects nfts for a user address */ -export class NftDetectionController extends StaticIntervalPollingController< +export class NftDetectionController extends BaseController< typeof controllerName, - Record, + NFTDetectionControllerState, NftDetectionControllerMessenger > { - #intervalId?: ReturnType; - - #interval: number; - #disabled: boolean; readonly #addNft: NftController['addNft']; @@ -375,7 +371,6 @@ export class NftDetectionController extends StaticIntervalPollingController< * The controller options * * @param options - The controller options. - * @param options.interval - The pooling interval. * @param options.messenger - A reference to the messaging system. * @param options.disabled - Represents previous value of useNftDetection. Used to detect changes of useNftDetection. Default value is true. * @param options.addNft - Add an NFT. @@ -383,14 +378,12 @@ export class NftDetectionController extends StaticIntervalPollingController< * @param options.getNftState - Gets the current state of the Assets controller. */ constructor({ - interval = DEFAULT_INTERVAL, messenger, disabled = false, addNft, updateNftFetchingProgressStatus, getNftState, }: { - interval?: number; messenger: NftDetectionControllerMessenger; disabled: boolean; addNft: NftController['addNft']; @@ -403,7 +396,6 @@ export class NftDetectionController extends StaticIntervalPollingController< metadata: {}, state: {}, }); - this.#interval = interval; this.#disabled = disabled; this.#getNftState = getNftState; @@ -414,51 +406,6 @@ export class NftDetectionController extends StaticIntervalPollingController< 'PreferencesController:stateChange', this.#onPreferencesControllerStateChange.bind(this), ); - - this.setIntervalLength(this.#interval); - } - - async _executePoll( - networkClientId: string, - options: { address: string }, - ): Promise { - await this.detectNfts({ networkClientId, userAddress: options.address }); - } - - /** - * Start polling for the currency rate. - */ - async start() { - if (!this.isMainnet() || this.#disabled) { - return; - } - - await this.#startPolling(); - } - - /** - * Stop polling for the currency rate. - */ - stop() { - this.#stopPolling(); - } - - #stopPolling() { - if (this.#intervalId) { - clearInterval(this.#intervalId); - } - } - - /** - * Starts a new polling interval. - * - */ - async #startPolling(): Promise { - this.#stopPolling(); - await this.detectNfts(); - this.#intervalId = setInterval(async () => { - await this.detectNfts(); - }, this.#interval); } /** From d372b438fedbbeb3306720018887a7f6b0c3cdc0 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 3 Jun 2024 15:35:40 +0200 Subject: [PATCH 04/11] fix: add test --- .../src/NftController.test.ts | 24 +++++++++++++++++++ .../assets-controllers/src/NftController.ts | 15 ++++++++++++ 2 files changed, 39 insertions(+) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 9188d3ead6..a2654b6110 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -3920,4 +3920,28 @@ describe('NftController', () => { expect(spy).toHaveBeenCalledTimes(1); }); }); + + describe('updateNftFetchingProgressStatus', () => { + it('should update nft fetching status correctly', async () => { + const selectedAddress = OWNER_ADDRESS; + const { nftController } = setupController({ + options: { + chainId: ChainId.mainnet, + selectedAddress, + getERC721AssetName: jest.fn().mockResolvedValue('Name'), + }, + }); + + nftController.updateNftFetchingProgressStatus(true); + + expect( + nftController.state.isNftFetchingInProgress[selectedAddress][ + ChainId.mainnet + ], + ).toStrictEqual({ + isFetchingInProgress: true, + lastFetchTimestamp: expect.any(Number), + }); + }); + }); }); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 2a99110b53..c7f80f7632 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -492,6 +492,14 @@ export class NftController extends BaseController< }); } + /** + * Helper function to update the status of nft fetching for a specific address. + * + * @param newStatus - The new status to set in state + * @param passedConfig - Object containing selectedAddress and chainId + * @param passedConfig.userAddress - the address passed through the detectNfts function + * @param passedConfig.chainId - the chainId passed through the detectNfts function + */ #updateNestedNftFetchingProgressStatus( newStatus: boolean, { userAddress, chainId }: { userAddress: string; chainId: Hex }, @@ -1845,6 +1853,13 @@ export class NftController extends BaseController< }); } + /** + * Update Nft fetching status for a selectedAddress on a specific chain + * @param newValue - fetching status to set in state + * @param passedConfig - Object containing selectedAddress and chainId + * @param passedConfig.userAddress - the address passed through the detectNfts function + * @param passedConfig.networkClientId - the networkClientId passed through the detectNfts function + */ updateNftFetchingProgressStatus( newValue: boolean, { From ca3049f892010ceeb7e03fef13b03de984777a23 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 6 Jun 2024 11:20:45 +0200 Subject: [PATCH 05/11] fix: remove timestamp check --- packages/assets-controllers/src/NftController.test.ts | 1 - packages/assets-controllers/src/NftController.ts | 2 -- packages/assets-controllers/src/NftDetectionController.ts | 8 +------- 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index a2654b6110..8fb47414e6 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -3940,7 +3940,6 @@ describe('NftController', () => { ], ).toStrictEqual({ isFetchingInProgress: true, - lastFetchTimestamp: expect.any(Number), }); }); }); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index c7f80f7632..cf083218eb 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -94,7 +94,6 @@ type NftUpdate = { export type NftFetchStatus = { isFetchingInProgress: boolean; - lastFetchTimestamp: number; }; /** @@ -511,7 +510,6 @@ export class NftController extends BaseController< ...addressState, [chainId]: { isFetchingInProgress: newStatus, - lastFetchTimestamp: Date.now(), }, }; state[IS_NFTS_FETCHING_IN_PROGRESS_KEY] = { diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 0dfa2734c4..af64ef2613 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -348,9 +348,6 @@ export type Metadata = { tokenURI?: string; }; -const RATE_LIMIT_NFT_DETECTION_DELAY = 600; // 10 mins -const RATE_LIMIT_NFT_DETECTION_INTERVAL = RATE_LIMIT_NFT_DETECTION_DELAY * 1000; - /** * Controller that passively detects nfts for a user address */ @@ -514,11 +511,8 @@ export class NftDetectionController extends BaseController< } const { isNftFetchingInProgress } = this.#getNftState(); - const time = Date.now(); - const timeSinceLastRequest = - time - isNftFetchingInProgress[userAddress]?.[chainId].lastFetchTimestamp; - if (timeSinceLastRequest < RATE_LIMIT_NFT_DETECTION_INTERVAL) { + if (isNftFetchingInProgress.isFetchingInProgress) { return; } From d9c2fc9fc06dce2b297677408794f461eec5a2cd Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 6 Jun 2024 11:21:53 +0200 Subject: [PATCH 06/11] fix: remove log --- packages/assets-controllers/src/NftController.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 8fb47414e6..12927fcd06 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -251,8 +251,6 @@ describe('NftController', () => { it('should set default state', () => { const { nftController } = setupController(); - console.log('here', nftController.state); - expect(nftController.state).toStrictEqual({ allNftContracts: {}, allNfts: {}, From 0a778e1e3011f91246b2578deb3863947144dc8f Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 6 Jun 2024 12:39:14 +0200 Subject: [PATCH 07/11] fix: fix --- packages/assets-controllers/src/NftDetectionController.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index af64ef2613..96b92d3f74 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -512,7 +512,7 @@ export class NftDetectionController extends BaseController< const { isNftFetchingInProgress } = this.#getNftState(); - if (isNftFetchingInProgress.isFetchingInProgress) { + if (isNftFetchingInProgress[userAddress]?.[chainId].isFetchingInProgress) { return; } From 61bdd23f8000deb9b0d0fbf1d9880496944bcab6 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Fri, 7 Jun 2024 10:55:41 +0200 Subject: [PATCH 08/11] fix: update fetching status on error --- .../src/NftDetectionController.test.ts | 43 +++++++++++++++++++ .../src/NftDetectionController.ts | 27 ++++++------ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index c2b70297b8..aa2405072a 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -779,6 +779,49 @@ describe('NftDetectionController', () => { ); }); + it('should call updateNftFetchingProgressStatus when call to NFT-API fails', async () => { + const selectedAddress = '0x4444'; + const mockAddNft = jest.fn(); + const mockUpdateNftFetchingProgressStatus = jest.fn(); + await withController( + { + mockPreferencesState: { selectedAddress }, + options: { + addNft: mockAddNft, + updateNftFetchingProgressStatus: mockUpdateNftFetchingProgressStatus, + }, + }, + async ({ controller, controllerEvents }) => { + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + selectedAddress, + useNftDetection: true, + }); + // Wait for detect call triggered by preferences state change to settle + await advanceTime({ + clock, + duration: 1, + }); + // This mock is for the call under test + nock(NFT_API_BASE_URL) + .get(`/users/${selectedAddress}/tokens`) + .query({ + continuation: '', + limit: '50', + chainIds: '1', + includeTopBid: true, + }) + .replyWithError(new Error('UNEXPECTED ERROR!!')); + + await expect(() => controller.detectNfts()).rejects.toThrow( + 'UNEXPECTED ERROR!!', + ); + expect(mockAddNft).not.toHaveBeenCalled(); + expect(mockUpdateNftFetchingProgressStatus).toHaveBeenCalledWith(false); + }, + ); + }); + it('should rethrow error when Nft APi server fails with error other than fetch failure', async () => { const selectedAddress = '0x4'; await withController( diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 96b92d3f74..ff091462f0 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -2,13 +2,12 @@ import type { AddApprovalRequest } from '@metamask/approval-controller'; import type { RestrictedControllerMessenger } from '@metamask/base-controller'; import { BaseController } from '@metamask/base-controller'; import { - fetchWithErrorHandling, toChecksumHexAddress, ChainId, NFT_API_BASE_URL, NFT_API_VERSION, - NFT_API_TIMEOUT, convertHexToDecimal, + handleFetch, } from '@metamask/controller-utils'; import type { NetworkClientId, @@ -459,20 +458,22 @@ export class NftDetectionController extends BaseController< ) { // Convert hex chainId to number const convertedChainId = convertHexToDecimal(chainId).toString(); - const nftApiResponse: ReservoirResponse = await fetchWithErrorHandling({ - url: this.#getOwnerNftApi({ - chainId: convertedChainId, - address, - next: cursor, - }), - options: { + const url = this.#getOwnerNftApi({ + chainId: convertedChainId, + address, + next: cursor, + }); + try { + const nftApiResponse: ReservoirResponse = await handleFetch(url, { headers: { Version: NFT_API_VERSION, }, - }, - timeout: NFT_API_TIMEOUT, - }); - return nftApiResponse; + }); + return nftApiResponse; + } catch (err) { + this.#updateNftFetchingProgressStatus(false); + throw err; + } } /** From 85d2f52fc7764088392ba9d9e88f21e27b84f7d3 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 11 Jun 2024 15:25:05 +0200 Subject: [PATCH 09/11] fix: rm isNftFetchingInProgress and use stateless approach instead --- .../src/NftController.test.ts | 26 +-- .../assets-controllers/src/NftController.ts | 64 ------ .../src/NftDetectionController.test.ts | 75 +++---- .../src/NftDetectionController.ts | 191 +++++++++--------- 4 files changed, 126 insertions(+), 230 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 91b5f7e308..c5e83da9c4 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -331,11 +331,11 @@ describe('NftController', () => { it('should set default state', () => { const { nftController } = setupController(); + expect(nftController.state).toStrictEqual({ allNftContracts: {}, allNfts: {}, ignoredNfts: [], - isNftFetchingInProgress: {}, }); }); @@ -4350,26 +4350,4 @@ describe('NftController', () => { expect(updateNftMetadataSpy).not.toHaveBeenCalled(); }); - - describe('updateNftFetchingProgressStatus', () => { - it('should update nft fetching status correctly', async () => { - const selectedAddress = OWNER_ADDRESS; - const { nftController } = setupController({ - options: { - chainId: ChainId.mainnet, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), - }, - }); - - nftController.updateNftFetchingProgressStatus(true); - - expect( - nftController.state.isNftFetchingInProgress[selectedAddress][ - ChainId.mainnet - ], - ).toStrictEqual({ - isFetchingInProgress: true, - }); - }); - }); -}); +}); \ No newline at end of file diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 6f9437bec2..8b79b778d8 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -98,10 +98,6 @@ type NftUpdate = { newMetadata: NftMetadata; }; -export type NftFetchStatus = { - isFetchingInProgress: boolean; -}; - /** * @type NftContract * @@ -179,7 +175,6 @@ export type NftMetadata = { * @property allNftContracts - Object containing NFT contract information * @property allNfts - Object containing NFTs per account and network * @property ignoredNfts - List of NFTs that should be ignored - * @property isNftFetchingInProgress - Object containing boolean param indicating whether core finished fetching nfts per account and network */ export type NftControllerState = { allNftContracts: { @@ -193,21 +188,16 @@ export type NftControllerState = { }; }; ignoredNfts: Nft[]; - isNftFetchingInProgress: { - [key: string]: { [chainId: Hex]: NftFetchStatus }; - }; }; const nftControllerMetadata = { allNftContracts: { persist: true, anonymous: false }, allNfts: { persist: true, anonymous: false }, ignoredNfts: { persist: true, anonymous: false }, - isNftFetchingInProgress: { persist: true, anonymous: false }, }; const ALL_NFTS_STATE_KEY = 'allNfts'; const ALL_NFTS_CONTRACTS_STATE_KEY = 'allNftContracts'; -const IS_NFTS_FETCHING_IN_PROGRESS_KEY = 'isNftFetchingInProgress'; type NftAsset = { address: string; @@ -261,7 +251,6 @@ export const getDefaultNftControllerState = (): NftControllerState => ({ allNftContracts: {}, allNfts: {}, ignoredNfts: [], - isNftFetchingInProgress: {}, }); /** @@ -523,34 +512,6 @@ export class NftController extends BaseController< }); } - /** - * Helper function to update the status of nft fetching for a specific address. - * - * @param newStatus - The new status to set in state - * @param passedConfig - Object containing selectedAddress and chainId - * @param passedConfig.userAddress - the address passed through the detectNfts function - * @param passedConfig.chainId - the chainId passed through the detectNfts function - */ - #updateNestedNftFetchingProgressStatus( - newStatus: boolean, - { userAddress, chainId }: { userAddress: string; chainId: Hex }, - ) { - this.update((state) => { - const oldState = state[IS_NFTS_FETCHING_IN_PROGRESS_KEY]; - const addressState = oldState[userAddress] || {}; - const newAddressState = { - ...addressState, - [chainId]: { - isFetchingInProgress: newStatus, - }, - }; - state[IS_NFTS_FETCHING_IN_PROGRESS_KEY] = { - ...oldState, - [userAddress]: newAddressState, - }; - }); - } - /** * Request individual NFT information from NFT API. * @@ -1929,31 +1890,6 @@ export class NftController extends BaseController< }); } - /** - * Update Nft fetching status for a selectedAddress on a specific chain - * @param newValue - fetching status to set in state - * @param passedConfig - Object containing selectedAddress and chainId - * @param passedConfig.userAddress - the address passed through the detectNfts function - * @param passedConfig.networkClientId - the networkClientId passed through the detectNfts function - */ - updateNftFetchingProgressStatus( - newValue: boolean, - { - networkClientId, - userAddress, - }: { - networkClientId?: NetworkClientId; - userAddress?: string; - } = {}, - ) { - const addressToSearch = this.#getAddressOrSelectedAddress(userAddress); - const chainId = this.#getCorrectChainId({ networkClientId }); - this.#updateNestedNftFetchingProgressStatus(newValue, { - userAddress: addressToSearch, - chainId, - }); - } - /** * Resets the transaction status of an NFT. * diff --git a/packages/assets-controllers/src/NftDetectionController.test.ts b/packages/assets-controllers/src/NftDetectionController.test.ts index 15535c3175..38b31b0d0d 100644 --- a/packages/assets-controllers/src/NftDetectionController.test.ts +++ b/packages/assets-controllers/src/NftDetectionController.test.ts @@ -841,54 +841,6 @@ describe('NftDetectionController', () => { ); }); - it('should call updateNftFetchingProgressStatus when call to NFT-API fails', async () => { - const selectedAddress = '0x4'; - const mockAddNft = jest.fn(); - const mockUpdateNftFetchingProgressStatus = jest.fn(); - const selectedAccount = createMockInternalAccount({ - address: selectedAddress, - }); - const mockGetSelectedAccount = jest.fn().mockReturnValue(selectedAccount); - await withController( - { - mockPreferencesState: {}, - options: { - addNft: mockAddNft, - updateNftFetchingProgressStatus: mockUpdateNftFetchingProgressStatus, - }, - mockGetSelectedAccount, - }, - async ({ controller, controllerEvents }) => { - controllerEvents.triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - selectedAddress, - useNftDetection: true, - }); - // Wait for detect call triggered by preferences state change to settle - await advanceTime({ - clock, - duration: 1, - }); - // This mock is for the call under test - nock(NFT_API_BASE_URL) - .get(`/users/${selectedAddress}/tokens`) - .query({ - continuation: '', - limit: '50', - chainIds: '1', - includeTopBid: true, - }) - .replyWithError(new Error('UNEXPECTED ERROR!!')); - - await expect(() => controller.detectNfts()).rejects.toThrow( - 'UNEXPECTED ERROR!!', - ); - expect(mockAddNft).not.toHaveBeenCalled(); - expect(mockUpdateNftFetchingProgressStatus).toHaveBeenCalledWith(false); - }, - ); - }); - it('should rethrow error when Nft APi server fails with error other than fetch failure', async () => { const selectedAddress = '0x4'; const selectedAccount = createMockInternalAccount({ @@ -993,6 +945,32 @@ describe('NftDetectionController', () => { }, ); }); + + it('should only updates once when detectNfts called twice', async () => { + const mockAddNft = jest.fn(); + const mockGetSelectedAccount = jest.fn(); + const selectedAddress = '0x9'; + const selectedAccount = createMockInternalAccount({ + address: selectedAddress, + }); + await withController( + { + options: { addNft: mockAddNft, disabled: false }, + mockPreferencesState: {}, + mockGetSelectedAccount, + }, + async ({ controller, controllerEvents }) => { + mockGetSelectedAccount.mockReturnValue(selectedAccount); + controllerEvents.triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + useNftDetection: true, + }); + await Promise.all([controller.detectNfts(), controller.detectNfts()]); + + expect(mockAddNft).toHaveBeenCalledTimes(1); + }, + ); + }); }); /** @@ -1099,7 +1077,6 @@ async function withController( messenger: controllerMessenger, disabled: true, addNft: jest.fn(), - updateNftFetchingProgressStatus: jest.fn(), getNftState: getDefaultNftControllerState, ...options, }); diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index 7bb3b669bb..a5cb2342c3 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -22,7 +22,7 @@ import type { PreferencesControllerStateChangeEvent, PreferencesState, } from '@metamask/preferences-controller'; -import type { Hex } from '@metamask/utils'; +import { createDeferredPromise, type Hex } from '@metamask/utils'; import { Source } from './constants'; import { @@ -411,10 +411,10 @@ export class NftDetectionController extends BaseController< readonly #addNft: NftController['addNft']; - readonly #updateNftFetchingProgressStatus: NftController['updateNftFetchingProgressStatus']; - readonly #getNftState: () => NftControllerState; + #inProcessNftFetchingUpdates: Record<`${Hex}:${string}`, Promise>; + /** * The controller options * @@ -422,20 +422,17 @@ export class NftDetectionController extends BaseController< * @param options.messenger - A reference to the messaging system. * @param options.disabled - Represents previous value of useNftDetection. Used to detect changes of useNftDetection. Default value is true. * @param options.addNft - Add an NFT. - * @param options.updateNftFetchingProgressStatus - updateNftFetchingProgressStatus. * @param options.getNftState - Gets the current state of the Assets controller. */ constructor({ messenger, disabled = false, addNft, - updateNftFetchingProgressStatus, getNftState, }: { messenger: NftDetectionControllerMessenger; disabled: boolean; addNft: NftController['addNft']; - updateNftFetchingProgressStatus: NftController['updateNftFetchingProgressStatus']; getNftState: () => NftControllerState; }) { super({ @@ -445,10 +442,10 @@ export class NftDetectionController extends BaseController< state: {}, }); this.#disabled = disabled; + this.#inProcessNftFetchingUpdates = {}; this.#getNftState = getNftState; this.#addNft = addNft; - this.#updateNftFetchingProgressStatus = updateNftFetchingProgressStatus; this.messagingSystem.subscribe( 'PreferencesController:stateChange', @@ -515,17 +512,12 @@ export class NftDetectionController extends BaseController< address, next: cursor, }); - try { - const nftApiResponse: ReservoirResponse = await handleFetch(url, { - headers: { - Version: NFT_API_VERSION, - }, - }); - return nftApiResponse; - } catch (err) { - this.#updateNftFetchingProgressStatus(false); - throw err; - } + const nftApiResponse: ReservoirResponse = await handleFetch(url, { + headers: { + Version: NFT_API_VERSION, + }, + }); + return nftApiResponse; } /** @@ -564,89 +556,102 @@ export class NftDetectionController extends BaseController< return; } - const { isNftFetchingInProgress } = this.#getNftState(); - - if (isNftFetchingInProgress[userAddress]?.[chainId].isFetchingInProgress) { + const updateKey: `${Hex}:${string}` = `${chainId}:${userAddress}`; + if (updateKey in this.#inProcessNftFetchingUpdates) { + // This prevents redundant updates + // This promise is resolved after the in-progress update has finished, + // and state has been updated. + await this.#inProcessNftFetchingUpdates[updateKey]; return; } + const { + promise: inProgressUpdate, + resolve: updateSucceeded, + reject: updateFailed, + } = createDeferredPromise({ suppressUnhandledRejection: true }); + this.#inProcessNftFetchingUpdates[updateKey] = inProgressUpdate; + let next; let apiNfts: TokensResponse[] = []; let resultNftApi: ReservoirResponse; - - do { - resultNftApi = await this.#getOwnerNfts(userAddress, chainId, next); - apiNfts = resultNftApi.tokens.filter( - (elm) => - elm.token.isSpam === false && - (elm.blockaidResult?.result_type - ? elm.blockaidResult?.result_type === BlockaidResultType.Benign - : true), - ); - const addNftPromises = apiNfts.map(async (nft) => { - const { - tokenId: TokenId, - contract, - kind, - image: ImageUrl, - imageSmall: ImageThumbnailUrl, - metadata: { imageOriginal: ImageOriginalUrl } = {}, - name, - description, - attributes, - topBid, - lastSale, - rarityRank, - rarityScore, - collection, - } = nft.token; - - let ignored; - /* istanbul ignore else */ - const { ignoredNfts } = this.#getNftState(); - if (ignoredNfts.length) { - ignored = ignoredNfts.find((c) => { + try { + do { + resultNftApi = await this.#getOwnerNfts(userAddress, chainId, next); + apiNfts = resultNftApi.tokens.filter( + (elm) => + elm.token.isSpam === false && + (elm.blockaidResult?.result_type + ? elm.blockaidResult?.result_type === BlockaidResultType.Benign + : true), + ); + const addNftPromises = apiNfts.map(async (nft) => { + const { + tokenId: TokenId, + contract, + kind, + image: ImageUrl, + imageSmall: ImageThumbnailUrl, + metadata: { imageOriginal: ImageOriginalUrl } = {}, + name, + description, + attributes, + topBid, + lastSale, + rarityRank, + rarityScore, + collection, + } = nft.token; + + let ignored; + /* istanbul ignore else */ + const { ignoredNfts } = this.#getNftState(); + if (ignoredNfts.length) { + ignored = ignoredNfts.find((c) => { + /* istanbul ignore next */ + return ( + c.address === toChecksumHexAddress(contract) && + c.tokenId === TokenId + ); + }); + } + + /* istanbul ignore else */ + if (!ignored) { /* istanbul ignore next */ - return ( - c.address === toChecksumHexAddress(contract) && - c.tokenId === TokenId + const nftMetadata: NftMetadata = Object.assign( + {}, + { name }, + description && { description }, + ImageUrl && { image: ImageUrl }, + ImageThumbnailUrl && { imageThumbnail: ImageThumbnailUrl }, + ImageOriginalUrl && { imageOriginal: ImageOriginalUrl }, + kind && { standard: kind.toUpperCase() }, + lastSale && { lastSale }, + attributes && { attributes }, + topBid && { topBid }, + rarityRank && { rarityRank }, + rarityScore && { rarityScore }, + collection && { collection }, ); - }); - } - - /* istanbul ignore else */ - if (!ignored) { - /* istanbul ignore next */ - const nftMetadata: NftMetadata = Object.assign( - {}, - { name }, - description && { description }, - ImageUrl && { image: ImageUrl }, - ImageThumbnailUrl && { imageThumbnail: ImageThumbnailUrl }, - ImageOriginalUrl && { imageOriginal: ImageOriginalUrl }, - kind && { standard: kind.toUpperCase() }, - lastSale && { lastSale }, - attributes && { attributes }, - topBid && { topBid }, - rarityRank && { rarityRank }, - rarityScore && { rarityScore }, - collection && { collection }, - ); - - await this.#addNft(contract, TokenId, { - nftMetadata, - userAddress, - source: Source.Detected, - networkClientId: options?.networkClientId, - }); - } - }); - await Promise.all(addNftPromises); - // update status - const isStillFetching = Boolean(resultNftApi.continuation); - - this.#updateNftFetchingProgressStatus(isStillFetching); - } while ((next = resultNftApi.continuation)); + + await this.#addNft(contract, TokenId, { + nftMetadata, + userAddress, + source: Source.Detected, + networkClientId: options?.networkClientId, + }); + } + }); + await Promise.all(addNftPromises); + } while ((next = resultNftApi.continuation)); + updateSucceeded(); + } catch (error) { + updateFailed(error); + throw error; + } finally { + delete this.#inProcessNftFetchingUpdates[updateKey]; + } } } From fe30b8c2cf06e11b30527392b383b4fe5f59e0e2 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 11 Jun 2024 15:49:50 +0200 Subject: [PATCH 10/11] fix: lint --- packages/assets-controllers/src/NftController.test.ts | 2 +- packages/assets-controllers/src/NftDetectionController.ts | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index c5e83da9c4..05e2961643 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -4350,4 +4350,4 @@ describe('NftController', () => { expect(updateNftMetadataSpy).not.toHaveBeenCalled(); }); -}); \ No newline at end of file +}); diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index a5cb2342c3..b60ea07659 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -495,6 +495,7 @@ export class NftDetectionController extends BaseController< address: string; next?: string; }) { + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions return `${NFT_API_BASE_URL}/users/${address}/tokens?chainIds=${chainId}&limit=50&includeTopBid=true&continuation=${ next ?? '' }`; @@ -556,6 +557,7 @@ export class NftDetectionController extends BaseController< return; } + // eslint-disable-next-line @typescript-eslint/restrict-template-expressions const updateKey: `${Hex}:${string}` = `${chainId}:${userAddress}`; if (updateKey in this.#inProcessNftFetchingUpdates) { // This prevents redundant updates From 860230baef2e3c496e012d543dca44b29e7c6ba2 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 11 Jun 2024 22:53:43 +0200 Subject: [PATCH 11/11] fix: minor refactoring --- .../src/NftDetectionController.ts | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/assets-controllers/src/NftDetectionController.ts b/packages/assets-controllers/src/NftDetectionController.ts index b60ea07659..63088a597f 100644 --- a/packages/assets-controllers/src/NftDetectionController.ts +++ b/packages/assets-controllers/src/NftDetectionController.ts @@ -495,8 +495,9 @@ export class NftDetectionController extends BaseController< address: string; next?: string; }) { - // eslint-disable-next-line @typescript-eslint/restrict-template-expressions - return `${NFT_API_BASE_URL}/users/${address}/tokens?chainIds=${chainId}&limit=50&includeTopBid=true&continuation=${ + return `${ + NFT_API_BASE_URL as string + }/users/${address}/tokens?chainIds=${chainId}&limit=50&includeTopBid=true&continuation=${ next ?? '' }`; } @@ -589,12 +590,12 @@ export class NftDetectionController extends BaseController< ); const addNftPromises = apiNfts.map(async (nft) => { const { - tokenId: TokenId, + tokenId, contract, kind, - image: ImageUrl, - imageSmall: ImageThumbnailUrl, - metadata: { imageOriginal: ImageOriginalUrl } = {}, + image: imageUrl, + imageSmall: imageThumbnailUrl, + metadata: { imageOriginal: imageOriginalUrl } = {}, name, description, attributes, @@ -613,7 +614,7 @@ export class NftDetectionController extends BaseController< /* istanbul ignore next */ return ( c.address === toChecksumHexAddress(contract) && - c.tokenId === TokenId + c.tokenId === tokenId ); }); } @@ -625,9 +626,9 @@ export class NftDetectionController extends BaseController< {}, { name }, description && { description }, - ImageUrl && { image: ImageUrl }, - ImageThumbnailUrl && { imageThumbnail: ImageThumbnailUrl }, - ImageOriginalUrl && { imageOriginal: ImageOriginalUrl }, + imageUrl && { image: imageUrl }, + imageThumbnailUrl && { imageThumbnail: imageThumbnailUrl }, + imageOriginalUrl && { imageOriginal: imageOriginalUrl }, kind && { standard: kind.toUpperCase() }, lastSale && { lastSale }, attributes && { attributes }, @@ -637,7 +638,7 @@ export class NftDetectionController extends BaseController< collection && { collection }, ); - await this.#addNft(contract, TokenId, { + await this.#addNft(contract, tokenId, { nftMetadata, userAddress, source: Source.Detected,