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

Ipfs base32 cid v1 #46

Closed
wants to merge 3 commits into from
Closed

Ipfs base32 cid v1 #46

wants to merge 3 commits into from

Conversation

pldespaigne
Copy link
Contributor

@pldespaigne pldespaigne commented Nov 27, 2019

Hello guys 👋 @makoto @Arachnid @jefflau

I just updated the decodeContenthash(encoded) function to convert ipfs hash to cid v1 base32.

Explenation

Recently the folks from IPFS have updated their libs & gateways to support cid v1 base 32.

That means that now we can resolve ipfs content with <hash>.gateway.com instead of gateway.com/ipfs/<hash>.

This is really more secure since ipfs served content doesn't share the same origin (cookies, localStorage, etc...) anymore.

The trick is that we need to use base32 cid v1 hash instead of regular cid v0 btc58 ipfs hash, because domains cannot contains capital letters.

So I updated my content-hash lib with a small helper function to convert cid v0 hash to cid v1 so that they can be resolved to a secure gateway. Then I have used this helpers function in this PR.

As always thanks a lot for your time and your work!

PS : I have also open a PR on the ens-app, but it depends on this PR to work correctly.

@pldespaigne
Copy link
Contributor Author

ping @makoto @jefflau

Copy link

@lidel lidel left a comment

Choose a reason for hiding this comment

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

@brantlymillegan
Copy link

@parkan

@parkan
Copy link

parkan commented Aug 20, 2020

this actually looks good to merge (minus the yarn.lock conflict), is something blocking it?

@makoto can we merge this + ensdomains/ens-app#538 ?

@makoto makoto self-assigned this Sep 9, 2020
@makoto
Copy link
Member

makoto commented Sep 14, 2020

@lidel @parkan

I tried this PR. It seems encoding IPNS ipns://bafybeico3uuyj3vphxpvbowchdwjlrlrh62awxscrnii7w7flu5z6fk77y with no error but failing to encode IPFS ipfs://bafybeico3uuyj3vphxpvbowchdwjlrlrh62awxscrnii7w7flu5z6fk77y at contentHash.encode('ipfs-ns', content) with error message Error: Non-base58 character.

I am suspecting that it is because the encoding method use Base 58 but that's for v0 and it is something else according to the IPFS content addressing doc.

Does it mean we need to add v1 specific encoding method at https://github.com/pldespaigne/content-hash ?

@lidel
Copy link

lidel commented Sep 15, 2020

@makoto for the specific snippet you pointed at, the fix is most likely something like this:

   ipfs: (value) => {
-    const multihash = multiH.fromB58String(value);
-    return new CID(1, 'dag-pb', multihash).buffer;
+    return new CID(value).toV1().buffer;
   },

As a rule of thumb, there should be just a generic code dealing with CID, parser should not care about the details of CID itself (v0, v1, codec, base, etc).

Note that this PR is over year old and:

@makoto @pldespaigne Let me know how I you need help with any of the above.

@makoto
Copy link
Member

makoto commented Sep 15, 2020

@lidel thanks for the explanation.

return new CID(value).toV1().buffer;

Is it always the case regardless of encoding v0 or v1?

should not be merged without rebasing on latest cids and multihashes libs
CIDv1 should not be hardcoded to Base32, but be updated to Base36 for ED25519 libp2p-keys for functional IPNS in subdomains (Base32 is too long, DNS label limit is 63 characters – details in ipfs/go-ipfs#7441)
we will see more and more of those, as go-ipfs 0.7.0 will generate ED25519 by default: ipfs/go-ipfs#6916

Are they changes required on content-hash not on ui?

lidel added a commit to lidel/content-hash that referenced this pull request Sep 15, 2020
@lidel
Copy link

lidel commented Sep 15, 2020

return new CID(value).toV1().buffer;
Is it always the case regardless of encoding v0 or v1?

Yes, it will create a valid CID from the string value, no matter if it is v0 or v1, then ensure it is v1 by upgrading v0 to v1 (if value was v0).

Are they changes required on content-hash not on ui?

all needs to happen in content-hash – submitted PR pldespaigne/content-hash#43, now @pldespaigne needs to review it

Copy link

@lidel lidel left a comment

Choose a reason for hiding this comment

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

Needs new release of content-hash, details inline

@@ -54,7 +54,7 @@
"base58check": "^2.0.0",
"bech32": "^1.1.3",
"bs58": "^4.0.1",
"content-hash": "^2.4.1",
"content-hash": "^2.5.2",
Copy link

Choose a reason for hiding this comment

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

Needs new release with fixes from pldespaigne/content-hash#43


// convert the ipfs from base58 to base32 (url host compatible)
// if needed the hash can now be resolved through a secured origin gateway (<hash>.gateway.com)
decoded = contentHash.helpers.cidV0ToV1Base32(decoded)
Copy link

@lidel lidel Sep 15, 2020

Choose a reason for hiding this comment

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

@makoto
Unsure about context, but if this is needed for subdomain gateways, it should use cidForWeb from pldespaigne/content-hash#43 instead.

Feel free to ping me if more context from IPFS side is needed.

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.

5 participants