feat: add support initial peerstore support #14
Conversation
jacobheun
changed the title
jacobheun
force-pushed
the
feat/update-protos
branch
from
6fdc45c
to
a6444ea
Compare
| message PeerstoreRequest { | ||
| enum Type { | ||
| GET_PROTOCOLS = 1; | ||
| GET_PEER_INFO = 2; |
There was a problem hiding this comment.
What about the remaining types of PeerstoreRequest?
libp2p/go-libp2p-daemon#91/files#diff-d864c93b68284fafd809efbf29a1f0ebR166
There was a problem hiding this comment.
So, I noticed that you have only implemented GET_PROTOCOLS here.
Can I recommend adding the other ones and then throw a ERR_NOT_IMPLEMENTED for that? Moreover, can you create an issue for tracking the remaining ones?
There was a problem hiding this comment.
Yeah I'll create an issue for when the spec gets done. I only added these initial two, as those likely won't change, but since the spec is still being discussed it didn't make sense to me to pull in a bunch of theoretical types.
I'll add ERR_NOT_IMPLEMENTED for the types that are currently in the proto file.
|
|
||
| required Type type = 1; | ||
| optional bytes id = 2; | ||
| repeated string protos = 3; |
There was a problem hiding this comment.
We are also missing some properties here and in the response.
There was a problem hiding this comment.
I've only added the required properties for the two types that are here. The rest should be added once the spec is merged for the other types.
| const message = await stream.first() | ||
| let response = Response.decode(message) | ||
| expect(response.type).to.eql(Response.Type.OK) | ||
| expect(response.peerStore).to.eql({ |
There was a problem hiding this comment.
Why not expect(response.peerStore.protos) instead?
peer: null is not part of the spec, and we could potentially accept anything else there in the future, right?
There was a problem hiding this comment.
peer is a response field for GET_PEER_INFO, which is currently not implemented. I'm validating the full response to ensure that no additional fields are being "leaked"/accidentally set in the response. While expect(response.peerStore.protos) verifies I am getting the correct protocols, it doesn't fully verify I am getting the expected response.
|
I added #15 to track the implementation of the remaining peerstore types, once the initial spec is merged. |
This also bumps to the latest release candidate for js-libp2p
Note: the spec for this is still underway and may be subject to change, libp2p/go-libp2p-daemon#91. This is to unblock interop testing with identify in support of the gossipsub needs for peer protocol knowledge.