clean up usage of string vs. protocol.ID

[marten-seemann]

It's all over the place, and we're converting between the two all the time.

[sukunrt]

can i pick this up?

[sukunrt]

I'm looking into this. Discussed the issue with @marten-seemann
Here a simple generic type constraint of ~string won't suffice since multistream.MultistreamMuxer is a struct and method parameters cannot be generic. To make a method like
func (msm *MultistreamMuxer) AddHandlerWithFunc(protocol string, match func(string) bool, handler HandlerFunc)
generic we'd need MultistreamMuxer to be generic with a signature like
func (msm *MultistreamMuxer[T ~string]) AddHandlerWithFunc(protocol T, match func(T) bool, handler HandlerFunc)
that seems tricky to do without breaking the api since this constructor NewMultistreamMuxer() expects to create a muxer without any type being specified. I'll come back on the possible options here.

[marten-seemann]

generic we'd need MultistreamMuxer to be generic with a signature like
func (msm *MultistreamMuxer[T ~string]) AddHandlerWithFunc(protocol T, match func(T) bool, handler HandlerFunc)

Yes, that would be great!

that seems tricky to do without breaking the api since this constructor NewMultistreamMuxer() expects to create a muxer without any type being specified. I'll come back on the possible options here.

I wouldn’t worry about that. Breaking the API is ok here, we can release this as a new major version.

If you really, really want to avoid breaking API changes you could do this

func NewGenericMultistreamMuxer[T ~string]() MultistreamMuxer[T] {
    …
}

and then have NewMultistreamMuxer return the concrete string type:

func NewMultistreamMuxer() MultistreamMuxer[string] {
    return NewGenericMultistreamMuxer[string]()
}

Not sure if that’s worth it here though, we don’t use it at so many places that avoiding the API change would actually pay off.

[sukunrt]

For go-libp2p the changes are here. I have changed PeerStore. There are a couple of other places where we can remove the converstions before integrating the changes go-multistream repo. I'll update those changes soon.

#2004

[sukunrt]

There is one source of conversion that remains, flow.MeterRegistry. However this only impacts bandwidth.go. @marten-seemann Should I change that to a generic implementation as well?

[marten-seemann]

There is one source of conversion that remains, flow.MeterRegistry. However this only impacts bandwidth.go. @marten-seemann Should I change that to a generic implementation as well?

I'm undecided on that one. I think for now it's ok to leave it as strings, since it's a fairly contained code path, and since we're not using the FindIdle function, which is the only function that returns a []string (and would need to be converted to []protocol.ID).