[KS-70] Minimal RageP2P wrapper

[bolekk]

A minimal counterpart of libOCR's peer_v2+endpoint_v2+bootstrapper_v2 with following differences:

1. Ability to change a set of peers dynamically.
2. Identify peers by their PeerIDs instead of Oracle IDs.

[github-actions]

I see that you haven't updated any README files. Would it make sense to do so?

[cedric-cordenier]

I'm generally not a fan of returning interfaces: it's idiomatic in Go to return concrete types (eg. structs) and accept interfaces. So in this case, it's fine (but odd) to return *peer and then expect any users to use an interface type throughout.

Arguably I'd say the same thing about the Peer interface type -- it should live in the consuming package (or in a types package). If its purpose is to define the public interface, you can do that on the struct directly.

[bolekk]

Agreed with returning concrete types (fixing here).

I'm a fan of having an explicit interface because then I can generate mocks with Mockery. Can that be done for structs too?
If not, I'm OK with moving the interface to a "types" sub-package. I understand the arguments for placing it in the consuming package but what if there are multiple consumers? They shouldn't depend on one another so there would be multiple copies of the same interface and mock.

[cedric-cordenier]

I'm not sure you can generate a mock from a struct, but in that case I would leave the interface as a private detail of the package (and specifically the test files) and use mockery on that

[cedric-cordenier]

Is there a benefit to exposing the internal detail of how reception happens (i.e. via a channel)? As opposed to exposing a generic Receive(Message) function?

[jmank88]

I would bet the channel makes things more difficult if you are mapping to grpc streams underneath.

[jmank88]

chan seems fine for a raw API though 🤷

[bolekk]

By "generic Receive(Message) function?" do you mean a callback? I want to push all messages to some Receiver/Dispatcher object as they arrive, channel seems fine to me.

[cedric-cordenier]

Yes, I mean hiding the channel as an internal detail as opposed to having it as an external one. One benefit for exposing the channel I guess is that you can use it for synchronization with select.

[patrick-dowell]

Is it common in golang to use continues in for loops? I was told to avoid using them in most languages because they act a bit like jumps and can make it a little harder to understand control flow in loops. It's pretty easy to avoid using them:

if pid != p.myID {
   .... rest of the for loop logic here
}

[bolekk]

You can make arguments either way. I remember reading an opinion that the more nested some logic is the harder it is to understand. I don't know of any Go-specific recommendations, maybe @jmank88 knows.

[jmank88]

I haven't seen much objection to continue in go, but there is definitely a tendency towards reducing nesting/indentation, so yeah maybe that alleviates this concern.

I suppose that there is an argument that using a continue in a for like this is actually analogous to the go proverb "return early, return often", which is meant to reduce indentation 🤷

[cl-sonarqube-production]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
40.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

[kaleofduty]

Double checking, is your goal to have a fully connected graph of all peers?

I believe you could avoid the need to recreate the group (and thus potentially rediscover already known peers) by slightly modifying the logic here:

• keep track of latest group id gid you used
• when UpdateConnections is called:
  • register the new set of peers under a new group id gid'
  • remove the old groupid gid
  • gid := gid'

[bolekk]

Nice! I didn't realize that it's OK for groups to overlap. I'll change it in a separate PR, thanks :)