Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add getObservers method #1249

Merged
merged 8 commits into from
Mar 21, 2023
Merged

feat: add getObservers method #1249

merged 8 commits into from
Mar 21, 2023

Conversation

weboko
Copy link
Collaborator

@weboko weboko commented Mar 16, 2023

Follow up to #1213

@weboko weboko requested a review from fryorcraken as a code owner March 16, 2023 20:54
@@ -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>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is any better than just exposing observers...

@fryorcraken

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have an idea. Will write once back on the laptop.

Copy link
Collaborator

@fryorcraken fryorcraken Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Promise would resolve instantly.
  • In the case of the new filter protocol, we can do a ping to the remote server and return undefined (or empty map?) if remote server returns an error or returned locally saved subscriptions if 200 OK is returned.

edit: fixed the interface

cc @jm-clius

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@fryorcraken fryorcraken Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, let's handle it when time comes

@github-actions
Copy link

github-actions bot commented Mar 17, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku core 116.74 KB (+0.01% 🔺) 2.4 s (+0.01% 🔺) 482 ms (+4.15% 🔺) 2.9 s
Waku default setup 387.55 KB (+0.01% 🔺) 7.8 s (+0.01% 🔺) 1.1 s (-0.39% 🔽) 8.8 s
ECIES encryption 27.99 KB (0%) 560 ms (0%) 260 ms (+30.77% 🔺) 820 ms
Symmetric encryption 27.99 KB (0%) 560 ms (0%) 266 ms (+27.86% 🔺) 825 ms
DNS discovery 108.07 KB (0%) 2.2 s (0%) 652 ms (+50.8% 🔺) 2.9 s
Privacy preserving protocols 116.22 KB (+0.01% 🔺) 2.4 s (+0.01% 🔺) 472 ms (-5.26% 🔽) 2.8 s
Light protocols 117.99 KB (+0.01% 🔺) 2.4 s (+0.01% 🔺) 335 ms (-30.76% 🔽) 2.7 s
History retrieval protocols 118.06 KB (+0.01% 🔺) 2.4 s (+0.01% 🔺) 419 ms (-4.53% 🔽) 2.8 s

Comment on lines +11 to +12
type PubSubTopic = string;
type ContentTopic = string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

future work: Would be good to actually use these two types across the codebase.

Copy link
Collaborator Author

@weboko weboko Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@weboko weboko merged commit 45284db into master Mar 21, 2023
@weboko weboko deleted the weboko/observers-dump branch March 21, 2023 01:44
@therealjmj
Copy link

Great! Will this be included in a waku core release in the near term?

@weboko
Copy link
Collaborator Author

weboko commented Mar 21, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants