diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 11463edf3430..42815021f0ca 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -151,6 +151,8 @@ import { } from '@metamask/snaps-utils'; ///: END:ONLY_INCLUDE_IF +import { Interface } from '@ethersproject/abi'; +import { abiERC1155, abiERC721 } from '@metamask/metamask-eth-abis'; import { isEvmAccountType } from '@metamask/keyring-api'; import { methodsRequiringNetworkSwitch, @@ -222,6 +224,10 @@ import { getCurrentChainSupportsSmartTransactions, } from '../../shared/modules/selectors'; import { BaseUrl } from '../../shared/constants/urls'; +import { + TOKEN_TRANSFER_LOG_TOPIC_HASH, + TRANSFER_SINFLE_LOG_TOPIC_HASH, +} from '../../shared/lib/transactions-controller-utils'; import { BalancesController as MultichainBalancesController } from './lib/accounts/BalancesController'; import { ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) @@ -6289,7 +6295,7 @@ export default class MetamaskController extends EventEmitter { } await this._createTransactionNotifcation(transactionMeta); - this._updateNFTOwnership(transactionMeta); + await this._updateNFTOwnership(transactionMeta); this._trackTransactionFailure(transactionMeta); } @@ -6317,46 +6323,158 @@ export default class MetamaskController extends EventEmitter { } } - _updateNFTOwnership(transactionMeta) { + async _updateNFTOwnership(transactionMeta) { // if this is a transferFrom method generated from within the app it may be an NFT transfer transaction // in which case we will want to check and update ownership status of the transferred NFT. - const { type, txParams, chainId } = transactionMeta; + const { type, txParams, chainId, txReceipt } = transactionMeta; + const selectedAddress = + this.accountsController.getSelectedAccount().address; - if ( - type !== TransactionType.tokenMethodTransferFrom || - txParams === undefined - ) { + const { allNfts } = this.nftController.state; + const txReceiptLogs = txReceipt?.logs; + + const isContractInteractionTx = + type === TransactionType.contractInteraction && txReceiptLogs; + const isTransferFromTx = + (type === TransactionType.tokenMethodTransferFrom || + type === TransactionType.tokenMethodSafeTransferFrom) && + txParams !== undefined; + + if (!isContractInteractionTx && !isTransferFromTx) { return; } - const { data, to: contractAddress, from: userAddress } = txParams; - const transactionData = parseStandardTokenTransactionData(data); - // Sometimes the tokenId value is parsed as "_value" param. Not seeing this often any more, but still occasionally: - // i.e. call approve() on BAYC contract - https://etherscan.io/token/0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d#writeContract, and tokenId shows up as _value, - // not sure why since it doesn't match the ERC721 ABI spec we use to parse these transactions - https://github.com/MetaMask/metamask-eth-abis/blob/d0474308a288f9252597b7c93a3a8deaad19e1b2/src/abis/abiERC721.ts#L62. - const transactionDataTokenId = - getTokenIdParam(transactionData) ?? getTokenValueParam(transactionData); + if (isTransferFromTx) { + const { data, to: contractAddress, from: userAddress } = txParams; + const transactionData = parseStandardTokenTransactionData(data); + // Sometimes the tokenId value is parsed as "_value" param. Not seeing this often any more, but still occasionally: + // i.e. call approve() on BAYC contract - https://etherscan.io/token/0xbc4ca0eda7647a8ab7c2061c2e118a18a936f13d#writeContract, and tokenId shows up as _value, + // not sure why since it doesn't match the ERC721 ABI spec we use to parse these transactions - https://github.com/MetaMask/metamask-eth-abis/blob/d0474308a288f9252597b7c93a3a8deaad19e1b2/src/abis/abiERC721.ts#L62. + const transactionDataTokenId = + getTokenIdParam(transactionData) ?? getTokenValueParam(transactionData); + + // check if its a known NFT + const knownNft = allNfts?.[userAddress]?.[chainId]?.find( + ({ address, tokenId }) => + isEqualCaseInsensitive(address, contractAddress) && + tokenId === transactionDataTokenId, + ); - const { allNfts } = this.nftController.state; + // if it is we check and update ownership status. + if (knownNft) { + this.nftController.checkAndUpdateSingleNftOwnershipStatus( + knownNft, + false, + // TODO add networkClientId once it is available in the transactionMeta + // the chainId previously passed here didn't actually allow us to check for ownership on a non globally selected network + // because the check would use the provider for the globally selected network, not the chainId passed here. + { userAddress }, + ); + } + } else { + // Else if contract interaction we will parse the logs + + const allNftTransferLog = txReceiptLogs.map((txReceiptLog) => { + const isERC1155NftTransfer = + txReceiptLog.topics && + txReceiptLog.topics[0] === TRANSFER_SINFLE_LOG_TOPIC_HASH; + const isERC721NftTransfer = + txReceiptLog.topics && + txReceiptLog.topics[0] === TOKEN_TRANSFER_LOG_TOPIC_HASH; + let isTransferToSelectedAddress; + + if (isERC1155NftTransfer) { + isTransferToSelectedAddress = + txReceiptLog.topics && + txReceiptLog.topics[3] && + txReceiptLog.topics[3].match(selectedAddress?.slice(2)); + } - // check if its a known NFT - const knownNft = allNfts?.[userAddress]?.[chainId]?.find( - ({ address, tokenId }) => - isEqualCaseInsensitive(address, contractAddress) && - tokenId === transactionDataTokenId, - ); + if (isERC721NftTransfer) { + isTransferToSelectedAddress = + txReceiptLog.topics && + txReceiptLog.topics[2] && + txReceiptLog.topics[2].match(selectedAddress?.slice(2)); + } - // if it is we check and update ownership status. - if (knownNft) { - this.nftController.checkAndUpdateSingleNftOwnershipStatus( - knownNft, - false, - // TODO add networkClientId once it is available in the transactionMeta - // the chainId previously passed here didn't actually allow us to check for ownership on a non globally selected network - // because the check would use the provider for the globally selected network, not the chainId passed here. - { userAddress }, - ); + return { + isERC1155NftTransfer, + isERC721NftTransfer, + isTransferToSelectedAddress, + ...txReceiptLog, + }; + }); + if (allNftTransferLog.length !== 0) { + const allNftParsedLog = []; + allNftTransferLog.forEach((singleLog) => { + if ( + singleLog.isTransferToSelectedAddress && + (singleLog.isERC1155NftTransfer || singleLog.isERC721NftTransfer) + ) { + let iface; + if (singleLog.isERC1155NftTransfer) { + iface = new Interface(abiERC1155); + } else { + iface = new Interface(abiERC721); + } + try { + const parsedLog = iface.parseLog({ + data: singleLog.data, + topics: singleLog.topics, + }); + allNftParsedLog.push({ + contract: singleLog.address, + ...parsedLog, + }); + } catch (err) { + // ignore + } + } + }); + // Filter known nfts and new Nfts + const knownNFTs = []; + const newNFTs = []; + allNftParsedLog.forEach((single) => { + const tokenIdFromLog = getTokenIdParam(single); + const existingNft = allNfts?.[selectedAddress]?.[chainId]?.find( + ({ address, tokenId }) => { + return ( + isEqualCaseInsensitive(address, single.contract) && + tokenId === tokenIdFromLog + ); + }, + ); + if (existingNft) { + knownNFTs.push(existingNft); + } else { + newNFTs.push({ + tokenId: tokenIdFromLog, + ...single, + }); + } + }); + // For known nfts only refresh ownership + const refreshOwnershipNFts = knownNFTs.map(async (singleNft) => { + return this.nftController.checkAndUpdateSingleNftOwnershipStatus( + singleNft, + false, + // TODO add networkClientId once it is available in the transactionMeta + // the chainId previously passed here didn't actually allow us to check for ownership on a non globally selected network + // because the check would use the provider for the globally selected network, not the chainId passed here. + { selectedAddress }, + ); + }); + await Promise.allSettled(refreshOwnershipNFts); + // For new nfts, add them to state + const addNftPromises = newNFTs.map(async (singleNft) => { + return this.nftController.addNft( + singleNft.contract, + singleNft.tokenId, + ); + }); + await Promise.allSettled(addNftPromises); + } } } diff --git a/shared/lib/transactions-controller-utils.js b/shared/lib/transactions-controller-utils.js index 425ff33b6f32..073ff922af67 100644 --- a/shared/lib/transactions-controller-utils.js +++ b/shared/lib/transactions-controller-utils.js @@ -9,6 +9,9 @@ export const TOKEN_TRANSFER_LOG_TOPIC_HASH = export const TRANSACTION_NO_CONTRACT_ERROR_KEY = 'transactionErrorNoContract'; +export const TRANSFER_SINFLE_LOG_TOPIC_HASH = + '0xc3d58168c5ae7397731d063d5bbf3d657854427343f4c083240f7aacaa2d0f62'; + export const TEN_SECONDS_IN_MILLISECONDS = 10_000; export function calcGasTotal(gasLimit = '0', gasPrice = '0') { diff --git a/test/e2e/tests/tokens/nft/erc721-interaction.spec.js b/test/e2e/tests/tokens/nft/erc721-interaction.spec.js index 37516fd716b8..a323077aa692 100644 --- a/test/e2e/tests/tokens/nft/erc721-interaction.spec.js +++ b/test/e2e/tests/tokens/nft/erc721-interaction.spec.js @@ -13,6 +13,73 @@ const FixtureBuilder = require('../../../fixture-builder'); describe('ERC721 NFTs testdapp interaction', function () { const smartContract = SMART_CONTRACTS.NFTS; + it('should add NFTs to state by parsing tx logs without having to click on watch NFT', async function () { + await withFixtures( + { + dapp: true, + fixtures: new FixtureBuilder() + .withPermissionControllerConnectedToTestDapp() + .build(), + ganacheOptions: defaultGanacheOptions, + smartContract, + title: this.test.fullTitle(), + }, + async ({ driver, _, contractRegistry }) => { + const contract = contractRegistry.getContractAddress(smartContract); + await unlockWallet(driver); + + // Open Dapp and wait for deployed contract + await openDapp(driver, contract); + await driver.findClickableElement('#deployButton'); + + // mint NFTs + await driver.fill('#mintAmountInput', '5'); + await driver.clickElement({ text: 'Mint', tag: 'button' }); + + // Notification + await driver.waitUntilXWindowHandles(3); + const windowHandles = await driver.getAllWindowHandles(); + const [extension] = windowHandles; + await driver.switchToWindowWithTitle( + WINDOW_TITLES.Dialog, + windowHandles, + ); + await driver.waitForSelector({ + css: '.confirm-page-container-summary__action__name', + text: 'Deposit', + }); + await driver.clickElement({ text: 'Confirm', tag: 'button' }); + await driver.waitUntilXWindowHandles(2); + await driver.switchToWindow(extension); + await driver.clickElement( + '[data-testid="account-overview__activity-tab"]', + ); + const transactionItem = await driver.waitForSelector({ + css: '[data-testid="activity-list-item-action"]', + text: 'Deposit', + }); + assert.equal(await transactionItem.isDisplayed(), true); + + // verify the mint transaction has finished + await driver.switchToWindowWithTitle('E2E Test Dapp', windowHandles); + const nftsMintStatus = await driver.findElement({ + css: '#nftsStatus', + text: 'Mint completed', + }); + assert.equal(await nftsMintStatus.isDisplayed(), true); + + await driver.switchToWindow(extension); + + await clickNestedButton(driver, 'NFTs'); + await driver.findElement({ text: 'TestDappNFTs (5)' }); + const nftsListItemsFirstCheck = await driver.findElements( + '.nft-item__container', + ); + assert.equal(nftsListItemsFirstCheck.length, 5); + }, + ); + }); + it('should prompt users to add their NFTs to their wallet (one by one) @no-mmi', async function () { await withFixtures( { @@ -97,14 +164,16 @@ describe('ERC721 NFTs testdapp interaction', function () { await driver.clickElement({ text: 'Add NFTs', tag: 'button' }); await driver.switchToWindow(extension); await clickNestedButton(driver, 'NFTs'); - await driver.findElement({ text: 'TestDappNFTs (3)' }); + // Changed this check from 3 to 6, because after mint all nfts has been added to state, + await driver.findElement({ text: 'TestDappNFTs (6)' }); const nftsListItemsFirstCheck = await driver.findElements( '.nft-item__container', ); - assert.equal(nftsListItemsFirstCheck.length, 3); + assert.equal(nftsListItemsFirstCheck.length, 6); await driver.switchToWindowWithTitle('E2E Test Dapp', windowHandles); await driver.fill('#watchNFTInput', '4'); + await driver.clickElement({ text: 'Watch NFT', tag: 'button' }); await driver.fill('#watchNFTInput', '5'); await driver.clickElement({ text: 'Watch NFT', tag: 'button' }); diff --git a/test/e2e/tests/tokens/nft/send-nft.spec.js b/test/e2e/tests/tokens/nft/send-nft.spec.js index 7df8febcab56..a9b89a2abb9b 100644 --- a/test/e2e/tests/tokens/nft/send-nft.spec.js +++ b/test/e2e/tests/tokens/nft/send-nft.spec.js @@ -145,8 +145,6 @@ describe('Send NFT', function () { // Go back to NFTs tab and check the imported NFT is shown as previously owned await driver.clickElement('[data-testid="account-overview__nfts-tab"]'); - await driver.clickElement('[data-testid="refresh-list-button"]'); - const previouslyOwnedNft = await driver.findElement({ css: 'h5', text: 'Previously Owned',