fix(bridge-ui): fix ERC721 and ERC1155 detection in NFT bridge

[ricecodekhmer]

I wanted to try out the NFT bridging feature, so I created my own NFT, which is an ERC-721 contract on holesky, but I encountered an issue where the 'Scan for NFT' button wasn't working as expected. So, attempting to use the "Add Manually" option, I encountered an error message saying "You must be the owner of all tokens" when entering the address and token id for my NFT.

After setting up the bridge locally and looking at the code, I came to the implementation of the isERC721 function in packages/bridge-ui/src/libs/token/detectContractType.ts, and I realised that the readContract call would always throw an exception if the specified token id didn't match the owner's token id, regardless of whether the contract was ERC-721 or not.

To fix this, I have altered both isERC721 and isERC1155 functions to properly check things by using the standard identifiers (0xd9b67a26 for ERC-1155 and 0x80ac58cd for ERC-721) which adhere to the EIP specifications for the contracts.

Screenshots

Before

After

Testing

I made sure to test with ERC-20, ERC-1155 and ERC-721 to check if all contracts are identified properly.

[KorbinianK]

@ricecodekhmer Thanks for this enhancement, been meaning to change it but tbh. it worked, supportsInterface is just checking it on contract level and requires more than a single function, so certainly the correct approach.

But: I am curious how this changes your ownership check. If the contract type check reverts or fails, you get This is not a valid NFT address on the current network not the one in your screenshot.
That one should only appear if ownerOf(tokenID) fails. Can you send me your verified contract address so I can have a look?

Regardless of this, please fix the comment and adjust the unit tests if you want to have this merged :)

[ricecodekhmer]

Hi @KorbinianK ,

If the contract type check reverts or fails, you get This is not a valid NFT address on the current network not the one in your screenshot.
That one should only appear if ownerOf(tokenID) fails.

Yes, it's the call to ownerOf that fails because it's wrongly detecting my NFT as an ERC1155 instead of ERC721, if it detected my NFT as an ERC20 or other then it would return This is not a valid NFT address on the current network instead.

After my fix, the functions isERC721 and isERC1155 now are dependant on readContract returning true or false as well as throwing an exception, so I had to change the unit tests because of this.

With regards to the change affecting checking ownership, because the file checkOwnership.ts has the call to readContract with the ownerOf function, it's not an issue.

My NFT is 0x8afe0455f3063426929d3b973d2366a914c47344, it's an ERC721. I've tested also with NFTs I've found on etherscan like this ERC1155 0xab50971078225d365994dc1edcb9b7fd72bb4862 and this ERC20 0x73d9a99c4bd6e5588df7681a1cd0d60e9b8b331c

This screenshot is when I try to input the wrong tokenid:

This is when I enter an ERC20 address:

Thank you.

[gitpoap-bot]

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2024 Taiko Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.