identify spec #97
identify spec #97
Conversation
ghost
assigned 
|
|
||
| These are the addresses on which the peer is listening as multi-addresses. | ||
|
|
||
| ### observedAddr |
There was a problem hiding this comment.
There are two possibilities here: this can be either the stack of protocols the listener used in order to reach back the dialer, or the stack of protocols we think the dialer used to reach us.
For the sake of forward compatibility, I think it'd be a nice idea to define this more precisely, and I'd be in favour of the former.
There was a problem hiding this comment.
Added a source disambiguator, per discussion in meatspace.
There was a problem hiding this comment.
Per discussion I'm not satisfied with the wording, because I'd like to disambiguate what is returned when p2p-circuit is used, but I can't find any appropriate wording.
There was a problem hiding this comment.
We can add an example for circuit addresses specifically, as they seem to be the contentious issue.
There was a problem hiding this comment.
Added some examples, hopefully it's clearer now.
There was a problem hiding this comment.
maybe something like: "This address describes the dialed peer's observed route to the dialing peer."
so wordy tho
There was a problem hiding this comment.
yeah, and it's not really any better than the current wording.
|
|
||
| ### protocols | ||
|
|
||
| This is a list of protocols supported by the peer. |
There was a problem hiding this comment.
not ordered in any specific manner.
There was a problem hiding this comment.
right, but do we need to say that?
There was a problem hiding this comment.
A list implies order. If order is not important, then we can say it's an unordered set. We shouldn't be sending duplicates anyway, right?
There was a problem hiding this comment.
it's a list on the wire, there is no set datatype.
There was a problem hiding this comment.
might be worth clarifying that this are multistream protocols (or protocol strings)?
identify/README.md
Outdated
| @@ -0,0 +1,56 @@ | |||
| # Identify v1.0.0 | |||
|
|
|||
| The identify protocol is used to identify a peer and its capabilities. | |||
There was a problem hiding this comment.
can we unpack the definition of "identify a peer"? the recursive definition is confusing for a spec
There was a problem hiding this comment.
uhm ok; me loves recursion.
There was a problem hiding this comment.
i really can't think of a non-verbose way of expressing this.
There was a problem hiding this comment.
How about "the identify protocol is used to interrogate a peer for basic information and its capabilities, the details of which are defined below"?
There was a problem hiding this comment.
query, not interrogate.
identify/README.md
Outdated
| ### protocolVersion | ||
|
|
||
| The protocol version identifies the family of protocols used by the peer. | ||
| The current protocol version is `ipfs/0.1.0`; if the protocol does not match |
There was a problem hiding this comment.
with the current logic, the micro/patch version can be different. only major and minor need to be equal.
There was a problem hiding this comment.
ok, will update. We'd also like to get to a point we don't close conns because of this.
|
|
||
| ### agentVersion | ||
|
|
||
| This is a free-form string, identifying the implementation of the peer. |
There was a problem hiding this comment.
can we encourage a particular format, even if non-normative? the default is go-libp2p/<semver>, and IPFS changes this to "go-ipfs/" + version.CurrentVersionNumber + "/" + version.CurrentCommit.
https://github.com/ipfs/go-ipfs/blob/master/core/core.go#L99
There was a problem hiding this comment.
nah, let's let people call their agents whatever they want!
There was a problem hiding this comment.
although an example might be useful.
|
|
||
| These are the addresses on which the peer is listening as multi-addresses. | ||
|
|
||
| ### observedAddr |
There was a problem hiding this comment.
maybe something like: "This address describes the dialed peer's observed route to the dialing peer."
so wordy tho
| ### listenAddrs | ||
|
|
||
| These are the addresses on which the peer is listening as multi-addresses. | ||
|
|
There was a problem hiding this comment.
Does the multiaddress also include /ipfs/Qm...? If so, is /p2p also supported?
There was a problem hiding this comment.
It doesn't include it. Well, it shouldn't.
|
Please make sure to take https://github.com/libp2p/js-libp2p-switch/issues/78 into account |
|
@vyzo this looks pretty solid to me - should we merge? I don't have the context behind @daviddias comment about the js / go interop, so maybe we need to address the changes made there? |
|
We also have |
|
@yusefnapora feel free to edit! |
|
@vyzo I added a bit about identify/push - lmk if it looks alright. If so, I think we should press the button 😃 |
|
hehe, fair enough :) I'll let you do the honor of pressing the big green button then |
|
Any objections to merging this? |
| `/ipfs/id/push/1.0.0` as the protocol id string. When the remote peer accepts the stream, | ||
| the local peer will send an `Identify` message and close the stream. | ||
|
|
||
| ## The Identify Message |
There was a problem hiding this comment.
We should probably note that any missing fields should be ignored (so we can do a partial identify push).
There was a problem hiding this comment.
@yusefnapora want to add it?
|
Alright, let's merge! |
No description provided.