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

@@ -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,

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)

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)

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

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)

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)

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

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)

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);
   }
 }