From 7eac8410af1c00cf9240f4034d6e0a19fa4e1090 Mon Sep 17 00:00:00 2001 From: Eric <5089238+emizzle@users.noreply.github.com> Date: Wed, 25 Oct 2023 10:42:25 +1100 Subject: [PATCH] prevent stuck transactions by async locking nonce sequencing (+ estimate gas) (#55) - 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 - combine ensureNonceSequence into populateTransaction --- ethers/contract.nim | 1 - ethers/providers/jsonrpc.nim | 11 ++-- ethers/signer.nim | 88 +++++++++++++++++++++++++----- ethers/testing.nim | 22 ++++++-- ethers/wallet.nim | 3 +- testmodule/helpers.nim | 14 +++++ testmodule/testContracts.nim | 31 +++++++++++ testmodule/testTesting.nim | 56 ++++++++++--------- testnode/contracts/TestHelpers.sol | 5 +- 9 files changed, 178 insertions(+), 53 deletions(-) create mode 100644 testmodule/helpers.nim diff --git a/ethers/contract.nim b/ethers/contract.nim index 84d5d8f..c590313 100644 --- a/ethers/contract.nim +++ b/ethers/contract.nim @@ -246,7 +246,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 6bf01ee..505944d 100644 --- a/ethers/providers/jsonrpc.nim +++ b/ethers/providers/jsonrpc.nim @@ -28,7 +28,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 +40,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 +228,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.} = @@ -240,7 +240,8 @@ method signMessage*(signer: JsonRpcSigner, method sendTransaction*(signer: JsonRpcSigner, transaction: Transaction): Future[TransactionResponse] {.async.} = convertError: - signer.updateNonce(transaction.nonce) + if nonce =? transaction.nonce: + signer.updateNonce(nonce) let client = await signer.provider.client hash = await client.eth_sendTransaction(transaction) diff --git a/ethers/signer.nim b/ethers/signer.nim index e22b118..089600c 100644 --- a/ethers/signer.nim +++ b/ethers/signer.nim @@ -6,11 +6,25 @@ export basics type Signer* = ref object of RootObj lastSeenNonce: ?UInt256 + populateLock: AsyncLock -type SignerError* = object of EthersError +type + SignerError* = object of EthersError + EstimateGasError* = object of SignerError + transaction*: Transaction -template raiseSignerError(message: string) = - raise newException(SignerError, message) +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" @@ -39,23 +53,27 @@ 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() method getNonce(signer: Signer): Future[UInt256] {.base, gcsafe, async.} = var nonce = await signer.getTransactionCount(BlockTag.pending) - + if lastSeen =? signer.lastSeenNonce and lastSeen >= nonce: nonce = (lastSeen + 1.u256) signer.lastSeenNonce = some nonce - + return nonce -method updateNonce*(signer: Signer, nonce: ?UInt256) {.base, gcsafe.} = - without nonce =? nonce: - return +method updateNonce*( + signer: Signer, + nonce: UInt256 +) {.base, gcsafe.} = without lastSeen =? signer.lastSeenNonce: signer.lastSeenNonce = some nonce @@ -64,6 +82,10 @@ method updateNonce*(signer: Signer, nonce: ?UInt256) {.base, gcsafe.} = if nonce > lastSeen: signer.lastSeenNonce = some nonce +method decreaseNonce*(signer: Signer) {.base, gcsafe.} = + if lastSeen =? signer.lastSeenNonce and lastSeen > 0: + signer.lastSeenNonce = some lastSeen - 1 + method populateTransaction*(signer: Signer, transaction: Transaction): Future[Transaction] {.base, async.} = @@ -73,17 +95,55 @@ method populateTransaction*(signer: Signer, if chainId =? transaction.chainId and chainId != await signer.getChainId(): raiseSignerError("chain id mismatch") + if signer.populateLock.isNil: + signer.populateLock = newAsyncLock() + + await signer.populateLock.acquire() + var populated = transaction if transaction.sender.isNone: populated.sender = some(await signer.getAddress()) - if transaction.nonce.isNone: - populated.nonce = some(await signer.getNonce()) if transaction.chainId.isNone: populated.chainId = some(await signer.getChainId()) - if transaction.gasPrice.isNone and (transaction.maxFee.isNone or transaction.maxPriorityFee.isNone): + if transaction.gasPrice.isNone and (populated.maxFee.isNone or populated.maxPriorityFee.isNone): populated.gasPrice = some(await signer.getGasPrice()) - if transaction.gasLimit.isNone: - populated.gasLimit = some(await signer.estimateGas(populated)) + + if transaction.nonce.isNone and transaction.gasLimit.isNone: + # when both nonce and gasLimit are not populated, we must ensure getNonce is + # followed by an estimateGas so we can determine if there was an error. If + # there is an error, the nonce must be deprecated to prevent nonce gaps and + # stuck transactions + try: + 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() + + else: + if transaction.nonce.isNone: + populated.nonce = some(await signer.getNonce()) + if transaction.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 5d38c1f..8162ab6 100644 --- a/ethers/testing.nim +++ b/ethers/testing.nim @@ -1,8 +1,9 @@ import std/strutils import ./provider +import ./signer -proc revertReason*(e: ref ProviderError): string = - var msg = e.msg +proc revertReason*(emsg: string): string = + var msg = emsg const revertPrefixes = @[ # hardhat "Error: VM Exception while processing transaction: reverted with " & @@ -15,6 +16,10 @@ proc revertReason*(e: ref ProviderError): 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: @@ -22,7 +27,7 @@ proc reverts*[T](call: Future[T]): Future[bool] {.async.} = else: discard await call return false - except ProviderError: + except ProviderError, EstimateGasError: return true proc reverts*[T](call: Future[T], reason: string): Future[bool] {.async.} = @@ -32,5 +37,12 @@ proc reverts*[T](call: Future[T], reason: string): Future[bool] {.async.} = else: discard await call return false - except ProviderError as error: - return reason == error.revertReason + except ProviderError, EstimateGasError: + let e = getCurrentException() + var passed = reason == (ref EthersError)(e).revertReason + if not passed and + 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/ethers/wallet.nim b/ethers/wallet.nim index 22e13e5..72177be 100644 --- a/ethers/wallet.nim +++ b/ethers/wallet.nim @@ -70,5 +70,6 @@ proc signTransaction*(wallet: Wallet, method sendTransaction*(wallet: Wallet, transaction: Transaction): Future[TransactionResponse] {.async.} = let signed = await signTransaction(wallet, transaction) - wallet.updateNonce(transaction.nonce) + if nonce =? transaction.nonce: + wallet.updateNonce(nonce) return await provider(wallet).sendTransaction(signed) 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 954e94e..2c85216 100644 --- a/testmodule/testContracts.nim +++ b/testmodule/testContracts.nim @@ -1,10 +1,12 @@ import std/json +import std/options import pkg/asynctest import pkg/questionable import pkg/stint import pkg/ethers import pkg/ethers/erc20 import ./hardhat +import ./helpers import ./miner import ./mocks @@ -235,3 +237,32 @@ for url in ["ws://localhost:8545", "http://localhost:8545"]: check logs == @[ Transfer(receiver: accounts[0], value: 100.u256) ] + + test "concurrent transactions with first failing increment nonce correctly": + let signer = provider.getSigner() + let token = TestToken.new(token.address, signer) + let helpersContract = TestHelpers.new(signer) + + # 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 "non-concurrent transactions with first failing increment nonce correctly": + let signer = provider.getSigner() + let token = TestToken.new(token.address, signer) + let helpersContract = TestHelpers.new(signer) + + expect EstimateGasError: + discard await helpersContract.doRevert("some reason") + + let receipt = await token + .mint(accounts[0], 100.u256) + .confirm(1) + + check receipt.status == TransactionStatus.Success diff --git a/testmodule/testTesting.nim b/testmodule/testTesting.nim index 1cc2720..ec1ba82 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,11 @@ 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) + check await helpersContract.doRevert(revertReason).reverts(revertReason) diff --git a/testnode/contracts/TestHelpers.sol b/testnode/contracts/TestHelpers.sol index d4316b5..b41b5c3 100644 --- a/testnode/contracts/TestHelpers.sol +++ b/testnode/contracts/TestHelpers.sol @@ -3,7 +3,8 @@ pragma solidity ^0.8.0; contract TestHelpers { - function revertsWith(string calldata revertReason) public pure { - require(false, revertReason); + function doRevert(string calldata reason) public pure { + // Revert every tx with given reason + require(false, reason); } }