From 875e8a19eaddcbb95599828db6ff3459b17dcecc Mon Sep 17 00:00:00 2001 From: Charly Chevalier Date: Thu, 16 Jan 2025 11:14:26 +0100 Subject: [PATCH 1/2] test(accounts-controller): improve uuid mocks (#5005) ## Explanation Unify the UUID mocking. With this new refactor we longer have to specify each returned values of `mockUUID`. It also handles the non-registered accounts (in this case, a random UUID will be generated). ## References N/A ## Changelog N/A (those changes only impact tests) ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [x] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --- .../src/AccountsController.test.ts | 135 +++++++++--------- 1 file changed, 65 insertions(+), 70 deletions(-) diff --git a/packages/accounts-controller/src/AccountsController.test.ts b/packages/accounts-controller/src/AccountsController.test.ts index b28666d152..8c612d1e18 100644 --- a/packages/accounts-controller/src/AccountsController.test.ts +++ b/packages/accounts-controller/src/AccountsController.test.ts @@ -133,9 +133,6 @@ const mockAccount4: InternalAccount = { }, }; -/** - * Mock generated normal account ID to an actual "hard-coded" one. - */ class MockNormalAccountUUID { #accountIds: Record = {}; @@ -158,6 +155,18 @@ class MockNormalAccountUUID { } } +/** + * Mock generated normal account ID to their actual mock ID. This function will + * automatically attaches those accounts to `mockUUID`. A random UUID will be + * generated if an account has not been registered. See {@link MockNormalAccountUUID}. + * + * @param accounts - List of normal accounts to map with their mock ID. + */ +function mockUUIDWithNormalAccounts(accounts: InternalAccount[]) { + const mockAccountUUIDs = new MockNormalAccountUUID(accounts); + mockUUID.mockImplementation(mockAccountUUIDs.mock.bind(mockAccountUUIDs)); +} + /** * Creates an `InternalAccount` object from the given normal account properties. * @@ -466,9 +475,8 @@ describe('AccountsController', () => { describe('onKeyringStateChange', () => { it('uses listMultichainAccounts', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const { accountsController } = setupAccountsController({ initialState: { @@ -562,10 +570,8 @@ describe('AccountsController', () => { describe('adding accounts', () => { it('add new accounts', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockNewKeyringState = { isUnlocked: true, @@ -604,7 +610,7 @@ describe('AccountsController', () => { }); it('add Snap accounts', async () => { - mockUUID.mockReturnValueOnce('mock-id'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -671,7 +677,8 @@ describe('AccountsController', () => { }); it('handle the event when a Snap deleted the account before the it was added', async () => { - mockUUID.mockReturnValueOnce('mock-id'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -729,11 +736,8 @@ describe('AccountsController', () => { it('increment the default account number when adding an account', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id3') // call to check if its a new account - .mockReturnValueOnce('mock-id3'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockNewKeyringState = { isUnlocked: true, @@ -785,11 +789,8 @@ describe('AccountsController', () => { it('use the next number after the total number of accounts of a keyring when adding an account, if the index is lower', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id3') // call to check if its a new account - .mockReturnValueOnce('mock-id3'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockAccount2WithCustomName = createExpectedInternalAccount({ id: 'mock-id2', @@ -847,7 +848,7 @@ describe('AccountsController', () => { }); it('handle when the account to set as selectedAccount is undefined', async () => { - mockUUID.mockReturnValueOnce('mock-id'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -897,10 +898,8 @@ describe('AccountsController', () => { it('selectedAccount remains the same after adding a new account', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2, mockAccount3]); const mockNewKeyringState = { isUnlocked: true, @@ -942,10 +941,8 @@ describe('AccountsController', () => { it('publishes accountAdded event', async () => { const messenger = buildMessenger(); const messengerSpy = jest.spyOn(messenger, 'publish'); - mockUUID - .mockReturnValueOnce(mockAccount.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); setupAccountsController({ initialState: { @@ -987,7 +984,8 @@ describe('AccountsController', () => { describe('deleting account', () => { it('delete accounts if its gone from the keyring state', async () => { const messenger = buildMessenger(); - mockUUID.mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount2]); const mockNewKeyringState = { isUnlocked: true, @@ -1027,11 +1025,8 @@ describe('AccountsController', () => { it('delete accounts and set the most recent lastSelected account', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2') - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const mockNewKeyringState = { isUnlocked: true, @@ -1083,11 +1078,8 @@ describe('AccountsController', () => { it('delete accounts and set the most recent lastSelected account when there are accounts that have never been selected', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2') - .mockReturnValueOnce('mock-id') - .mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const mockAccount2WithoutLastSelected = { ...mockAccount2, @@ -1147,7 +1139,8 @@ describe('AccountsController', () => { it('delete the account and select the account with the most recent lastSelected', async () => { const currentTime = Date.now(); const messenger = buildMessenger(); - mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const mockAccountWithoutLastSelected = { ...mockAccount, @@ -1222,10 +1215,8 @@ describe('AccountsController', () => { it('publishes accountRemoved event', async () => { const messenger = buildMessenger(); const messengerSpy = jest.spyOn(messenger, 'publish'); - mockUUID - .mockReturnValueOnce(mockAccount.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id) // call to check if its a new account - .mockReturnValueOnce(mockAccount2.id); // call to add account + + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); setupAccountsController({ initialState: { @@ -1278,9 +1269,11 @@ describe('AccountsController', () => { address: '0x456', keyringType: KeyringTypes.hd, }); - mockUUID - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to add account + + mockUUIDWithNormalAccounts([ + mockInitialAccount, + mockReinitialisedAccount, + ]); const mockNewKeyringState = { isUnlocked: true, @@ -1361,9 +1354,7 @@ describe('AccountsController', () => { }); mockExistingAccount2.metadata.lastSelected = lastSelectedForAccount2; - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); const { accountsController } = setupAccountsController({ initialState: { @@ -1447,7 +1438,8 @@ describe('AccountsController', () => { }); it('update accounts with normal accounts', async () => { - mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getAccounts', @@ -1492,6 +1484,7 @@ describe('AccountsController', () => { keyringType: KeyringTypes.hd, }), ]; + mockUUIDWithNormalAccounts(expectedAccounts); await accountsController.updateAccounts(); @@ -1588,7 +1581,8 @@ describe('AccountsController', () => { }); it('set the account with the correct index', async () => { - mockUUID.mockReturnValueOnce('mock-id').mockReturnValueOnce('mock-id2'); + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getAccounts', @@ -1630,6 +1624,7 @@ describe('AccountsController', () => { keyringType: KeyringTypes.hd, }), ]; + mockUUIDWithNormalAccounts(expectedAccounts); await accountsController.updateAccounts(); @@ -1639,7 +1634,8 @@ describe('AccountsController', () => { }); it('filter Snap accounts from normalAccounts', async () => { - mockUUID.mockReturnValueOnce('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -1696,7 +1692,8 @@ describe('AccountsController', () => { }); it('filter Snap accounts from normalAccounts even if the snap account is listed before normal accounts', async () => { - mockUUID.mockReturnValue('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); + const messenger = buildMessenger(); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -1762,7 +1759,7 @@ describe('AccountsController', () => { KeyringTypes.qr, 'Custody - JSON - RPC', ])('should add accounts for %s type', async (keyringType) => { - mockUUID.mockReturnValue('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -1811,7 +1808,7 @@ describe('AccountsController', () => { }); it('throw an error if the keyring type is unknown', async () => { - mockUUID.mockReturnValue('mock-id'); + mockUUIDWithNormalAccounts([mockAccount]); const messenger = buildMessenger(); messenger.registerActionHandler( @@ -1892,9 +1889,7 @@ describe('AccountsController', () => { }); mockExistingAccount2.metadata.lastSelected = lastSelectedForAccount2; - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2'); // call to check if its a new account + mockUUIDWithNormalAccounts([mockAccount, mockAccount2]); messenger.registerActionHandler( 'KeyringController:getKeyringsByType', @@ -2544,12 +2539,12 @@ describe('AccountsController', () => { it('return the next account number', async () => { const messenger = buildMessenger(); - mockUUID - .mockReturnValueOnce('mock-id') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to check if its a new account - .mockReturnValueOnce('mock-id3') // call to check if its a new account - .mockReturnValueOnce('mock-id2') // call to add account - .mockReturnValueOnce('mock-id3'); // call to add account + + mockUUIDWithNormalAccounts([ + mockAccount, + mockSimpleKeyring1, + mockSimpleKeyring2, + ]); const { accountsController } = setupAccountsController({ initialState: { @@ -2582,13 +2577,13 @@ describe('AccountsController', () => { it('return the next account number even with an index gap', async () => { const messenger = buildMessenger(); - const mockAccountUUIDs = new MockNormalAccountUUID([ + + mockUUIDWithNormalAccounts([ mockAccount, mockSimpleKeyring1, mockSimpleKeyring2, mockSimpleKeyring3, ]); - mockUUID.mockImplementation(mockAccountUUIDs.mock.bind(mockAccountUUIDs)); const { accountsController } = setupAccountsController({ initialState: { From 21c4ec654786b994196adf68474cd1e9401804bc Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 16 Jan 2025 13:18:09 +0100 Subject: [PATCH 2/2] fix: add thresshold for update NFT metadata (#5134) ## Explanation When `onPreferencesControllerStateChange` is triggered inside nftController, it also triggers a [check](https://github.com/MetaMask/core/blob/76af6ebde290a270f4da6493f0f3b6c9354545cb/packages/assets-controllers/src/NftController.ts#L431) if nfts saved in state needs to be updated. We keep retrying to update NFTs (fetching their metadata) when they have no image, name or description. Note: When switching newtworks; this [line](https://github.com/MetaMask/metamask-extension/blob/b91a9622393f37b79a63ba4a240cda7efa3cc5bc/ui/components/multichain/network-list-menu/network-list-menu.tsx#L296) is the trigger for the `onPreferencesControllerStateChange`. While the updateNftUpdateForAccount should be triggered once for NFTs that actually have a (name, description, image) We will keep retrying for NFTs that don't. (Could be nfts created for testing purposes or some raffle tickets do not have those fields). We had a user reporting that he is seeing an excessive amount of RPC calls made on one of his accounts on Arbitrum Sepolia. This account holds more than a 1000 NFT. Every time he switches the network to Arbitrum Sepolia, he sees more than a 1000 eth_call made. That was the result of updateNftUpdateForAccount being triggered everytime the network is switched + The nfts not having (name,image, description) This PR adds a 500 threshold to avoid sending an excessive amount of RPC calls to fetch nft metadata and adds a check if the triggered onPreferencesControllerStateChange updates any of the concerned params ( ipfsGateway, openseaEnabled, isIpfsGatewayEnabled) **_Better improvement:_** A better improvement for this would be to add support for [BalanceScanner](https://github.com/MetaMask/metafi-sdk/blob/main/packages/eth-scan/contracts/BalanceScanner.sol) contract, which allows to fetch tokenURI for multiple addresses and tokenIds with fct tokenURIsofTokenIdsForTokens. We will control how often this is refreshed. For users that have nfts > NFT_UPDATE_THRESHOLD, we save the timestamp of the first update in a mapping chainId => lastTimestamp of updateNftMetadata, and only update after a specific period of time NFT_UPDATE_REFRESH_PERIOD = 48hours. ## References ## Changelog ### `@metamask/controller-utils` - **ADDED**: Added NFT_UPDATE_THRESHOLD constant ### `@metamask/assets-controllers` - **CHANGED**: Added a check if the nftsToUpdate is less than NFT_UPDATE_THRESHOLD inside `updateNftUpdateForAccount` ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate - [ ] I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes --------- Co-authored-by: Michele Esposito <34438276+mikesposito@users.noreply.github.com> --- .../src/NftController.test.ts | 43 +++++++++++++++++++ .../assets-controllers/src/NftController.ts | 42 +++++++++++------- 2 files changed, 69 insertions(+), 16 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index dc9c9a4a8f..49e48178c7 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -4471,6 +4471,49 @@ describe('NftController', () => { }); describe('updateNftMetadata', () => { + it('should not update Nft metadata when preferences change and current and incoming state are the same', async () => { + const { + nftController, + triggerPreferencesStateChange, + triggerSelectedAccountChange, + } = setupController(); + const spy = jest.spyOn(nftController, 'updateNftMetadata'); + triggerSelectedAccountChange(OWNER_ACCOUNT); + // trigger preference change + triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + }); + + expect(spy).toHaveBeenCalledTimes(0); + }); + + it('should call update Nft metadata when preferences change is triggered and at least ipfsGateway, openSeaEnabled or isIpfsGatewayEnabled change', async () => { + const { + nftController, + mockGetAccount, + triggerPreferencesStateChange, + triggerSelectedAccountChange, + } = setupController({ + defaultSelectedAccount: OWNER_ACCOUNT, + }); + const spy = jest.spyOn(nftController, 'updateNftMetadata'); + const testNetworkClientId = 'mainnet'; + mockGetAccount.mockReturnValue(OWNER_ACCOUNT); + await nftController.addNft('0xtest', '3', { + nftMetadata: { name: '', description: '', image: '', standard: '' }, + networkClientId: testNetworkClientId, + }); + + triggerSelectedAccountChange(OWNER_ACCOUNT); + // trigger preference change + triggerPreferencesStateChange({ + ...getDefaultPreferencesState(), + ipfsGateway: 'https://toto/ipfs/', + }); + + expect(spy).toHaveBeenCalledTimes(1); + }); + it('should update Nft metadata successfully', async () => { const tokenURI = 'https://api.pudgypenguins.io/lil/4'; const mockGetERC721TokenURI = jest.fn().mockResolvedValue(tokenURI); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index df159b91a9..292611dc0d 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -100,12 +100,11 @@ type SuggestedNftMeta = { * @property isCurrentlyOwned - Boolean indicating whether the address/chainId combination where it's currently stored currently owns this NFT * @property transactionId - Transaction Id associated with the NFT */ -export type Nft = - | { - tokenId: string; - address: string; - isCurrentlyOwned?: boolean; - } & NftMetadata; +export type Nft = { + tokenId: string; + address: string; + isCurrentlyOwned?: boolean; +} & NftMetadata; type NftUpdate = { nft: Nft; @@ -274,6 +273,8 @@ export const getDefaultNftControllerState = (): NftControllerState => ({ ignoredNfts: [], }); +const NFT_UPDATE_THRESHOLD = 500; + /** * Controller that stores assets and exposes convenience methods */ @@ -421,15 +422,21 @@ export class NftController extends BaseController< 'AccountsController:getSelectedAccount', ); this.#selectedAccountId = selectedAccount.id; - this.#ipfsGateway = ipfsGateway; - this.#openSeaEnabled = openSeaEnabled; - this.#isIpfsGatewayEnabled = isIpfsGatewayEnabled; - - const needsUpdateNftMetadata = - (isIpfsGatewayEnabled && ipfsGateway !== '') || openSeaEnabled; - - if (needsUpdateNftMetadata && selectedAccount) { - await this.#updateNftUpdateForAccount(selectedAccount); + // Get current state values + if ( + this.#ipfsGateway !== ipfsGateway || + this.#openSeaEnabled !== openSeaEnabled || + this.#isIpfsGatewayEnabled !== isIpfsGatewayEnabled + ) { + this.#ipfsGateway = ipfsGateway; + this.#openSeaEnabled = openSeaEnabled; + this.#isIpfsGatewayEnabled = isIpfsGatewayEnabled; + const needsUpdateNftMetadata = + (isIpfsGatewayEnabled && ipfsGateway !== '') || openSeaEnabled; + + if (needsUpdateNftMetadata && selectedAccount) { + await this.#updateNftUpdateForAccount(selectedAccount); + } } } @@ -2053,7 +2060,10 @@ export class NftController extends BaseController< (singleNft) => !singleNft.name && !singleNft.description && !singleNft.image, ); - if (nftsToUpdate.length !== 0) { + if ( + nftsToUpdate.length !== 0 && + nftsToUpdate.length < NFT_UPDATE_THRESHOLD + ) { await this.updateNftMetadata({ nfts: nftsToUpdate, userAddress: account.address,