Using nim-protobuf-serialization for the RendezVous protocol

[lchenut]

Basically what the title says.
It will be necessary to change the require "protobuf_serialization#..." once status-im/nim-protobuf-serialization#36 is merged.

[Menduist]

Even without enum support, that looks miles better! Thanks @arnetheduck

[codecov]

Codecov Report

Merging #838 (e372a08) into unstable (5e3323d) will decrease coverage by 0.24%.
The diff coverage is 85.24%.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #838      +/-   ##
============================================
- Coverage     83.74%   83.50%   -0.25%     
============================================
  Files            82       84       +2     
  Lines         14908    14770     -138     
============================================
- Hits          12485    12333     -152     
- Misses         2423     2437      +14     

Impacted Files	Coverage Δ
libp2p/protocols/rendezvous.nim	88.78% <85.24%> (-3.78%)	⬇️
libp2p/discovery/discoverymngr.nim	81.63% <0.00%> (-15.94%)	⬇️
libp2p/protocols/secure/secure.nim	71.15% <0.00%> (-3.85%)	⬇️
libp2p/muxers/mplex/mplex.nim	87.70% <0.00%> (-1.38%)	⬇️
libp2p/protocols/pubsub/pubsub.nim	82.13% <0.00%> (-1.04%)	⬇️
libp2p/muxers/mplex/lpchannel.nim	83.13% <0.00%> (-0.89%)	⬇️
libp2p/connmanager.nim	90.79% <0.00%> (-0.34%)	⬇️
libp2p/multiaddress.nim	84.93% <0.00%> (-0.17%)	⬇️
libp2p/utils/semaphore.nim	100.00% <0.00%> (ø)
... and 11 more

[arnetheduck]

not needed

[lchenut]

@arnetheduck I am reopening this comment as it appears that combparser is indeed needed.
It's used by protobuf-ser here and is therefore installed / needed by the nim-libp2p. I don't know if it's an oversight on your part or if you eventually want to remove the use of combparser in protobuf-ser.
Or maybe I'm just missing something.

[arnetheduck]

as long as you don't use import_proto3 it's not needed - ie that file should not be imported by default, per the nps readme.

[Menduist]

The pinned file is generated from the nimble dependency, so it will still be bringed in, as long as nimble doesn't support optional deps

[arnetheduck]

cc @Menduist this is where you'd use a streaming writer instead - ie each of these functions is pointlessly inefficient

[Menduist]

So you would imagine a Protobuf.streamEncode macro that takes

Protobuf.streamEncode(Message(
  msgType: RdvRegisterResponse,
  registerResponse: RegisterResponse(status: status, text: text))
)
# and transforms into
block:
  var pb = initProtoBuf()
  pb.writeField(1, RdvRegisterResponse)
  pb.writeField(3, Protobuf.streamEncode(RegisterResponse(status: status, text: text))
  pb.finish()
  pb.buffer

? That shouldn't be that hard to do

For deeply nested objects like in gossipsub, that may be hard to use though: https://github.com/status-im/nim-libp2p/blob/unstable/libp2p/protocols/pubsub/rpc/messages.nim#L35-L67

[arnetheduck]

Well, or some variations on that theme:

Protobuf.encode(Message, encoder):
  encoder.write(fieldname1, value)
  encoder.write(somesubobject, subencoder):
    subencoder.write(subfield, value)

Protobuf.encode(Message, encoder)
    .field1(value).
    .field2(subobject):
       suboject
          .subField(value)
    .field3(value)

not sure, haven't really looked into it, though the obvious point is to grab the field number and encoding from the object definition and for that you need the type and the field name - the rest is really a syntax game.

There are some caveats though - streaming inherently becomes more "freeform" in that you can repeat the same field multiple times (this is valid in pb even for non-repeated) and there are some quirks with packed fields (which are length-prefixed and therefore would need an intermediate store - either the fancy but buggy faststreams buffers or something equivalent.

[arnetheduck]

When I look at your example, maybe transforming the object constructor isn't that bad :) That said, similar caveats apply: for example, to correctly encode a nested object you need to evaluate it twice: once to know if it has any non-default values and the second time to actually write the bytes - do you then evaluate the "source" twice or introduce a temporary buffer? tradeoffs, tradeoffs...

[arnetheduck]

before we go there though, I'd really love to see the conformance test suite implemented - ie all these features first and foremost must result in valid protobufs (that ideally also are efficiently encoded), so before making any further drastic changes or adding more macro lipstick onto the pig, the test suite needs expansion - it's really quite underwhelming right now.