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

Feature: Include Base64 library in utils #2859

Closed
ernestognw opened this issue Sep 14, 2021 · 14 comments · Fixed by #2884
Closed

Feature: Include Base64 library in utils #2859

ernestognw opened this issue Sep 14, 2021 · 14 comments · Fixed by #2884

Comments

@ernestognw
Copy link
Member

ernestognw commented Sep 14, 2021

🧐 Motivation
Recently, with the NFT rising and on-chain metadata protocols such as Uniswap and Loot (including all of its derivatives), Base64 encoder has become a widely used library, but there's no further documentation about it, and everybody seems to be copying the usage from the base Uniswap and Loot contracts:

Even though it seems to be working correctly, it might be needed to include in Open Zeppelin as one of the available libraries, so there is an official reference for it.

Currently, there is this Base64 outdated experiment, and this is the implementation that most of the projects are using up to this date

📝 Details

  • Include @Brechtpd implementation into contracts/utils folder
  • Add documentation and tests for them

Edit: Base64 data prepended with its media type is considered a valid URI. This library will enable NFTs on-chain tokenURIs

I can help if there's somebody who can guide me through the process

@ernestognw ernestognw changed the title Include Base64 library in utils Feature: Include Base64 library in utils Sep 14, 2021
@frangio
Copy link
Contributor

frangio commented Sep 15, 2021

I agree it looks like this is a useful library we should include, but there is one thing I don't understand. What is the goal of using Base64 encoding for this? I don't think it's technically necessary. Sure it will result in shorter output but at the expense of more computation. Is there a reason I'm not seeing?

Note: If we agree to include this, if we want to use @Brechtpd's implementation we need to ask him if he's interested in contributing the code to the library and having us maintain it. Otherwise we need to write our own implementation from scratch.

@ernestognw
Copy link
Member Author

ernestognw commented Sep 15, 2021

My best guess about its importance is that it's the only way to store JSON metadata on-chain and make it compatible with Platforms like Open Sea. Since ERC721 tokenURI function expects to return a URL pointing to the JSON metadata, using a Base64 encoded JSON prepended with data:application/json;base64, is a valid HTTP response for those platforms.

Actually, this is the reason why I ended up using this, and seems to be generally accepted for it (also, encoded base64 images are also read by Open Sea). Anyway, I think that lack of documentation about its usage and importance is exactly what's missing.

I can try to contact @Brechtpd if needed

Edit: I don't know how I missed this, but the base64 is not just a valid response for Open Sea and others. It is considered an actual URI, so Base64 library will enable native on-chain tokenURIs

@0xjjpa
Copy link

0xjjpa commented Sep 15, 2021

Edit: I don't know how I missed this, but the base64 is not just a valid response for Open Sea and others. It is considered an actual URI, so Base64 library will enable native on-chain tokenURIs

That's correct, by using base64 you can display images in any format for any browser, and not only OpenSea. E.g. you can copy and paste any image encoded in your browser tab (e.g. data:image/png;base64,iVBORw0KGgoAA...) and it will show and display the image.

@ernestognw
Copy link
Member Author

Edit: I don't know how I missed this, but the base64 is not just a valid response for Open Sea and others. It is considered an actual URI, so Base64 library will enable native on-chain tokenURIs

That's correct, by using base64 you can display images in any format for any browser, and not only OpenSea. E.g. you can copy and paste any image encoded in your browser tab (e.g. data:image/png;base64,iVBORw0KGgoAA...) and it will show and display the image.

Thanks for supporting @jjperezaguinaga, I think this is enough reason to include the library.

Also, @frangio, here's a confirmation from @brechtpd , is there anything else needed? I can push forward to get any other formal confirmation

@frangio
Copy link
Contributor

frangio commented Sep 15, 2021

But it is possible to build a data URI using ASCII/UTF8 encoding. I guess Base64 is needed when the image is a binary format like PNG, but if you're using the Base64 library you're likely producing an SVG on chain and in that case it is possible to just use ASCII and skip the conversion step.

@ernestognw Thanks for going ahead and asking. I actually would like to be sure he understands we mean shipping his code as a part of OpenZeppelin Contracts.

@ernestognw
Copy link
Member Author

ernestognw commented Sep 15, 2021

But it is possible to build a data URI using ASCII/UTF8 encoding. I guess Base64 is needed when the image is a binary format like PNG, but if you're using the Base64 library you're likely producing an SVG on chain and in that case it is possible to just use ASCII and skip the conversion step.

@ernestognw Thanks for going ahead and asking. I actually would like to be sure he understands we mean shipping his code as a part of OpenZeppelin Contracts.

Can you provide an example of a tokenURI using ASCII/UTF8 encoding? Mozilla docs states that base64 token if non-textual, which I understand applies for both SVG and PNG (even for JSON)

Data URLs are composed of four parts: a prefix (data:), a MIME type indicating the type of data, an optional base64 token if non-textual, and the data itself:

data:[<mediatype>][;base64],<data>

Maybe there's a misunderstanding here, the Base64 doesn't aim to encode the image (which is useful too), but to encode the JSON that the tokenURI URI is retrieving.

Captura de Pantalla 2021-09-15 a la(s) 16 46 13

From what I understand, there's no other way to build a URI that retrieves a JSON and is on-chain.

And sure! Let's determine first if this is needed or not, and if we proceed I can contact again with @bretchpd to formalize it :)

@frangio
Copy link
Contributor

frangio commented Sep 15, 2021

Yeah sorry it was wrong to talk about SVG vs PNG. The same argument applies though.

Here is an ERC721 that uses UTF8 encoding for its tokenURI: Etherscan, OpenSea.

Just to clarify I'm not saying it's wrong to use Base64 but I don't understand why it's necessary and would love to know if there is a reason to prefer it.

@ernestognw
Copy link
Member Author

Wonderful, I see.

This actually solves the problem I had when I ended up using Base64 encoding. I don't see any other benefit rather than size optimization purposes but I'm not too convinced about it.

If you don't find it useful, you can close, but I'll still look into the possibility of including it given its popularity. Even some docs telling you that you can use UTF8 encoding would be great for those who ran into the dilemma of providing this on-chain metadata URI

@frangio
Copy link
Contributor

frangio commented Sep 15, 2021

Let's see if someone else can offer an answer to why Base64 would be preferred!

If you find a place in the docs where we should mention this, a PR would be welcome! Or you can write something more standalone and share in the OpenZeppelin Forum.

@ernestognw
Copy link
Member Author

ernestognw commented Sep 15, 2021

Neat,

I'm planning to release a tutorial (on OZ Forum oc) on how to retrieve a base64 encoded tokenURI (as well as a UTF8), I'll add the link here once done.

Thanks @frangio

@Amxx
Copy link
Collaborator

Amxx commented Sep 16, 2021

Side comment: if We add base64, it could also be nice to add base58, which is is used by IPFS hashes

@0xjjpa
Copy link

0xjjpa commented Sep 22, 2021

Just to clarify I'm not saying it's wrong to use Base64 but I don't understand why it's necessary and would love to know if there is a reason to prefer it.

@frangio I believe they are encoding it to save space, as AFAIK there's a 65k limit when using base64. Here are some projects with their authors which could explain a bit better on why base64 encode the metadata rather than escape it like in the example you shared.

Those are just some I could find from the top of my head, and as you can see, all used a different library or format for encoding base64, helping the case of using a standard one.

@ernestognw let me know if you need help with the PR and docs.

@wilsoncusack
Copy link

@frangio base64 encoding is URL safe, is the reason. If you know exactly what data you are encoding, and it is all url safe with UTF8, you could use UTF8. But if you want to encode non-url-safe characters OR have any sort of user input encoded, then you want to use base64

@ernestognw
Copy link
Member Author

ernestognw commented Sep 23, 2021

Just to clarify I'm not saying it's wrong to use Base64 but I don't understand why it's necessary and would love to know if there is a reason to prefer it.

@frangio I believe they are encoding it to save space, as AFAIK there's a 65k limit when using base64. Here are some projects with their authors which could explain a bit better on why base64 encode the metadata rather than escape it like in the example you shared.

Those are just some I could find from the top of my head, and as you can see, all used a different library or format for encoding base64, helping the case of using a standard one.

@ernestognw let me know if you need help with the PR and docs.

I think this is enough to justify the inclusion of the library. I've been kinda busy but I can start opening a branch on a fork and try to document all of these things you're sharing.

@frangio, what's the procedure to make a PR with their corresponding docs?

I'll reach out to you on Twitter if I have any question

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

Successfully merging a pull request may close this issue.

5 participants