diff --git a/packages/assets-controllers/CHANGELOG.md b/packages/assets-controllers/CHANGELOG.md index e36d7a3ef56..5201641bd24 100644 --- a/packages/assets-controllers/CHANGELOG.md +++ b/packages/assets-controllers/CHANGELOG.md @@ -11,16 +11,20 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **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. -- `checkAndUpdateAllNftsOwnershipStatus` now removes NFTs confirmed as unowned from state instead of setting `isCurrentlyOwned: false` ([#8281](https://github.com/MetaMask/core/pull/8281)) +- **BREAKING:** `checkAndUpdateSingleNftOwnershipStatus` no longer accepts a `batch` boolean as its second argument; the signature is now `(nft, networkClientId, { userAddress }?)` ([#8281](https://github.com/MetaMask/core/pull/8281), [#8435](https://github.com/MetaMask/core/pull/8435)) + - The method now always writes the updated NFT to state and returns it. Remove the `batch` argument from all call sites. +- **BREAKING:** `checkAndUpdateAllNftsOwnershipStatus` now removes NFTs confirmed as unowned from state instead of setting `isCurrentlyOwned: false` on them ([#8281](https://github.com/MetaMask/core/pull/8281)) - The `isCurrentlyOwned: false` flag was originally used to power a "Previously Owned" NFTs section in MetaMask, which is no longer supported. NFTs that are confirmed as no longer owned are now removed from state immediately rather than being retained with a stale flag. -- `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)) +- `NftController` NFT ownership checks (`isNftOwner`, `checkAndUpdateSingleNftOwnershipStatus`, `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)) - Bump `@metamask/accounts-controller` from `^37.1.1` to `^37.2.0` ([#8363](https://github.com/MetaMask/core/pull/8363)) - Bump `@metamask/keyring-controller` from `^25.1.1` to `^25.2.0` ([#8363](https://github.com/MetaMask/core/pull/8363)) - Bump `@metamask/messenger` from `^1.0.0` to `^1.1.1` ([#8364](https://github.com/MetaMask/core/pull/8364), [#8373](https://github.com/MetaMask/core/pull/8373)) - Bump `@metamask/transaction-controller` from `^64.0.0` to `^64.1.0` ([#8432](https://github.com/MetaMask/core/pull/8432)) +### Fixed + +- Restore `checkAndUpdateSingleNftOwnershipStatus` to `NftController` to fix a regression where consumers (e.g. the MetaMask extension) that call this method individually were broken by its removal in [#8281](https://github.com/MetaMask/core/pull/8281) ([#8435](https://github.com/MetaMask/core/pull/8435)) + ## [103.1.1] ### Changed diff --git a/packages/assets-controllers/src/NftController.test.ts b/packages/assets-controllers/src/NftController.test.ts index 7cc9953a55c..e5f4d693c78 100644 --- a/packages/assets-controllers/src/NftController.test.ts +++ b/packages/assets-controllers/src/NftController.test.ts @@ -4363,6 +4363,108 @@ describe('NftController', () => { ).toBe(true); }); }); + + describe('checkAndUpdateSingleNftOwnershipStatus', () => { + it('should check whether the passed NFT is still owned by the current selectedAccount/chainId combination and update its isCurrentlyOwned property in state when isNftOwner returns false', 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, + 'mainnet', + ); + + 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 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, + }); + + const updatedNft = + await nftController.checkAndUpdateSingleNftOwnershipStatus( + nft, + 'sepolia', + { + userAddress: OWNER_ADDRESS, + }, + ); + + expect(updatedNft).toStrictEqual({ + ...nft, + isCurrentlyOwned: false, + }); + + expect( + nftController.state.allNfts[OWNER_ADDRESS][SEPOLIA.chainId][0] + .isCurrentlyOwned, + ).toBe(false); + }); + }); }); describe('findNftByAddressAndTokenId', () => { diff --git a/packages/assets-controllers/src/NftController.ts b/packages/assets-controllers/src/NftController.ts index 16becb507a2..5c4e9649b75 100644 --- a/packages/assets-controllers/src/NftController.ts +++ b/packages/assets-controllers/src/NftController.ts @@ -1858,6 +1858,68 @@ 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 networkClientId - The networkClientId that can be used to identify the network client to use for this request. + * @param accountParams - The userAddress 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, + 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, + { standard: nft.standard }, + ); + } 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, + }; + + 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.#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.