fix: block filters can be also recreated #85
Conversation
| if subscriptions.filters.hasKey(originalId): | ||
| let filter = subscriptions.filters[originalId] | ||
| newId = await subscriptions.client.eth_newFilter(filter) | ||
| else: |
There was a problem hiding this comment.
This seems loose... maybe we could store an object in subscriptionMapping that has an id and kind, and then here we can check the kind before deciding what to do... wdyt?
There was a problem hiding this comment.
I was also thinking a bit about this "looseness", but I did not really find any reason why it should lead to bug.
If there is a callback (and hence, that is why this function is called), then the callback had to be created either through subscribeLogs, and hence it will have an entry in filters or through suscribeBlocks and then it will not have an entry in filters. IMHO this is good enough reasoning. If you think there will "magically disappear filters" then we have a problem and a bug somewhere anyway and will have to track that case down...
Soooo I would leave it as is.
There was a problem hiding this comment.
If you think there will "magically disappear filters" then we have a problem and a bug somewhere anyway and will have to track that case down...
It's more about if filters functionality is changed at some point in the future, then we may introduce a bug that isn't easy to spot/debug.
It's also not the most easy to reason about... As I was reading the change, i thought "why wouldn't filters also contain a block filter?" Then I had to look at how creating a block filter doesn't add to filters.
It seems a bit fragile honestly.
There was a problem hiding this comment.
Alright, so what about renaming filters to logFilters in order to better communicate it? Because that is what that really is. In the context of this Table, there are no "block filters", because block filters are only subscriptions for new blocks, not for "notify me when some specific blocks arrive" and hence it does not really make sense to store them in that Table...
There was a problem hiding this comment.
there are no "block filters"
Might be worth updating the comment?
AuHau
force-pushed
the
fix/filters-renewal-for-blocks
branch
from
353f736 to
1c2610a
Compare
AuHau
force-pushed
the
fix/filters-renewal-for-blocks
branch
from
26d3e6d to
c827403
Compare
AuHau
force-pushed
the
fix/filters-renewal-for-blocks
branch
from
c827403 to
569664e
Compare
Adds also support for the recreation of blocks filters.