-
Notifications
You must be signed in to change notification settings - Fork 54
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.
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
chore(rln-relay): remove websocket from OnchainGroupManager #2364
Conversation
You can find the image built from this PR at
Built from d9e245b |
751dcb6
to
9aacfa1
Compare
proc getAndHandleEvents(g: OnchainGroupManager, | ||
fromBlock: BlockNumber, | ||
toBlock: BlockNumber): Future[void] {.async: (raises: [Exception]).} = | ||
toBlock: BlockNumber): Future[bool] {.async: (raises: [Exception]).} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this proc either returns true or raises an exception.
Am I missing something or is it supposed to be like that? If so, what's the point of returning bool
if the return value is always true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's for compatibility with the retryWrapper template, it expects a return value to assign the result of an async operation, just set to true here since we don't use it (discarded)
This PR may contain changes to configuration options of one of the apps. If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed. Please also make sure the label |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks so much! 🤩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm thanks! minor comment regarding DefaultBlockPollRate
|
||
const DefaultKeyStorePath* = "rlnKeystore.json" | ||
const DefaultKeyStorePassword* = "password" | ||
|
||
const DefaultBlockPollRate* = 1.seconds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this is too often? Would suggest 6s (half-block time). Thinking here of people that may be using freemium providers. Once per second is 86.400 requests a day (and that just for knowing the current block).
On the other hand, since we consider as valid multiple roots, we could relax this further (thinking of >30sec). But lets leave this by now, as it may open up some edge cases. 6s should be ok by now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed in dbd6cc3
e01e883
to
dbd6cc3
Compare
Description
Removes dependence on eth_subscribe since websocket connections can be flaky.
Changes
1
second. default can be changedHow to test
make -j16 test
insert your https rpc provider as well as private key in the required positions
Issue
addresses part of #2345