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

Support CIDv0 #571

Closed
NoRulesJustFeels opened this issue Oct 9, 2021 · 10 comments
Closed

Support CIDv0 #571

NoRulesJustFeels opened this issue Oct 9, 2021 · 10 comments
Assignees
Labels
effort/quickwin kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up

Comments

@NoRulesJustFeels
Copy link

I'm trying to use Nft.Storage for uploading the metadata for the Hic Et Nunc NFTs. It seems that the HEN v1 minting smart contract is checking that the metadata URI is exactly 53 chars long which means it only supports CIDv0.

It's not clear if Nft.Storage supports CIDv0; I don't see anything in the docs for the Nft.Storage JS client library. By default, I'm always getting CIDv1 with Nft.Storage.

@NoRulesJustFeels NoRulesJustFeels added kind/enhancement A net-new feature or improvement to an existing feature need/triage Needs initial labeling and prioritization labels Oct 9, 2021
@alanshaw
Copy link
Contributor

In general we want folks to move to v1 CIDs so we currently don't offer this as an option in our file upload API. If there's a need for a shorter string then v1 CIDs can be re-encoded using a different multibase. e.g.

$ ipfs cid format bafkreiem4twkqzsq2aj4shbycd4yvoj2cx72vezicletlhi7dijjciqpui -b base58btc
zb2rhg8GjTeiGgWe6HVCgps3wMMV8CX6BrBZxebQKZwA4wEzd

There's also a JS library that can help with this: https://www.npmjs.com/package/cid-tool

Would it be possible to PR the HEN minting contract?

@NoRulesJustFeels
Copy link
Author

Unfortunately, the contact is hardcoded to only accept an IPFS URI of 53 chars. HEN would have to deploy a new contract to change this requirement. Infura and Pinata both support CIDv0 so it looks like HEN would have to keep using either of those for storing their metadata.

@NoRulesJustFeels
Copy link
Author

Also, I tried the cids library but it seems the NFT.Storage CIDs can't be converted to CIDv0 since I get this error when calling cid.toV0():
Cannot convert a non dag-pb CID to CIDv0

@dchoi27 dchoi27 added P1 High: Likely tackled by core team if no one steps up and removed need/triage Needs initial labeling and prioritization labels Oct 11, 2021
@Gozala
Copy link
Contributor

Gozala commented Oct 12, 2021

Also, I tried the cids library but it seems the NFT.Storage CIDs can't be converted to CIDv0 since I get this error when calling cid.toV0():
Cannot convert a non dag-pb CID to CIDv0

@NoRulesJustFeels that depends on the CID, I'm guessing you are using client.store API which would produce dag-cbor node as opposed to dag-pb which can not be converted to v0 because v0 is implicitly dag-pb and does not supports specifying any other encoding.

We had relevant issue #387 with some more details.

Unfortunately, the contact is hardcoded to only accept an IPFS URI of 53 chars

Given that base58btc encoding is 49 chars would not that be enough ? Or does 53 chars expect ipfs:// prefix as well ? If later I think that HEN probably needs to address another problem with ipfs://Qm...hash not been a valid IPFS URL, because host part of the URL is case insensitive which means lot of programs (like browsers and such) would interpret that as qm...hash.

@NoRulesJustFeels
Copy link
Author

53 chars includes the ipfs:// prefix.
This is more about the HEN smart contract that checks for this exact length before storing it in its storage.
Retrieval of the IPFS URI by various clients from that storage can handle the various CID formats.

@Gozala
Copy link
Contributor

Gozala commented Oct 12, 2021

53 chars includes the ipfs:// prefix.
This is more about the HEN smart contract that checks for this exact length before storing it in its storage.
Retrieval of the IPFS URI by various clients from that storage can handle the various CID formats.

I do get it but I presume that ERC721 tokenURI would come out as ipfs://Qm...hash which is invalid and will likely cause various issues. And probably should be addressed.

You could workaround the char limit problem by:

  1. Taking cid of the metadata itself which is what url in const { url } = cliest.store({ ... }) would point to. At the moment that would be represented as ipfs://bafy...hash/metadata.json however and we do no right now have a simple way to resolve CID other using ipfs to do it. Exposing a cid for the metadata is probably something we could do though.
  2. Take metadata cid and transcode it to CID v0.

Unfortunately that would not fully address the problem however because if metadata is <1 mb in size it would end up encoded as a raw node as opposed to dag-pb and there for it won't be convertible to CIDv0. You could work around that as well by patting metadata with spaces to force chunking and making it dag-pb.

You could alternatively use storeDirectory API which would take list of files and give you back the CID for the directory containing all of them. Given that it is a directory it will be dag-pb and that could be converted to CIDv0. However you probably still want tokenURI to point to a JSON as opposed to directory containing it, in which case you're back to possibility that metadata.json will end up as raw node and inability to convert it to CIDv0. 😭

I think best solution would be to get HEN smart contract address the char amount limitation and invaild URLs which they seem to be putting on chain.

In the meantime it's probably best to fork the client to retrofit it's client.store API so that:

  1. Metadata is padded when necessary to force it to be dag-pb which would be here
    https://github.com/ipfs-shipyard/nft.storage/blob/b2345563c27cdf7052e2d62ca0865f397d96014e/packages/client/src/token.js#L146

  2. Make a change on our backend to include cid for the metadata, which is
    https://github.com/ipfs-shipyard/nft.storage/blob/main/packages/api/src/routes/nfts-store.js#L56
    in our response e.g. metadataCID field here
    https://github.com/ipfs-shipyard/nft.storage/blob/b2345563c27cdf7052e2d62ca0865f397d96014e/packages/api/src/routes/nfts-store.js#L90-L99

@NoRulesJustFeels
Copy link
Author

Thanks for the detailed explanation.

I don't believe HEN is following ERC-1155 (especially since this is the Tezos blockchain), so it wouldn't call client.store({}), but rather client.store(blob) or client.storeDirectory(form)
The mapping from NFT to metadata is kept in this contract's storage: https://tzkt.io/KT1RJ6PbjHpwc3M5rw5s2Nbmefwbuwbdxton/storage/514

Here is a typical metadata URI: ipfs://QmWuyrBArCUvzCQjcSctbyajMaNvmg5J2pxTLkFSVeTqU1
The indexers simply extract that URI and use any IPFS gateway to download the metadata file, so I'm not sure why it would be considered invalid.

I'm not a HEN developer so I don't know what the implications are for having to change the minting contract.
I'm working on a minter for HEN and wanted to use NFT.Storage as one of the IPFS APIs: https://github.com/NoRulesJustFeels/hicetnunc-minter-project

@dchoi27
Copy link
Contributor

dchoi27 commented Oct 18, 2021

@Gozala will link to this issue in a new issue in HEN's repos!

@dchoi27
Copy link
Contributor

dchoi27 commented Jan 4, 2022

With the update to ipfs-car supporting raw leaves, can we close this @alanshaw ?

@dchoi27
Copy link
Contributor

dchoi27 commented Jan 4, 2022

#991 will add to the docs how to use CIDv0. Closing this issue, let us know if any questions!

@dchoi27 dchoi27 closed this as completed Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/quickwin kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up
Projects
None yet
Development

No branches or pull requests

4 participants