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

directly parse peer IDs as peer IDs #5409

Merged
merged 3 commits into from
Aug 30, 2018
Merged

Conversation

Stebalien
Copy link
Member

No need to parse it as a hash first.

No need to parse it as a hash first.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien requested a review from Kubuxu as a code owner August 29, 2018 23:08
@ghost ghost assigned Stebalien Aug 29, 2018
@ghost ghost added the status/in-progress In progress label Aug 29, 2018
@Stebalien Stebalien requested a review from kevina August 29, 2018 23:09
@Stebalien
Copy link
Member Author

@kevina this is one of the few to only changes we'll need to make to go-ipfs to use non-byte-array CIDs.

@Stebalien Stebalien added the need/review Needs a review label Aug 29, 2018
License: MIT
Signed-off-by: Steven Allen <[email protected]>
@@ -55,19 +55,13 @@ func (r *IpnsResolver) resolveOnce(ctx context.Context, name string, options *op
}

name = strings.TrimPrefix(name, "/ipns/")
hash, err := mh.FromB58String(name)
pid, err := peer.IDB58Decode(name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I am missing something, this is not performing the same set of checks. In particular peer.IDB58Decode does not check it is a valid Multihash: https://github.com/libp2p/go-libp2p-peer/blob/master/peer.go#L121:L127

Copy link
Member Author

Choose a reason for hiding this comment

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

It calls mh.FromB58String internally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops sorry.

@Stebalien Stebalien requested a review from kevina August 30, 2018 00:16
@Stebalien
Copy link
Member Author

@kevina could I get a ✔️?

@@ -55,19 +55,13 @@ func (r *IpnsResolver) resolveOnce(ctx context.Context, name string, options *op
}

name = strings.TrimPrefix(name, "/ipns/")
hash, err := mh.FromB58String(name)
pid, err := peer.IDB58Decode(name)
if err != nil {
// name should be a multihash. if it isn't, error out here.
log.Debugf("RoutingResolver: bad input hash: [%s]\n", name)
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message should likely change to

log.Debugf("RoutingResolver: could not convert public key hash %s to peer ID: %s\n", name, err)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed (although I chose a different error message as that one isn't quite right either).

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien Stebalien merged commit 9a21a8c into master Aug 30, 2018
@ghost ghost removed the status/in-progress In progress label Aug 30, 2018
@Stebalien Stebalien deleted the nit/directly-parse-peer-id branch February 28, 2019 22:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants