vacp2p / multiprotocol

Self-describing protocol identifiers
MIT License
4 stars 1 forks source link

Some feedback and thoughts on integration #2

Open raulk opened 4 years ago

raulk commented 4 years ago

Apologies for the slow turnaround! This has been sitting in my queue for quite some time.

decanus commented 4 years ago

Hey @raulk, thanks for the reply. Lots of our thoughts are in sync.

Unless I missed it, it's unclear from the spec what the lengths/binary forms of the namespace, protocol, version are.

All those fields are currently uvarints, lengths are not defined but we could look into this if necessary.

Would you envision adding multiprotocols as a field in Signed Peer Records? libp2p/specs#217 That way peers can exchange authenticated records that express the protocols and capabilities they support, potentially with a flag indicating whether the enumeration is complete or partial.

Yes that would be optimal, multiprotocol was inspired by ethereums ENR which is essentially a signed record too.

multiaddr integration is cool and really appreciated. In practice, it could be rather cumbersome to advertise multiple protocols under the same endpoint.

Yeah, this is a point I have been stuck on and thinking about for a while, I think the easiest way would be to add a multicodec that signals a variadic anchor as you mention.

For integration with multiaddr, we could have a multicodec that stands for "multiprotocol". That way you're not blindly tacking on a multiprotocol and hoping the other party would interpret the correct way, but you're being self-descriptive and deterministic about what follows.

Yes exactly, throw the multiprotocol into a key value and call it a day.

In my view, a multiprotocol should not mandate a TSV format; instead, the metadata should be opaque.

Yeah, completely agree. I think that section is more of an artefact that can be removed.

And it needn't be limited to capabilities. I see this as a general mechanism to communicate out-of-band protocol-scoped information.

Yeah I completely agree

I'd also like to mandate that implementations should offer a way to index iterate/into metadata elements.

That sounds like a smart idea. How about you PR it in?

Again thanks for the review, I'll make some of the changes would love if you could PR the index iteration. Also, how should we move forward on this? I'd love to make it part of the multiformat family