MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
12.05k stars 4.92k forks source link

[Bug]: Not import proxy token contract #20522

Closed ruymaster closed 9 months ago

ruymaster commented 1 year ago

Describe the bug

Some contracts implements ERC20 and ERC1155 both with EIP2535 diamond protocol. In that case, we want to import the ERC20 token. https://polygonscan.com/token/0x127e47aba094a9a87d084a3a93732909ff031419

Steps to reproduce

Error messages or log output

This token is an NFT. Add on the Import NFT page

Version

10.34.3

Build type

None

Browser

Chrome

Operating system

Windows, MacOS

Hardware wallet

No response

Additional context

No response

Super-Genius commented 1 year ago

Just a note on this, the code shouldn't be checking for only 1 contract type when adding tokens. It should check for the type of token being added, i.e. ERC20, NFT and then check Interfaces available

      function checkInterfaceType(interfaceType: string) : bool {
        switch (interfaceType) {
          case "ERC1155":
            const IERC1155Interface = IERC1155__factory.createInterface();
            // interface ID does not include base contract(s) functions.
            const IERC1155InterfaceID = getInterfaceID(IERC1155Interface);
            if !(await contract.supportsInterface(IERC1155Interface._hex))
            { 
              console.log("Doesn't support IERC1155Interface");
              // in case interface isn't there, you could double check GetNFTInfo()
              try {
                GetERC1155NFTInfo();
              } catch()
               return false;
             }
         }
         return true;
         case "ERC20":
          const IERC20Interface = IERC20__factory.createInterface();
            // interface ID does not include base contract(s) functions.
            const IERC20InterfaceID = getInterfaceID(IERC20Interface);
            if !(await contract.supportsInterface(IERC20Interface._hex))
            { 
              console.log("Doesn't support IERC20Interface");
              // in case interface isn't there, you could double check GetTokenInfo()
              try {
                GetTokenInfo();
              } catch()
               return false;
             }
             return true;
          case "ERC721":
            const IERC721Interface = IERC721__factory.createInterface();
            // interface ID does not include base contract(s) functions.
            const IERC721InterfaceID = getInterfaceID(IERC721Interface);
            if !(await contract.supportsInterface(IERC721Interface._hex))
            { 
              console.log("Doesn't support IERC721Interface");
              // in case interface isn't there, you could double check GetNFTInfo()
              try {
                GetERC721NFTInfo();
              } catch()
               return false;
             }
           return true;
         }       
    }

Also, using the GetERC1155NFTInfo() is not a good practice as with ERC1155 there can be multiple ID's for the TokenID.

anaamolnar commented 1 year ago

Hello, @ruymaster. Thanks for reporting! I will ask the team to take a look.

Super-Genius commented 1 year ago

There could be a quick fix to change the error to just a warning and let the user add the ERC-20 token anyway!!

github-actions[bot] commented 11 months ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days if there is no further activity. The MetaMask team intends on reviewing this issue before close, and removing the stale label if it is still a bug. We welcome new comments on this issue. We do not intend on closing issues if they report bugs that are still reproducible. Thank you for your contributions.

github-actions[bot] commented 9 months ago

This issue was closed because there has been no follow up activity in the last 45 days. If you feel this was closed in error, please reopen and provide evidence on the latest release of the extension. Thank you for your contributions.

Super-Genius commented 9 months ago

There is a simple solution for this, just a pop-up dialog box warning to let the user add the token to ERC-20 anyway, instead of just blocking them from adding it.