Description

Links to any relevant issues

#2650

Open Questions

Change checklist

• [x] I have performed a self-review of my own code ~- [ ] I have made corresponding changes to the documentation~ ~- [ ] I have added tests that prove my fix is effective or that my feature works~
• [x] A changelog entry has been made in the appropriate crates

This does not compile because the blanket impl for ProtocolName over anything that implements AsRef<u8> is conflicting with the implementation on Either.

I don't have a strong opinion on this. Given the complication above I am inclined to just leave it as is. Any additional benefits of this patch beyond consistency?

This does not compile because the blanket impl for ProtocolName over anything that implements AsRef<u8> is conflicting with the implementation on Either.

I don't have a strong opinion on this. Given the complication above I am inclined to just leave it as is. Any additional benefits of this patch beyond consistency?

I'll give it a bit more thought but I don't think so.

Instead of just adding impls that we need, I can see to refactor some code to streamline, how we generate ProtocolNames.

It is a bit weird that we allow so many things to be ProtocolNames and on the trait, we define two requirements that aren't really enforced:

• Max length of 140
• Must start with /

Maybe we should introduce a ProtocolName newtype?

On the meta level, I think we could do better modelling protocol names.

Part of the problem is the conflict between multistream select and libp2p-identify on the specification level. One accepts a [u8], the other expects a String.

Maybe we should introduce a ProtocolName newtype?

Or a unit type for each protocol name, each implementing ProtocolName.

On the meta level, I think we could do better modelling protocol names.

Part of the problem is the conflict between multistream select and libp2p-identify on the specification level. One accepts a [u8], the other expects a String.

Damn. Of course it wasn't that easy to fix :)

Maybe we should introduce a ProtocolName newtype?

Or a unit type for each protocol name, each implementing ProtocolName.

Nice idea but it won't allow us to enforce invariants, right?

Nice idea but it won't allow us to enforce invariants, right?

True. Would be great to enforce these at runtime. Would be wonderful to enforce these at compile time :innocent:

Nice idea but it won't allow us to enforce invariants, right?

True. Would be great to enforce these at runtime. Would be wonderful to enforce these at compile time :innocent:

Something for a future issue? I have some ideas :)

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

Nice idea but it won't allow us to enforce invariants, right?

True. Would be great to enforce these at runtime. Would be wonderful to enforce these at compile time innocent

Something for a future issue? I have some ideas :)

:+1:

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

Given that Either also implements AsRef<[u8]> when the two subtypes implement AsRef<[u8]>, is the implementation of ProtocolName on Either still needed?

https://docs.rs/either/latest/either/enum.Either.html#impl-AsRef%3C%5BTarget%5D%3E

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

Given that Either also implements AsRef<[u8]> when the two subtypes implement AsRef<[u8]>, is the implementation of ProtocolName on Either still needed?

https://docs.rs/either/latest/either/enum.Either.html#impl-AsRef%3C%5BTarget%5D%3E

(Un)fortunately yes. I remember trying this and it is not working.

I think the reason is in Rust's explicit strong-typing system. Just because something implements all super traits of a trait A does not mean it implements trait A by itself unless it is actively declared as such. In Golang, this would work :)

To progress on this issue, would it be okay to transition all/most usages to std types like &static [u8] and String?

I am fine with that.

I would actually suggest making String the default protocol representation. See #2831. What do you think @thomaseizinger?

Yeah we can move to string! Should we do that as part of this PR or make another one? It is getting a bit OT from the original goal haha.

I am happy to fix #2831 first and then come back to this!

I think the following order makes most sense:

1. #2831
2. Move ProtocolName to String
3. Do the actual change here, namely to use Either

That said, I don't think we have to stick with that order.

I think the following order makes most sense:

1. [misc/multistream-select: Treat protocol as `String` and not `[u8]` #2831](https://github.com/libp2p/rust-libp2p/issues/2831)

2. Move `ProtocolName` to `String`

3. Do the actual change here, namely to use `Either`

That said, I don't think we have to stick with that order.

I agree, leave it with me 😊

Closing in favor of #2966.