From b68bea99092dd1220f95097ea290caeb0574b6cd Mon Sep 17 00:00:00 2001 From: Eric <5089238+emizzle@users.noreply.github.com> Date: Fri, 25 Oct 2024 14:58:45 +1100 Subject: [PATCH] fix: modify unsubscribe cleanup routine and tests (#84) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: modify unsubscribe cleanup routine Ignore exceptions (other than CancelledError) if uninstallation of the filter fails. If it's the last step in the subscription cleanup, then filter changes for this filter will no longer be polled so if the filter continues to live on in geth for whatever reason, then it doesn't matter. This includes a number of fixes: - `CancelledError` is now caught inside of `getChanges`. This was causing conditions during `subscriptions.close`, where the `CancelledError` would get consumed by the `except CatchableError`, if there was an ongoing `poll` happening at the time of close. - After creating a new filter inside of `getChanges`, the new filter is polled for changes before returning. - `getChanges` also does not swallow `CatchableError` by returning an empty array, and instead re-raises the error if it is not `filter not found`. - The tests were simplified by accessing the private fields of `PollingSubscriptions`. That way, there wasn't a race condition for the `newFilterId` counter inside of the mock. - The `MockRpcHttpServer` was simplified by keeping track of the active filters only, and invalidation simply removes the filter. The tests then only needed to rely on the fact that the filter id changed in the mapping. - Because of the above changes, we no longer needed to sleep inside of the tests, so the sleeps were removed, and the polling interval could be changed to 1ms, which not only makes the tests faster, but would further highlight any race conditions if present. * docs: rpc custom port documentation --------- Co-authored-by: Adam Uhlíř --- .gitignore | 1 + Readme.md | 3 + ethers/providers/jsonrpc/subscriptions.nim | 31 ++++++++-- testmodule/providers/jsonrpc/rpc_mock.nim | 22 ++++--- .../providers/jsonrpc/testJsonRpcProvider.nim | 4 +- .../providers/jsonrpc/testJsonRpcSigner.nim | 4 +- .../jsonrpc/testJsonRpcSubscriptions.nim | 57 +++++++++---------- testmodule/testContracts.nim | 4 +- testmodule/testCustomErrors.nim | 4 +- testmodule/testEnums.nim | 4 +- testmodule/testErc20.nim | 4 +- testmodule/testGasEstimation.nim | 4 +- testmodule/testReturns.nim | 4 +- testmodule/testTesting.nim | 4 +- testmodule/testWallet.nim | 4 +- 15 files changed, 98 insertions(+), 56 deletions(-) diff --git a/.gitignore b/.gitignore index 57dfc22..332b1f5 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ nimble.develop nimble.paths .idea .nimble +.envrc diff --git a/Readme.md b/Readme.md index 2581e2b..df34b0c 100644 --- a/Readme.md +++ b/Readme.md @@ -202,6 +202,9 @@ $ npm ci $ npm start ``` +If you need to use different port for the RPC node, then you can start with `npm start -- --port 1111` and +then run the tests with `ETHERS_TEST_PROVIDER=1111 nimble test`. + Thanks ------ diff --git a/ethers/providers/jsonrpc/subscriptions.nim b/ethers/providers/jsonrpc/subscriptions.nim index e2bc6cb..d9ea1dd 100644 --- a/ethers/providers/jsonrpc/subscriptions.nim +++ b/ethers/providers/jsonrpc/subscriptions.nim @@ -17,6 +17,7 @@ type methodHandlers: Table[string, MethodHandler] MethodHandler* = proc (j: JsonNode) {.gcsafe, raises: [].} SubscriptionCallback = proc(id, arguments: JsonNode) {.gcsafe, raises:[].} + SubscriptionError* = object of EthersError {.push raises:[].} @@ -130,7 +131,7 @@ method unsubscribe*(subscriptions: WebSocketSubscriptions, # Polling type - PollingSubscriptions = ref object of JsonRpcSubscriptions + PollingSubscriptions* = ref object of JsonRpcSubscriptions polling: Future[void] # We need to keep around the filters that are used to create log filters on the RPC node @@ -151,14 +152,24 @@ proc new*(_: type JsonRpcSubscriptions, proc getChanges(originalId: JsonNode): Future[JsonNode] {.async.} = try: let mappedId = subscriptions.subscriptionMapping[originalId] - return await subscriptions.client.eth_getFilterChanges(mappedId) + let changes = await subscriptions.client.eth_getFilterChanges(mappedId) + if changes.kind == JNull: + return newJArray() + elif changes.kind != JArray: + raise newException(SubscriptionError, + "HTTP polling: unexpected value returned from eth_getFilterChanges." & + " Expected: JArray, got: " & $changes.kind) + return changes + except CancelledError as e: + 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) subscriptions.subscriptionMapping[originalId] = newId - - return newJArray() + return await getChanges(originalId) + else: + raise e proc poll(id: JsonNode) {.async.} = for change in await getChanges(id): @@ -218,7 +229,17 @@ method subscribeLogs(subscriptions: PollingSubscriptions, method unsubscribe*(subscriptions: PollingSubscriptions, id: JsonNode) {.async.} = - discard await subscriptions.client.eth_uninstallFilter(subscriptions.subscriptionMapping[id]) subscriptions.filters.del(id) subscriptions.callbacks.del(id) + let sub = subscriptions.subscriptionMapping[id] subscriptions.subscriptionMapping.del(id) + try: + discard await subscriptions.client.eth_uninstallFilter(sub) + except CancelledError as e: + raise e + except CatchableError: + # Ignore if uninstallation of the filter fails. If it's the last step in our + # cleanup, then filter changes for this filter will no longer be polled so + # if the filter continues to live on in geth for whatever reason then it + # doesn't matter. + discard diff --git a/testmodule/providers/jsonrpc/rpc_mock.nim b/testmodule/providers/jsonrpc/rpc_mock.nim index d9cae74..5142a82 100644 --- a/testmodule/providers/jsonrpc/rpc_mock.nim +++ b/testmodule/providers/jsonrpc/rpc_mock.nim @@ -3,40 +3,39 @@ import ../../../ethers/provider import ../../../ethers/providers/jsonrpc/conversions import std/tables +import std/sequtils import pkg/stew/byteutils import pkg/json_rpc/rpcserver except `%`, `%*` import pkg/json_rpc/errors - type MockRpcHttpServer* = ref object - filters*: Table[string, bool] - newFilterCounter*: int + filters*: seq[string] srv: RpcHttpServer proc new*(_: type MockRpcHttpServer): MockRpcHttpServer = - MockRpcHttpServer(filters: initTable[string, bool](), newFilterCounter: 0, srv: newRpcHttpServer(["127.0.0.1:0"])) + let srv = newRpcHttpServer(["127.0.0.1:0"]) + MockRpcHttpServer(filters: @[], srv: srv) -proc invalidateFilter*(server: MockRpcHttpServer, id: string) = - server.filters[id] = false +proc invalidateFilter*(server: MockRpcHttpServer, jsonId: JsonNode) = + server.filters.keepItIf it != jsonId.getStr proc start*(server: MockRpcHttpServer) = server.srv.router.rpc("eth_newFilter") do(filter: EventFilter) -> string: let filterId = "0x" & (array[16, byte].example).toHex - server.filters[filterId] = true - server.newFilterCounter += 1 + server.filters.add filterId return filterId server.srv.router.rpc("eth_getFilterChanges") do(id: string) -> seq[string]: - if(not hasKey(server.filters, id) or not server.filters[id]): + if id notin server.filters: raise (ref ApplicationError)(code: -32000, msg: "filter not found") return @[] server.srv.router.rpc("eth_uninstallFilter") do(id: string) -> bool: - if(not hasKey(server.filters, id)): + if id notin server.filters: raise (ref ApplicationError)(code: -32000, msg: "filter not found") - server.filters.del(id) + server.invalidateFilter(%id) return true server.srv.start() @@ -45,6 +44,5 @@ proc stop*(server: MockRpcHttpServer) {.async.} = await server.srv.stop() await server.srv.closeWait() - proc localAddress*(server: MockRpcHttpServer): seq[TransportAddress] = return server.srv.localAddress() diff --git a/testmodule/providers/jsonrpc/testJsonRpcProvider.nim b/testmodule/providers/jsonrpc/testJsonRpcProvider.nim index 0a3620f..0f6e435 100644 --- a/testmodule/providers/jsonrpc/testJsonRpcProvider.nim +++ b/testmodule/providers/jsonrpc/testJsonRpcProvider.nim @@ -1,3 +1,4 @@ +import std/os import pkg/asynctest import pkg/chronos import pkg/ethers @@ -6,7 +7,8 @@ import pkg/stew/byteutils import ../../examples import ../../miner -for url in ["ws://localhost:8545", "http://localhost:8545"]: +let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") +for url in ["ws://" & providerUrl, "http://" & providerUrl]: suite "JsonRpcProvider (" & url & ")": diff --git a/testmodule/providers/jsonrpc/testJsonRpcSigner.nim b/testmodule/providers/jsonrpc/testJsonRpcSigner.nim index 34d78ea..e63a9a3 100644 --- a/testmodule/providers/jsonrpc/testJsonRpcSigner.nim +++ b/testmodule/providers/jsonrpc/testJsonRpcSigner.nim @@ -1,3 +1,4 @@ +import std/os import pkg/asynctest import pkg/ethers import pkg/stew/byteutils @@ -7,9 +8,10 @@ suite "JsonRpcSigner": var provider: JsonRpcProvider var accounts: seq[Address] + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new() + provider = JsonRpcProvider.new("http://" & providerUrl) accounts = await provider.listAccounts() teardown: diff --git a/testmodule/providers/jsonrpc/testJsonRpcSubscriptions.nim b/testmodule/providers/jsonrpc/testJsonRpcSubscriptions.nim index d26f8d8..d0e2fff 100644 --- a/testmodule/providers/jsonrpc/testJsonRpcSubscriptions.nim +++ b/testmodule/providers/jsonrpc/testJsonRpcSubscriptions.nim @@ -1,5 +1,7 @@ import std/json +import std/os import std/sequtils +import std/importutils import pkg/asynctest import pkg/serde import pkg/json_rpc/rpcclient @@ -68,7 +70,7 @@ suite "Web socket subscriptions": setup: client = newRpcWebSocketClient() - await client.connect("ws://localhost:8545") + await client.connect("ws://" & getEnv("ETHERS_TEST_PROVIDER", "localhost:8545")) subscriptions = JsonRpcSubscriptions.new(client) subscriptions.start() @@ -85,7 +87,7 @@ suite "HTTP polling subscriptions": setup: client = newRpcHttpClient() - await client.connect("http://localhost:8545") + await client.connect("http://" & getEnv("ETHERS_TEST_PROVIDER", "localhost:8545")) subscriptions = JsonRpcSubscriptions.new(client, pollingInterval = 100.millis) subscriptions.start() @@ -98,10 +100,12 @@ suite "HTTP polling subscriptions": suite "HTTP polling subscriptions - filter not found": - var subscriptions: JsonRpcSubscriptions + var subscriptions: PollingSubscriptions var client: RpcHttpClient var mockServer: MockRpcHttpServer + privateAccess(PollingSubscriptions) + setup: mockServer = MockRpcHttpServer.new() mockServer.start() @@ -109,8 +113,10 @@ suite "HTTP polling subscriptions - filter not found": client = newRpcHttpClient() await client.connect("http://" & $mockServer.localAddress()[0]) - subscriptions = JsonRpcSubscriptions.new(client, - pollingInterval = 15.millis) + subscriptions = PollingSubscriptions( + JsonRpcSubscriptions.new( + client, + pollingInterval = 1.millis)) subscriptions.start() teardown: @@ -122,35 +128,28 @@ suite "HTTP polling subscriptions - filter not found": let filter = EventFilter(address: Address.example, topics: @[array[32, byte].example]) let emptyHandler = proc(log: Log) = discard - check mockServer.newFilterCounter == 0 - let jsonId = await subscriptions.subscribeLogs(filter, emptyHandler) - let id = string.fromJson(jsonId).tryGet - check mockServer.newFilterCounter == 1 + check subscriptions.filters.len == 0 + check subscriptions.subscriptionMapping.len == 0 + + let id = await subscriptions.subscribeLogs(filter, emptyHandler) + + check subscriptions.filters[id] == filter + check subscriptions.subscriptionMapping[id] == id + check subscriptions.filters.len == 1 + check subscriptions.subscriptionMapping.len == 1 - await sleepAsync(50.millis) mockServer.invalidateFilter(id) - await sleepAsync(50.millis) - check mockServer.newFilterCounter == 2 + + check eventually subscriptions.subscriptionMapping[id] != id test "recreated 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 - - check mockServer.newFilterCounter == 0 - let jsonId = await subscriptions.subscribeLogs(filter, emptyHandler) - let id = string.fromJson(jsonId).tryGet - check mockServer.newFilterCounter == 1 - - await sleepAsync(50.millis) + let id = await subscriptions.subscribeLogs(filter, emptyHandler) mockServer.invalidateFilter(id) - check eventually mockServer.newFilterCounter == 2 - check mockServer.filters[id] == false - check mockServer.filters.len() == 2 - await subscriptions.unsubscribe(jsonId) - check mockServer.filters.len() == 1 + check eventually subscriptions.subscriptionMapping[id] != id - # invalidateFilter sets the filter's value to false which will return the "filter not found" - # unsubscribing will actually delete the key from filters table - # hence after unsubscribing the only key left in the table should be the original id - for key in mockServer.filters.keys(): - check key == id + await subscriptions.unsubscribe(id) + + check not subscriptions.filters.hasKey id + check not subscriptions.subscriptionMapping.hasKey id diff --git a/testmodule/testContracts.nim b/testmodule/testContracts.nim index 86079fa..ed2158a 100644 --- a/testmodule/testContracts.nim +++ b/testmodule/testContracts.nim @@ -1,4 +1,5 @@ import std/json +import std/os import std/options import pkg/asynctest import pkg/questionable @@ -16,7 +17,8 @@ type method mint(token: TestToken, holder: Address, amount: UInt256): Confirmable {.base, contract.} method myBalance(token: TestToken): UInt256 {.base, contract, view.} -for url in ["ws://localhost:8545", "http://localhost:8545"]: +let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") +for url in ["ws://" & providerUrl, "http://" & providerUrl]: suite "Contracts (" & url & ")": diff --git a/testmodule/testCustomErrors.nim b/testmodule/testCustomErrors.nim index 2347d7d..e03ed95 100644 --- a/testmodule/testCustomErrors.nim +++ b/testmodule/testCustomErrors.nim @@ -1,3 +1,4 @@ +import std/os import std/json import pkg/asynctest import pkg/ethers @@ -22,9 +23,10 @@ suite "Contract custom errors": var contract: TestCustomErrors var provider: JsonRpcProvider var snapshot: JsonNode + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new() + provider = JsonRpcProvider.new("http://" & providerUrl) snapshot = await provider.send("evm_snapshot") let deployment = readDeployment() let address = !deployment.address(TestCustomErrors) diff --git a/testmodule/testEnums.nim b/testmodule/testEnums.nim index 3686b93..50da630 100644 --- a/testmodule/testEnums.nim +++ b/testmodule/testEnums.nim @@ -1,3 +1,4 @@ +import std/os import pkg/asynctest import pkg/ethers import pkg/serde @@ -14,9 +15,10 @@ suite "Contract enum parameters and return values": var contract: TestEnums var provider: JsonRpcProvider var snapshot: JsonNode + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new() + provider = JsonRpcProvider.new("http://" & providerUrl) snapshot = await provider.send("evm_snapshot") let deployment = readDeployment() contract = TestEnums.new(!deployment.address(TestEnums), provider) diff --git a/testmodule/testErc20.nim b/testmodule/testErc20.nim index 7f597f6..2eea2f7 100644 --- a/testmodule/testErc20.nim +++ b/testmodule/testErc20.nim @@ -1,3 +1,4 @@ +import std/os import std/json import pkg/asynctest import pkg/questionable @@ -11,7 +12,8 @@ type method mint(token: TestToken, holder: Address, amount: UInt256): Confirmable {.base, contract.} -for url in ["ws://localhost:8545", "http://localhost:8545"]: +let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") +for url in ["ws://" & providerUrl, "http://" & providerUrl]: suite "ERC20 (" & url & ")": diff --git a/testmodule/testGasEstimation.nim b/testmodule/testGasEstimation.nim index 5ee69c3..7c5001e 100644 --- a/testmodule/testGasEstimation.nim +++ b/testmodule/testGasEstimation.nim @@ -1,3 +1,4 @@ +import std/os import pkg/asynctest import pkg/ethers import pkg/serde @@ -14,9 +15,10 @@ suite "gas estimation": var contract: TestGasEstimation var provider: JsonRpcProvider var snapshot: JsonNode + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new() + provider = JsonRpcProvider.new("http://" & providerUrl) snapshot = await provider.send("evm_snapshot") let deployment = readDeployment() let signer = provider.getSigner() diff --git a/testmodule/testReturns.nim b/testmodule/testReturns.nim index 2893f33..e4bab94 100644 --- a/testmodule/testReturns.nim +++ b/testmodule/testReturns.nim @@ -1,3 +1,4 @@ +import std/os import pkg/asynctest import pkg/ethers import pkg/serde @@ -13,9 +14,10 @@ suite "Contract return values": var contract: TestReturns var provider: JsonRpcProvider var snapshot: JsonNode + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new() + provider = JsonRpcProvider.new("http://" & providerUrl) snapshot = await provider.send("evm_snapshot") let deployment = readDeployment() contract = TestReturns.new(!deployment.address(TestReturns), provider) diff --git a/testmodule/testTesting.nim b/testmodule/testTesting.nim index b5d80bd..30d95e9 100644 --- a/testmodule/testTesting.nim +++ b/testmodule/testTesting.nim @@ -1,3 +1,4 @@ +import std/os import std/strformat import pkg/asynctest import pkg/chronos @@ -92,9 +93,10 @@ suite "Testing helpers - contracts": var snapshot: JsonNode var accounts: seq[Address] let revertReason = "revert reason" + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new("ws://127.0.0.1:8545") + provider = JsonRpcProvider.new("ws://" & providerUrl) snapshot = await provider.send("evm_snapshot") accounts = await provider.listAccounts() helpersContract = TestHelpers.new(provider.getSigner()) diff --git a/testmodule/testWallet.nim b/testmodule/testWallet.nim index a3cdf2d..271cdd9 100644 --- a/testmodule/testWallet.nim +++ b/testmodule/testWallet.nim @@ -1,3 +1,4 @@ +import std/os import pkg/asynctest import pkg/serde import pkg/stew/byteutils @@ -12,9 +13,10 @@ proc transfer*(erc20: Erc20, recipient: Address, amount: UInt256) {.contract.} suite "Wallet": var provider: JsonRpcProvider var snapshot: JsonNode + let providerUrl = getEnv("ETHERS_TEST_PROVIDER", "localhost:8545") setup: - provider = JsonRpcProvider.new() + provider = JsonRpcProvider.new("http://" & providerUrl) snapshot = await provider.send("evm_snapshot") teardown: