feat: validate messages for individual filter nodes & perform renewals #2057
Conversation
size-limit report 📦
|
| @@ -326,7 +427,7 @@ class FilterSDK extends BaseProtocolSDK implements IFilterSDK { | |||
| ) { | |||
| super( | |||
| new FilterCore( | |||
| async (pubsubTopic: PubsubTopic, wakuMessage: WakuMessage) => { | |||
| async (pubsubTopic, wakuMessage, peerIdStr) => { | |||
| const subscription = this.getActiveSubscription(pubsubTopic); | |||
There was a problem hiding this comment.
Wow, I just noticed - we keep only one subscription per pubsubTopic.
That mean that if another one will be created in the application - first one would be dropped.
Also we probably still have this one #1678
Let's follow up if I am not wrong
There was a problem hiding this comment.
No no. We have the ability to have subscriptions with multiple pubsub topics -- each pubsub topic is represented by a new SubscriptionManager object.
The line of code you are reading is actually a callback function:
handleIncomingMessage: (pubsubTopic: PubsubTopic, wakuMessage: WakuMessage, peerIdStr: string) => Promise<void>TL;DR: We can indeed subscribe to multiple pubsub topics
There was a problem hiding this comment.
oh right, there is a fork here -
js-waku/packages/sdk/src/protocols/filter.ts
Line 502 in 128dd58
if a subscription exists - then we return it which will be used for subscribe request
packages/sdk/src/protocols/filter.ts
Outdated
| return [...this.receivedMessagesHashes.all]; | ||
| } | ||
|
|
||
| addHash(hash: string, peerIdStr?: string): void { |
There was a problem hiding this comment.
nit: let's not forget access modifiers
There was a problem hiding this comment.
I agree. Added to this PR part of 128dd58
I believe we should introduce it into eslint config as well so it's not skipped
There was a problem hiding this comment.
Opened a PR adding the rule to eslint: #2068
| @@ -35,7 +35,8 @@ export class FilterCore extends BaseProtocol implements IBaseProtocolCore { | |||
| constructor( | |||
| private handleIncomingMessage: ( | |||
There was a problem hiding this comment.
nit: can we make it `private handleIncomingMessage: (options: IncomingMessageOptions) => Promise
Problem
We (may) subscribe to multiple peers with the Filter protocol, over the same content topics, in order to increase the reliability/probability of the message being received by our node. However, the nodes that aren't sending the desired messages aren't being acted upon, and they continue to stay in the connection pool.
Solution
Renew the nodes that aren't sending us the messages, while the other nodes do.
Notes
Contribution checklist:
!in title if breaks public API;