feat(filter): reliability monitor as a separate class to handle reliability logic #2127
Conversation
size-limit report 📦
|
danisharora099
changed the title
| toAsyncIterator | ||
| } from "@waku/utils"; | ||
|
|
||
| import { BaseProtocolSDK } from "../base_protocol"; |
There was a problem hiding this comment.
don't forget to add .js otherwise it will break
There was a problem hiding this comment.
Right! Probably should add some eslint check for this 😅
Thanks for the catch :P
packages/sdk/src/waku.ts
Outdated
| @@ -115,6 +116,7 @@ export class WakuNode implements Waku { | |||
| if (protocolsEnabled.filter) { | |||
| const filter = wakuFilter(this.connectionManager); | |||
| this.filter = filter(libp2p); | |||
| new MessageReliabilityManager(this.filter); | |||
There was a problem hiding this comment.
we should keep a reference to it somewhere
let's do the same as with protocols
There was a problem hiding this comment.
Yes, absolutely - that's correct to point out. However, if we store it in a variable, as we currently don't use the variable, typescript complains
| public readonly protocol: FilterCore; | ||
| private readonly _connectionManager: ConnectionManager; | ||
|
|
||
| public activeSubscriptions = new Map<PubsubTopic, SubscriptionManager>(); |
There was a problem hiding this comment.
do we need to make it public only because of the monitor?
There was a problem hiding this comment.
yes, indeed!
this is why I suggest to follow up with masking a lot of these things behind one public/consumer facing interface
|
Moving to draft in favor of #2117 instead |
|
closing in favor of #2117 |
Problem
Based on #2075, the logic to detect messages that were missed from the Filter protocol were being detected inside of the Filter SDK implementation. This leads to a tight couple of primary objective of FilterSDK, and reliability goals.
This obstacle also extends to adding further actions that might be taken with additional protocols such as Store pings, and LightPush retries for certain messages.
Solution
Decouple reliability logic into a new ReliabilityMonitor class.
Notes
Contribution checklist:
!in title if breaks public API;