diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index f8f9d20407a..5659826d185 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -13,6 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- Optimize NFT Controller state writes from O(2n) to O(2) for NFT detection by batching contract and NFT additions ([#8079](https://github.com/MetaMask/core/pull/8079)) +- Remove on-chain ERC-721 RPC fallback for contract name and symbol in `NftController`; contract metadata is now sourced exclusively from the NFT API, eliminating `getERC721AssetName`/`getERC721AssetSymbol` calls during NFT detection. NFT contracts that previously had `name` or `symbol` populated via on-chain calls will no longer have those fields if the NFT API does not supply them ([#8079](https://github.com/MetaMask/core/pull/8079)) - When `isHomepageSectionsV1Enabled` is true, `AccountTrackerController` now uses all popular EVM networks (via `NetworkEnablementController:listPopularEvmNetworks`) for balance refresh on account change and keyring unlock, instead of only the enabled networks from `NetworkEnablementController` state ([#8117](https://github.com/MetaMask/core/pull/8117)) ### Fixed diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index d78ec771e4c..4b60131d371 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -166,6 +166,10 @@ jest.mock('uuid', () => { * `AccountsController:getSelectedAccount` action. * @param args.bulkScanUrlsMock - Used to construct mock versions of the * `PhishingController:bulkScanUrls` action. + * @param args.approvalAddRequest - When provided, registered directly as the + * `ApprovalController:addRequest` action handler instead of creating a real + * ApprovalController. Use this when the test needs to assert on or auto-resolve + * approval requests without the full approval flow. * @param args.defaultSelectedAccount - The default selected account to use in * @param args.displayNftMedia - The default displayNftMedia to use in * @returns A collection of test controllers and mocks. @@ -181,6 +185,7 @@ function setupController({ getAccount, getSelectedAccount, bulkScanUrlsMock, + approvalAddRequest, mockNetworkClientConfigurationsByNetworkClientId = {}, defaultSelectedAccount = OWNER_ACCOUNT, mockGetNetworkClientIdByChainId = {}, @@ -223,6 +228,7 @@ function setupController({ Promise, [string[]] >; + approvalAddRequest?: jest.Mock; mockNetworkClientConfigurationsByNetworkClientId?: Record< NetworkClientId, NetworkClientConfiguration @@ -330,20 +336,32 @@ function setupController({ mockGetERC1155TokenURI, ); - const approvalControllerMessenger = new Messenger< - 'ApprovalController', - MessengerActions, - MessengerEvents, - RootMessenger - >({ - namespace: 'ApprovalController', - parent: messenger, - }); + let approvalController: ApprovalController; - const approvalController = new ApprovalController({ - messenger: approvalControllerMessenger, - showApprovalRequest: jest.fn(), - }); + if (approvalAddRequest) { + messenger.registerActionHandler( + 'ApprovalController:addRequest', + approvalAddRequest, + ); + // Provide a stub so callers can still destructure `approvalController` + // without needing to branch. The stub is intentionally minimal. + approvalController = { addRequest: approvalAddRequest } as never; + } else { + const approvalControllerMessenger = new Messenger< + 'ApprovalController', + MessengerActions, + MessengerEvents, + RootMessenger + >({ + namespace: 'ApprovalController', + parent: messenger, + }); + + approvalController = new ApprovalController({ + messenger: approvalControllerMessenger, + showApprovalRequest: jest.fn(), + }); + } // Register the phishing controller mock if provided if (bulkScanUrlsMock) { @@ -647,12 +665,12 @@ describe('NftController', () => { }); it('should error if the user does not own the suggested ERC721 NFT', async function () { - const { nftController, nftControllerMessenger } = setupController({ + const addRequestMock = jest.fn(); + const { nftController } = setupController({ getERC721OwnerOf: jest.fn().mockImplementation(() => '0x12345abcefg'), + approvalAddRequest: addRequestMock, }); - const callActionSpy = jest.spyOn(nftControllerMessenger, 'call'); - await expect(() => nftController.watchNft( ERC721_NFT, @@ -661,12 +679,7 @@ describe('NftController', () => { 'mainnet', ), ).rejects.toThrow('Suggested NFT is not owned by the selected account'); - // First call is getInternalAccount. Second call is the approval request. - expect(callActionSpy).not.toHaveBeenNthCalledWith( - 2, - 'ApprovalController:addRequest', - expect.any(Object), - ); + expect(addRequestMock).not.toHaveBeenCalled(); }); it('should error if the call to isNftOwner fail', async function () { @@ -686,12 +699,12 @@ describe('NftController', () => { }); it('should error if the user does not own the suggested ERC1155 NFT', async function () { - const { nftController, nftControllerMessenger } = setupController({ + const addRequestMock = jest.fn(); + const { nftController, mockGetAccount } = setupController({ getERC1155BalanceOf: jest.fn().mockImplementation(() => new BN(0)), + approvalAddRequest: addRequestMock, }); - const callActionSpy = jest.spyOn(nftControllerMessenger, 'call'); - await expect(() => nftController.watchNft( ERC1155_NFT, @@ -700,12 +713,9 @@ describe('NftController', () => { 'mainnet', ), ).rejects.toThrow('Suggested NFT is not owned by the selected account'); - // First call is to get InternalAccount - expect(callActionSpy).toHaveBeenNthCalledWith( - 1, - 'AccountsController:getAccount', - expect.any(String), - ); + // getAccount must be called to look up the owner to compare against + expect(mockGetAccount).toHaveBeenCalledWith(expect.any(String)); + expect(addRequestMock).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API disabled and IPFS gateway enabled', async function () { @@ -719,19 +729,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); @@ -744,75 +756,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 3. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 4. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 5. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 6. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 7. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -831,8 +785,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API enabled and IPFS gateway enabled', async function () { @@ -846,19 +801,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -870,75 +827,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 5. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 6. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 10. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -957,8 +856,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API disabled and IPFS gateway disabled', async function () { @@ -972,19 +872,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -996,75 +898,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 5. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 6. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 10. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -1083,8 +927,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC721 type and add pending request to ApprovalController with the OpenSea API enabled and IPFS gateway disabled', async function () { @@ -1098,19 +943,21 @@ describe('NftController', () => { description: 'testERC721Description', }), ); + + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, + mockGetERC721AssetName, + mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), - getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); @@ -1123,75 +970,17 @@ describe('NftController', () => { const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockResolvedValueOnce(OWNER_ADDRESS) - // 3. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 5. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 6. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 7. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockResolvedValueOnce('testERC721Name') - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockResolvedValueOnce('testERC721Symbol') - // 10. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC721_NFT, ERC721, 'https://test-dapp.com', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(10); - expect(callActionSpy).toHaveBeenNthCalledWith( - 5, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://test-dapp.com', @@ -1210,8 +999,9 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); + // No on-chain RPC fallback for name/symbol — sourced from API only + expect(mockGetERC721AssetName).not.toHaveBeenCalled(); + expect(mockGetERC721AssetSymbol).not.toHaveBeenCalled(); }); it('should handle ERC1155 type and add to suggestedNfts with the OpenSea API disabled', async function () { @@ -1226,9 +1016,9 @@ describe('NftController', () => { }), ); + const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, - nftControllerMessenger, triggerPreferencesStateChange, triggerSelectedAccountChange, } = setupController({ @@ -1243,6 +1033,7 @@ describe('NftController', () => { getERC1155TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), + approvalAddRequest: addRequestMock, }); triggerSelectedAccountChange(OWNER_ACCOUNT); @@ -1251,82 +1042,21 @@ describe('NftController', () => { isIpfsGatewayEnabled: true, displayNftMedia: false, }); + const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 3. `AssetsContractController:getERC1155BalanceOf` - .mockResolvedValueOnce(new BN(1)) - // 4. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 5. `AssetsContractController:getERC721TokenURI` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 6. `AssetsContractController:getERC1155TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 7. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 8. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 9. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 10. `AssetsContractController:getERC721AssetName` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 11. `AssetsContractController:getERC721AssetSymbol` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 12. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC1155_NFT, ERC1155, 'https://etherscan.io', 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(12); - expect(callActionSpy).toHaveBeenNthCalledWith( - 7, - 'ApprovalController:addRequest', + + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://etherscan.io', @@ -1345,8 +1075,6 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); }); it('should handle ERC1155 type and add to suggestedNfts with the OpenSea API enabled', async function () { @@ -1361,11 +1089,8 @@ describe('NftController', () => { }), ); - const { - nftController, - nftControllerMessenger, - triggerPreferencesStateChange, - } = setupController({ + const addRequestMock = jest.fn().mockResolvedValue(undefined); + const { nftController, triggerPreferencesStateChange } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), getERC721OwnerOf: jest .fn() @@ -1377,78 +1102,19 @@ describe('NftController', () => { getERC1155TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), + approvalAddRequest: addRequestMock, }); triggerPreferencesStateChange({ ...getDefaultPreferencesState(), isIpfsGatewayEnabled: true, displayNftMedia: true, }); + const requestId = 'approval-request-id-1'; jest.spyOn(Date, 'now').mockReturnValue(1); - (v4 as jest.Mock).mockImplementationOnce(() => requestId); - const callActionSpy = jest - .spyOn(nftControllerMessenger, 'call') - // 1. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 2. `AssetsContractController:getERC721OwnerOf` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 3. `AssetsContractController:getERC1155BalanceOf` - .mockResolvedValueOnce(new BN(1)) - // 4. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 4. `AssetsContractController:getERC721TokenURI` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 5. `AssetsContractController:getERC1155TokenURI` - .mockResolvedValueOnce('https://testtokenuri.com') - // 6. `ApprovalController:addRequest` - .mockResolvedValueOnce({}) - // 7. `AccountsController:getAccount` - .mockReturnValueOnce(OWNER_ACCOUNT) - // 9. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any) - // 8. `AssetsContractController:getERC721AssetName` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 9. `AssetsContractController:getERC721AssetSymbol` - .mockRejectedValueOnce(new Error('Not an ERC721 contract')) - // 9. `NetworkClientController:getNetworkClientById` - .mockReturnValueOnce({ - configuration: { - type: 'infura', - network: 'mainnet', - failoverRpcUrls: [], - infuraProjectId: 'test-infura-project-id', - chainId: '0x1', - ticker: 'ETH', - rpcUrl: 'https://mainnet.infura.io/v3/test-infura-project-id', - }, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any); - await nftController.watchNft( ERC1155_NFT, ERC1155, @@ -1456,10 +1122,8 @@ describe('NftController', () => { 'mainnet', ); - expect(callActionSpy).toHaveBeenCalledTimes(12); - expect(callActionSpy).toHaveBeenNthCalledWith( - 7, - 'ApprovalController:addRequest', + expect(addRequestMock).toHaveBeenCalledTimes(1); + expect(addRequestMock).toHaveBeenCalledWith( { id: requestId, origin: 'https://etherscan.io', @@ -1478,8 +1142,6 @@ describe('NftController', () => { }, true, ); - - jest.restoreAllMocks(); }); it('should add the NFT to the correct chainId/selectedAddress in state when passed a userAddress in the options argument', async function () { @@ -1726,7 +1388,6 @@ describe('NftController', () => { it('should add the nft contract to the correct chain in state when source is detected', async () => { const { nftController } = setupController({ options: {}, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), }); await nftController.addNft('0x01', '1', 'mainnet', { @@ -1752,7 +1413,6 @@ describe('NftController', () => { ).toStrictEqual({ address: '0x01', logo: 'url', - name: 'Name', schemaName: 'standard', totalSupply: '0', }); @@ -1761,7 +1421,6 @@ describe('NftController', () => { it('should add the nft contract to the correct chain in state when source is custom', async () => { const { nftController } = setupController({ options: {}, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), }); await nftController.addNft('0x01', '1', 'sepolia', { @@ -1785,7 +1444,6 @@ describe('NftController', () => { ).toStrictEqual({ address: '0x01', logo: 'url', - name: 'Name', schemaName: 'standard', totalSupply: '0', }); @@ -1795,7 +1453,6 @@ describe('NftController', () => { options: { // chainId: ChainId.mainnet, }, - getERC721AssetName: jest.fn().mockResolvedValue('Name'), }); await nftController.addNft('0x01', '1', 'mainnet', { @@ -1837,7 +1494,6 @@ describe('NftController', () => { ).toStrictEqual({ address: '0x01', logo: 'url', - name: 'Name', totalSupply: '0', schemaName: 'standard', }); @@ -2231,10 +1887,8 @@ describe('NftController', () => { }); }); - it('should add NFT erc721 and aggregate NFT data from both contract and NFT-API even if call to Get Collections fails', async () => { + it('should add NFT erc721 and aggregate NFT data from NFT-API even if call to Get Collections fails', async () => { const { nftController } = setupController({ - getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), getERC721TokenURI: jest .fn() .mockResolvedValue( @@ -2306,15 +1960,11 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_KUDOSADDRESS, - name: 'KudosToken', - symbol: 'KDO', schemaName: ERC721, }); }); - it('should add NFT erc721 and aggregate NFT data from both contract and NFT-API when call to Get Collections succeeds', async () => { + it('should add NFT erc721 and aggregate NFT data from NFT-API when call to Get Collections succeeds', async () => { const { nftController } = setupController({ - getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), getERC721TokenURI: jest .fn() .mockResolvedValue( @@ -2382,8 +2032,6 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_KUDOSADDRESS, - name: 'KudosToken', - symbol: 'KDO', schemaName: ERC721, }); }); @@ -2435,10 +2083,8 @@ describe('NftController', () => { }); }); - it('should add NFT erc721 and get NFT information only from contract', async () => { + it('should add NFT erc721 and get NFT information from tokenURI when NFT API returns 404', async () => { const { nftController } = setupController({ - getERC721AssetName: jest.fn().mockResolvedValue('KudosToken'), - getERC721AssetSymbol: jest.fn().mockResolvedValue('KDO'), getERC721TokenURI: jest.fn().mockImplementation((tokenAddress) => { switch (tokenAddress) { case ERC721_KUDOSADDRESS: @@ -2491,8 +2137,6 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_KUDOSADDRESS, - name: 'KudosToken', - symbol: 'KDO', schemaName: ERC721, }); }); @@ -3019,10 +2663,6 @@ describe('NftController', () => { it('should add NFT with metadata hosted in IPFS', async () => { const { nftController, triggerPreferencesStateChange, mockGetAccount } = setupController({ - getERC721AssetName: jest - .fn() - .mockResolvedValue("Maltjik.jpg's Depressionists"), - getERC721AssetSymbol: jest.fn().mockResolvedValue('DPNS'), getERC721TokenURI: jest.fn().mockImplementation((tokenAddress) => { switch (tokenAddress) { case ERC721_DEPRESSIONIST_ADDRESS: @@ -3053,8 +2693,6 @@ describe('NftController', () => { ][0], ).toStrictEqual({ address: ERC721_DEPRESSIONIST_ADDRESS, - name: "Maltjik.jpg's Depressionists", - symbol: 'DPNS', schemaName: ERC721, }); expect( @@ -3516,6 +3154,117 @@ describe('NftController', () => { tokenId: '1', }); }); + + it('should skip an NFT whose chain ID is not registered and still add the remaining NFTs', async () => { + // Chain ID 0x999 is not registered in any mock network client config, so + // findNetworkClientIdByChainId throws for it naturally — no spy needed. + const UNREGISTERED_CHAIN_ID = 0x999; + + const { nftController } = setupController({}); + + await nftController.addNfts( + [ + { + tokenAddress: '0x01', + tokenId: '1', + nftMetadata: { + name: 'NFT 1', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + { + tokenAddress: '0x02', + tokenId: '2', + nftMetadata: { + name: 'NFT 2', + image: null, + description: null, + standard: ERC721, + chainId: UNREGISTERED_CHAIN_ID, + }, + }, + { + tokenAddress: '0x03', + tokenId: '3', + nftMetadata: { + name: 'NFT 3', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + ], + OWNER_ACCOUNT.address, + ); + + // NFTs 1 and 3 (mainnet) should be added despite NFT 2 failing + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet], + ).toHaveLength(2); + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet], + ).toMatchObject([ + { address: '0x01', tokenId: '1' }, + { address: '0x03', tokenId: '3' }, + ]); + // NFT 2 (unknown chain) should not have been added + expect( + nftController.state.allNfts[OWNER_ACCOUNT.address]?.['0x999'], + ).toBeUndefined(); + }); + + it('should fire onNftAdded callbacks only after all NFT state has been written', async () => { + let stateWhenFirstCallbackFired: NftControllerState | undefined; + const mockOnNftAdded = jest.fn(); + + const { nftController } = setupController({ + options: { onNftAdded: mockOnNftAdded }, + }); + + mockOnNftAdded.mockImplementationOnce(() => { + stateWhenFirstCallbackFired = nftController.state; + }); + + await nftController.addNfts( + [ + { + tokenAddress: '0x01', + tokenId: '1', + nftMetadata: { + name: 'NFT 1', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + { + tokenAddress: '0x02', + tokenId: '2', + nftMetadata: { + name: 'NFT 2', + image: null, + description: null, + standard: ERC721, + chainId: 1, + }, + }, + ], + OWNER_ACCOUNT.address, + ); + + expect(mockOnNftAdded).toHaveBeenCalledTimes(2); + // Both NFTs must already be in state when the first callback fires + expect( + stateWhenFirstCallbackFired?.allNfts[OWNER_ACCOUNT.address][ + ChainId.mainnet + ], + ).toHaveLength(2); + }); }); describe('addNftVerifyOwnership', () => { diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 10f5e28bc83..8d2bf8a5515 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -266,6 +266,22 @@ type NftAsset = { tokenId: string; }; +type NftContractToAdd = { + networkClientId: NetworkClientId; + tokenAddress: string; + nftMetadata: NftMetadata; + source: Source; +}; + +type NftToAdd = { + tokenAddress: string; + tokenId: string; + nftMetadata: NftMetadata; + nftContract: NftContract; + chainId: Hex; + source: Source; +}; + /** * The name of the {@link NftController}. */ @@ -803,73 +819,31 @@ export class NftController extends BaseController< } /** - * Request NFT contract information from the contract itself. - * - * @param contractAddress - Hex address of the NFT contract. - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @returns Promise resolving to the current NFT name and image. - */ - async #getNftContractInformationFromContract( - // TODO for calls to blockchain we need to explicitly pass the currentNetworkClientId since its relying on the provider - contractAddress: string, - networkClientId: NetworkClientId, - ): Promise< - Partial & - Pick & - Pick - > { - const [name, symbol] = await Promise.all([ - this.messenger.call( - 'AssetsContractController:getERC721AssetName', - contractAddress, - networkClientId, - ), - this.messenger.call( - 'AssetsContractController:getERC721AssetSymbol', - contractAddress, - networkClientId, - ), - ]); - - return { - collection: { name }, - symbol, - address: contractAddress, - }; - } - - /** - * Request NFT contract information from Blockchain and aggregate with received data from NFTMetadata. + * Builds NFT contract information from metadata already received from the + * NFT API. No on-chain RPC calls are made. * * @param contractAddress - Hex address of the NFT contract. - * @param nftMetadataFromApi - Received NFT information to be aggregated with blockchain contract information. - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @returns Promise resolving to the NFT contract name, image and description. + * @param nftMetadataFromApi - NFT information received from the API. + * @returns The aggregated NFT contract information. */ - async #getNftContractInformation( + #getNftContractInformation( contractAddress: string, nftMetadataFromApi: NftMetadata, - networkClientId: NetworkClientId, - ): Promise< - Partial & - Pick & - Pick - > { - const blockchainContractData = await safelyExecute(() => - this.#getNftContractInformationFromContract( - contractAddress, - networkClientId, - ), - ); + ): Partial & + Pick & + Pick { + const name = nftMetadataFromApi.collection?.name; + const symbol = nftMetadataFromApi.collection?.symbol; if ( - blockchainContractData || + name !== undefined || + symbol !== undefined || !Object.values(nftMetadataFromApi).every((value) => value === null) ) { return { address: contractAddress, - ...blockchainContractData, schema_name: nftMetadataFromApi?.standard ?? null, + ...(symbol !== undefined && { symbol }), collection: { name: null, image_url: @@ -878,7 +852,7 @@ export class NftController extends BaseController< null, tokenCount: nftMetadataFromApi?.collection?.tokenCount ?? null, ...nftMetadataFromApi?.collection, - ...blockchainContractData?.collection, + ...(name !== undefined && { name }), }, }; } @@ -898,92 +872,126 @@ export class NftController extends BaseController< } /** - * Adds an individual NFT to the stored NFT list. + * Adds multiple NFTs to the stored NFT list for a given user. * - * @param tokenAddress - Hex address of the NFT contract. - * @param tokenId - The NFT identifier. - * @param nftMetadata - NFT optional information (name, image and description). - * @param nftContract - An object containing contract data of the NFT being added. - * @param chainId - The chainId of the network where the NFT is being added. - * @param userAddress - The address of the account where the NFT is being added. - * @param source - Whether the NFT was detected, added manually or suggested by a dapp. - * @returns A promise resolving to `undefined`. + * @param userAddress - The address of the account where the NFTs are being added. + * @param nfts - Array of NFT objects to add. + * @param nfts[].tokenAddress - Hex address of the NFT contract. + * @param nfts[].tokenId - The NFT identifier. + * @param nfts[].nftMetadata - NFT optional information (name, image and description). + * @param nfts[].nftContract - An object containing contract data of the NFT being added. + * @param nfts[].chainId - The chainId of the network where the NFT is being added. + * @param nfts[].source - Whether the NFT was detected, added manually or suggested by a dapp. */ - async #addIndividualNft( - tokenAddress: string, - tokenId: string, - nftMetadata: NftMetadata, - nftContract: NftContract, - chainId: Hex, - userAddress: string, - source: Source, - ): Promise { + async #addMultipleNfts(userAddress: string, nfts: NftToAdd[]): Promise { const releaseLock = await this.#mutex.acquire(); try { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); const { allNfts } = this.state; + const allNftsForUser = allNfts[userAddress] || {}; + const allNftsForUserPerChain: { + [chainId: `0x${string}`]: Nft[]; + } = {}; + const modifiedChainIds = new Set(); + const pendingCallbacks: { + address: string; + symbol: string | undefined; + tokenId: string; + standard: string | null; + source: Source; + }[] = []; + + for (const { + tokenAddress, + tokenId, + nftMetadata, + nftContract, + chainId, + source, + } of nfts) { + try { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); + + if (!allNftsForUserPerChain[chainId]) { + allNftsForUserPerChain[chainId] = [ + ...(allNftsForUser?.[chainId] ?? []), + ]; + } - const nfts = [...(allNfts[userAddress]?.[chainId] ?? [])]; + const existingEntry = allNftsForUserPerChain[chainId].find( + (nft) => + nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && + nft.tokenId === tokenId, + ); - const existingEntry = nfts.find( - (nft) => - nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && - nft.tokenId === tokenId, - ); + if (existingEntry) { + const differentMetadata = compareNftMetadata( + nftMetadata, + existingEntry, + ); - if (existingEntry) { - const differentMetadata = compareNftMetadata( - nftMetadata, - existingEntry, - ); + const hasNewFields = hasNewCollectionFields( + nftMetadata, + existingEntry, + ); - const hasNewFields = hasNewCollectionFields(nftMetadata, existingEntry); + if ( + !differentMetadata && + existingEntry.isCurrentlyOwned && + !hasNewFields + ) { + continue; + } - if ( - !differentMetadata && - existingEntry.isCurrentlyOwned && - !hasNewFields - ) { - return; - } + const indexToUpdate = allNftsForUserPerChain[chainId].findIndex( + (nft) => + nft.address.toLowerCase() === + checksumHexAddress.toLowerCase() && nft.tokenId === tokenId, + ); - const indexToUpdate = nfts.findIndex( - (nft) => - nft.address.toLowerCase() === checksumHexAddress.toLowerCase() && - nft.tokenId === tokenId, - ); + if (indexToUpdate !== -1) { + allNftsForUserPerChain[chainId][indexToUpdate] = { + ...existingEntry, + ...nftMetadata, + }; + } + } else { + const newEntry: Nft = { + address: checksumHexAddress, + tokenId, + favorite: false, + isCurrentlyOwned: true, + ...nftMetadata, + }; + + allNftsForUserPerChain[chainId].push(newEntry); + } - if (indexToUpdate !== -1) { - nfts[indexToUpdate] = { - ...existingEntry, - ...nftMetadata, - }; - } - } else { - const newEntry: Nft = { - address: checksumHexAddress, - tokenId, - favorite: false, - isCurrentlyOwned: true, - ...nftMetadata, - }; + modifiedChainIds.add(chainId); - nfts.push(newEntry); + if (this.#onNftAdded) { + pendingCallbacks.push({ + address: checksumHexAddress, + symbol: nftContract.symbol, + tokenId: tokenId.toString(), + standard: nftMetadata.standard, + source, + }); + } + } catch (error) { + console.error('Failed to add NFT', tokenAddress, tokenId, error); + } } - this.#updateNestedNftState(nfts, ALL_NFTS_STATE_KEY, { - chainId, - userAddress, - }); + for (const chainId of modifiedChainIds) { + this.#updateNestedNftState( + allNftsForUserPerChain[chainId], + ALL_NFTS_STATE_KEY, + { chainId, userAddress }, + ); + } - if (this.#onNftAdded) { - this.#onNftAdded({ - address: checksumHexAddress, - symbol: nftContract.symbol, - tokenId: tokenId.toString(), - standard: nftMetadata.standard, - source, - }); + for (const callbackData of pendingCallbacks) { + this.#onNftAdded?.(callbackData); } } finally { releaseLock(); @@ -991,112 +999,129 @@ export class NftController extends BaseController< } /** - * Adds an NFT contract to the stored NFT contracts list. + * Adds multiple NFT contracts to the stored NFT contracts list for a given user. * - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @param options - options. - * @param options.tokenAddress - Hex address of the NFT contract. - * @param options.userAddress - The address of the account where the NFT is being added. - * @param options.nftMetadata - The retrieved NFTMetadata from API. - * @param options.source - Whether the NFT was detected, added manually or suggested by a dapp. - * @returns Promise resolving to the current NFT contracts list. + * @param userAddress - The address of the account where the NFT contracts are being added. + * @param contracts - Array of contract objects to add. + * @param contracts[].networkClientId - The networkClientId used to identify the network client for the request. + * @param contracts[].tokenAddress - Hex address of the NFT contract. + * @param contracts[].nftMetadata - The retrieved NFT metadata from the API. + * @param contracts[].source - Whether the NFT was detected, added manually or suggested by a dapp. + * @returns Promise resolving to an object mapping chainIds to their updated NFT contract arrays. */ - async #addNftContract( - networkClientId: NetworkClientId, - { - tokenAddress, - userAddress, - source, - nftMetadata, - }: { - tokenAddress: string; - userAddress: string; - nftMetadata: NftMetadata; - source?: Source; - }, - ): Promise { + async #addNftContracts( + userAddress: string, + contracts: NftContractToAdd[], + ): Promise<{ contracts: { [chainId: `0x${string}`]: NftContract[] } }> { const releaseLock = await this.#mutex.acquire(); try { - const checksumHexAddress = toChecksumHexAddress(tokenAddress); const { allNftContracts } = this.state; - const { - configuration: { chainId }, - } = this.messenger.call( - 'NetworkController:getNetworkClientById', + const allNftContractsForUser = allNftContracts[userAddress] || {}; + const nftContractsForUserPerChain: { + [chainId: `0x${string}`]: NftContract[]; + } = {}; + const modifiedChainIds = new Set(); + + for (const { networkClientId, - ); + tokenAddress, + source, + nftMetadata, + } of contracts) { + try { + const checksumHexAddress = toChecksumHexAddress(tokenAddress); + const { + configuration: { chainId }, + } = this.messenger.call( + 'NetworkController:getNetworkClientById', + networkClientId, + ); - const nftContracts = allNftContracts[userAddress]?.[chainId] || []; + // Initialised before the existingEntry check so pre-existing contracts + // are still present in the returned map for callers to look up. + if (!nftContractsForUserPerChain[chainId]) { + nftContractsForUserPerChain[chainId] = [ + ...(allNftContractsForUser?.[chainId] ?? []), + ]; + } - const existingEntry = nftContracts.find( - (nftContract) => - nftContract.address.toLowerCase() === - checksumHexAddress.toLowerCase(), - ); - if (existingEntry) { - return nftContracts; - } + const existingEntry = nftContractsForUserPerChain[chainId].find( + (nftContract) => + nftContract.address.toLowerCase() === + checksumHexAddress.toLowerCase(), + ); - // this doesn't work currently for detection if the user switches networks while the detection is processing - // will be fixed once detection uses networkClientIds - // get name and symbol if ERC721 then put together the metadata - const contractInformation = await this.#getNftContractInformation( - checksumHexAddress, - nftMetadata, - networkClientId, - ); - const { - asset_contract_type, - created_date, - symbol, - description, - external_link, - schema_name, - collection: { name, image_url, tokenCount }, - } = contractInformation; - - // If the nft is auto-detected we want some valid metadata to be present - if ( - source === Source.Detected && - 'address' in contractInformation && - typeof contractInformation.address === 'string' && - 'collection' in contractInformation && - contractInformation.collection.name === null && - 'image_url' in contractInformation.collection && - contractInformation.collection.image_url === null && - Object.entries(contractInformation).every(([key, value]) => { - return key === 'address' || key === 'collection' || !value; - }) - ) { - return nftContracts; + if (existingEntry) { + continue; + } + + // this doesn't work currently for detection if the user switches networks while the detection is processing + // will be fixed once detection uses networkClientIds + // get name and symbol if ERC721 then put together the metadata + const contractInformation = this.#getNftContractInformation( + checksumHexAddress, + nftMetadata, + ); + + // If the nft is auto-detected we want some valid metadata to be present + if ( + source === Source.Detected && + 'address' in contractInformation && + typeof contractInformation.address === 'string' && + 'collection' in contractInformation && + contractInformation.collection.name === null && + 'image_url' in contractInformation.collection && + contractInformation.collection.image_url === null && + Object.entries(contractInformation).every(([key, value]) => { + return key === 'address' || key === 'collection' || !value; + }) + ) { + continue; + } + + const { + asset_contract_type, + created_date, + symbol, + description, + external_link, + schema_name, + collection: { name, image_url, tokenCount }, + } = contractInformation; + + /* istanbul ignore next */ + const newEntry: NftContract = Object.assign( + {}, + { address: checksumHexAddress }, + description && { description }, + name && { name }, + image_url && { logo: image_url }, + symbol && { symbol }, + tokenCount !== null && + typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, + asset_contract_type && { assetContractType: asset_contract_type }, + created_date && { createdDate: created_date }, + schema_name && { schemaName: schema_name }, + external_link && { externalLink: external_link }, + ); + + nftContractsForUserPerChain[chainId].push(newEntry); + modifiedChainIds.add(chainId); + } catch (error) { + console.error('Failed to add NFT contract', tokenAddress, error); + } } - /* istanbul ignore next */ - const newEntry: NftContract = Object.assign( - {}, - { address: checksumHexAddress }, - description && { description }, - name && { name }, - image_url && { logo: image_url }, - symbol && { symbol }, - tokenCount !== null && - typeof tokenCount !== 'undefined' && { totalSupply: tokenCount }, - asset_contract_type && { assetContractType: asset_contract_type }, - created_date && { createdDate: created_date }, - schema_name && { schemaName: schema_name }, - external_link && { externalLink: external_link }, - ); - const newNftContracts = [...nftContracts, newEntry]; - this.#updateNestedNftState( - newNftContracts, - ALL_NFTS_CONTRACTS_STATE_KEY, - { - chainId, - userAddress, - }, - ); + // Loops once per chain (not once per NFT contract) + for (const chainId of modifiedChainIds) { + this.#updateNestedNftState( + nftContractsForUserPerChain[chainId], + ALL_NFTS_CONTRACTS_STATE_KEY, + { chainId, userAddress }, + ); + } - return newNftContracts; + return { contracts: nftContractsForUserPerChain }; } finally { releaseLock(); } @@ -1472,41 +1497,50 @@ export class NftController extends BaseController< nftMetadata = await this.#sanitizeNftMetadata(nftMetadata); } - const newNftContracts = await this.#addNftContract(networkClientId, { - tokenAddress: checksumHexAddress, - userAddress: addressToSearch, - source, - nftMetadata, - }); + const { contracts: newNftContracts } = await this.#addNftContracts( + addressToSearch, + [ + { + tokenAddress: checksumHexAddress, + networkClientId, + source, + nftMetadata, + }, + ], + ); // If NFT contract was not added, do not add individual NFT - const nftContract = newNftContracts.find( - (contract) => - contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), - ); + const nftContract = Object.values(newNftContracts) + .flat() + .find( + (contract) => + contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), + ); + const { configuration: { chainId }, } = this.messenger.call( 'NetworkController:getNetworkClientById', networkClientId, ); + // This is the case when the NFT is added manually and not detected automatically // TODO: An improvement would be to make the chainId a required field and return it when getting the NFT information if (!nftMetadata.chainId) { nftMetadata.chainId = convertHexToDecimal(chainId); } - // If NFT contract information, add individual NFT if (nftContract) { - await this.#addIndividualNft( - checksumHexAddress, - tokenId, - nftMetadata, - nftContract, - chainId, - addressToSearch, - source, - ); + await this.#addMultipleNfts(addressToSearch, [ + { + tokenAddress: checksumHexAddress, + tokenId, + nftMetadata, + nftContract, + chainId, + source, + }, + ]); } } @@ -1540,42 +1574,80 @@ export class NftController extends BaseController< nfts.map((nft) => nft.nftMetadata), ); + // Resolve network client IDs per item up front. Items that fail (e.g., + // the user removes a network during detection) are skipped individually + // so the rest of the batch is unaffected. Resolved data is bundled into + // one object per NFT to avoid index-alignment issues between the two loops. + const resolvedNfts: { + contractToAdd: NftContractToAdd; + tokenId: string; + checksumHexAddress: string; + hexChainId: Hex; + sanitizedMetadata: NftMetadata; + }[] = []; + for (const [index, nft] of nfts.entries()) { - const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); + try { + const checksumHexAddress = toChecksumHexAddress(nft.tokenAddress); + const hexChainId = toHex(nft.nftMetadata.chainId); + const networkClientId = this.messenger.call( + 'NetworkController:findNetworkClientIdByChainId', + hexChainId, + ); - const hexChainId = toHex(nft.nftMetadata.chainId); + resolvedNfts.push({ + contractToAdd: { + networkClientId, + tokenAddress: checksumHexAddress, + source, + nftMetadata: sanitizedNftMetadata[index], + }, + tokenId: nft.tokenId, + checksumHexAddress, + hexChainId, + sanitizedMetadata: sanitizedNftMetadata[index], + }); + } catch (error) { + console.error( + 'Failed to resolve network for NFT', + nft.tokenAddress, + error, + ); + } + } - const networkClientId = this.messenger.call( - 'NetworkController:findNetworkClientIdByChainId', - hexChainId, - ); + const { contracts: newNftContracts } = await this.#addNftContracts( + addressToSearch, + resolvedNfts.map((item) => item.contractToAdd), + ); - const newNftContracts = await this.#addNftContract(networkClientId, { - tokenAddress: checksumHexAddress, - userAddress: addressToSearch, - source, - nftMetadata: sanitizedNftMetadata[index], - }); + const nftsToAdd: NftToAdd[] = []; - // If NFT contract was not added, do not add individual NFT - const nftContract = newNftContracts.find( + for (const { + checksumHexAddress, + tokenId, + hexChainId, + sanitizedMetadata, + } of resolvedNfts) { + const nftContract = newNftContracts[hexChainId]?.find( (contract) => contract.address.toLowerCase() === checksumHexAddress.toLowerCase(), ); - - // If NFT contract information, add individual NFT if (nftContract) { - await this.#addIndividualNft( - checksumHexAddress, - nft.tokenId, - sanitizedNftMetadata[index], + nftsToAdd.push({ + tokenAddress: checksumHexAddress, + tokenId, + nftMetadata: sanitizedMetadata, nftContract, - hexChainId, - addressToSearch, + chainId: hexChainId, source, - ); + }); } } + + if (nftsToAdd.length > 0) { + await this.#addMultipleNfts(addressToSearch, nftsToAdd); + } } /**