-
Notifications
You must be signed in to change notification settings - Fork 23
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
Timeout #251
Closed
Closed
Timeout #251
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
39241c0
feat: implement local ritual verificaiton
piotr-roslaniec e241bb7
test local verification
piotr-roslaniec c54c38f
apply pr suggestions
piotr-roslaniec 7f54da4
feat! add ritual initialization
piotr-roslaniec 2d1aa90
add feedback from live testing
piotr-roslaniec 49fa8ab
Add ritual initialization to client (#233)
piotr-roslaniec 1d53cae
Handle ritual initialization timing out
theref 35df81f
Remove double quotation
theref 6d7ae37
Add timeout buffer, catch timeout errors, check the ritual state, and…
theref 16903d7
Use endTime of ritual initiation for managing timeouts
theref d6bbedf
Address PR comments by refactoring timeout waiting
theref 311b755
Add tests for timeout
theref File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Timeout is the number of seconds counted since the ritual started, right? What happens if my transaction gets stuck (low gas etc.) but I'm still counting from when I send the transaction? Should we instead count the timeout from the "ritual start" event or some other cut-off?
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.
No,
timeout
is a variable on the Coordinator https://github.com/nucypher/nucypher-contracts/blob/da35b9d9f13ddebd7ce7bbbc3fbc7d5fb0d95411/contracts/contracts/coordination/Coordinator.sol#L61It's set at Coordinator level and applies to all rituals
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 I'm missing something here. Timeout is defined as
ritual.initTimestamp + timeout < block.timestamp
, and the initial value ofritual.initTimestamp
is taken fromblock.timestamp
. So it seems to me like thetimeout
denotes some number of seconds, and the timeout occurs after we reach some number of seconds after the ritual started.Do you see any edge cases here? If we use
setTimeout
, is it going to match exactly the calculation performed in ritual state checks? I.e.ritual.initTimestamp + timeout < block.timestamp
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.
setTimeout
is changing the config of Coordinator, so not sure that it helps us.But i see what your original comment implied now. When we do:
The fact that we
await
doesn't always mean that the transaction went through? and it could therefore be sitting around in the mempool whilst thePromise.race
starts counting down. Is that correct?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.
initializeRitual
is blocked until the ritual is started (and some event is emitted, etc.) so it may not be an issue after all.I was trying to give a concrete example of something that @manumonti already hinted on in his comment:
I didn't make a full analysis on what are the different edge cases we may have here, but one other that comes to mind is where we're awaiting for
timeout
and theCoordinator
admin changes the value oftimeout
. I think this case and some basket of other cases can be handled by replacingsetTimeout
bysetInterval
and by redoing the contract calculationritual.initTimestamp + timeout < block.timestamp
in JS. Or alternatively, querying the contract for the ritual state. The latter seems to be more robust.