fix: parse tx logs on contractInteraction to refresh NFT state (#25380)

## **Description**

On this PR, we aim to refresh NFT ownership status or add NFTs to state
if necessary by parsing transaction logs once the user submits a
transaction with MM.

MM controller already had the logic that calls `_updateNFTOwnership`
after creating the transaction notification.

That logic refreshed NFT ownerhsip when transaction type is
`transferfrom`.
We are adding the case when transaction type is a contract interaction
and then look for specific topics.



[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/25380?quickstart=1)

## **Related issues**

Fixes:

## **Manual testing steps**

1. Switch to Ethereum mainnet and go to NFTs tab (this will trigger a
call to fetch your NFTs)
2. Go to opensea to buy a new NFT.
3. Click on buy and submit transaction with your MM
4. Go back to NFTs tab and you should see your new NFT without having to
import it.

Im using Ethereum mainnet in the videos because we support NFT detection
on Ethereum mainnet.
But this should also work if you are submitting transaction on Sepolia
or any other test network.


## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### **Before**

<!-- [screenshots/recordings] -->


https://github.com/MetaMask/metamask-extension/assets/10994169/7a21efd4-c93c-4ec6-9a3d-3649b6b553df



### **After**

<!-- [screenshots/recordings] -->


https://github.com/MetaMask/metamask-extension/assets/10994169/b3cbaf49-f73b-4a09-96b8-b6156a5f3b98




## **Pre-merge author checklist**

- [ ] I've followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask
Extension Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.

app/scripts/metamask-controller.js

@@ -139,6 +139,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,
@@ -207,6 +209,10 @@ import {
 } from '../../shared/modules/selectors';
 import { createCaipStream } from '../../shared/modules/caip-stream';
 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)
@@ -6344,7 +6350,7 @@ export default class MetamaskController extends EventEmitter {
     }
 
     await this._createTransactionNotifcation(transactionMeta);
-    this._updateNFTOwnership(transactionMeta);
+    await this._updateNFTOwnership(transactionMeta);
     this._trackTransactionFailure(transactionMeta);
   }
 
@@ -6372,46 +6378,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);
+      }
     }
   }
 

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') {

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' });

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',