docs: make the comments on using the new confirmTransaction clearer #29251
Conversation
web3.js/src/connection.ts
Outdated
| @@ -3603,7 +3603,7 @@ export class Connection { | |||
| commitment?: Commitment, | |||
| ): Promise<RpcResponseAndContext<SignatureResult>>; | |||
|
|
|||
| /** @deprecated Instead, call `confirmTransaction` using a `TransactionConfirmationConfig` */ | |||
| /** @deprecated Instead, call `confirmTransaction` and pass in `BlockheightBasedTransactionConfirmationStrategy` or `DurableNonceTransactionConfirmationStrategy` */ | |||
There was a problem hiding this comment.
So, one thing that this introduces is the possibility that – when we add a third strategy – someone could forget to update this comment.
Does this work?
- Extract
BlockheightBasedTransactionConfirmationStrategy | DurableNonceTransactionConfirmationStrategyinto a union type calledTransactionConfirmationStrategy. - Refer to it here using
{@link TransactionConfirmationStrategy}
I think that should make the docs autolink to that type, where folks can see the up-to-date list of strategies.
There was a problem hiding this comment.
Hi @steveluscher thanks for the suggestion. I updated the PR. Hope this works. My comment describing TransactionConfirmationStrategy may need work. Thanks.
|
Thanks for the submission! |
mergify
bot
dismissed
steveluscher’s stale review
Pull request has been modified.
web3.js/src/connection.ts
Outdated
| commitment?: Commitment, | ||
| ): Promise<RpcResponseAndContext<SignatureResult>>; | ||
|
|
||
| /** @deprecated Instead, call `confirmTransaction` using a `TransactionConfirmationConfig` */ | ||
| /** @deprecated Instead, call `confirmTransaction` and pass in {@link TransactionConfirmationStrategy} @ */ |
There was a problem hiding this comment.
What does the trailing @ do here? Was that an accident?
| * @param {ConfirmOptions} [options] | ||
| * @returns {Promise<TransactionSignature>} | ||
| */ | ||
| export async function sendAndConfirmRawTransaction( | ||
| connection: Connection, | ||
| rawTransaction: Buffer, | ||
| confirmationStrategy: BlockheightBasedTransactionConfirmationStrategy, | ||
| confirmationStrategy: TransactionConfirmationStrategy, |
There was a problem hiding this comment.
Ooh, nice. You found a bug!
Codecov Report
@@ Coverage Diff @@
## master #29251 +/- ##
=========================================
+ Coverage 76.4% 76.5% +0.1%
=========================================
Files 54 54
Lines 3120 3120
Branches 468 468
=========================================
+ Hits 2385 2389 +4
+ Misses 571 567 -4
Partials 164 164 |
Problem
Connection.confirmTransaction using TransactionSignature is deprecated but the docs comment on using the new confirmTransaction wasn't clear
Summary of Changes
Clearer comment on using the new confirmTransaction method
Fixes #