fetch: Change identifier pb type from string to bytes?

[achingbrain]

The fetch protocol uses this protobuf:

message FetchRequest {
	string identifier = 1;
}

One use case for fetch is resolving IPNS records directly from other nodes (ref).

The identifier for this operation is "/ipns/" + public-key-multihash-bytes.

Treating identifier as a string means we need to stringify the value before serializing it to pb bytes, and we'll also interpret any received value as a string.

golang treats public-key-multihash-bytes as a char array so this works as expected.

JavaScript strings are all UTF-16, all the time so it's possible to have a series of bytes that we can't round trip to a string if some pairs of byte values happen to be interpretable as a multi-byte character.

Since the on-the-wire format of string and bytes in protobuf is identical, JS could just unilaterally treat identifier as bytes to make the problem go away, or we can change the type in the spec definition so it's clear for implementers?

cc @aschmahmann

[MarcoPolo]

This is a backwards compatible change, as you say. Given that we are already treating it as bytes, I think this is a reasonable change. Could you please add a link to the code that treats this as bytes just for reference?

[aschmahmann]

Makes sense to me, this is likely just a bug in the original spec since it is used in practice for arbitrary bytes (e.g. the IPNS example you gave).

[achingbrain]

Could you please add a link to the code that treats this as bytes just for reference?

I think it's this line:

https://github.com/libp2p/go-libp2p-pubsub-router/blob/ea64ffe9cd424c492a129e4d4dc39031cb3a174d/pubsub.go#L311C2-L311C65

getLocal is passed to newFetchProtocol as the getData function here:

https://github.com/libp2p/go-libp2p-pubsub-router/blob/ea64ffe9cd424c492a129e4d4dc39031cb3a174d/pubsub.go#L106

...which receives the identifier here:

https://github.com/libp2p/go-libp2p-pubsub-router/blob/ea64ffe9cd424c492a129e4d4dc39031cb3a174d/fetch.go#L49

Unless I'm mis-reading the code, I think the problem is that the fetch protocol as implemented in go-libp2p-pubsub-router accepts datastore keys as identifier.

If it had some sort of per-prefix lookup function or other indirection we could use the IPNS Name string representation instead and identifier could remain as a string, which generally has better DX?