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: return supported protocols in id output #7409

Merged
merged 4 commits into from
Aug 14, 2020

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented Jun 3, 2020

This is really useful for debugging.

TODO: fix tests if @aschmahmann and @achingbrain OK this change.

@jacobheun
Copy link
Contributor

I was trying to do this the other day, this would be great to have support for.

Stebalien and others added 4 commits August 7, 2020 19:46
Otherwise, the ID is going to be incorrect.

Note: technically, the previous logic didn't need to connect to the target peer
to complete. However, I'm fine dropping that capability in favor of more
up-to-date information.
Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

@Stebalien made a few changes and left some comments.

@Stebalien @achingbrain are we ok merging this change without the corresponding js-ipfs one, or do we need to block on that?

Comment on lines +102 to +103
// We need to actually connect to run identify.
err = n.PeerHost.Connect(req.Context, peer.AddrInfo{ID: id})
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fine conceptual change. Just a note that this doesn't actually do anything as the DHT (our only PeerRouting system) will connect to the peer, and will not return any addresses if it fails. https://github.com/libp2p/go-libp2p-kad-dht/blob/fc3558cc35274f5d1727dad8201f821df1ee6317/routing.go#L675-L682

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. I thought I had some trouble where we weren't actually waiting for the connect event to finish.

But honestly, we should do this anyways. We shouldn't assume that the router will always end up connecting to the target peer.

@@ -163,6 +168,13 @@ func printPeer(ps pstore.Peerstore, p peer.ID) (interface{}, error) {
for _, a := range addrs {
info.Addresses = append(info.Addresses, a.String())
}
sort.Strings(info.Addresses)
Copy link
Contributor

Choose a reason for hiding this comment

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

I added address sorting to this function, any objections?

Copy link
Member Author

Choose a reason for hiding this comment

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

LGTM!

@@ -198,6 +210,9 @@ func printSelf(node *core.IpfsNode) (interface{}, error) {
for _, a := range addrs {
info.Addresses = append(info.Addresses, a.String())
}
sort.Strings(info.Addresses)
Copy link
Contributor

Choose a reason for hiding this comment

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

I added address sorting to this function, any objections?

@jacobheun
Copy link
Contributor

are we ok merging this change without the corresponding js-ipfs one, or do we need to block on that?

No need to block on JS, I created ipfs/js-ipfs#3219 to track adding support there.

Copy link
Member Author

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

LGTM!

@aschmahmann aschmahmann merged commit dc869c9 into master Aug 14, 2020
@achingbrain
Copy link
Member

This is being added to js in ipfs/js-ipfs#3250

@aschmahmann aschmahmann mentioned this pull request Sep 22, 2020
72 tasks
@hacdias hacdias deleted the feat/id-list-protocols branch May 9, 2023 10:59
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.

4 participants