Reject failed transactions in TransactionProcessor #22
Conversation
…ue to program error
rawfalafel
requested review from
philcchen and
odcheung
and removed request for
philcchen
| ); | ||
| let results: PromiseSettledResult<string>[] = []; | ||
| const confirmTx = async (txId: string) => { | ||
| const result = await this.connection.confirmTransaction({ |
There was a problem hiding this comment.
this PR is simpler than before, because web3.js itself now supports proper transaction confrimation
There was a problem hiding this comment.
for documentation - unseen in this change is our under the hood upgrade of web3.js.
relevant PR to upgrade confirmation logic is released on v1.42.0. We previously upgraded to 1.43.1
solana-labs/solana#25227
| blockhash, | ||
| }); | ||
|
|
||
| if (result.value.err) { |
There was a problem hiding this comment.
note that our transaction confirmation promise now rejects if the program fails (e.g. slippage failure). this is different from the previous behavior, where the promise did not reject, and we showed the "Transaction succeeded" snackbar
imo, this new implementation is not ideal. the ideal is that the function immediately returns the txid after sending the transaction but before confirming the transaction. this allows the UI to show a pending snackbar. afterwards, the txid should be used to separately confirm the transaction
however, this would require a deeper change in TransactionProcessor, which i think we can reserve for our future refactor
rawfalafel
changed the title
| // of the implications of the resigning - could be quite annoying from a user perspective | ||
| // if their wallet forces them to sign for each | ||
| results.push(await promiseToSettled(txPromise)); | ||
| const results = []; |
There was a problem hiding this comment.
the old implementation didn't actually send the transactions serially when parallel=false. it only confirmed the transactions serially
There was a problem hiding this comment.
patch lgtm.
It feels like with the web3 upgrade the responsibilities of TransactionProcessor is reduced to a promise handler for a group of transaction. Maybe we can drop this class and migrate it's responsibility to the future transaction packer down the road.
| ); | ||
| let results: PromiseSettledResult<string>[] = []; | ||
| const confirmTx = async (txId: string) => { | ||
| const result = await this.connection.confirmTransaction({ |
There was a problem hiding this comment.
for documentation - unseen in this change is our under the hood upgrade of web3.js.
relevant PR to upgrade confirmation logic is released on v1.42.0. We previously upgraded to 1.43.1
solana-labs/solana#25227
…)"" This reverts commit 3d008ce.
No description provided.