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

feat: provide cidv1 in base36 when needed #32

Merged
merged 1 commit into from
Sep 18, 2020
Merged

feat: provide cidv1 in base36 when needed #32

merged 1 commit into from
Sep 18, 2020

Conversation

lidel
Copy link
Member

@lidel lidel commented Aug 26, 2020

This adds Base36 version of CIDv1 if it fits inside a single DNS label.
Follows convention introduced in ipfs/kubo#7441 where we redirect to Base36 version if it helps with DNS limit.

2020-08-26--22-12-53

Examples:

Q: Is it ok to hide Base36 when it is not really needed? Or maybe should we always show it, to educate users that the same CID can be encoded in different bases? In that case we could have CIDV1 header and then multiple base versions listed under that.

cc @ribasushi @aschmahmann @jessicaschilling

This adds Base36 version  of CIDv1 if it fits inside of a single DNS
label. Follows convention introduced in:
ipfs/kubo#7441
@ribasushi
Copy link

maybe should we always show it, to educate users that the same CID can be encoded in different bases

👍 I like that idea

we could have CIDV1 header and then multiple base versions

I wouldn't list too many, just 32/36, anything else is kinda... esoteric

@lidel
Copy link
Member Author

lidel commented Sep 15, 2020

Folks, go-ipfs 0.7.0 will land soon (ipfs/kubo#7560) and we should merge this before that happens, to improve UX of working with ED25519 keys.

Can I have a quick temperature check about merging this as-is? (we can refine later)

@Gozala
Copy link

Gozala commented Sep 15, 2020

@lidel I like the educational idea of this. I think it would be great to visually illustrates what fits and what does not fit for subdomains. Maybe it would help to bring it into the context ? And actually render the link along with the CID e.g.

For: bafzaajaiaejcat4yhiwnr2qz73mtu6vrnj2krxlpfoa3wo2pllfi37quorgwh2jw

https://k51qzi5uqu5di608geewp3nqkg0bpujoasmka7ftkyxgcm3fh1aroup0gsdrna.ipfs.dweb.link/

For long CIDs that don't fit the subdomain requirements it would be great to render a broken link, maybe with fragment of URL highlighted red and comment explaining this is what tool X would interpret as.

@jessicaschilling
Copy link
Contributor

Agree with @Gozala and @ribasushi, but suggest these additions go into their own issue for sake of expediency.

Copy link

@jacobheun jacobheun left a comment

Choose a reason for hiding this comment

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

@lidel lidel merged commit 3bcef5e into master Sep 18, 2020
@lidel lidel deleted the feat/cidb36 branch September 18, 2020 14:01
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 this pull request may close these issues.

7 participants