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

CIDv1 is not supported by Loopring Contracts #193

Open
sk33z3r opened this issue Mar 12, 2022 · 10 comments
Open

CIDv1 is not supported by Loopring Contracts #193

sk33z3r opened this issue Mar 12, 2022 · 10 comments

Comments

@sk33z3r
Copy link

sk33z3r commented Mar 12, 2022

Issue

Loopring's API does not support CIDv1 for metadata files

Example CIDv1:

bafkreicheapqqbwjm2j46gi5kprbkfa3d2ls7kvrcyt6jeh73pem6byk4u

User Impact

  1. If a user was to use, say, https://nft.storage to upload their metadata they would only receive a CIDv1. When they then go to mint on the Loopring UI, and paste their CIDv1, the UI uses a pinata gateway to look up the metadata, read, and verify the fields are all there and also display a preview for the user. Where it falls apart is that Pinata gateways handle CIDv1 just fine, so it appears as though the mint is all good. But in reality when that NFT is then read back from the API the returned CIDv0 is incorrect because the nftID was originally derived from a CIDv1.
  2. Users can't use the latest CID standard, or move NFTs from L1 to L2 that are minted with CIDv1.
  3. The issue being present pretty much locks users into using Pinata or IPFS desktop. Pinata just changed their policy and made their free accounts even more strict than before, so it's quickly becoming a bad option for users on a budget. NFT.storage however is free and also utilizes filecoin for replication, not to mention that their single file size upload limit is 31GB.
  4. Due to (2) and (3), CAR files are much less accessible to end users.

The Quick Fix (not ideal)

Block CIDv1 from being minted in the "Advanced Mint" screen. You can do this with a simple HTML pattern attribute on the input text box.

pattern="^Qm[a-zA-Z0-9]{44}$"

The Real Solution

Support CIDv1. In other words, convert the nftID hex to CIDv1 or CIDv0, based on which was used to mint an NFT.

It's possible this is just a contract change to lines 1221 - 1271

Click to Expand Code Section
// File: contracts/external/IPFS.sol

pragma solidity ^0.8.0;
pragma experimental ABIEncoderV2;


/// @title IPFS
/// @author Brecht Devos - <brecht@loopring.org>
library IPFS
{
    bytes constant ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz';

    // Encodes the 32 byte data as an IPFS v0 CID
    function encode(uint256 data)
        internal
        pure
        returns (string memory)
    {
        // We'll be always be encoding 34 bytes
        bytes memory out = new bytes(46);

        // Copy alphabet to memory
        bytes memory alphabet = ALPHABET;

        // We have to encode 0x1220data, which is 34 bytes and doesn't fit in a single uint256.
        // Keep the first 32 bytes in the uint, but do the encoding as if 0x1220 was part of the data value.
        // 0 = (0x12200000000000000000000000000000000000000000000000000000000000000000) % 58
        out[45] = alphabet[data % 58];
        data /= 58;
        // 4 = (0x12200000000000000000000000000000000000000000000000000000000000000000 / 58) % 58
        data += 4;
        out[44] = alphabet[data % 58];
        data /= 58;
        // 40 = (0x12200000000000000000000000000000000000000000000000000000000000000000 / 58 / 58) % 58
        data += 40;
        out[43] = alphabet[data % 58];
        data /= 58;

        // Add the top bytes now there is anough space in the uint256
        // This constant is 0x12200000000000000000000000000000000000000000000000000000000000000000 / 58 / 58 / 58
        data += 2753676319555676466672318311740497214108679778017611511045364661305900823779;

        // The rest is just simple base58 encoding
        for (uint i = 3; i < 46; i++) {
            out[45 - i] = alphabet[data % 58];
            data /= 58;
        }

        return string(out);
    }
}

Additional Info

Broken NFT Examples:

The issue is if I go to mint using this CID in the web wallet, I am not blocked from minting. In fact, this screen will read the metadata and display the image/name properly, leading me to believe that everything is OK. I and another user minted a few items last night this way.

image

However, after mint in both the web wallet and the explorer, these NFTs cannot be loaded.

screenshot_20220312_002532

The XHR request response is:

ipfs cat /ipfs/QmT8J4gNgfGvRYvn29NBBVdcSgUN2dA6hiGGLZ4J6EMuDE: protobuf: (PBNode) invalid wireType, expected 2, got 3
 - ERR_ID:00019

As you can see, Explorer has a CIDv0 that it is attempting to load, but it does not exist.

@sk33z3r
Copy link
Author

sk33z3r commented Apr 14, 2022

Additionally, the Loopring owned Pinata gateway supports CIDv1, so I would argue that Loopring should just support CIDv1 across the board since it appears to require no additional work, just using the proper gateways.

Example of a file uploaded through nft.storage using a CIDv1 that is functional through the Loopring Gateway:

https://loopring.mypinata.cloud/ipfs/bafybeibeqbwbvsbtduqx4u2tvvjuko57z2jkdogf6mt3z6jn7qg3cmjila

@sk33z3r sk33z3r changed the title Request: support CIDv1 in explorer.loopring.io or block CIDv1 from being used in the minting UI Request: support CIDv1 in explorer.loopring.io Apr 14, 2022
@sk33z3r sk33z3r changed the title Request: support CIDv1 in explorer.loopring.io Request: support CIDv1 in explorer.loopring.io and loopring.io Apr 14, 2022
@sk33z3r
Copy link
Author

sk33z3r commented Apr 15, 2022

I finally understand what is happening when the explorer and web wallets fail, and why the mint UI succeeds.

Using the Loopring Pinata gateway, it resolves CIDv1 just fine. This is why the minting form shows the images and reads metadata just fine initially.

However, the nftID that Loopring creates is based on the CID. So it appears that loopring generates a hex encoded version of the CIDv1 input. Then, later when the UI is simply trying to read get the IPFS data, it is trying to convert the nftID into CIDv0 and fails, because the NFT does not have an associated CIDv0.

The only way to keep people from uploading DOA NFTs is to add an HTML form pattern to the minting UI. The regex is super simple, and the below attribute just needs to be added to the <input type="text" /> tag:

pattern="^Qm[a-zA-Z0-9]{44}$"

This simple, simple addition would save someone from minting broken NFTs.

The real solution is to fix how the Layer 2 API is generating and converting the nftID, which is a bit more complicated.

@sk33z3r
Copy link
Author

sk33z3r commented Jun 20, 2022

Adding some additional food-for-thought I proposed in LRC discord:

The end-user impact as I see it is quad-fold:

  1. If a user was to use, say, https://nft.storage to upload their metadata they would only receive a CIDv1. When they then go to mint on the Loopring UI, and paste their CIDv1, the UI uses a pinata gateway to look up the metadata, read, and verify the fields are all there and also display a preview for the user. Where it falls apart is that Pinata gateways handle CIDv1 just fine, so it appears as though the mint is all good. But in reality when that NFT is then read back from the API the returned CIDv0 is incorrect because the nftID was originally derived from a CIDv1.
  2. Users can't use the latest CID standard, or move NFTs from L1 to L2 that are minted with CIDv1.
  3. The issue being present pretty much locks users into using Pinata or IPFS desktop. Pinata just changed their policy and made their free accounts even more strict than before, so it's quickly becoming a bad option for users on a budget. NFT.storage however is free and also utilizes filecoin for replication, not to mention that their single file size upload limit is 31GB.
  4. Due to (2) and (3), CAR files are much less accessible to end users.

@sk33z3r sk33z3r changed the title Request: support CIDv1 in explorer.loopring.io and loopring.io CIDv1 is not supported by Loopring API Jun 20, 2022
@sk33z3r sk33z3r changed the title CIDv1 is not supported by Loopring API CIDv1 is not supported by Loopring Contracts Jun 21, 2022
@sk33z3r
Copy link
Author

sk33z3r commented Jun 21, 2022

A user ran into this today, please see the Discord conversations:

https://discord.com/channels/488848270525857792/948112932900851732/988868432139280434

I think that the Quick Solution should be implemented ASAP to prevent any one else from the same mistake until the contract supports CIDv1.

@Brechtpd
Copy link

The problem with CIDv1 is that it stores more than 32 bytes of data, with 32 bytes being the size of the NFT token ID in NFT contracts. For counterfactual NFTs we depend on the token ID to store all the necessary data to link to the NFT so we don't have to store any additional data elsewhere (which we would have to support either on the protocol level or store elsewhere which would be expensive). There may be a solution if the extra data for CIDv1 would be the same for all NFTs, but even that would complicate things quite a bit while CIDv0 are much more straightforward.

Users can't use the latest CID standard, or move NFTs from L1 to L2 that are minted with CIDv1.

I don't think this is an issue. All NFTs, not matter how they work internally, are identified by their NFT token ID. Their NFT contract will have to do some extra work to translate to the IPFS URL but Loopring L2 doesn't need to care about that. The difficulty with CIDv1 for us it for the case with counterfactual NFTs where we want costs to be as low as possible for minting directly on L2.

@sk33z3r
Copy link
Author

sk33z3r commented Jun 24, 2022

Thanks @Brechtpd

Although it sounds like LRC may never support CIDv1 with this description.

How do you propose people to use a service like nft.storage (they do not return a CIDv0 anywhere to the user, and as far as I know it can't just be converted)? Or is LRC going to force users into using CIDv0 always, in perpetuity, and as such must use an IPFS service that is compatible?

@sk33z3r
Copy link
Author

sk33z3r commented Jun 24, 2022

@adam-bro if CIDv1 is not going to be supported I would suggest two things ASAP:

  1. Add the pattern to the Advance Mint screen to prevent people from minting CIDv1 (see the "Quick Fix" in the main ticket)
  2. There needs to be clear wording about this in LRC documentation for new people coming over from L1. They will want to know about this when it comes to designing or planning any new collections.

@Brechtpd
Copy link

How do you propose people to use a service like nft.storage (they do not return a CIDv0 anywhere to the user, and as far as I know it can't just be converted)? Or is LRC going to force users into using CIDv0 always, in perpetuity, and as such must use an IPFS service that is compatible?

Looking into this a bit more, it is possible to convert a CIDv1 to a CIDv0 if the encoding format is compatible: https://stackoverflow.com/questions/44930892/cids-with-dag-cbor-formats-as-v0-addresses/45425982#45425982

So as long as this holds we could support it. For nft.storage I found this however: nftstorage/nft.storage#387. Don't know if that's still up to date, but it seems like only files larger than 1MB are currently compatible is a problem (would be a bit crazy to have to make the metadata file that large just to make it compatible).

@sk33z3r
Copy link
Author

sk33z3r commented Jun 24, 2022

Thanks again for the info @Brechtpd. There are definitely some limitations I'm not fully aware of yet with some of this, so just trying to find avenues to both understand and come up with some sort of community solution/workaround to using a cost-free service like nft.storage.

I think I found something with nft.storage APIs that will allow me to create CAR files where the content of the archive are all compatible with CIDv0 and the returned CAR CIDv1 is also CIDv0 convertible:
nftstorage/nft.storage#991

Not necessarily a simple process for most end-users, but something that can be automated in app form. I'll take this road for community stuffs.

As far as this ticket goes, I suppose just getting some documentation up and the quick fix would be the items needed to close at this point.

@Brechtpd
Copy link

I think I found something with nft.storage APIs that will allow me to create CAR files where the content of the archive are all compatible with CIDv0 and the returned CAR CIDv1 is also CIDv0 convertible:
nftstorage/nft.storage#991

Ah this seems promising!

Ideally the UI would indeed only accept CIDv0 or compatible CIDv1 (and would transform the CIDv1 into a CIDv0 for the user automatically).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants