search

pancakeswap / pancake-frontend

:pancakes: Pancake main features (farms, pools, IFO, lottery, profiles)
https://pancakeswap.finance
GNU General Public License v3.0
2.67k stars 3.53k forks source link

[BUG] Non-ERC721Enumerable collections break NFT profile page #3028

Closed fab153crypto closed 2 years ago

fab153crypto commented 2 years ago

Is there an existing issue for this?

Product

Other

Current Behavior

Users owning NFTs of collections listed on pancake swap, which do not implement ERC721Enumerable face problems with missing NFTs on the profile page. This does also include missing NFTs from other, ERC721Enumerable collections because the code which fetches the NFTs fails.

Expected Behavior

Worst case only the non-ERC721Enumerable should be missing. Best case there are other ways to query all NFTs of a user for non-ERC721Enumerable collections.

Steps To Reproduce

Visit any profile page of users holding a non-ERC721Enumerable (i.e. CatBread 0xc448498ddc536ad6f5d437325725dcf504d2d964)

https://pancakeswap.finance/nfts/profile/0x4688a4d85e274527de4c24bb6cdcff6575c11427

Environment

Any

Anything else?

https://github.com/pancakeswap/pancake-frontend/blob/4a00fb193b102641cddce81126dc735f6b068718/src/state/nftMarket/helpers.ts#L711 this function does assume ERC721Enumerable collections.

The error is propagated because an array of null is returned when the issue arrises, which makes following execution fail.

memoyil commented 2 years ago

There are ways to do this

  1. Using IndexedDB to create off chain data for that contract Currently there is no such that pcs is using right now as far as i am aware of
  2. Using query filters on on chain data Nodes that pcs is using currently allowing up to 5000 blocks at once to query, so in order to fetch all transfer data of that account on that contract, there would have to be done so many queries which is practically possible but would cause a lot of traffic thinking about the size of pcs users.

So Imho this is not a bug of PCS. It can be feature request in either here or that collection project.

fab153crypto commented 2 years ago

I agree that

A. not showing non-ERC721Enumerable collections in profile is not a bug, but showing then is a feature request.

however

B. This resulting in a chain of exceptions, which leads to other ERC721Enumerable collection NFT not showing is a bug. Pancakeswap has control over which collections are listed. When non-ERC721Enumerable collections are allowed to be listed on the marketplace, the frontend has to handle them.

A possible way to fix this is to filter null elements in tokenIds in https://github.com/pancakeswap/pancake-frontend/blob/4a00fb193b102641cddce81126dc735f6b068718/src/state/nftMarket/helpers.ts#L754

memoyil commented 2 years ago

@fab153crypto https://github.com/pancakeswap/pancake-frontend/pull/3036

fab153crypto commented 2 years ago

https://deploy-preview-3036--pancakeswap-dev.netlify.app/nfts/profile/0x4688a4d85e274527de4c24bb6cdcff6575c11427 Looks good, thx for the fast fix @memoyil :)