Feat/pubsub topics #3630
Feat/pubsub topics #3630
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3630 +/- ##
======================================
- Coverage 47% 46% -1%
======================================
Files 269 269
Lines 17133 17130 -3
======================================
- Hits 8112 8047 -65
- Misses 7926 7995 +69
+ Partials 1095 1088 -7 |
ZenGround0
force-pushed
the
feat/pubsub-topics
branch
from
0b44b4e to
a87a2f6
Compare
| @@ -46,7 +45,7 @@ type NetworkSubmodule struct { | |||
| // Router is a router from IPFS | |||
| Router routing.Routing | |||
|
|
|||
| fsub *libp2pps.PubSub | |||
| Fsub *libp2pps.PubSub | |||
There was a problem hiding this comment.
"F" for flood is no longer a good name here.
| topic, err := network.Fsub.Join(net.MessageTopic(network.NetworkName)) | ||
| if err != nil { | ||
| return MessagingSubmodule{}, err | ||
| } |
There was a problem hiding this comment.
I'm a bit confused about the difference between this Join and Topic.Subscribe() calls below. I've been code diving to try to reconcile this with the code I see in libp2p (on master) but haven't 100% figured it out.
Subscribing (having any network side-effect) at module construction is undesirable. It looks like in libp2p, this Join() is a no-op (but also with no return value?). If that's an assumption we're making in order to not have side-effects here, we should document as such. But with GossipSub, Join does do a bunch, so we're not getting any abstraction here.
If constructing a Topic object involves interacting with libp2p in a way that has side effects, I think we're better representing a topic as a string until we actually want to subscribe to it.
There was a problem hiding this comment.
I want to move away from the deprecated endpoints so I'm keeping Topics. However your point on side effects during construction is well taken. I'll construct both the topic and subscription during start.
Motivation
We want to move to gossipsub and keep race conditions out of tests. This required waiting for recent upgrades to libp2p. This PR takes the first step of 1) updating libp2p 2) refactoring our internal wrappers to make use of the new topic abstraction
Proposed changes
Topictype. Construction modified. Publish and subscribe calls modified to not take in topic string.Helps close #3561