Confirmations #105
Confirmations #105
Conversation
nlordell
force-pushed
the
confirmations
branch
from
4423bc3 to
c537a51
Compare
src/contract/deploy.rs
Outdated
| /// specified will use a period of 7 seconds. | ||
| pub fn poll_interval(mut self, value: Duration) -> DeployBuilder<T, D> { | ||
| self.poll_interval = Some(value); | ||
| self.tx = self.tx.nonce(value); |
There was a problem hiding this comment.
All these copy pasta errors that are being fixed here 🙈
There was a problem hiding this comment.
This was addressed in #108, it’s just the github GUI showing it.
src/transaction.rs
Outdated
|
|
||
| /// Represents the result of a sent transaction that can either be a transaction | ||
| /// hash, in the case the transaction was not confirmed, or a full transaction | ||
| /// receipt is the `TransactionBuilder` was configured to wait for confirmation |
| Pending, | ||
| /// Wait for confirmation with the specified `ConfirmParams`. A confirmed | ||
| /// transaction is always mined. There is a chance, however, that the block | ||
| /// in which the transaction was mined becomes an ommer block. Confirming |
There was a problem hiding this comment.
uncle block? Unless you feel strongly about trading general comprehension for gender neutrality.
There was a problem hiding this comment.
Isn’t the official term “ommer block”? It was originally chosen for gender neutrality reasons, but I think it is pretty widely understood now.
There was a problem hiding this comment.
I have never heard the term ommer block before.
src/transaction/confirm.rs
Outdated
| #[cfg(test)] | ||
| pub const DEFAULT_POLL_INTERVAL: Duration = Duration::from_secs(0); | ||
|
|
||
| /// The default poll interval to use for confirming transactions. |
There was a problem hiding this comment.
Copy Pasta, is this number of blocks or seconds? From reading below I found its blocks. Maybe rename to DEFAULT_TIMEOUT_IN_BLOCK? A block timeout could be interpreted as the time it takes to get a new block.
There was a problem hiding this comment.
It is a copy pasta, should already be fixed in confirmation future PR
src/transaction/confirm.rs
Outdated
| /// the node (for example when using Infura over HTTP(S)). | ||
| pub poll_interval: Duration, | ||
| /// The maximum number of blocks to wait for a transaction to get confirmed. | ||
| pub block_timeout: usize, |
There was a problem hiding this comment.
Does it make sense for this to be an option? Given that the tx will be in the mem pool you might not want to stop waiting for it (I realize you could use a very high value here).
There was a problem hiding this comment.
We can do that, it’s not too hard.
| fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> { | ||
| let unpinned = self.get_mut(); | ||
| loop { | ||
| unpinned.state = match &mut unpinned.state { |
There was a problem hiding this comment.
nice state machine. Is this a common pattern in rust? I was wondering if it made sense for the SendFuture (as the flow is only linear) but here it makes a lot of sense with all the jumps.
There was a problem hiding this comment.
It is a common pattern for futures and is how the generated async code by the compiler looks like in theory.
| if block_num == *target_block_num { | ||
| ConfirmState::Checking(future::try_join( | ||
| MaybeReady::ready(Ok(block_num)), | ||
| unpinned |
There was a problem hiding this comment.
This is where you were wondering if you can reuse the old tx hash once it's mined or if you have to always refetch it?
| } | ||
|
|
||
| #[test] | ||
| fn confirmations_with_reorg_tx_receipt() { |
This PR unifies the confirmation API between all transaction (deployment and method calls) and eliminates the
send_and_confirmmethod as now confirmation parameters are part of the transaction builder.Note that this also addresses some issues with confirmation. A new case was created to upstream some of these issues to
web3crate here: #104. Details of the issues with the current confirmation implementation are in the aforementioned issue.This closes #95
Test Plan
New unit tests for the confirmation implementation and add waiting for confirmation with Ganache example. (Note that Rinkeby example currently waits for confirmation already).