-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
feat: a nonce-based transaction confirmation strategy for web3.js #25839
feat: a nonce-based transaction confirmation strategy for web3.js #25839
Conversation
Codecov Report
@@ Coverage Diff @@
## master #25839 +/- ##
=========================================
- Coverage 76.8% 76.5% -0.3%
=========================================
Files 55 55
Lines 2986 3084 +98
Branches 426 454 +28
=========================================
+ Hits 2294 2362 +68
- Misses 544 564 +20
- Partials 148 158 +10 |
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.
Can you PR the non-confirmation changes separately so that we can get those in more quickly and break up this large PR?
web3.js/src/connection.ts
Outdated
while ( | ||
true // eslint-disable-line no-constant-condition | ||
) { |
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.
Could this be refactored to while (!done)
and then each loop we either sleep or get the current nonce value? That way we don't need so many done
checks.
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.
There’s a done
check after each async operation, because it’s at that point we need to check to make sure a confirmation didn’t come in while we were waiting for the async op to finish.
For instance, we want to check for done after the sleep, so that we don’t waste time on another nonce fetch for no reason. Putting the done check at the loop level would make this impossible.
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.
Putting the done check at the loop level would make this impossible
My suggestion was to make the loop only run one async operation per loop so that done is still called immediately after. So it's possible, but I suppose it's less obvious so this isn't a big deal.
let readyToPoll = false;
let currentBlockHeight = await checkBlockHeight();
while (!done && currentBlockHeight <= lastValidBlockHeight) {
if (readyToPoll) {
currentBlockHeight = await checkBlockHeight();
} else {
await sleep(1000);
}
}
if (!done) {
resolve({ __type: TransactionStatus.BLOCKHEIGHT_EXCEEDED });
}
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’ll massage this further when I get back to a computer!
Updated!
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Apologies for dropping off on the review of this one. The main points to address are:
|
… nonce was discovered to have advanced
Alright; rebased and shipping this, at long last. If it ends up sucking we can revert it. If we find there are improvements to make, we can follow up with those improvements! |
…lana-labs#25839) * feat: you can now construct a `Transaction` with durable nonce information * chore: refactor confirmation logic so that each strategy gets its own method * feat: `geNonce` now accepts a `minContextSlot` param * feat: a nonce-based transaction confirmation strategy * feat: add nonce confirmation strategy to send-and-confirm helpers * fix: nits from July 8 review * Use Typescript narrowing to determine which strategy to use * Double check the signature confirmation against the slot in which the nonce was discovered to have advanced
…lana-labs#25839) * feat: you can now construct a `Transaction` with durable nonce information * chore: refactor confirmation logic so that each strategy gets its own method * feat: `geNonce` now accepts a `minContextSlot` param * feat: a nonce-based transaction confirmation strategy * feat: add nonce confirmation strategy to send-and-confirm helpers * fix: nits from July 8 review * Use Typescript narrowing to determine which strategy to use * Double check the signature confirmation against the slot in which the nonce was discovered to have advanced
Usage
Closes #25303.