On transaction failure, fetch revert reason with replayed transaction #57
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AuHau
reviewed
Oct 18, 2023
There was a problem hiding this comment.
It does look LGTM, but honestly I don't have that good understanding here so I would leave approval to @markspanbroek
emizzle
force-pushed
the
feat/include-nonce-in-error
branch
2 times, most recently
from
822dc67
to
28c3c82
Compare
emizzle
force-pushed
the
feat/include-nonce-in-error
branch
from
28c3c82
to
6f41cbc
Compare
|
Ready for another review 👍 |
|
Note: nim-chronicles was added in this PR (originally was in #55), with some discussion around not including it due to it not being versioned. It does, however appear to be versioned, but I'm not entirely sure if this is enough. The other option, which would suit a future PR, is to add chronicles as an optional dependency. |
markspanbroek
approved these changes
Oct 23, 2023
There was a problem hiding this comment.
Looks great @emizzle! This is going to help us a lot in debugging transactions.
ethers.nimble
Outdated
| @@ -4,6 +4,7 @@ description = "library for interacting with Ethereum" | |||
| license = "MIT" | |||
|
|
|||
| requires "nim >= 1.6.0" | |||
| requires "chronicles >= 0.10.3 & < 0.10.4" | |||
There was a problem hiding this comment.
I would be a little less strict in the patch version; allowing all 0.10.x is probably ok.
Suggested change
| requires "chronicles >= 0.10.3 & < 0.10.4" | |
| requires "chronicles >= 0.10.3 & < 0.11.0" |
ethers/testing.nim
Outdated
| @@ -1,4 +1,5 @@ | |||
| import std/strutils | |||
| import ./contract | |||
testmodule/testContracts.nim
Outdated
| @@ -5,6 +5,7 @@ import pkg/questionable | |||
| import pkg/stint | |||
| import pkg/ethers | |||
| import pkg/ethers/erc20 | |||
| import pkg/ethers/wallet | |||
emizzle
force-pushed
the
feat/include-nonce-in-error
branch
from
549a2e7
to
efc8ec0
Compare
Base automatically changed from
feat/cancel-tx-on-estimateGas-failure
to
main
October 24, 2023 23:42
…t estimateGas failure
- remove auto-cancellation of failed transaction (failed during estimate gas) to prevent stuck txs - replace it with an async lock during nonce sequencing + gas estimation - simplified cancelTransaction (still exported) such that the new transaction is populated using populateTransaction, so that all gas and fees are reset - moved reverting contract function into its own testing helpers module, and refactored any tests to use it - updated the test helper reverts to check EstimateGasErrors
This would allow applications to use the nonce in case of an error, eg cancel the transaction to prevent stuck txs from occurring
eth_getTransactionByHash responses from geth don't include data, and there was an exception raised, "key not found: data"
…r Transaction object
Add transactionType and chainId to PastTransaction, so they can be sent along with replayed txs
somehow this didn't break the nim-ethers tests?
- remove auto cancellation of failed tx (during estimate gas) - add asynclock to populate transaction, to ensure nonce sequence (signer level) - move checking of receipt status and obtaining revert reason from contract level to provider level
emizzle
force-pushed
the
feat/include-nonce-in-error
branch
from
efc8ec0
to
8c363f2
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When calling
.sendTransaction, the flow is:estimateGasIn step 1, if there is an error during gas estimation, an
EstimateGasErroris raised with the revert reason.In step 2, if the transaction receipt is returned with a status of failed (requires the transaction to be confirmed), the transaction is replayed at the previous block (the full state transitions of the previous block are including, and the state transitions from earlier txs of the same block are not included), and a revert reason is obtained from the replay. A
ProviderErroris raised with the revert reason.The replay flow is:
PastTransactiontypePastTransactionto aTransactioneth_sendTransaction, and providing the transaction object and a block number.NOTE: