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.

Sign up for GitHub

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

fix: block filters can be also recreated #85

Merged
merged 2 commits into from
Oct 30, 2024
Merged

fix: block filters can be also recreated #85

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bcf6d19
fix: block filters can be also recreated
AuHau Oct 24, 2024
569664e
refactor: rename filter to logFilter
AuHau Oct 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions ethers/providers/jsonrpc/subscriptions.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ type
# We need to keep around the filters that are used to create log filters on the RPC node
# as there might be a time when they need to be recreated as RPC node might prune/forget
# about them
filters: Table[JsonNode, EventFilter]
logFilters: Table[JsonNode, EventFilter]

# Used when filters are recreated to translate from the id that user
# originally got returned to new filter id
Expand All @@ -167,8 +167,15 @@ proc new*(_: type JsonRpcSubscriptions,
raise e
except CatchableError as e:
if "filter not found" in e.msg:
let filter = subscriptions.filters[originalId]
let newId = await subscriptions.client.eth_newFilter(filter)
var newId: JsonNode
# Log filters are stored in logFilters, block filters are not persisted
# there is they do not need any specific data for their recreation.
# We use this to determine if the filter was log or block filter here.
if subscriptions.logFilters.hasKey(originalId):
let filter = subscriptions.logFilters[originalId]
newId = await subscriptions.client.eth_newFilter(filter)
else:
Copy link
Contributor

@emizzle emizzle Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that works 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

there are no "block filters"

Might be worth updating the comment?

newId = await subscriptions.client.eth_newBlockFilter()
subscriptions.subscriptionMapping[originalId] = newId
return await getChanges(originalId)
else:
Expand Down Expand Up @@ -225,14 +232,14 @@ method subscribeLogs(subscriptions: PollingSubscriptions,

let id = await subscriptions.client.eth_newFilter(filter)
subscriptions.callbacks[id] = callback
subscriptions.filters[id] = filter
subscriptions.logFilters[id] = filter
subscriptions.subscriptionMapping[id] = id
return id

method unsubscribe*(subscriptions: PollingSubscriptions,
id: JsonNode)
{.async.} =
subscriptions.filters.del(id)
subscriptions.logFilters.del(id)
subscriptions.callbacks.del(id)
let sub = subscriptions.subscriptionMapping[id]
subscriptions.subscriptionMapping.del(id)
Expand Down
5 changes: 5 additions & 0 deletions testmodule/providers/jsonrpc/rpc_mock.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ proc start*(server: MockRpcHttpServer) =
server.filters.add filterId
return filterId

server.srv.router.rpc("eth_newBlockFilter") do() -> string:
let filterId = "0x" & (array[16, byte].example).toHex
server.filters.add filterId
return filterId

server.srv.router.rpc("eth_getFilterChanges") do(id: string) -> seq[string]:
if id notin server.filters:
raise (ref ApplicationError)(code: -32000, msg: "filter not found")
Expand Down
33 changes: 27 additions & 6 deletions testmodule/providers/jsonrpc/testJsonRpcSubscriptions.nim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,25 +124,25 @@ suite "HTTP polling subscriptions - filter not found":
await client.close()
await mockServer.stop()

test "filter not found error recreates filter":
test "filter not found error recreates log filter":
let filter = EventFilter(address: Address.example, topics: @[array[32, byte].example])
let emptyHandler = proc(log: Log) = discard

check subscriptions.filters.len == 0
check subscriptions.logFilters.len == 0
check subscriptions.subscriptionMapping.len == 0

let id = await subscriptions.subscribeLogs(filter, emptyHandler)

check subscriptions.filters[id] == filter
check subscriptions.logFilters[id] == filter
check subscriptions.subscriptionMapping[id] == id
check subscriptions.filters.len == 1
check subscriptions.logFilters.len == 1
check subscriptions.subscriptionMapping.len == 1

mockServer.invalidateFilter(id)

check eventually subscriptions.subscriptionMapping[id] != id

test "recreated filter can be still unsubscribed using the original id":
test "recreated log filter can be still unsubscribed using the original id":
let filter = EventFilter(address: Address.example, topics: @[array[32, byte].example])
let emptyHandler = proc(log: Log) = discard
let id = await subscriptions.subscribeLogs(filter, emptyHandler)
Expand All @@ -151,5 +151,26 @@ suite "HTTP polling subscriptions - filter not found":

await subscriptions.unsubscribe(id)

check not subscriptions.filters.hasKey id
check not subscriptions.logFilters.hasKey id
check not subscriptions.subscriptionMapping.hasKey id

test "filter not found error recreates block filter":
let emptyHandler = proc(blck: Block) = discard

check subscriptions.subscriptionMapping.len == 0
let id = await subscriptions.subscribeBlocks(emptyHandler)
check subscriptions.subscriptionMapping[id] == id

mockServer.invalidateFilter(id)

check eventually subscriptions.subscriptionMapping[id] != id

test "recreated block filter can be still unsubscribed using the original id":
let emptyHandler = proc(blck: Block) = discard
let id = await subscriptions.subscribeBlocks(emptyHandler)
mockServer.invalidateFilter(id)
check eventually subscriptions.subscriptionMapping[id] != id

await subscriptions.unsubscribe(id)

check not subscriptions.subscriptionMapping.hasKey id