search

libp2p / js-peer-info

libp2p Peer abstraction Node.js implementation
https://libp2p.io
MIT License
37 stars 28 forks source link

Understanding the types of variables in js-peer-info constructor #72

Closed Mikerah closed 5 years ago

Mikerah commented 5 years ago

I am in the process of attempting a gossipsub implementation in JS.

As I was going through the code for js-peer-info, I noticed that there is a variable this.protocols that is of type Set (see here). However, it is not at all clear what this contains. Is it analogous to the Go protocol string type? If so, would simply using a String suffice for the elements of this.protocols?

pgte commented 5 years ago

This is not documented and is not covered by the tests. I've poked around bit on dependent repos and I haven't been able to find if / how peerInfo.protocols is being used. @jacobheun do you have any idea?

jacobheun commented 5 years ago

@pgte I've never seen peerInfo.protocols being used. Perhaps the original idea was to be able track the known protocols a peer accepts, but that's not being done anywhere and I'm not aware of any plans to do that. I think it would be safe to remove that.

Mikerah commented 5 years ago

Even though it's not being done currently, wouldn't be a good idea to keep it so that it is interoperable with the Go implementation and for potential users that do want to specify the protocols that a peer accepts?

jacobheun commented 5 years ago

Apparently PeerBook is actually copying protocols https://github.com/libp2p/js-peer-book/blob/v0.9.1/src/index.js#L63 on puts (there are no tests), but I don't know of anything that is setting it. Switch would be the thing to set that, but it's not.

Interop with go wouldn't really be affected by this, as that info isn't passed across nodes. Performing an ls on multistream would be the appropriate way to determine initial protocol lists for peers. This may be why it was originally added to peerinfo and Switch was just never updated to actually perform the ls and store the protocols. This would allow us to avoid connecting/negotiating with nodes for a protocol we know they don't support, but we'd also need to ensure we're not preventing that indefinitely, as nodes may add support later.

We can leave it, but it should get documented (as a set of strings) and we should add some basic tests for it. Ideally to PeerBook too.