feat: add getObservers method #1249
Conversation
packages/core/src/lib/relay/index.ts
Outdated
| @@ -208,6 +183,12 @@ class Relay extends GossipSub implements IRelay { | |||
| getMeshPeers(topic?: TopicStr): PeerIdStr[] { | |||
| return super.getMeshPeers(topic ?? this.pubSubTopic); | |||
| } | |||
|
|
|||
| public getObservers<T extends IDecodedMessage>( | |||
There was a problem hiding this comment.
I don't think it is any better than just exposing observers...
There was a problem hiding this comment.
By dump method, I would mean a method that returns a string rendered version of the observers for logging purposes.
Not sure what are common practice in JavaScript. For example, in Rust, one would implement the fmt::Debug trait.
There was a problem hiding this comment.
I have an idea. Will write once back on the laptop.
There was a problem hiding this comment.
The idea is to inspire ourselves from the new filter protocol https://github.com/vacp2p/rfc/pull/562/files and see if we can provide an interface on active subscription that could then be added to IReceiver.
Look at the filter request, it has 1 optional pubsub and an array of content topics.
There is a also a new "ping" request to check that the remote has active subscriptions for the local node.
Hence, we could have an interface such as:
type PubSubTopic = string;
type ContentTopic = string;
type ActiveSubscriptions = Map<PubSubTopic, ContentTopic[]>
interface IReceiver {
// ... subscribe
getActiveSubscriptions: () => Promise<ActiveSubscriptions | undefined>
}- In the case of relay, the
Promisewould resolve instantly. - In the case of the new filter protocol, we can do a
pingto the remote server and returnundefined(or empty map?) if remote server returns an error or returned locally saved subscriptions if200 OKis returned.
edit: fixed the interface
cc @jm-clius
There was a problem hiding this comment.
Looks really neat. 👍 I will update this PR to close original issue and update IReceiver PR to correlate with this.
One thing I what to change is Promise<ActiveSubscriptions | undefined> - I believe if operation is sync it shouldn't be overcomplicated with Promise.
There was a problem hiding this comment.
Fair to not use a Promise and stick to ActiveSubscriptions return type for now.
When we implement the new filter protocol and implement this interface, I think it would make sense to do a "ping" to confirm the subscriptions are active. At this point in time, we may need to change the return type to Promise<ActiveSubscriptions | undefined> | ActiveSubscriptions | undefined or something like that.
But this can be done when it becomes necessary.
There was a problem hiding this comment.
Agree, let's handle it when time comes
size-limit report 📦
|
| type PubSubTopic = string; | ||
| type ContentTopic = string; |
There was a problem hiding this comment.
future work: Would be good to actually use these two types across the codebase.
There was a problem hiding this comment.
Good point from the perspective of improving readability but I would try to refrain from pushing it's usage on the consumers since these types are primitive, at least for now.
Here is the issue #1254
|
Great! Will this be included in a waku core release in the near term? |
|
It was not included in the latest release but for sure will be in the next one. I think it will be soon enough since now we have a faster way for releasing. |
Follow up to #1213