From a7939db90e4ccfc214e824d26334769fd9bd221a Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Tue, 24 Mar 2026 13:29:27 +0100 Subject: [PATCH 1/5] fix: do not make O(n) calls for NFT refresh --- .../src/NftController.test.ts | 409 +++++------------- .../assets-controllers/src/NftController.ts | 188 +++----- .../assets-controllers/src/multicall.test.ts | 341 ++++++++++++++- packages/assets-controllers/src/multicall.ts | 269 ++++++++++++ 4 files changed, 773 insertions(+), 434 deletions(-) diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 84507bbfd90..5116c28dd74 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -37,20 +37,19 @@ import { RecommendedAction } from '@metamask/phishing-controller'; import { getDefaultPreferencesState } from '@metamask/preferences-controller'; import type { PreferencesState } from '@metamask/preferences-controller'; import type { Hex } from '@metamask/utils'; -import BN from 'bn.js'; import nock from 'nock'; import { v4 } from 'uuid'; import type { - AssetsContractControllerGetERC1155BalanceOfAction, AssetsContractControllerGetERC1155TokenURIAction, AssetsContractControllerGetERC721AssetNameAction, AssetsContractControllerGetERC721AssetSymbolAction, - AssetsContractControllerGetERC721OwnerOfAction, AssetsContractControllerGetERC721TokenURIAction, } from './AssetsContractController'; import { getFormattedIpfsUrl } from './assetsUtil'; import { Source } from './constants'; +import type { NftOwnershipResult } from './multicall'; +import { getNftOwnershipForMultipleNfts } from './multicall'; import type { Nft, NftControllerState, @@ -139,6 +138,11 @@ jest.mock('uuid', () => { }; }); +jest.mock('./multicall', () => ({ + ...jest.requireActual('./multicall'), + getNftOwnershipForMultipleNfts: jest.fn(), +})); + /** * Setup a test controller instance. * @@ -150,10 +154,6 @@ jest.mock('uuid', () => { * `AssetsContractController:getERC721AssetSymbol` action. * @param args.getERC721TokenURI - Used to construct mock versions of the * `AssetsContractController:getERC721TokenURI` action. - * @param args.getERC721OwnerOf - Used to construct mock versions of the - * `AssetsContractController:getERC721OwnerOf` action. - * @param args.getERC1155BalanceOf - Used to construct mock versions of the - * `AssetsContractController:getERC1155BalanceOf` action. * @param args.getERC1155TokenURI - Used to construct mock versions of the * `AssetsContractController:getERC1155TokenURI` action. * @param args.mockNetworkClientConfigurationsByNetworkClientId - Used to construct @@ -179,8 +179,6 @@ function setupController({ getERC721AssetName, getERC721AssetSymbol, getERC721TokenURI, - getERC721OwnerOf, - getERC1155BalanceOf, getERC1155TokenURI, getAccount, getSelectedAccount, @@ -204,14 +202,6 @@ function setupController({ ReturnType, Parameters >; - getERC721OwnerOf?: jest.Mock< - ReturnType, - Parameters - >; - getERC1155BalanceOf?: jest.Mock< - ReturnType, - Parameters - >; getERC1155TokenURI?: jest.Mock< ReturnType, Parameters @@ -303,28 +293,6 @@ function setupController({ mockGetERC721TokenURI, ); - const mockGetERC721OwnerOf = - getERC721OwnerOf ?? - jest.fn< - ReturnType, - Parameters - >(); - messenger.registerActionHandler( - 'AssetsContractController:getERC721OwnerOf', - mockGetERC721OwnerOf, - ); - - const mockGetERC1155BalanceOf = - getERC1155BalanceOf ?? - jest.fn< - ReturnType, - Parameters - >(); - messenger.registerActionHandler( - 'AssetsContractController:getERC1155BalanceOf', - mockGetERC1155BalanceOf, - ); - const mockGetERC1155TokenURI = getERC1155TokenURI ?? jest.fn< @@ -390,8 +358,6 @@ function setupController({ 'AssetsContractController:getERC721AssetName', 'AssetsContractController:getERC721AssetSymbol', 'AssetsContractController:getERC721TokenURI', - 'AssetsContractController:getERC721OwnerOf', - 'AssetsContractController:getERC1155BalanceOf', 'AssetsContractController:getERC1155TokenURI', 'NetworkController:findNetworkClientIdByChainId', 'PhishingController:bulkScanUrls', @@ -439,11 +405,9 @@ function setupController({ triggerSelectedAccountChange, mockGetAccount, mockGetSelectedAccount, - mockGetERC1155BalanceOf, mockGetERC1155TokenURI, mockGetERC721AssetName, mockGetERC721AssetSymbol, - mockGetERC721OwnerOf, mockGetERC721TokenURI, }; } @@ -581,8 +545,10 @@ describe('NftController', () => { getERC721TokenURI: jest .fn() .mockImplementation(() => 'https://testtokenuri.com'), - getERC721OwnerOf: jest.fn().mockImplementation(() => OWNER_ADDRESS), }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); // eslint-disable-next-line @typescript-eslint/ban-ts-comment // @ts-ignore-next-line @@ -667,9 +633,11 @@ describe('NftController', () => { it('should error if the user does not own the suggested ERC721 NFT', async function () { const addRequestMock = jest.fn(); const { nftController } = setupController({ - getERC721OwnerOf: jest.fn().mockImplementation(() => '0x12345abcefg'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: false }, + ]); await expect(() => nftController.watchNft( @@ -701,9 +669,11 @@ describe('NftController', () => { it('should error if the user does not own the suggested ERC1155 NFT', async function () { const addRequestMock = jest.fn(); const { nftController, mockGetAccount } = setupController({ - getERC1155BalanceOf: jest.fn().mockImplementation(() => new BN(0)), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: false }, + ]); await expect(() => nftController.watchNft( @@ -739,12 +709,14 @@ describe('NftController', () => { mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), - getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -811,12 +783,14 @@ describe('NftController', () => { mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), - getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ ...getDefaultPreferencesState(), @@ -882,12 +856,14 @@ describe('NftController', () => { mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), - getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ ...getDefaultPreferencesState(), @@ -953,12 +929,14 @@ describe('NftController', () => { mockGetERC721AssetSymbol, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), - getERC721OwnerOf: jest.fn().mockResolvedValue(OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -1023,10 +1001,6 @@ describe('NftController', () => { triggerSelectedAccountChange, } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), - getERC721OwnerOf: jest - .fn() - .mockRejectedValue(new Error('Not an ERC721 contract')), - getERC1155BalanceOf: jest.fn().mockResolvedValue(new BN(1)), getERC721TokenURI: jest .fn() .mockRejectedValue(new Error('Not an ERC721 contract')), @@ -1035,6 +1009,9 @@ describe('NftController', () => { .mockResolvedValue('https://testtokenuri.com'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); triggerSelectedAccountChange(OWNER_ACCOUNT); triggerPreferencesStateChange({ @@ -1092,10 +1069,6 @@ describe('NftController', () => { const addRequestMock = jest.fn().mockResolvedValue(undefined); const { nftController, triggerPreferencesStateChange } = setupController({ getAccount: jest.fn().mockReturnValue(OWNER_ACCOUNT), - getERC721OwnerOf: jest - .fn() - .mockRejectedValue(new Error('Not an ERC721 contract')), - getERC1155BalanceOf: jest.fn().mockResolvedValue(new BN(1)), getERC721TokenURI: jest .fn() .mockRejectedValue(new Error('Not an ERC721 contract')), @@ -1104,6 +1077,9 @@ describe('NftController', () => { .mockResolvedValue('https://testtokenuri.com'), approvalAddRequest: addRequestMock, }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); triggerPreferencesStateChange({ ...getDefaultPreferencesState(), isIpfsGatewayEnabled: true, @@ -1163,13 +1139,15 @@ describe('NftController', () => { triggerPreferencesStateChange, triggerSelectedAccountChange, } = setupController({ - getERC721OwnerOf: jest.fn().mockResolvedValue(SECOND_OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); const requestId = 'approval-request-id-1'; @@ -1266,13 +1244,15 @@ describe('NftController', () => { triggerPreferencesStateChange, triggerSelectedAccountChange, } = setupController({ - getERC721OwnerOf: jest.fn().mockImplementation(() => OWNER_ADDRESS), getERC721TokenURI: jest .fn() .mockResolvedValue('https://testtokenuri.com'), getERC721AssetName: jest.fn().mockResolvedValue('testERC721Name'), getERC721AssetSymbol: jest.fn().mockResolvedValue('testERC721Symbol'), }); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); const requestId = 'approval-request-id-1'; @@ -1362,8 +1342,9 @@ describe('NftController', () => { it('should throw an error when calls to `ownerOf` and `balanceOf` revert', async function () { const { nftController } = setupController(); - // getERC721OwnerOf not mocked - // getERC1155BalanceOf not mocked + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: undefined }, + ]); const requestId = 'approval-request-id-1'; (v4 as jest.Mock).mockImplementationOnce(() => requestId); @@ -3765,14 +3746,10 @@ describe('NftController', () => { describe('isNftOwner', () => { it('should verify the ownership of an NFT when passed a networkClientId', async () => { - const mockGetERC721OwnerOf = jest.fn().mockResolvedValue(OWNER_ADDRESS); - const mockGetERC1155BalanceOf = jest - .fn() - .mockRejectedValue(new Error('ERC1155 error')); - const { nftController } = setupController({ - getERC721OwnerOf: mockGetERC721OwnerOf, - getERC1155BalanceOf: mockGetERC1155BalanceOf, - }); + const { nftController } = setupController(); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); const isOwner = await nftController.isNftOwner( OWNER_ADDRESS, @@ -3784,14 +3761,10 @@ describe('NftController', () => { }); it('should verify the ownership of an ERC-721 NFT with the correct owner address', async () => { - const mockGetERC721OwnerOf = jest.fn().mockResolvedValue(OWNER_ADDRESS); - const mockGetERC1155BalanceOf = jest - .fn() - .mockRejectedValue(new Error('ERC1155 error')); - const { nftController } = setupController({ - getERC721OwnerOf: mockGetERC721OwnerOf, - getERC1155BalanceOf: mockGetERC1155BalanceOf, - }); + const { nftController } = setupController(); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); const isOwner = await nftController.isNftOwner( OWNER_ADDRESS, @@ -3803,14 +3776,10 @@ describe('NftController', () => { }); it('should not verify the ownership of an ERC-721 NFT with the wrong owner address', async () => { - const mockGetERC721OwnerOf = jest.fn().mockResolvedValue(OWNER_ADDRESS); - const mockGetERC1155BalanceOf = jest - .fn() - .mockRejectedValue(new Error('ERC1155 error')); - const { nftController } = setupController({ - getERC721OwnerOf: mockGetERC721OwnerOf, - getERC1155BalanceOf: mockGetERC1155BalanceOf, - }); + const { nftController } = setupController(); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: false }, + ]); const isOwner = await nftController.isNftOwner( '0x0000000000000000000000000000000000000000', @@ -3822,14 +3791,10 @@ describe('NftController', () => { }); it('should verify the ownership of an ERC-1155 NFT with the correct owner address', async () => { - const mockGetERC721OwnerOf = jest - .fn() - .mockRejectedValue(new Error('ERC721 error')); - const mockGetERC1155BalanceOf = jest.fn().mockResolvedValue(new BN(1)); - const { nftController } = setupController({ - getERC721OwnerOf: mockGetERC721OwnerOf, - getERC1155BalanceOf: mockGetERC1155BalanceOf, - }); + const { nftController } = setupController(); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: true }, + ]); const isOwner = await nftController.isNftOwner( OWNER_ADDRESS, @@ -3841,14 +3806,10 @@ describe('NftController', () => { }); it('should not verify the ownership of an ERC-1155 NFT with the wrong owner address', async () => { - const mockGetERC721OwnerOf = jest - .fn() - .mockRejectedValue(new Error('ERC721 error')); - const mockGetERC1155BalanceOf = jest.fn().mockResolvedValue(new BN(0)); - const { nftController } = setupController({ - getERC721OwnerOf: mockGetERC721OwnerOf, - getERC1155BalanceOf: mockGetERC1155BalanceOf, - }); + const { nftController } = setupController(); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: false }, + ]); const isOwner = await nftController.isNftOwner( '0x0000000000000000000000000000000000000000', @@ -3861,16 +3822,10 @@ describe('NftController', () => { }); it('should throw an error for an unsupported standard', async () => { - const mockGetERC721OwnerOf = jest - .fn() - .mockRejectedValue(new Error('ERC721 error')); - const mockGetERC1155BalanceOf = jest - .fn() - .mockRejectedValue(new Error('ERC1155 error')); - const { nftController } = setupController({ - getERC721OwnerOf: mockGetERC721OwnerOf, - getERC1155BalanceOf: mockGetERC1155BalanceOf, - }); + const { nftController } = setupController(); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValueOnce([ + { isOwned: undefined }, + ]); const error = "Unable to verify ownership. Possibly because the standard is not supported or the user's currently selected network does not match the chain of the asset in question."; const result = async () => { @@ -4221,11 +4176,19 @@ describe('NftController', () => { describe('checkAndUpdateNftsOwnershipStatus', () => { describe('checkAndUpdateAllNftsOwnershipStatus', () => { - it('should check whether NFTs for the current selectedAccount/chainId combination are still owned by the selectedAccount and update the isCurrentlyOwned value to false when NFT is not still owned', async () => { + const nftOwnershipResult = ( + isOwned: boolean | undefined, + ): NftOwnershipResult[] => [ + { nftAddress: '0x02', tokenId: '1', isOwned }, + ]; + + it('should update isCurrentlyOwned to false when NFT is no longer owned', async () => { const { nftController } = setupController({ defaultSelectedAccount: OWNER_ACCOUNT, }); - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValue( + nftOwnershipResult(false), + ); await nftController.addNft('0x02', '1', 'mainnet', { nftMetadata: { @@ -4249,11 +4212,13 @@ describe('NftController', () => { ).toBe(false); }); - it('should check whether NFTs for the current selectedAccount/chainId combination are still owned by the selectedAccount and leave/set the isCurrentlyOwned value to true when NFT is still owned', async () => { + it('should leave isCurrentlyOwned as true when NFT is still owned', async () => { const { nftController } = setupController({ defaultSelectedAccount: OWNER_ACCOUNT, }); - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(true); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValue( + nftOwnershipResult(true), + ); await nftController.addNft('0x02', '1', 'mainnet', { nftMetadata: { @@ -4277,13 +4242,13 @@ describe('NftController', () => { ).toBe(true); }); - it('should check whether NFTs for the current selectedAccount/chainId combination are still owned by the selectedAccount and leave the isCurrentlyOwned value as is when NFT ownership check fails', async () => { + it('should leave isCurrentlyOwned unchanged when ownership check is inconclusive', async () => { const { nftController } = setupController({ defaultSelectedAccount: OWNER_ACCOUNT, }); - jest - .spyOn(nftController, 'isNftOwner') - .mockRejectedValue('Unable to verify ownership'); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValue( + nftOwnershipResult(undefined), + ); await nftController.addNft('0x02', '1', 'mainnet', { nftMetadata: { @@ -4307,7 +4272,7 @@ describe('NftController', () => { ).toBe(true); }); - it('should check whether NFTs for the current selectedAccount/chainId combination are still owned by the selectedAccount and update the isCurrentlyOwned value to false when NFT is not still owned, when the currently configured selectedAccount/chainId are different from those passed', async () => { + it('should respect an explicit userAddress when checking ownership', async () => { const { nftController, triggerPreferencesStateChange, mockGetAccount } = setupController(); @@ -4332,7 +4297,9 @@ describe('NftController', () => { .isCurrentlyOwned, ).toBe(true); - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); + (getNftOwnershipForMultipleNfts as jest.Mock).mockResolvedValue( + nftOwnershipResult(false), + ); triggerPreferencesStateChange({ ...getDefaultPreferencesState(), @@ -4352,7 +4319,6 @@ describe('NftController', () => { it('should handle default case where selectedAccount is not set', async () => { const { nftController, mockGetAccount } = setupController({}); mockGetAccount.mockReturnValue(null); - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); await nftController.addNft('0x02', '1', 'mainnet', { nftMetadata: { @@ -4369,208 +4335,35 @@ describe('NftController', () => { expect(nftController.state.allNfts['']).toBeUndefined(); }); - }); - describe('checkAndUpdateSingleNftOwnershipStatus', () => { - it('should check whether the passed NFT is still owned by the the current selectedAccount/chainId combination and update its isCurrentlyOwned property in state if batch is false and isNftOwner returns false', async () => { + it('should preserve current state when getNftOwnershipForMultipleNfts throws', async () => { const { nftController } = setupController({ defaultSelectedAccount: OWNER_ACCOUNT, }); - - const nft = { - address: '0x02', - tokenId: '1', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - favorite: false, - }; - - await nftController.addNft(nft.address, nft.tokenId, 'mainnet', { - nftMetadata: nft, - }); - - expect( - nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0] - .isCurrentlyOwned, - ).toBe(true); - - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); - - await nftController.checkAndUpdateSingleNftOwnershipStatus( - nft, - false, - 'mainnet', + (getNftOwnershipForMultipleNfts as jest.Mock).mockRejectedValue( + new Error('provider connection lost'), ); - expect( - nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0] - .isCurrentlyOwned, - ).toBe(false); - }); - - it('should check whether the passed NFT is still owned by the the current selectedAddress/chainId combination and return the updated NFT object without updating state if batch is true', async () => { - const { nftController } = setupController({ - defaultSelectedAccount: OWNER_ACCOUNT, - }); - - const nft = { - address: '0x02', - tokenId: '1', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - favorite: false, - }; - - await nftController.addNft(nft.address, nft.tokenId, 'mainnet', { - nftMetadata: nft, + await nftController.addNft('0x02', '1', 'mainnet', { + nftMetadata: { + name: 'name', + image: 'image', + description: 'description', + standard: 'standard', + favorite: false, + }, }); - expect( nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0] .isCurrentlyOwned, ).toBe(true); - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); - - const updatedNft = - await nftController.checkAndUpdateSingleNftOwnershipStatus( - nft, - true, - 'mainnet', - ); + await nftController.checkAndUpdateAllNftsOwnershipStatus('mainnet'); expect( nftController.state.allNfts[OWNER_ACCOUNT.address][ChainId.mainnet][0] .isCurrentlyOwned, ).toBe(true); - - expect(updatedNft?.isCurrentlyOwned).toBe(false); - }); - - it('should check whether the passed NFT is still owned by the the selectedAddress/chainId combination passed in the accountParams argument and update its isCurrentlyOwned property in state, when the currently configured selectedAddress/chainId are different from those passed', async () => { - const firstSelectedAddress = OWNER_ACCOUNT.address; - const { - nftController, - triggerPreferencesStateChange, - triggerSelectedAccountChange, - } = setupController(); - - triggerSelectedAccountChange(OWNER_ACCOUNT); - triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - displayNftMedia: true, - }); - - const nft = { - address: '0x02', - tokenId: '1', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - favorite: false, - }; - - await nftController.addNft(nft.address, nft.tokenId, 'sepolia', { - nftMetadata: nft, - }); - - expect( - nftController.state.allNfts[firstSelectedAddress][ChainId.sepolia][0] - .isCurrentlyOwned, - ).toBe(true); - - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); - const secondAccount = createMockInternalAccount({ - address: SECOND_OWNER_ADDRESS, - }); - triggerSelectedAccountChange(secondAccount); - triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - displayNftMedia: true, - }); - - await nftController.checkAndUpdateSingleNftOwnershipStatus( - nft, - false, - 'sepolia', - { - userAddress: OWNER_ADDRESS, - }, - ); - - expect( - nftController.state.allNfts[OWNER_ADDRESS][SEPOLIA.chainId][0] - .isCurrentlyOwned, - ).toBe(false); - }); - - it('should check whether the passed NFT is still owned by the the selectedAddress/chainId combination passed in the accountParams argument and return the updated NFT object without updating state, when the currently configured selectedAddress/chainId are different from those passed and batch is true', async () => { - const firstSelectedAddress = OWNER_ACCOUNT.address; - const { - nftController, - triggerPreferencesStateChange, - triggerSelectedAccountChange, - } = setupController(); - - triggerSelectedAccountChange(OWNER_ACCOUNT); - triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - displayNftMedia: true, - }); - - const nft = { - address: '0x02', - tokenId: '1', - name: 'name', - image: 'image', - description: 'description', - standard: 'standard', - favorite: false, - }; - - await nftController.addNft(nft.address, nft.tokenId, 'sepolia', { - nftMetadata: nft, - }); - - expect( - nftController.state.allNfts[firstSelectedAddress][ChainId.sepolia][0] - .isCurrentlyOwned, - ).toBe(true); - - jest.spyOn(nftController, 'isNftOwner').mockResolvedValue(false); - const secondAccount = createMockInternalAccount({ - address: SECOND_OWNER_ADDRESS, - }); - triggerSelectedAccountChange(secondAccount); - triggerPreferencesStateChange({ - ...getDefaultPreferencesState(), - displayNftMedia: true, - }); - - const updatedNft = - await nftController.checkAndUpdateSingleNftOwnershipStatus( - nft, - false, - 'sepolia', - { - userAddress: OWNER_ADDRESS, - }, - ); - - expect(updatedNft).toStrictEqual({ - ...nft, - isCurrentlyOwned: false, - }); - - expect( - nftController.state.allNfts[OWNER_ADDRESS][SEPOLIA.chainId][0] - .isCurrentlyOwned, - ).toBe(false); }); }); }); diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 13a81487f10..5363001d9a0 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -1,4 +1,5 @@ import { isAddress } from '@ethersproject/address'; +import { Web3Provider } from '@ethersproject/providers'; import type { AccountsControllerSelectedEvmAccountChangeEvent, AccountsControllerGetAccountAction, @@ -43,11 +44,9 @@ import BN from 'bn.js'; import { v4 as random } from 'uuid'; import type { - AssetsContractControllerGetERC1155BalanceOfAction, AssetsContractControllerGetERC1155TokenURIAction, AssetsContractControllerGetERC721AssetNameAction, AssetsContractControllerGetERC721AssetSymbolAction, - AssetsContractControllerGetERC721OwnerOfAction, AssetsContractControllerGetERC721TokenURIAction, } from './AssetsContractController-method-action-types'; import { @@ -57,6 +56,8 @@ import { reduceInBatchesSerially, } from './assetsUtil'; import { Source } from './constants'; +import { getNftOwnershipForMultipleNfts } from './multicall'; +import type { NftOwnershipResult } from './multicall'; import type { ApiNftContract, ReservoirResponse, @@ -304,8 +305,6 @@ export type AllowedActions = | AssetsContractControllerGetERC721AssetNameAction | AssetsContractControllerGetERC721AssetSymbolAction | AssetsContractControllerGetERC721TokenURIAction - | AssetsContractControllerGetERC721OwnerOfAction - | AssetsContractControllerGetERC1155BalanceOfAction | AssetsContractControllerGetERC1155TokenURIAction | NetworkControllerFindNetworkClientIdByChainIdAction | PhishingControllerBulkScanUrlsAction; @@ -1275,6 +1274,7 @@ export class NftController extends BaseController< contractAddress, tokenId, networkClientId, + { standard: type }, ); if (!isOwner) { throw rpcErrors.invalidInput( @@ -1369,6 +1369,9 @@ export class NftController extends BaseController< * @param nftAddress - NFT contract address. * @param tokenId - NFT token ID. * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. + * @param options - Optional parameters. + * @param options.standard - The NFT standard ('ERC721' or 'ERC1155'). When provided, only the + * relevant ownership check is performed, halving the number of RPC subcalls. * @returns Promise resolving the NFT ownership. */ async isNftOwner( @@ -1376,37 +1379,34 @@ export class NftController extends BaseController< nftAddress: string, tokenId: string, networkClientId: NetworkClientId, + { standard }: { standard?: string | null } = {}, ): Promise { - // Checks the ownership for ERC-721. - try { - const owner = await this.messenger.call( - 'AssetsContractController:getERC721OwnerOf', - nftAddress, - tokenId, - networkClientId, - ); - return ownerAddress.toLowerCase() === owner.toLowerCase(); - } catch { - // Ignore ERC-721 contract error - } + const client = this.messenger.call( + 'NetworkController:getNetworkClientById', + networkClientId, + ); + const provider = new Web3Provider(client.provider); + const { chainId } = client.configuration; - // Checks the ownership for ERC-1155. - try { - const balance = await this.messenger.call( - 'AssetsContractController:getERC1155BalanceOf', - ownerAddress, - nftAddress, - tokenId, - networkClientId, + const [result] = await getNftOwnershipForMultipleNfts( + [ + { + nftAddress, + tokenId, + userAddress: ownerAddress, + standard: standard ?? null, + }, + ], + chainId, + provider, + ); + + if (result.isOwned === undefined) { + throw new Error( + `Unable to verify ownership. Possibly because the standard is not supported or the user's currently selected network does not match the chain of the asset in question.`, ); - return !balance.isZero(); - } catch { - // Ignore ERC-1155 contract error } - - throw new Error( - `Unable to verify ownership. Possibly because the standard is not supported or the user's currently selected network does not match the chain of the asset in question.`, - ); + return result.isOwned; } /** @@ -1858,86 +1858,10 @@ export class NftController extends BaseController< }); } - /** - * Checks whether input NFT is still owned by the user - * And updates the isCurrentlyOwned value on the NFT object accordingly. - * - * @param nft - The NFT object to check and update. - * @param batch - A boolean indicating whether this method is being called as part of a batch or single update. - * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. - * @param accountParams - The userAddress and chainId to check ownership against - * @param accountParams.userAddress - the address passed through the confirmed transaction flow to ensure assets are stored to the correct account - * @returns the NFT with the updated isCurrentlyOwned value - */ - async checkAndUpdateSingleNftOwnershipStatus( - nft: Nft, - batch: boolean, - networkClientId: NetworkClientId, - { userAddress }: { userAddress?: string } = {}, - ): Promise { - const addressToSearch = this.#getAddressOrSelectedAddress(userAddress); - const { - configuration: { chainId }, - } = this.messenger.call( - 'NetworkController:getNetworkClientById', - networkClientId, - ); - const { address, tokenId } = nft; - let isOwned = nft.isCurrentlyOwned; - try { - isOwned = await this.isNftOwner( - addressToSearch, - address, - tokenId, - networkClientId, - ); - } catch { - // ignore error - // this will only throw an error 'Unable to verify ownership' in which case - // we want to keep the current value of isCurrentlyOwned for this flow. - } - - const updatedNft = { - ...nft, - isCurrentlyOwned: isOwned, - }; - - if (batch) { - return updatedNft; - } - - // if this is not part of a batched update we update this one NFT in state - const { allNfts } = this.state; - const nfts = [...(allNfts[addressToSearch]?.[chainId] || [])]; - const indexToUpdate = nfts.findIndex( - (item) => - item.tokenId === tokenId && - item.address.toLowerCase() === address.toLowerCase(), - ); - - if (indexToUpdate !== -1) { - nfts[indexToUpdate] = updatedNft; - this.update((state) => { - state.allNfts[addressToSearch] = Object.assign( - {}, - state.allNfts[addressToSearch], - { - [chainId]: nfts, - }, - ); - }); - this.#updateNestedNftState(nfts, ALL_NFTS_STATE_KEY, { - userAddress: addressToSearch, - chainId, - }); - } - - return updatedNft; - } - /** * Checks whether NFTs associated with current selectedAddress/chainId combination are still owned by the user * And updates the isCurrentlyOwned value on each accordingly. + * Uses Multicall3 to batch all ownership checks into a single RPC request when available. * * @param networkClientId - The networkClientId that can be used to identify the network client to use for this request. * @param options - an object of arguments @@ -1952,28 +1876,42 @@ export class NftController extends BaseController< } = {}, ): Promise { const addressToSearch = this.#getAddressOrSelectedAddress(userAddress); - const { - configuration: { chainId }, - } = this.messenger.call( + const client = this.messenger.call( 'NetworkController:getNetworkClientById', networkClientId, ); + const { chainId } = client.configuration; const { allNfts } = this.state; const nfts = allNfts[addressToSearch]?.[chainId] || []; - const updatedNfts = await Promise.all( - nfts.map(async (nft) => { - return ( - (await this.checkAndUpdateSingleNftOwnershipStatus( - nft, - true, - networkClientId, - { - userAddress, - }, - )) ?? nft - ); - }), - ); + + if (nfts.length === 0) { + return; + } + + const provider = new Web3Provider(client.provider); + + let ownershipResults: NftOwnershipResult[]; + try { + ownershipResults = await getNftOwnershipForMultipleNfts( + nfts.map((nft) => ({ + nftAddress: nft.address, + tokenId: nft.tokenId, + userAddress: addressToSearch, + standard: nft.standard, + })), + chainId, + provider, + ); + } catch { + return; + } + + const updatedNfts = nfts.map((nft, index) => { + const { isOwned } = ownershipResults[index]; + return isOwned === undefined + ? nft + : { ...nft, isCurrentlyOwned: isOwned }; + }); this.#updateNestedNftState(updatedNfts, ALL_NFTS_STATE_KEY, { userAddress: addressToSearch, diff --git a/packages/assets-controllers/src/multicall.test.ts b/packages/assets-controllers/src/multicall.test.ts index d9795f8e591..f549980527b 100644 --- a/packages/assets-controllers/src/multicall.test.ts +++ b/packages/assets-controllers/src/multicall.test.ts @@ -10,8 +10,9 @@ import { aggregate3, getTokenBalancesForMultipleAddresses, getStakedBalancesForAddresses, + getNftOwnershipForMultipleNfts, } from './multicall'; -import type { Aggregate3Call } from './multicall'; +import type { Aggregate3Call, NftOwnershipQuery } from './multicall'; const provider = new Web3Provider(jest.fn()); @@ -1556,4 +1557,342 @@ describe('multicall', () => { }); }); }); + + describe('getNftOwnershipForMultipleNfts', () => { + const ownerAddress = '0x0000000000000000000000000000000000000001'; + const otherAddress = '0x0000000000000000000000000000000000000099'; + const nftAddress = '0x0000000000000000000000000000000000000ABC'; + const supportedChainId: Hex = '0x1'; + const unsupportedChainId: Hex = '0x999999'; + + const makeQuery = ( + overrides: Partial = {}, + ): NftOwnershipQuery => ({ + nftAddress, + tokenId: '1', + userAddress: ownerAddress, + standard: null, + ...overrides, + }); + + const encodeAggregate3Response = ( + results: { success: boolean; data: string }[], + ): string => + defaultAbiCoder.encode( + ['tuple(bool,bytes)[]'], + [results.map(({ success, data }) => [success, data])], + ); + + const encodeOwnerOfResult = (owner: string): string => + defaultAbiCoder.encode(['address'], [owner]); + + const encodeBalanceOfResult = (balance: number): string => + defaultAbiCoder.encode(['uint256'], [balance]); + + it('should return empty array for empty input', async () => { + const results = await getNftOwnershipForMultipleNfts( + [], + supportedChainId, + provider, + ); + expect(results).toStrictEqual([]); + }); + + describe('via multicall (supported chain)', () => { + it('should detect ERC-721 ownership when ownerOf matches', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + { success: false, data: '0x' }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + }); + + it('should detect ERC-721 non-ownership when ownerOf returns a different address', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(otherAddress) }, + { success: false, data: '0x' }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: false }, + ]); + }); + + it('should fall back to ERC-1155 balanceOf when ownerOf fails', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: false, data: '0x' }, + { success: true, data: encodeBalanceOfResult(3) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + }); + + it('should return isOwned=false when ERC-1155 balance is zero', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: false, data: '0x' }, + { success: true, data: encodeBalanceOfResult(0) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: false }, + ]); + }); + + it('should return isOwned=undefined when both calls fail', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: false, data: '0x' }, + { success: false, data: '0x' }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: undefined }, + ]); + }); + + it('should skip ERC-1155 call when standard is ERC721', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery({ standard: 'ERC721' })], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + // Only 1 subcall should have been sent (ownerOf), not 2 + expect(provider.call).toHaveBeenCalledTimes(1); + }); + + it('should skip ERC-721 call when standard is ERC1155', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeBalanceOfResult(1) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery({ standard: 'ERC1155' })], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + expect(provider.call).toHaveBeenCalledTimes(1); + }); + + it('should not let ERC-1155 result override a definitive ERC-721 result', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + { success: true, data: encodeBalanceOfResult(0) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + }); + + it('should handle multiple NFTs in a single batch', async () => { + const nftAddress2 = '0x0000000000000000000000000000000000000DEF'; + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + { success: false, data: '0x' }, + { success: false, data: '0x' }, + { success: true, data: encodeBalanceOfResult(5) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [ + makeQuery(), + makeQuery({ nftAddress: nftAddress2, tokenId: '42' }), + ], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + { nftAddress: nftAddress2, tokenId: '42', isOwned: true }, + ]); + }); + }); + + describe('via individual calls (unsupported chain)', () => { + it('should detect ERC-721 ownership via individual ownerOf call', async () => { + jest + .spyOn(provider, 'call') + .mockResolvedValueOnce(encodeOwnerOfResult(ownerAddress)); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + unsupportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + }); + + it('should fall back to ERC-1155 when ERC-721 call reverts', async () => { + jest + .spyOn(provider, 'call') + .mockRejectedValueOnce(new Error('not ERC721')) + .mockResolvedValueOnce(encodeBalanceOfResult(2)); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + unsupportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + }); + + it('should return isOwned=undefined when both individual calls fail', async () => { + jest + .spyOn(provider, 'call') + .mockRejectedValueOnce(new Error('not ERC721')) + .mockRejectedValueOnce(new Error('not ERC1155')); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + unsupportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: undefined }, + ]); + }); + + it('should skip ERC-1155 when standard is ERC721', async () => { + jest + .spyOn(provider, 'call') + .mockResolvedValueOnce(encodeOwnerOfResult(ownerAddress)); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery({ standard: 'ERC721' })], + unsupportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + expect(provider.call).toHaveBeenCalledTimes(1); + }); + + it('should skip ERC-721 when standard is ERC1155', async () => { + jest + .spyOn(provider, 'call') + .mockResolvedValueOnce(encodeBalanceOfResult(1)); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery({ standard: 'ERC1155' })], + unsupportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + expect(provider.call).toHaveBeenCalledTimes(1); + }); + }); + + describe('multicall fallback', () => { + it('should fall back to individual calls when multicall3 throws', async () => { + const callSpy = jest.spyOn(provider, 'call'); + // First call: multicall3 aggregate3 fails + callSpy.mockRejectedValueOnce(new Error('multicall3 reverted')); + // Subsequent calls: individual ownerOf succeeds + callSpy.mockResolvedValueOnce(encodeOwnerOfResult(ownerAddress)); + + const consoleSpy = jest + .spyOn(console, 'warn') + .mockImplementation(() => undefined); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery()], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + expect(consoleSpy).toHaveBeenCalledWith( + 'Multicall3 NFT ownership check failed, falling back to individual calls', + expect.any(Error), + ); + + consoleSpy.mockRestore(); + }); + }); + }); }); diff --git a/packages/assets-controllers/src/multicall.ts b/packages/assets-controllers/src/multicall.ts index 91c63d7b1b7..8f47d91576c 100644 --- a/packages/assets-controllers/src/multicall.ts +++ b/packages/assets-controllers/src/multicall.ts @@ -394,10 +394,37 @@ export type Aggregate3Result = { // Constants for encoded strings and addresses const ZERO_ADDRESS = '0x0000000000000000000000000000000000000000'; const BALANCE_OF_FUNCTION = 'balanceOf(address)'; +const OWNER_OF_FUNCTION = 'ownerOf(uint256)'; +const ERC1155_BALANCE_OF_FUNCTION = 'balanceOf(address,uint256)'; const GET_ETH_BALANCE_FUNCTION = 'getEthBalance'; const GET_SHARES_FUNCTION = 'getShares'; const CONVERT_TO_ASSETS_FUNCTION = 'convertToAssets'; +// ERC-721 ownerOf ABI +const ERC721_OWNER_OF_ABI = [ + { + name: 'ownerOf', + type: 'function', + inputs: [{ name: 'tokenId', type: 'uint256' }], + outputs: [{ name: 'owner', type: 'address' }], + stateMutability: 'view', + }, +]; + +// ERC-1155 balanceOf ABI +const ERC1155_BALANCE_OF_ABI = [ + { + name: 'balanceOf', + type: 'function', + inputs: [ + { name: 'account', type: 'address' }, + { name: 'id', type: 'uint256' }, + ], + outputs: [{ name: 'balance', type: 'uint256' }], + stateMutability: 'view', + }, +]; + // ERC20 balanceOf ABI const ERC20_BALANCE_OF_ABI = [ { @@ -1161,3 +1188,245 @@ export const getTokenBalancesForMultipleAddresses = async ( return result; } }; + +export type NftOwnershipQuery = { + nftAddress: string; + tokenId: string; + userAddress: string; + standard: string | null; +}; + +export type NftOwnershipResult = { + nftAddress: string; + tokenId: string; + isOwned: boolean | undefined; +}; + +type NftCallMeta = { nftIndex: number; callVariant: 'erc721' | 'erc1155' }; + +const normalizeNftStandard = ( + standard: string | null, +): 'ERC721' | 'ERC1155' | null => { + if (!standard) { + return null; + } + const upper = standard.toUpperCase(); + if (upper === 'ERC721') { + return 'ERC721'; + } + if (upper === 'ERC1155') { + return 'ERC1155'; + } + return null; +}; + +const getNftOwnershipViaMulticall = async ( + nfts: NftOwnershipQuery[], + chainId: Hex, + provider: Web3Provider, +): Promise => { + const erc721Contract = new Contract( + ZERO_ADDRESS, + ERC721_OWNER_OF_ABI, + provider, + ); + const erc1155Contract = new Contract( + ZERO_ADDRESS, + ERC1155_BALANCE_OF_ABI, + provider, + ); + + const calls: Aggregate3Call[] = []; + const meta: NftCallMeta[] = []; + + // When the standard is known, emit only the relevant call to halve the number + // of multicall subcalls. When unknown (null), emit both — ERC-721 first so + // the `!== undefined` guard below replicates the original early-return + // behavior: once ERC-721 gives a definitive answer, ERC-1155 is skipped. + nfts.forEach(({ nftAddress, tokenId, userAddress, standard }, i) => { + const normalized = normalizeNftStandard(standard); + const tryErc721 = normalized !== 'ERC1155'; + const tryErc1155 = normalized !== 'ERC721'; + + if (tryErc721) { + calls.push({ + target: nftAddress, + allowFailure: true, + callData: erc721Contract.interface.encodeFunctionData( + OWNER_OF_FUNCTION, + [tokenId], + ), + }); + meta.push({ nftIndex: i, callVariant: 'erc721' }); + } + + if (tryErc1155) { + calls.push({ + target: nftAddress, + allowFailure: true, + callData: erc1155Contract.interface.encodeFunctionData( + ERC1155_BALANCE_OF_FUNCTION, + [userAddress, tokenId], + ), + }); + meta.push({ nftIndex: i, callVariant: 'erc1155' }); + } + }); + + const maxCallsPerBatch = 300; + const allReturnData: Aggregate3Result[] = []; + // Batches are processed serially and results are pushed in order, so + // allReturnData[i] always corresponds to meta[i]. Do NOT change this to + // parallel batching without also reworking the index correspondence. + await reduceInBatchesSerially({ + values: calls, + batchSize: maxCallsPerBatch, + initialResult: undefined, + eachBatch: async (_, batch) => { + const batchResults = await aggregate3(batch, chainId, provider); + allReturnData.push(...batchResults); + }, + }); + + const results: NftOwnershipResult[] = nfts.map(({ nftAddress, tokenId }) => ({ + nftAddress, + tokenId, + isOwned: undefined, + })); + + allReturnData.forEach(({ success, returnData: data }, i) => { + if (!success) { + return; + } + const { nftIndex, callVariant } = meta[i]; + // Once we have a definitive answer (true or false) from any call for this NFT, + // don't override it — mirrors the original isNftOwner behavior which returned + // immediately after the first successful contract call. + if (results[nftIndex].isOwned !== undefined) { + return; + } + if (callVariant === 'erc721') { + const [owner] = erc721Contract.interface.decodeFunctionResult( + OWNER_OF_FUNCTION, + data, + ); + results[nftIndex].isOwned = + owner.toLowerCase() === nfts[nftIndex].userAddress.toLowerCase(); + } else { + const [balance] = erc1155Contract.interface.decodeFunctionResult( + ERC1155_BALANCE_OF_FUNCTION, + data, + ); + results[nftIndex].isOwned = new BN(balance.toString()).gt(new BN(0)); + } + }); + + return results; +}; + +const MAX_PARALLEL_NFT_CALLS = 50; + +const getNftOwnershipIndividually = async ( + nfts: NftOwnershipQuery[], + provider: Web3Provider, +): Promise => { + const erc721Iface = new Contract(ZERO_ADDRESS, ERC721_OWNER_OF_ABI, provider) + .interface; + const erc1155Iface = new Contract( + ZERO_ADDRESS, + ERC1155_BALANCE_OF_ABI, + provider, + ).interface; + + return reduceInBatchesSerially({ + values: nfts, + batchSize: MAX_PARALLEL_NFT_CALLS, + initialResult: [], + eachBatch: async (workingResult, batch) => { + const batchResults = await Promise.all( + batch.map(async ({ nftAddress, tokenId, userAddress, standard }) => { + let isOwned: boolean | undefined; + const normalized = normalizeNftStandard(standard); + const tryErc721 = normalized !== 'ERC1155'; + const tryErc1155 = normalized !== 'ERC721'; + + if (tryErc721) { + try { + const callData = erc721Iface.encodeFunctionData( + OWNER_OF_FUNCTION, + [tokenId], + ); + const raw = await provider.call({ + to: nftAddress, + data: callData, + }); + const [owner] = erc721Iface.decodeFunctionResult( + OWNER_OF_FUNCTION, + raw, + ); + isOwned = owner.toLowerCase() === userAddress.toLowerCase(); + } catch { + // ERC-721 unavailable; try ERC-1155 below if applicable. + } + } + + if (isOwned === undefined && tryErc1155) { + try { + const callData = erc1155Iface.encodeFunctionData( + ERC1155_BALANCE_OF_FUNCTION, + [userAddress, tokenId], + ); + const raw = await provider.call({ + to: nftAddress, + data: callData, + }); + const [balance] = erc1155Iface.decodeFunctionResult( + ERC1155_BALANCE_OF_FUNCTION, + raw, + ); + isOwned = new BN(balance.toString()).gt(new BN(0)); + } catch { + // ownership remains undefined + } + } + + return { nftAddress, tokenId, isOwned }; + }), + ); + return [...workingResult, ...batchResults]; + }, + }); +}; + +/** + * Check ownership for multiple NFTs, using Multicall3 when available to batch + * all calls into a single RPC request, falling back to individual calls otherwise. + * + * @param nfts - Array of NFT queries containing address, tokenId, owner address, and standard. + * @param chainId - The hexadecimal chain id. + * @param provider - An ethers rpc provider. + * @returns Promise resolving to array of ownership results. `isOwned` is `undefined` + * when ownership could not be determined (e.g. unsupported standard or RPC error). + */ +export const getNftOwnershipForMultipleNfts = async ( + nfts: NftOwnershipQuery[], + chainId: Hex, + provider: Web3Provider, +): Promise => { + if (nfts.length === 0) { + return []; + } + + if (MULTICALL_CONTRACT_BY_CHAINID[chainId]) { + try { + return await getNftOwnershipViaMulticall(nfts, chainId, provider); + } catch (error) { + console.warn( + 'Multicall3 NFT ownership check failed, falling back to individual calls', + error, + ); + } + } + + return getNftOwnershipIndividually(nfts, provider); +}; From a62cbb433f6976e39304f82b83c1923b6c8cb3f7 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Tue, 24 Mar 2026 13:36:08 +0100 Subject: [PATCH 2/5] chore: update changelog --- packages/assets-controllers/CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index fe89887921e..034a5f0f00e 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **BREAKING:** `NftController` no longer uses the `AssetsContractController:getERC721OwnerOf` and `AssetsContractController:getERC1155BalanceOf` messenger actions for ownership checks; these have been removed from `AllowedActions` ([#8281](https://github.com/MetaMask/core/pull/8281)) + - Consumers that construct the `NftController` messenger and register handlers for these two actions must remove them from their allowed actions list. +- **BREAKING:** Removed the `checkAndUpdateSingleNftOwnershipStatus` method from `NftController` ([#8281](https://github.com/MetaMask/core/pull/8281)) + - Use `checkAndUpdateAllNftsOwnershipStatus` instead, which now batches all ownership checks via Multicall3 in a single RPC request. +- `NftController` NFT ownership checks (`isNftOwner`, `checkAndUpdateAllNftsOwnershipStatus`) now use Multicall3 to batch ERC-721 `ownerOf` and ERC-1155 `balanceOf` calls into fewer RPC requests, falling back to individual calls on unsupported chains ([#8281](https://github.com/MetaMask/core/pull/8281)) + ## [101.0.1] ### Changed From 7b4faee8be96ea57a51c0f4e428bf0ac0ed5fa73 Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Tue, 24 Mar 2026 13:56:54 +0100 Subject: [PATCH 3/5] fix: linting --- .../assets-controllers/src/multicall.test.ts | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/packages/assets-controllers/src/multicall.test.ts b/packages/assets-controllers/src/multicall.test.ts index f549980527b..c23d286f467 100644 --- a/packages/assets-controllers/src/multicall.test.ts +++ b/packages/assets-controllers/src/multicall.test.ts @@ -1695,11 +1695,13 @@ describe('multicall', () => { }); it('should skip ERC-1155 call when standard is ERC721', async () => { - jest.spyOn(provider, 'call').mockResolvedValueOnce( - encodeAggregate3Response([ - { success: true, data: encodeOwnerOfResult(ownerAddress) }, - ]), - ); + jest + .spyOn(provider, 'call') + .mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + ]), + ); const results = await getNftOwnershipForMultipleNfts( [makeQuery({ standard: 'ERC721' })], @@ -1715,11 +1717,13 @@ describe('multicall', () => { }); it('should skip ERC-721 call when standard is ERC1155', async () => { - jest.spyOn(provider, 'call').mockResolvedValueOnce( - encodeAggregate3Response([ - { success: true, data: encodeBalanceOfResult(1) }, - ]), - ); + jest + .spyOn(provider, 'call') + .mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeBalanceOfResult(1) }, + ]), + ); const results = await getNftOwnershipForMultipleNfts( [makeQuery({ standard: 'ERC1155' })], @@ -1764,10 +1768,7 @@ describe('multicall', () => { ); const results = await getNftOwnershipForMultipleNfts( - [ - makeQuery(), - makeQuery({ nftAddress: nftAddress2, tokenId: '42' }), - ], + [makeQuery(), makeQuery({ nftAddress: nftAddress2, tokenId: '42' })], supportedChainId, provider, ); From 5ec873734ce5b41777e1b43405bb0da9f5c0caac Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Wed, 25 Mar 2026 13:48:43 +0100 Subject: [PATCH 4/5] fix: minor issue --- .../assets-controllers/src/multicall.test.ts | 60 +++++++++++++++++++ packages/assets-controllers/src/multicall.ts | 58 ++++++++++++++++-- 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/packages/assets-controllers/src/multicall.test.ts b/packages/assets-controllers/src/multicall.test.ts index c23d286f467..30c3eb6ef12 100644 --- a/packages/assets-controllers/src/multicall.test.ts +++ b/packages/assets-controllers/src/multicall.test.ts @@ -1895,5 +1895,65 @@ describe('multicall', () => { consoleSpy.mockRestore(); }); }); + + describe('unknown-standard NFT exclusion', () => { + it('should skip NFTs with explicitly unrecognized standard entirely', async () => { + const cryptoPunksAddress = + '0xb47e3cd837dDF8e4c57F05d70Ab865de6e193BBB'; + const callSpy = jest.spyOn(provider, 'call'); + + // Only call: multicall aggregate3 for the known-standard NFT + callSpy.mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [ + makeQuery({ standard: 'ERC721' }), + makeQuery({ + nftAddress: cryptoPunksAddress, + tokenId: '1434', + standard: 'UNKNOWN', + }), + ], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + { + nftAddress: cryptoPunksAddress, + tokenId: '1434', + isOwned: undefined, + }, + ]); + // Only 1 multicall call — no individual calls for the UNKNOWN NFT + expect(callSpy).toHaveBeenCalledTimes(1); + }); + + it('should still include standard=null NFTs in multicall batch', async () => { + jest.spyOn(provider, 'call').mockResolvedValueOnce( + encodeAggregate3Response([ + { success: true, data: encodeOwnerOfResult(ownerAddress) }, + { success: false, data: '0x' }, + ]), + ); + + const results = await getNftOwnershipForMultipleNfts( + [makeQuery({ standard: null })], + supportedChainId, + provider, + ); + + expect(results).toStrictEqual([ + { nftAddress, tokenId: '1', isOwned: true }, + ]); + // Single multicall call (no individual calls) + expect(provider.call).toHaveBeenCalledTimes(1); + }); + }); }); }); diff --git a/packages/assets-controllers/src/multicall.ts b/packages/assets-controllers/src/multicall.ts index e09cf9a8475..d6a58b5a8da 100644 --- a/packages/assets-controllers/src/multicall.ts +++ b/packages/assets-controllers/src/multicall.ts @@ -1301,9 +1301,6 @@ const getNftOwnershipViaMulticall = async ( return; } const { nftIndex, callVariant } = meta[i]; - // Once we have a definitive answer (true or false) from any call for this NFT, - // don't override it — mirrors the original isNftOwner behavior which returned - // immediately after the first successful contract call. if (results[nftIndex].isOwned !== undefined) { return; } @@ -1419,9 +1416,50 @@ export const getNftOwnershipForMultipleNfts = async ( return []; } - if (MULTICALL_CONTRACT_BY_CHAINID[chainId]) { + const results: NftOwnershipResult[] = nfts.map(({ nftAddress, tokenId }) => ({ + nftAddress, + tokenId, + isOwned: undefined, + })); + + // Filter out NFTs whose standard is explicitly unrecognized (e.g. + // CryptoPunks with standard="UNKNOWN"). Such contracts use pre-Solidity- + // 0.4.10 bytecode that compiles unrecognized selectors to the INVALID + // opcode, which consumes ALL forwarded gas. Including them in a Multicall3 + // aggregate3 batch causes the entire batch to revert, and calling them + // individually also always fails. They stay as isOwned=undefined. + // + // NFTs with `standard: null` (not yet categorized) are still included + // because they are likely valid ERC-721/ERC-1155 contracts. + const callable = nfts.reduce<{ nft: NftOwnershipQuery; index: number }[]>( + (acc, nft, index) => { + const hasExplicitNonStandard = + nft.standard !== null && normalizeNftStandard(nft.standard) === null; + if (!hasExplicitNonStandard) { + acc.push({ nft, index }); + } + return acc; + }, + [], + ); + + if (callable.length === 0) { + return results; + } + + const multicallAddress = MULTICALL_CONTRACT_BY_CHAINID[chainId]; + + if (multicallAddress) { try { - return await getNftOwnershipViaMulticall(nfts, chainId, provider); + const batchResults = await getNftOwnershipViaMulticall( + callable.map(({ nft }) => nft), + chainId, + provider, + ); + batchResults.forEach((result, batchIndex) => { + results[callable[batchIndex].index] = result; + }); + return results; } catch (error) { console.warn( 'Multicall3 NFT ownership check failed, falling back to individual calls', @@ -1430,5 +1468,13 @@ export const getNftOwnershipForMultipleNfts = async ( } } - return getNftOwnershipIndividually(nfts, provider); + const individualResults = await getNftOwnershipIndividually( + callable.map(({ nft }) => nft), + provider, + ); + individualResults.forEach((result, batchIndex) => { + results[callable[batchIndex].index] = result; + }); + + return results; }; From 10d305cdcebd504da8891e622cd17c8ec811330e Mon Sep 17 00:00:00 2001 From: juanmigdr Date: Wed, 25 Mar 2026 14:21:16 +0100 Subject: [PATCH 5/5] chore: minor formatting --- packages/assets-controllers/src/multicall.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/assets-controllers/src/multicall.test.ts b/packages/assets-controllers/src/multicall.test.ts index 30c3eb6ef12..78b4bb2e97e 100644 --- a/packages/assets-controllers/src/multicall.test.ts +++ b/packages/assets-controllers/src/multicall.test.ts @@ -1898,8 +1898,7 @@ describe('multicall', () => { describe('unknown-standard NFT exclusion', () => { it('should skip NFTs with explicitly unrecognized standard entirely', async () => { - const cryptoPunksAddress = - '0xb47e3cd837dDF8e4c57F05d70Ab865de6e193BBB'; + const cryptoPunksAddress = '0xb47e3cd837dDF8e4c57F05d70Ab865de6e193BBB'; const callSpy = jest.spyOn(provider, 'call'); // Only call: multicall aggregate3 for the known-standard NFT