chore(marketplace): switch to websocket #1166
Conversation
I am still exploring the issue, I am not sure how to reproduce it. |
tests/ethertest.nim
Outdated
| proc resubscribeOnTimeout() {.async.} = | ||
| while true: | ||
| await sleepAsync(5.int64.minutes) | ||
| #await ethProvider.resubscribeAll() |
There was a problem hiding this comment.
@emizzle What do you think about this approach ? I would add a resubscribeAll to JsonRpcProvider in nim-ethers.
There was a problem hiding this comment.
I don't mind the idea, though I'm not convinced we should put this functionality in ethers, as it is a client-specific (hardhat) workaround. However, i'm not entirely sure we'll have a choice.
Also, we are going to run into issues when we cancel all subscriptions and then resubscribe: there will have been blocks mined in between that potentially have events in the logs and we may not have subscribed in time to call the respective subscription callbacks. I believe we discussed this in the past and we reasoned that we might need to inspect the logs for the blocks that had been mined between unsubscribing and re-subscribing. Maybe we could get away with simply re-subscribing only (not unsubscribing), then deduplicating subscription callbacks somehow?
There was a problem hiding this comment.
I don't mind the idea, though I'm not convinced we should put this functionality in ethers, as it is a client-specific (hardhat) workaround. However, i'm not entirely sure we'll have a choice.
Yes I wanted to keep the workaround out of ethers but it is hard. Let's say I move the resubscribeAll into the client, I would need to expose some fields from ethers: the subscriptions from JsonRpcProvider, the callbacks from JsonRpcSubscriptions and potentially adding and exposing a mapping between the JsonNode and the EventFilter. I feel it safer to add a method resubscribeAll to ethers rather than exposing those fields.
Also, we are going to run into issues when we cancel all subscriptions and then resubscribe: there will have been blocks mined in between that potentially have events in the logs and we may not have subscribed in time to call the respective subscription callbacks. I believe we discussed this in the past and we reasoned that we might need to inspect the logs for the blocks that had been mined between unsubscribing and re-subscribing. Maybe we could get away with simply re-subscribing only (not unsubscribing), then deduplicating subscription callbacks somehow?
We could just re-subscribe first then remove the old subscription. deduplicating subscription callbacks seems simple, we just need to delete the callback for the JsonNode corresponding to that subscription.
There was a problem hiding this comment.
I found another approach to keep the resubscribe in the client, but it requires one change in ethers: move the logEvents (currently in PollingSubscriptions) to JsonRpcSubscriptions. Thanks to that, we can keep track of the events per subscription in WebSocketSubscriptions as well (see here).
Then, in the client, I access the private fields of the JsonRpcSubscriptions and register first a new subscription and then remove the old one so we don't lose any block mined event.
2-towns
force-pushed
the
chore/re-enable-hardhat-websocket
branch
from
89b8fdc to
b6e6596
Compare
tests/helpers.nim
Outdated
| # from pkg/json_rpc/rpcclient import RpcClient | ||
| # from pkg/ethers import JsonRpcProvider, JsonRpcSubscriptions |
There was a problem hiding this comment.
Remove commented out imports please ;-)
|
Actually, I have (I think 😅) a better idea - what about adding this functionality (automatically resubscriptions) directly to the nim-ethers under compile flag? No hacky private property access needed. It will be cleanly turned on only when the nim-ether's user needs... IMHO better then this. |
Linea is supporting only Websocket.
So it would be better to use it in our testing.
Depends on codex-storage/nim-ethers#112.