From 2103744aea42bb637e882114816fa39341346a94 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Tue, 18 Jun 2024 13:56:40 +0200 Subject: [PATCH 1/8] fix: parse tx logs on contractInteraction to refresh NFT state --- app/scripts/metamask-controller.js | 178 ++++++++++++++++---- shared/lib/transactions-controller-utils.js | 3 + 2 files changed, 150 insertions(+), 31 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4d5145e876f6..9289ee83d18e 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -149,6 +149,8 @@ import { } from '@metamask/snaps-utils'; ///: END:ONLY_INCLUDE_IF +import { Interface } from '@ethersproject/abi'; +import { abiERC1155, abiERC721 } from '@metamask/metamask-eth-abis'; import { methodsRequiringNetworkSwitch, methodsWithConfirmation, @@ -218,6 +220,10 @@ import { getSmartTransactionsOptInStatus, getCurrentChainSupportsSmartTransactions, } from '../../shared/modules/selectors'; +import { + TOKEN_TRANSFER_LOG_TOPIC_HASH, + TRANSFER_SINFLE_LOG_TOPIC_HASH, +} from '../../shared/lib/transactions-controller-utils'; import { ///: BEGIN:ONLY_INCLUDE_IF(build-mmi) handleMMITransactionUpdate, @@ -6180,7 +6186,7 @@ export default class MetamaskController extends EventEmitter { } await this._createTransactionNotifcation(transactionMeta); - this._updateNFTOwnership(transactionMeta); + await this._updateNFTOwnership(transactionMeta); this._trackTransactionFailure(transactionMeta); } @@ -6208,47 +6214,157 @@ 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) && // TODO add check for tokenMethodSafeTransferFrom + txParams !== undefined; + + if (!isContractInteractionTx && !isTransferFromTx) { return; } - const { data, to: contractAddress, from: userAddress } = txParams; - const transactionData = parseStandardTokenTransactionData(data); + 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, + ); - // 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 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)); + } - const { allNfts } = this.nftController.state; + if (isERC721NftTransfer) { + isTransferToSelectedAddress = + txReceiptLog.topics && + txReceiptLog.topics[2] && + txReceiptLog.topics[2].match(selectedAddress?.slice(2)); + } - // check if its a known NFT - const knownNft = allNfts?.[userAddress]?.[chainId]?.find( - ({ address, tokenId }) => - isEqualCaseInsensitive(address, contractAddress) && - tokenId === transactionDataTokenId, - ); + 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); + } + const parsedLog = iface.parseLog({ + data: singleLog.data, + topics: singleLog.topics, + }); + allNftParsedLog.push({ + contract: singleLog.address, + ...parsedLog, + }); + } + }); + // 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 + console.log('here are known NFTS', knownNFTs); + console.log('here are new NFTS', newNFTs); - // 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 }, - ); + const refreshOwnershipNFts = knownNFTs.map(async (singleNft) => { + return await 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 await 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') { From 8e3c9fa4b0912291ff368a7679d84e56f9609ce7 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 19 Jun 2024 11:33:27 +0200 Subject: [PATCH 2/8] fix: fix --- app/scripts/metamask-controller.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 9289ee83d18e..3472e28c0d23 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -6237,6 +6237,10 @@ export default class MetamaskController extends EventEmitter { } if (isTransferFromTx) { + console.log( + '🚀 ~ MetamaskController ~ _updateNFTOwnership ~ isTransferFromTx:', + 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: @@ -6346,7 +6350,7 @@ export default class MetamaskController extends EventEmitter { console.log('here are new NFTS', newNFTs); const refreshOwnershipNFts = knownNFTs.map(async (singleNft) => { - return await this.nftController.checkAndUpdateSingleNftOwnershipStatus( + return this.nftController.checkAndUpdateSingleNftOwnershipStatus( singleNft, false, // TODO add networkClientId once it is available in the transactionMeta @@ -6355,15 +6359,15 @@ export default class MetamaskController extends EventEmitter { { selectedAddress }, ); }); - // await Promise.allSettled(refreshOwnershipNFts); + await Promise.allSettled(refreshOwnershipNFts); // For new nfts, add them to state const addNftPromises = newNFTs.map(async (singleNft) => { - return await this.nftController.addNft( + return this.nftController.addNft( singleNft.contract, singleNft.tokenId, ); }); - // await Promise.allSettled(addNftPromises) + await Promise.allSettled(addNftPromises); } } } From fd77f6a449305969da3ab71b2c1c8e2e954aa209 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 19 Jun 2024 11:52:49 +0200 Subject: [PATCH 3/8] fix: fix --- app/scripts/metamask-controller.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 3472e28c0d23..bd24ba52f8f8 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -6237,10 +6237,6 @@ export default class MetamaskController extends EventEmitter { } if (isTransferFromTx) { - console.log( - '🚀 ~ MetamaskController ~ _updateNFTOwnership ~ isTransferFromTx:', - 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: @@ -6346,9 +6342,6 @@ export default class MetamaskController extends EventEmitter { } }); // For known nfts only refresh ownership - console.log('here are known NFTS', knownNFTs); - console.log('here are new NFTS', newNFTs); - const refreshOwnershipNFts = knownNFTs.map(async (singleNft) => { return this.nftController.checkAndUpdateSingleNftOwnershipStatus( singleNft, From 6098a7380981e8be093fb9905d6ead51f1ed351c Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 19 Jun 2024 16:07:57 +0200 Subject: [PATCH 4/8] fix: fix e2e --- .../tokens/nft/erc721-interaction.spec.js | 73 ++++++++++++++++++- 1 file changed, 71 insertions(+), 2 deletions(-) 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' }); From 163c1331df84bc32cc0d99ec2be6b2e6af1daf19 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 19 Jun 2024 21:47:24 +0200 Subject: [PATCH 5/8] fix: update e2e --- test/e2e/tests/tokens/nft/send-nft.spec.js | 2 -- 1 file changed, 2 deletions(-) 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', From bd812d7f292d4e54c81d2f45910ce4e00c4e8843 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Mon, 24 Jun 2024 14:28:31 +0200 Subject: [PATCH 6/8] fix: fix import --- app/scripts/metamask-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 9fa397c8a561..4dd870fb1d48 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -224,11 +224,11 @@ import { getCurrentChainSupportsSmartTransactions, } from '../../shared/modules/selectors'; import { BaseUrl } from '../../shared/constants/urls'; -import { BalancesController as MultichainBalancesController } from './lib/accounts/BalancesController'; 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) handleMMITransactionUpdate, From 3c1c3b6a4f4a9ad6a646a5413b0c2eb900481b19 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Wed, 26 Jun 2024 14:56:42 +0200 Subject: [PATCH 7/8] fix: add check for ERC721 topic hash length --- app/scripts/metamask-controller.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4dd870fb1d48..cd7f8e30984f 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -6338,7 +6338,7 @@ export default class MetamaskController extends EventEmitter { type === TransactionType.contractInteraction && txReceiptLogs; const isTransferFromTx = (type === TransactionType.tokenMethodTransferFrom || - type === TransactionType.tokenMethodSafeTransferFrom) && // TODO add check for tokenMethodSafeTransferFrom + type === TransactionType.tokenMethodSafeTransferFrom) && txParams !== undefined; if (!isContractInteractionTx && !isTransferFromTx) { @@ -6381,6 +6381,7 @@ export default class MetamaskController extends EventEmitter { txReceiptLog.topics[0] === TRANSFER_SINFLE_LOG_TOPIC_HASH; const isERC721NftTransfer = txReceiptLog.topics && + txReceiptLog.topics.length === 2 && // Added this check because TOKEN_TRANSFER_LOG_TOPIC_HASH is the same for ERC20 and ERC721 txReceiptLog.topics[0] === TOKEN_TRANSFER_LOG_TOPIC_HASH; let isTransferToSelectedAddress; From 88f138866d2fd1b70215ca186e1cc0df9300bd61 Mon Sep 17 00:00:00 2001 From: sahar-fehri Date: Thu, 27 Jun 2024 11:19:02 +0200 Subject: [PATCH 8/8] fix: catch err on parse abi --- app/scripts/metamask-controller.js | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index cd7f8e30984f..42815021f0ca 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -6381,7 +6381,6 @@ export default class MetamaskController extends EventEmitter { txReceiptLog.topics[0] === TRANSFER_SINFLE_LOG_TOPIC_HASH; const isERC721NftTransfer = txReceiptLog.topics && - txReceiptLog.topics.length === 2 && // Added this check because TOKEN_TRANSFER_LOG_TOPIC_HASH is the same for ERC20 and ERC721 txReceiptLog.topics[0] === TOKEN_TRANSFER_LOG_TOPIC_HASH; let isTransferToSelectedAddress; @@ -6419,14 +6418,18 @@ export default class MetamaskController extends EventEmitter { } else { iface = new Interface(abiERC721); } - const parsedLog = iface.parseLog({ - data: singleLog.data, - topics: singleLog.topics, - }); - allNftParsedLog.push({ - contract: singleLog.address, - ...parsedLog, - }); + 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