Convert confirm and eth_filter to async #394
Conversation
src/confirm.rs
Outdated
| // TODO: The stream should have additional checks: | ||
| // * We should not continue calling next on a stream that has completed (has returned None). We expect this to never | ||
| // happen for the blocks filter but to be safe we should handle this case for example by `fuse`ing the stream or | ||
| // erroring when it does complete. | ||
| // * We do not handle the case where the stream returns an error which means we are wrongly counting it as a | ||
| // confirmation. | ||
| let filter_stream = filter.stream(poll_interval).skip(confirmations); | ||
| futures::pin_mut!(filter_stream); | ||
| loop { | ||
| let _ = filter_stream.next().await; |
There was a problem hiding this comment.
I presume this behaviour was already broken before the refactor, right? Can we please log this as a separate issue and add an issue number to the TODO above?
There was a problem hiding this comment.
Yes, this PR doesn't change the behavior (bug was already there). I don't want to fix it in this PR. Added issue link.
src/confirm.rs
Outdated
| // TODO: We should remove this `expect`. No matter what happens inside the node, this shouldn't be a panic. | ||
| // Should convert this into an some error. InternalError probably makes sense but then we cannot provide the error | ||
| // message. Maybe make new error for this? Or allow InternalError to carry a message. | ||
| let receipt = eth | ||
| .transaction_receipt(hash) | ||
| .await? | ||
| .expect("receipt can't be null after wait for confirmations; qed"); |
There was a problem hiding this comment.
Maybe let's put the responsibility to unwrap on the user? I.e. returning Option<TransactionReceipt> here?
There was a problem hiding this comment.
Like above the issue was already there and I don't want to fix it in this PR. Added issue link.
e00E
force-pushed
the
confirm-async
branch
2 times, most recently
from
ec29987
to
cc3fd3b
Compare
src/confirm.rs
Outdated
| // TODO: The stream should have additional checks: | ||
| // * We should not continue calling next on a stream that has completed (has returned None). We expect this to never | ||
| // happen for the blocks filter but to be safe we should handle this case for example by `fuse`ing the stream or | ||
| // erroring when it does complete. | ||
| // * We do not handle the case where the stream returns an error which means we are wrongly counting it as a | ||
| // confirmation. | ||
| let filter_stream = filter.stream(poll_interval).skip(confirmations); | ||
| futures::pin_mut!(filter_stream); | ||
| loop { | ||
| let _ = filter_stream.next().await; |
There was a problem hiding this comment.
I presume this behaviour was already broken before the refactor, right? Can we please log this as a separate issue and add an issue number to the TODO above?
src/confirm.rs
Outdated
| // TODO: We should remove this `expect`. No matter what happens inside the node, this shouldn't be a panic. | ||
| // Should convert this into an some error. InternalError probably makes sense but then we cannot provide the error | ||
| // message. Maybe make new error for this? Or allow InternalError to carry a message. | ||
| let receipt = eth | ||
| .transaction_receipt(hash) | ||
| .await? | ||
| .expect("receipt can't be null after wait for confirmations; qed"); |
There was a problem hiding this comment.
Maybe let's put the responsibility to unwrap on the user? I.e. returning Option<TransactionReceipt> here?
It would be good if someone could double check that I did not accidentally introduce any logic changes when converting the state machine manual future implementations to async.
I've marked some problems that I noticed in the existing code while converting it. This PR doesn't fix them but I put a comment there to mark them.
Fixes #393