From f16ec78a6186f38a455e3f331e9ea5bf7475f254 Mon Sep 17 00:00:00 2001 From: Eric <5089238+emizzle@users.noreply.github.com> Date: Fri, 20 Oct 2023 17:14:12 +1100 Subject: [PATCH] PR feedback: remove auto-cancellation of failed transactions - 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 --- ethers/contract.nim | 36 +----------- ethers/providers/jsonrpc.nim | 9 +-- ethers/signer.nim | 111 +++++++++++++++++++++-------------- ethers/testing.nim | 22 ++++--- testmodule/helpers.nim | 14 +++++ testmodule/testContracts.nim | 35 +++++------ testmodule/testTesting.nim | 67 +++++++++++++-------- 7 files changed, 158 insertions(+), 136 deletions(-) create mode 100644 testmodule/helpers.nim diff --git a/ethers/contract.nim b/ethers/contract.nim index 851af30..20704ac 100644 --- a/ethers/contract.nim +++ b/ethers/contract.nim @@ -3,6 +3,7 @@ import std/macros import std/sequtils import pkg/chronos import pkg/contractabi +import pkg/stew/byteutils import ./basics import ./provider import ./signer @@ -117,38 +118,6 @@ proc call(contract: Contract, let response = await contract.provider.call(transaction, overrides) return decodeResponse(ReturnType, returnMultiple, response) -proc populateTransaction( - contract: Contract, - tx: Transaction -): Future[Transaction] {.async.} = - - if signer =? contract.signer: - try: - return await signer.populateTransaction(tx) - except EstimateGasError as e: - if nonce =? e.transaction.nonce: - - if lastSeenNonce =? signer.lastSeenNonce and - lastSeenNonce > nonce: - discard await signer.cancelTransaction(e.transaction) - let revertReason = if not e.parent.isNil: e.parent.msg - else: "unknown" - info "A cancellation transaction has been sent to prevent stuck " & - "transactions", - nonce = e.transaction.nonce, - revertReason - - # nonce wasn't incremented by another transaction, so force update the - # lastSeenNonce - else: - signer.updateNonce(nonce - 1, force = true) - trace "nonce decremented -- estimateGas failed but no further " & - "nonces were generated. Prevents stuck txs.", - failedNonce = nonce, - newNonce = nonce - 1 - - raiseContractError e.msgStack - proc send(contract: Contract, function: string, parameters: tuple, @@ -156,7 +125,7 @@ proc send(contract: Contract, Future[?TransactionResponse] {.async.} = if signer =? contract.signer: let transaction = createTransaction(contract, function, parameters, overrides) - let populated = await contract.populateTransaction(transaction) + let populated = await signer.populateTransaction(transaction) let txResp = await signer.sendTransaction(populated) return txResp.some else: @@ -279,7 +248,6 @@ proc confirm*(tx: Future[?TransactionResponse], ## `await token.connect(signer0) ## .mint(accounts[1], 100.u256) ## .confirm(3)` - without response =? (await tx): raise newException( EthersError, diff --git a/ethers/providers/jsonrpc.nim b/ethers/providers/jsonrpc.nim index ba30925..315ae2e 100644 --- a/ethers/providers/jsonrpc.nim +++ b/ethers/providers/jsonrpc.nim @@ -3,6 +3,7 @@ import std/tables import std/uri import pkg/json_rpc/rpcclient import pkg/json_rpc/errors +import pkg/stew/byteutils import ../basics import ../provider import ../signer @@ -28,7 +29,7 @@ type subscriptions: JsonRpcSubscriptions id: JsonNode -proc raiseProviderError(message: string) {.upraises: [JsonRpcProviderError].} = +proc raiseJsonRpcProviderError(message: string) {.upraises: [JsonRpcProviderError].} = var message = message try: message = parseJson(message){"message"}.getStr @@ -40,11 +41,11 @@ template convertError(body) = try: body except JsonRpcError as error: - raiseProviderError(error.msg) + raiseJsonRpcProviderError(error.msg) # Catch all ValueErrors for now, at least until JsonRpcError is actually # raised. PR created: https://github.com/status-im/nim-json-rpc/pull/151 except ValueError as error: - raiseProviderError(error.msg) + raiseJsonRpcProviderError(error.msg) # Provider @@ -228,7 +229,7 @@ method getAddress*(signer: JsonRpcSigner): Future[Address] {.async.} = if accounts.len > 0: return accounts[0] - raiseProviderError "no address found" + raiseJsonRpcProviderError "no address found" method signMessage*(signer: JsonRpcSigner, message: seq[byte]): Future[seq[byte]] {.async.} = diff --git a/ethers/signer.nim b/ethers/signer.nim index f0d7916..b19eddc 100644 --- a/ethers/signer.nim +++ b/ethers/signer.nim @@ -11,6 +11,7 @@ logScope: type Signer* = ref object of RootObj lastSeenNonce: ?UInt256 + populateLock: AsyncLock type SignerError* = object of EthersError @@ -20,6 +21,16 @@ type template raiseSignerError(message: string, parent: ref ProviderError = nil) = raise newException(SignerError, message, parent) +proc raiseEstimateGasError( + transaction: Transaction, + parent: ref ProviderError = nil +) = + let e = (ref EstimateGasError)( + msg: "Estimate gas failed", + transaction: transaction, + parent: parent) + raise e + method provider*(signer: Signer): Provider {.base, gcsafe.} = doAssert false, "not implemented" @@ -47,14 +58,17 @@ method estimateGas*(signer: Signer, transaction: Transaction): Future[UInt256] {.base, async.} = var transaction = transaction transaction.sender = some(await signer.getAddress) - return await signer.provider.estimateGas(transaction) + try: + return await signer.provider.estimateGas(transaction) + except ProviderError as e: + raiseEstimateGasError transaction, e method getChainId*(signer: Signer): Future[UInt256] {.base, gcsafe.} = signer.provider.getChainId() func lastSeenNonce*(signer: Signer): ?UInt256 = signer.lastSeenNonce -method getNonce*(signer: Signer): Future[UInt256] {.base, gcsafe, async.} = +method getNonce(signer: Signer): Future[UInt256] {.base, gcsafe, async.} = var nonce = await signer.getTransactionCount(BlockTag.pending) if lastSeen =? signer.lastSeenNonce and lastSeen >= nonce: @@ -65,48 +79,48 @@ method getNonce*(signer: Signer): Future[UInt256] {.base, gcsafe, async.} = method updateNonce*( signer: Signer, - nonce: UInt256, - force = false + nonce: UInt256 ) {.base, gcsafe.} = - ## When true, force updates nonce without first checking if it is higher than - ## the last seen nonce. NOTE: This should ONLY be used when absolutely needed, - ## eg when estimateGas fails, but no other nonces have been generated between - ## the estimateGas and updateNonce calls without lastSeen =? signer.lastSeenNonce: signer.lastSeenNonce = some nonce return - if force or nonce > lastSeen: + if nonce > lastSeen: signer.lastSeenNonce = some nonce -method cancelTransaction*( +method decreaseNonce*(signer: Signer) {.base, gcsafe.} = + if lastSeen =? signer.lastSeenNonce and lastSeen > 0: + signer.lastSeenNonce = some lastSeen - 1 + +proc ensureNonceSequence( signer: Signer, tx: Transaction -): Future[TransactionResponse] {.async, base.} = - # cancels a transaction by sending with a 0-valued transaction to ourselves - # with the failed tx's nonce +): Future[Transaction] {.async.} = + ## Ensures that once the nonce is incremented, if estimate gas fails, the + ## nonce is decremented. Disallows concurrent calls by use of AsyncLock. + ## Immediately returns if tx already has a nonce or gasLimit. - without sender =? tx.sender: - raiseSignerError "transaction must have sender" - if sender != await signer.getAddress(): - raiseSignerError "can only cancel a tx this signer has sent" - without nonce =? tx.nonce: - raiseSignerError "transaction must have nonce" + if not (tx.nonce.isNone and tx.gasLimit.isNone): + return tx + + var populated = tx + if signer.populateLock.isNil: + signer.populateLock = newAsyncLock() + + await signer.populateLock.acquire() - var cancelTx = tx - cancelTx.to = sender - cancelTx.value = 0.u256 - cancelTx.nonce = some nonce try: - cancelTx.gasLimit = some(await signer.estimateGas(cancelTx)) - except ProviderError: - warn "failed to estimate gas for cancellation tx, sending anyway", - tx = $cancelTx - discard + populated.nonce = some(await signer.getNonce()) + populated.gasLimit = some(await signer.estimateGas(populated)) + except ProviderError, EstimateGasError: + let e = getCurrentException() + signer.decreaseNonce() + raise e + finally: + signer.populateLock.release() - trace "cancelling transaction to prevent stuck transactions", nonce - return await signer.sendTransaction(cancelTx) + return populated method populateTransaction*(signer: Signer, transaction: Transaction): @@ -117,24 +131,33 @@ method populateTransaction*(signer: Signer, if chainId =? transaction.chainId and chainId != await signer.getChainId(): raiseSignerError("chain id mismatch") - var populated = transaction + var populated = await signer.ensureNonceSequence(transaction) - if transaction.sender.isNone: + if populated.sender.isNone: populated.sender = some(await signer.getAddress()) - if transaction.chainId.isNone: + if populated.chainId.isNone: populated.chainId = some(await signer.getChainId()) - if transaction.gasPrice.isNone and (transaction.maxFee.isNone or transaction.maxPriorityFee.isNone): + if populated.gasPrice.isNone and (populated.maxFee.isNone or populated.maxPriorityFee.isNone): populated.gasPrice = some(await signer.getGasPrice()) - if transaction.nonce.isNone: + if populated.nonce.isNone: populated.nonce = some(await signer.getNonce()) - if transaction.gasLimit.isNone: - try: - populated.gasLimit = some(await signer.estimateGas(populated)) - except ProviderError as e: - let e = (ref EstimateGasError)( - msg: "Estimate gas failed", - transaction: populated, - parent: e) - raise e + if populated.gasLimit.isNone: + populated.gasLimit = some(await signer.estimateGas(populated)) return populated + +method cancelTransaction*( + signer: Signer, + tx: Transaction +): Future[TransactionResponse] {.async, base.} = + # cancels a transaction by sending with a 0-valued transaction to ourselves + # with the failed tx's nonce + + without sender =? tx.sender: + raiseSignerError "transaction must have sender" + without nonce =? tx.nonce: + raiseSignerError "transaction must have nonce" + + var cancelTx = Transaction(to: sender, value: 0.u256, nonce: some nonce) + cancelTx = await signer.populateTransaction(cancelTx) + return await signer.sendTransaction(cancelTx) diff --git a/ethers/testing.nim b/ethers/testing.nim index 1304dd5..8162ab6 100644 --- a/ethers/testing.nim +++ b/ethers/testing.nim @@ -2,8 +2,8 @@ import std/strutils import ./provider import ./signer -proc revertReason*(e: ref EthersError): string = - var msg = e.msg +proc revertReason*(emsg: string): string = + var msg = emsg const revertPrefixes = @[ # hardhat "Error: VM Exception while processing transaction: reverted with " & @@ -16,6 +16,10 @@ proc revertReason*(e: ref EthersError): string = msg = msg.replace("\'") return msg +proc revertReason*(e: ref EthersError): string = + var msg = e.msg + msg.revertReason + proc reverts*[T](call: Future[T]): Future[bool] {.async.} = try: when T is void: @@ -23,7 +27,7 @@ proc reverts*[T](call: Future[T]): Future[bool] {.async.} = else: discard await call return false - except ProviderError, SignerError: + except ProviderError, EstimateGasError: return true proc reverts*[T](call: Future[T], reason: string): Future[bool] {.async.} = @@ -33,12 +37,12 @@ proc reverts*[T](call: Future[T], reason: string): Future[bool] {.async.} = else: discard await call return false - except ProviderError, SignerError: - let error = getCurrentException() - var passed = reason == (ref EthersError)(error).revertReason + except ProviderError, EstimateGasError: + let e = getCurrentException() + var passed = reason == (ref EthersError)(e).revertReason if not passed and - not error.parent.isNil and - error.parent of (ref EthersError): - let revertReason = (ref EthersError)(error.parent).revertReason + not e.parent.isNil and + e.parent of (ref EthersError): + let revertReason = (ref EthersError)(e.parent).revertReason passed = reason == revertReason return passed diff --git a/testmodule/helpers.nim b/testmodule/helpers.nim new file mode 100644 index 0000000..ced15ca --- /dev/null +++ b/testmodule/helpers.nim @@ -0,0 +1,14 @@ +import pkg/ethers +import ./hardhat + +type + TestHelpers* = ref object of Contract + +method doRevert*( + self: TestHelpers, + revertReason: string +): ?TransactionResponse {.base, contract.} + +proc new*(_: type TestHelpers, signer: Signer): TestHelpers = + let deployment = readDeployment() + TestHelpers.new(!deployment.address(TestHelpers), signer) diff --git a/testmodule/testContracts.nim b/testmodule/testContracts.nim index fb49262..2c85216 100644 --- a/testmodule/testContracts.nim +++ b/testmodule/testContracts.nim @@ -1,22 +1,20 @@ import std/json import std/options -import std/strutils import pkg/asynctest import pkg/questionable import pkg/stint import pkg/ethers import pkg/ethers/erc20 import ./hardhat +import ./helpers import ./miner import ./mocks -import ./examples type TestToken = ref object of Erc20Token method mint(token: TestToken, holder: Address, amount: UInt256): ?TransactionResponse {.base, contract.} method myBalance(token: TestToken): UInt256 {.base, contract, view.} -method doRevert(token: TestToken, reason: string): ?TransactionResponse {.base, contract.} for url in ["ws://localhost:8545", "http://localhost:8545"]: @@ -240,29 +238,28 @@ for url in ["ws://localhost:8545", "http://localhost:8545"]: Transfer(receiver: accounts[0], value: 100.u256) ] - test "transactions are cancelled once nonce has been incremented to prevent stuck transactions": + test "concurrent transactions with first failing increment nonce correctly": let signer = provider.getSigner() - token = TestToken.new(token.address, signer) + let token = TestToken.new(token.address, signer) + let helpersContract = TestHelpers.new(signer) - # emulate concurrent getNonce calls - let nonce0 = await signer.getNonce() - let nonce1 = await signer.getNonce() - - expect ContractError: - discard await token.doRevert("some reason", TransactionOverrides(nonce: some nonce0)) - - let receipt = await token - .mint(accounts[0], 100.u256, TransactionOverrides(nonce: some nonce1)) - .confirm(1) + # emulate concurrent populateTransaction calls, where the first one fails + let futs = await allFinished( + helpersContract.doRevert("some reason"), + token.mint(accounts[0], 100.u256) + ) + check futs[0].error of EstimateGasError + let receipt = await futs[1].confirm(1) check receipt.status == TransactionStatus.Success - test "transactions are not cancelled if nonce has not been incremented": + test "non-concurrent transactions with first failing increment nonce correctly": let signer = provider.getSigner() - token = TestToken.new(token.address, signer) + let token = TestToken.new(token.address, signer) + let helpersContract = TestHelpers.new(signer) - expect ContractError: - discard await token.doRevert("some reason") + expect EstimateGasError: + discard await helpersContract.doRevert("some reason") let receipt = await token .mint(accounts[0], 100.u256) diff --git a/testmodule/testTesting.nim b/testmodule/testTesting.nim index 1cc2720..8fec151 100644 --- a/testmodule/testTesting.nim +++ b/testmodule/testTesting.nim @@ -3,7 +3,7 @@ import pkg/asynctest import pkg/chronos import pkg/ethers import pkg/ethers/testing -import ./hardhat +import ./helpers suite "Testing helpers": @@ -13,13 +13,13 @@ suite "Testing helpers": test "checks that call reverts": proc call() {.async.} = - raise newException(ProviderError, $rpcResponse) + raise newException(EstimateGasError, $rpcResponse) check await call().reverts() test "checks reason for revert": proc call() {.async.} = - raise newException(ProviderError, $rpcResponse) + raise newException(EstimateGasError, $rpcResponse) check await call().reverts(revertReason) @@ -28,19 +28,31 @@ suite "Testing helpers": check not await call().reverts() - test "reverts only checks ProviderErrors": - proc call() {.async.} = - raise newException(ContractError, "test") + test "reverts only checks ProviderErrors, EstimateGasErrors": + proc callProviderError() {.async.} = + raise newException(ProviderError, "test") + proc callEstimateGasError() {.async.} = + raise newException(EstimateGasError, "test") + proc callEthersError() {.async.} = + raise newException(EthersError, "test") - expect ContractError: - check await call().reverts() + check await callProviderError().reverts() + check await callEstimateGasError().reverts() + expect EthersError: + check await callEthersError().reverts() - test "reverts with reason only checks ProviderErrors": - proc call() {.async.} = - raise newException(ContractError, "test") + test "reverts with reason only checks ProviderErrors, EstimateGasErrors": + proc callProviderError() {.async.} = + raise newException(ProviderError, revertReason) + proc callEstimateGasError() {.async.} = + raise newException(EstimateGasError, revertReason) + proc callEthersError() {.async.} = + raise newException(EthersError, revertReason) - expect ContractError: - check await call().reverts(revertReason) + check await callProviderError().reverts(revertReason) + check await callEstimateGasError().reverts(revertReason) + expect EthersError: + check await callEthersError().reverts(revertReason) test "reverts with reason is false when there is no revert": proc call() {.async.} = discard @@ -49,29 +61,24 @@ suite "Testing helpers": test "reverts is false when the revert reason doesn't match": proc call() {.async.} = - raise newException(ProviderError, "other reason") + raise newException(EstimateGasError, "other reason") check not await call().reverts(revertReason) test "revert handles non-standard revert prefix": let nonStdMsg = fmt"Provider VM Exception: reverted with {revertReason}" proc call() {.async.} = - raise newException(ProviderError, nonStdMsg) + raise newException(EstimateGasError, nonStdMsg) check await call().reverts(nonStdMsg) test "works with functions that return a value": proc call(): Future[int] {.async.} = return 42 check not await call().reverts() - check not await call().reverts("some reason") + check not await call().reverts(revertReason) -type - TestHelpers* = ref object of Contract -method revertsWith*(self: TestHelpers, - revertReason: string) {.base, contract, view.} - -suite "Testing helpers - provider": +suite "Testing helpers - contracts": var helpersContract: TestHelpers var provider: JsonRpcProvider @@ -83,12 +90,20 @@ suite "Testing helpers - provider": provider = JsonRpcProvider.new("ws://127.0.0.1:8545") snapshot = await provider.send("evm_snapshot") accounts = await provider.listAccounts() - let deployment = readDeployment() - helpersContract = TestHelpers.new(!deployment.address(TestHelpers), provider) + helpersContract = TestHelpers.new(provider.getSigner()) teardown: discard await provider.send("evm_revert", @[snapshot]) await provider.close() - test "revert works with provider": - check await helpersContract.revertsWith(revertReason).reverts(revertReason) + test "revert reason can be retrieved when transaction fails": + let txResp = helpersContract.doRevert( + revertReason, + # override gasLimit to skip estimating gas + TransactionOverrides(gasLimit: some 10000000.u256) + ) + check await txResp.confirm(1).reverts(revertReason) + + test "revert reason can be retrieved when estimate gas fails": + let txResp = helpersContract.doRevert(revertReason) + check await txResp.reverts(revertReason)