Skip to content
This repository has been archived by the owner on Mar 10, 2020. It is now read-only.

feat: support name.resolve of peerid as cid #1145

Merged
merged 2 commits into from
Nov 7, 2019

Conversation

lidel
Copy link
Contributor

@lidel lidel commented Nov 5, 2019

This PR updates to interface-ipfs-core": "^0.118.0 which includes test for resolving paths with libp2p-key represented as CIDv1 Base32.

Open Problem

The problem is that go-ipfs 0.4.22 does not support PeerID as CID, so test added in ipfs-inactive/interface-js-ipfs-core#553 fails:

  1) interface-ipfs-core tests
       .name.resolve offline
         should resolve a record from peerid as cidv1 in base32:
     Error: not a valid proquint string

@alanshaw not sure how to handle this. Wait for go-ipfs v0.5.0? Fixup on the fly in the meantime?

A hacky way to support CIDs when API is go-ipfs 0.4.22 or older is to convert toBase58btc before sending. See below.

For now we skip cidv1b32 test, added todo to unskip when go-ipfs ships with native support.

closes #1147

This is a hacky way to support CIDs when API is go-ipfs 0.4.22 or older
(convert toBase58btc before sending)

License: MIT
Signed-off-by: Marcin Rataj <[email protected]>
Comment on lines 18 to 29
// normalize PeerIDs to Base58btc, so it works with go-ipfs <=0.4.22
if (typeof (args) === 'string') {
try {
const path = args.split('/')
if (path.length > 2) {
path[2] = multihash.toB58String(new CID(path[2]).multihash)
args = path.join('/')
}
} catch (err) {
// noop
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a better way? :/

Copy link
Contributor

Choose a reason for hiding this comment

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

All we need to do is stringify args if it is a CID. We will skip the interface-ipfs-core test here until go-ipfs supports it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't stringify CIDs anywhere else, removed this hack and added TODO to unskip mentioned test when go-ipfs implements this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah interesting, I guess the querystring module calls toString on the CID then...

@alanshaw
Copy link
Contributor

alanshaw commented Nov 6, 2019

Please would you add the updated interface-ipfs-core dep here and skip the relevant test?

License: MIT
Signed-off-by: Marcin Rataj <[email protected]>
@lidel lidel marked this pull request as ready for review November 7, 2019 15:41
@lidel lidel requested a review from alanshaw November 7, 2019 15:41
@lidel
Copy link
Contributor Author

lidel commented Nov 7, 2019

@alanshaw done

@alanshaw alanshaw merged commit 2d9afc8 into master Nov 7, 2019
@alanshaw alanshaw deleted the feat/support-name-resolve-cidv1b32 branch November 7, 2019 22:53
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.

2 participants