Simplify FilterManager #4665
Simplify FilterManager #4665
Conversation
Jenkins BuildsClick to see older builds (191)
|
vitvly
force-pushed
the
fix/filter-subscribes-order
branch
from
8aed250 to
16c3d47
Compare
Ideally such functionality should be part of the SDK layer, it could be a thin layer above the actual Filter implementation in go-waku. Would not recommend adding it to the Filter code in go-waku as it has become too complicated. Wrt MinPeersForFilter, status-go should specify the count for it as the idea is for users for Filter to be able to specify and choose based on the use-case. |
|
@chaitanyaprem regarding |
|
This, i think, implies, that the notion of |
Hmm, interesting point. Haven't thought about it this way, but kind of makes sense to have SDK abstract this from status-go. Have to think a little more if this brings any unnecessary complexities. |
|
Indeed, having a single subscription would make sense for status-go, since it makes dealing with multiple peers something simpler. Like @chaitanyaprem , i think this should go a layer above the actual filter protocol implementation, since that's 'raw' protocol usage, and it's code in this moment is somewhat complex, while a separate abstraction could help make its usage to be more palatable, while offering an API that would make sense for status-go. |
|
When testing this PR in status-mobile a PeerExchange problem was uncovered: status-im/status-mobile#18769 (comment). Debugging/troubleshooting notes here: status-im/status-mobile#18769 (comment). |
vitvly
force-pushed
the
fix/filter-subscribes-order
branch
3 times, most recently
from
b4694a2 to
1842d88
Compare
vitvly
force-pushed
the
fix/filter-subscribes-order
branch
from
1842d88 to
da31d82
Compare
There was a problem hiding this comment.
LGTM
Can be merged once waku-org/go-waku#1048 is ready and CI errors are addressed.
@vitvly the relevant go-waku PR has been merged. This also can be merged once CI errors are addressed. We can test Filter in CI for any issues. |
|
|
|
|
|
|
chaitanyaprem
force-pushed
the
fix/filter-subscribes-order
branch
from
fde4e11 to
c95256b
Compare
chaitanyaprem
force-pushed
the
fix/filter-subscribes-order
branch
from
c95256b to
ea0ed08
Compare
chaitanyaprem
force-pushed
the
fix/filter-subscribes-order
branch
from
03b8e4a to
e21dc8b
Compare
|
@richard-ramos I had noticed you haven't taken a look at the updated PR. Please review and add any comments. |
Co-authored-by: richΛrd <info@richardramos.me>
chaitanyaprem
force-pushed
the
fix/filter-subscribes-order
branch
from
fd41493 to
3ff4be0
Compare
|
Disabled pxClient for now as that is causing issues. This is being tracked as part of #5344 so that we can enable it again in lightClients. |
|
Merging this PR as it has passed mobile e2e tests and also manual test with some observations which should not be related to filter. Ref status-mobile PR : status-im/status-mobile#20369 |
Fixes #4659
After some looking on FilterManager, it appears that it's too complicated. Since the logic for setting specific peers has been removed in favour of automatic selection, i don't actually see any need to maintain any knowledge about particular peers on FilterManager level. Also, maintaining
MinPeersForFilterconnections for each filter is also out of status-go's purview, i think. Let go-waku handle that.There are some changes to be made in go-waku as well (will push separately):
IsSubscriptionAlivechecks have to be smart enough in order to not to issue too many requests to same peers (multiple subs can share identical peers)List of other changes to
FilterManager: