Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

feat: human-readable cache keys for IPNS #38

Merged
merged 3 commits into from
Mar 8, 2023

Conversation

laurentsenta
Copy link
Contributor

@laurentsenta laurentsenta commented Feb 22, 2023

Closes #37

Use ipnsKey.String() to generate cache keys, which generate a b58 encoding instead of using non-printable characters.
This makes it practical to populate the IPFS_NS_MAP env variable with IPNS records:

export IPFS_NS_MAP="dnslink-test.example.com:/ipfs/${CID},12D3KooWQbpsnyzdBcxw6GUMbijV8WgXE4L8EtfnbcQWLfyxBKho:/ipfs/${CID}"

Copy link
Member

@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.

lgtm, but we need to normalize key

namesys.go Outdated
@@ -215,7 +215,7 @@ func (ns *mpns) resolveOnceAsync(ctx context.Context, name string, options opts.

cacheKey := key
if err == nil {
cacheKey = string(ipnsKey)
cacheKey = ipnsKey.String()
Copy link
Member

Choose a reason for hiding this comment

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

This won't be enough because of legacy mess: /ipns/id can be Base58btc of peerid (12... which is not a valid CID), or CIDv1 with libp2p-key multihash in base36 (k51..).

This screates surface for bugs, where someone tries to use CIDv1 base36 (k..) produced by ipfs key list -l and it does not work because raw peerid 12... is expected here.

I think we need something like:

import ( 
  iface "github.com/ipfs/interface-go-ipfs-core"
...
		cacheKey = iface.FormatKeyID(ipnsKey)

This way the key will be canonical cidv1+libp2p-key+base36, the same user can see in ipfs key list -l

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for the review and details.

I guess since all the cache keys go through the same calls, that's "just" a problem for the user that tries to use something other than a raw peer id in IPFS_NS_MAP.

I updated the PR so that we normalize all the keys loaded from IPFS_NS_MAP with the same FormatKeyID, and added tests!

@lidel lidel changed the title feat: use peer.String() to produce a human-readable cache key feat: human-readable cache keys for IPNS Feb 22, 2023
@laurentsenta laurentsenta force-pushed the feat/simplify-libp2pkeys-in-ipfs-ns-map branch 2 times, most recently from a42ecbe to 5fc1bc5 Compare February 23, 2023 09:56
@laurentsenta laurentsenta force-pushed the feat/simplify-libp2pkeys-in-ipfs-ns-map branch from 5fc1bc5 to 7c65803 Compare February 23, 2023 09:57
@laurentsenta laurentsenta force-pushed the feat/simplify-libp2pkeys-in-ipfs-ns-map branch from 7c65803 to f9ab490 Compare February 23, 2023 10:04
@laurentsenta laurentsenta requested a review from hacdias February 23, 2023 11:01
Copy link
Member

@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.

Thank you @laurentsenta.
Merging, so we include this in future release.

@lidel lidel merged commit af35385 into master Mar 8, 2023
@lidel lidel deleted the feat/simplify-libp2pkeys-in-ipfs-ns-map branch March 8, 2023 17:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

extend IPFS_NS_MAP feature to ipns records
3 participants