Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add IPFS gateway to remote image #3297

Merged
merged 9 commits into from
Oct 20, 2021
Merged

Conversation

wachunei
Copy link
Member

@wachunei wachunei commented Oct 13, 2021

Description

This PR uses current IPFS gateway selected in advanced settings to resolve IPFS scheme URIs

TODO:

  • Add support for tokens in token list, eg: FNX token
  • Add support for NFTs

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #3274 #2817 #2838

@wachunei wachunei force-pushed the feature/use-ipfs-gateway-for-images branch from 2971b33 to 9b0f73c Compare October 13, 2021 20:46
@kinggongzilla
Copy link

kinggongzilla commented Oct 14, 2021

thank you for the proposed fix! Looking forward to being accepted.
We are facing an issue where the token URI to the metadata is also a "ipfs://" URI. When adding the NFT to MetaMask wallet it does not show up at all. Not even a "gray square".
Does this fix also address "ipfs://" token URIs or only the "media" URI in "ipfs://" format?

@wachunei
Copy link
Member Author

Hello @kinggongzilla, can you provide an example of "ipfs:// token URI" please? This is currently fixing the resolution of the token icon URL.

@kinggongzilla
Copy link

kinggongzilla commented Oct 14, 2021

Hi @wachunei

Oh okay, I thought this was a fix for using IPFS protocol URIs in the tokenURI or for the "image" key in the metadata of an NFT.

This is the base URI we use in our smart contract:

function _baseURI() internal pure override returns (string memory) {
        return "ipfs://";
    }

It points to the JSON Metadata on IPFS which looks like this for example:
{ "name":"Example title ", "description":"example description", "image":"ipfs://bafybeifaouybr23dzhe77m4vvgztarlcjzwuuogm7lr4hf24rvaucfbrbi/nft.mp4", "artist":"example artist" }

The problem is that MetaMask mobile app does not display our NFTs at all, while they are correctly displayed on OpenSea: https://opensea.io/collection/beyond-mars

@wachunei
Copy link
Member Author

@kinggongzilla I see your point, yes, I'm aiming to add support to that case as well, thank you the example.

@kinggongzilla
Copy link

@wachunei thank you!

I removed the Provider and just mocked the selector function the component uses
@maximgeerinck
Copy link

Happy to help on this if needed, we just tackled this issue on our marketplace with extensive test cases

@wachunei
Copy link
Member Author

wachunei commented Oct 18, 2021

Hey @maximgeerinck, how can I reach out to you? Having some testnet NFTs with ipfs media types to test against would be very helpful.

@maximgeerinck
Copy link

@wachunei you can mint different types here: https://app-testnet.refinable.com/

  • Video (MOV, MPV4)
  • Image (PNG, JPG, GIF, ...)

let me know if you need something else. For the ipfs i used following:

  • first we validate the ipfs url (there are way too many broken incorrect token urls)
import isIpfs, { url } from "is-ipfs";
 const isValidUrl =
    isIpfs.ipfsUrl(url) ||
    isIpfs.ipnsUrl(url) ||
    isIpfs.path(url) ||
    isIpfs.cid(url.replace("ipfs://", "")) ||
    isIpfs.cidPath(url.replace("ipfs://", ""));
  if (!isValidUrl) {
    console.error(url);
    throw new Error("given ipfs url is not valid!");
  }

then we replace the hash by a public node, we did add fallback for multiple nodes as it can be that for very new NFTs, not all public nodes picked up the document yet

const IPFS_GATEWAY = "https://cloudflare-ipfs.com";
gateway = gateway.replace("/ipfs/", "");

const hasIpfsHash = url.match(
  /\/ipfs\/Qm[1-9A-HJ-NP-Za-km-z]{44}(\/.*)?|^\/ipns\/.+/gm
);

if (hasIpfsHash) {
  return `${gateway}${hasIpfsHash[0]}`;
}

return url.replace("ipfs://", `${gateway}/ipfs/`);

some don't even use ipfs, so we return when the url starts with http

Test case can look like:

it.each([
  [
    "ipfs://QmY8VYrfNV1kQ9YjGjA2YdYZbZS9a1AUBM69ABkvNafH4U",
    "https://cloudflare-ipfs.com/ipfs/QmY8VYrfNV1kQ9YjGjA2YdYZbZS9a1AUBM69ABkvNafH4U",
  ],
  [
    "https://gateway.pinata.cloud/ipfs/QmSb8FZBPJA3mxmYnMP5SGWiMwvFtYbNokTp8PU4453ax4/token/63.json",
    "https://gateway.pinata.cloud/ipfs/QmSb8FZBPJA3mxmYnMP5SGWiMwvFtYbNokTp8PU4453ax4/token/63.json",
  ],
  ["https://something-random", "https://something-random"],
  [
    "https://cloudflare-ipfs.com/ipfs/QmY8VYrfNV1kQ9YjGjA2YdYZbZS9a1AUBM69ABkvNafH4U",
    "https://cloudflare-ipfs.com/ipfs/QmY8VYrfNV1kQ9YjGjA2YdYZbZS9a1AUBM69ABkvNafH4U",
  ],
  [
    "ipfs://Qmeg18ZUcmcZG7Lne5bWF1kRPrSQ7LCToBRukYZcvoq8fw/nft.png",
    "https://cloudflare-ipfs.com/ipfs/Qmeg18ZUcmcZG7Lne5bWF1kRPrSQ7LCToBRukYZcvoq8fw/nft.png",
  ],
])(`%s is a valid ipfs url`, async (url, result) => {
  expect(resolvePublicIPFSUrl(url)).toEqual(result);
});

it.each([
  ["gewgwegwge"],
  ["ipfs://weweg/few/weg/gwe"],
  ["ipfs://00545411850545614840"],
])(`%s is not a valid ipfs url`, async (url) => {
  expect(() => resolvePublicIPFSUrl(url)).toThrowError();
});

^All the types above we've already encountered on public collections (also the invalid ones sadly)

@wachunei
Copy link
Member Author

This PR will focus on ERC720 tokens with IPFS logo image url. NFTs support will be tracked in this issue: #3310

@wachunei wachunei marked this pull request as ready for review October 19, 2021 20:36
@wachunei wachunei requested a review from a team as a code owner October 19, 2021 20:36
@wachunei wachunei added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Oct 19, 2021
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wachunei wachunei added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Oct 20, 2021
@wachunei
Copy link
Member Author

Hey @cortisiko, here are some tokens with ipfs:// images to check:

  • 0x9a48bd0ec040ea4f1d3147c025cd4076a2e71e3e
  • 0x158079ee67fce2f58472a96584a73c7ab9ac95c1
  • 0xe17f017475a709de58e976081eb916081ff4c9d5

Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This guy is 👍

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Oct 20, 2021
@wachunei wachunei merged commit 98e5b05 into develop Oct 20, 2021
@wachunei wachunei deleted the feature/use-ipfs-gateway-for-images branch October 20, 2021 18:33
@github-actions github-actions bot locked and limited conversation to collaborators Oct 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
QA Passed A successful QA run through has been done
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for IPFS protocol on Tokens
6 participants