address PR feedback

- add comments to hashes shim
- remove .catch from callback condition
- derive SignerError from EthersError instead of ProviderError. This allows Providers and Signers to be separate, as Ledger does it, to isolate functionality. Some signer functions now raise both ProviderError and SignerError
- Update reverts to check for SignerError
- Update ERC-20 method comment

ethers/erc20.nim

@@ -76,5 +76,5 @@ method transferFrom*(token: Erc20Token,
                      spender: Address,
                      recipient: Address,
                      amount: UInt256): ?TransactionResponse {.base, contract.}
-  ## Moves `amount` tokens from sender to `to` using the allowance
+  ## Moves `amount` tokens from `spender` to `recipient` using the allowance
   ## mechanism. `amount` is then deducted from the caller's allowance.

ethers/nimshims/hashes.nim

@@ -1,9 +1,14 @@
+## Fixes an underlying Exception caused by missing forward declarations for
+## `std/json.JsonNode.hash`, eg when using `JsonNode` as a `Table` key. Adds
+## {.raises: [].} for proper exception tracking. Copied from the std/json module
+
 import std/json
 import std/hashes
-import std/tables
+
+{.push raises:[].}
 
 when (NimMajor) >= 2:
-  proc hash*[A](x: openArray[A]): Hash {.raises: [].} =
+  proc hash*[A](x: openArray[A]): Hash =
     ## Efficient hashing of arrays and sequences.
     ## There must be a `hash` proc defined for the element type `A`.
     when A is byte:
@@ -18,9 +23,9 @@ when (NimMajor) >= 2:
         result = result !& hash(a)
       result = !$result
 
-func hash*(n: OrderedTable[string, JsonNode]): Hash {.raises: [].}
+func hash*(n: OrderedTable[string, JsonNode]): Hash
 
-proc hash*(n: JsonNode): Hash {.noSideEffect, raises: [].} =
+func hash*(n: JsonNode): Hash =
   ## Compute the hash for a JSON node
   case n.kind
   of JArray:
@@ -38,7 +43,7 @@ proc hash*(n: JsonNode): Hash {.noSideEffect, raises: [].} =
   of JNull:
     result = Hash(0)
 
-func hash*(n: OrderedTable[string, JsonNode]): Hash {.raises: [].} =
+func hash*(n: OrderedTable[string, JsonNode]): Hash =
   for key, val in n:
     result = result xor (hash(key) !& hash(val))
   result = !$result

ethers/providers/jsonrpc.nim

@@ -296,19 +296,21 @@ template convertSignerError(body) =
   except CatchableError as error:
     raise newException(JsonRpcSignerError, error.msg)
 
-method provider*(signer: JsonRpcSigner): Provider {.gcsafe, raises: [SignerError].} =
+method provider*(signer: JsonRpcSigner): Provider
+  {.gcsafe, raises: [SignerError].} =
+
   signer.provider
 
 method getAddress*(
-  signer: JsonRpcSigner): Future[Address] {.async: (raises:[SignerError]).} =
+  signer: JsonRpcSigner): Future[Address]
+  {.async: (raises:[ProviderError, SignerError]).} =
 
   if address =? signer.address:
     return address
 
-  convertSignerError:
-    let accounts = await signer.provider.listAccounts()
-    if accounts.len > 0:
-      return accounts[0]
+  let accounts = await signer.provider.listAccounts()
+  if accounts.len > 0:
+    return accounts[0]
 
   raiseJsonRpcSignerError "no address found"
 

ethers/providers/jsonrpc/subscriptions.nim

@@ -1,18 +1,14 @@
-# import std/hashes
 import std/tables
 import std/sequtils
 import pkg/chronos
 import pkg/json_rpc/rpcclient
-# import pkg/serde
 import ../../basics
 import ../../provider
-include ../../nimshims/hashes #as hashes_shim
+include ../../nimshims/hashes
 import ./rpccalls
 import ./conversions
 import ./looping
 
-# export hashes_shim
-
 type
   JsonRpcSubscriptions* = ref object of RootObj
     client: RpcClient
@@ -78,11 +74,11 @@ method close*(subscriptions: JsonRpcSubscriptions) {.async, base.} =
 proc getCallback(subscriptions: JsonRpcSubscriptions,
                  id: JsonNode): ?SubscriptionCallback =
   try:
-    if subscriptions.callbacks.hasKey(id):
+    if not id.isNil and id in subscriptions.callbacks:
       subscriptions.callbacks[id].some
     else:
       SubscriptionCallback.none
-  except Exception:
+  except KeyError:
     SubscriptionCallback.none
 
 # Web sockets
@@ -92,10 +88,11 @@ type
 
 proc new*(_: type JsonRpcSubscriptions,
           client: RpcWebSocketClient): JsonRpcSubscriptions =
+
   let subscriptions = WebSocketSubscriptions(client: client)
   proc subscriptionHandler(arguments: JsonNode) {.raises:[].} =
-    if id =? arguments{"subscription"}.catch and
-       callback =? subscriptions.getCallback(id):
+    let id = arguments{"subscription"} or newJString("")
+    if callback =? subscriptions.getCallback(id):
       callback(id, arguments)
   subscriptions.setMethodHandler("eth_subscription", subscriptionHandler)
   subscriptions

ethers/signer.nim

@@ -10,7 +10,7 @@ type
   Signer* = ref object of RootObj
     lastSeenNonce: ?UInt256
     populateLock: AsyncLock
-  SignerError* = object of ProviderError
+  SignerError* = object of EthersError
   EstimateGasError* = object of SignerError
     transaction*: Transaction
 
@@ -40,13 +40,15 @@ method provider*(
   doAssert false, "not implemented"
 
 method getAddress*(
-  signer: Signer): Future[Address] {.base, async: (raises:[SignerError]).} =
+  signer: Signer): Future[Address]
+  {.base, async: (raises:[ProviderError, SignerError]).} =
 
   doAssert false, "not implemented"
 
 method signMessage*(
   signer: Signer,
-  message: seq[byte]): Future[seq[byte]] {.base, async: (raises: [SignerError]).} =
+  message: seq[byte]): Future[seq[byte]]
+  {.base, async: (raises: [SignerError]).} =
 
   doAssert false, "not implemented"
 
@@ -58,10 +60,10 @@ method sendTransaction*(
   doAssert false, "not implemented"
 
 method getGasPrice*(
-  signer: Signer): Future[UInt256] {.base, gcsafe, async: (raises: [SignerError]).} =
+  signer: Signer): Future[UInt256]
+  {.base, async: (raises: [ProviderError, SignerError]).} =
 
-  convertError:
-    return await signer.provider.getGasPrice()
+  return await signer.provider.getGasPrice()
 
 method getTransactionCount*(
   signer: Signer,
@@ -91,10 +93,10 @@ method estimateGas*(
     raiseEstimateGasError transaction, e
 
 method getChainId*(
-  signer: Signer): Future[UInt256] {.base, async: (raises: [SignerError]).} =
+  signer: Signer): Future[UInt256]
+  {.base, async: (raises: [ProviderError, SignerError]).} =
 
-  convertError:
-    return await signer.provider.getChainId()
+  return await signer.provider.getChainId()
 
 method getNonce(
   signer: Signer): Future[UInt256] {.base, async: (raises: [SignerError]).} =
@@ -126,7 +128,7 @@ method decreaseNonce*(signer: Signer) {.base, gcsafe.} =
 method populateTransaction*(
   signer: Signer,
   transaction: Transaction): Future[Transaction]
-  {.base, async: (raises: [CancelledError, AsyncLockError, SignerError]).} =
+  {.base, async: (raises: [CancelledError, AsyncLockError, ProviderError, SignerError]).} =
 
   var address: Address
   convertError:

ethers/signers/wallet.nim

@@ -68,7 +68,8 @@ method provider*(wallet: Wallet): Provider {.gcsafe, raises: [SignerError].} =
   provider
 
 method getAddress*(
-  wallet: Wallet): Future[Address] {.async: (raises:[SignerError]).} =
+  wallet: Wallet): Future[Address]
+  {.async: (raises:[ProviderError, SignerError]).} =
 
   return wallet.address
 

ethers/testing.nim

@@ -27,7 +27,7 @@ proc reverts*[T](call: Future[T]): Future[bool] {.async.} =
     else:
       discard await call
     return false
-  except ProviderError, EstimateGasError:
+  except ProviderError, SignerError, EstimateGasError:
     return true
 
 proc reverts*[T](call: Future[T], reason: string): Future[bool] {.async.} =
@@ -37,7 +37,7 @@ proc reverts*[T](call: Future[T], reason: string): Future[bool] {.async.} =
     else:
       discard await call
     return false
-  except ProviderError, EstimateGasError:
+  except ProviderError, SignerError, EstimateGasError:
     let e = getCurrentException()
     var passed = reason == (ref EthersError)(e).revertReason
     if not passed and

testmodule/mocks.nim

@@ -12,7 +12,8 @@ method provider*(signer: MockSigner): Provider =
   signer.provider
 
 method getAddress*(
-  signer: MockSigner): Future[Address] {.async: (raises:[SignerError]).} =
+  signer: MockSigner): Future[Address]
+  {.async: (raises:[ProviderError, SignerError]).} =
 
   return signer.address
 

testmodule/providers/jsonrpc/testConversions.nim

@@ -20,14 +20,12 @@ suite "JSON Conversions":
       "timestamp":"0x6285c293"
     }
 
-    without blk1 =? Block.fromJson(json):
-      fail
+    let blk1 = !Block.fromJson(json)
     check blk1.number.isNone
 
     json["number"] = newJString("")
 
-    without blk2 =? Block.fromJson(json):
-      fail
+    let blk2 = !Block.fromJson(json)
     check blk2.number.isSome
     check blk2.number.get.isZero
 

testmodule/testTesting.nim

@@ -32,12 +32,15 @@ suite "Testing helpers":
   test "reverts only checks ProviderErrors, EstimateGasErrors":
     proc callProviderError() {.async.} =
       raise newException(ProviderError, "test")
+    proc callSignerError() {.async.} =
+      raise newException(SignerError, "test")
     proc callEstimateGasError() {.async.} =
       raise newException(EstimateGasError, "test")
     proc callEthersError() {.async.} =
       raise newException(EthersError, "test")
 
     check await callProviderError().reverts()
+    check await callSignerError().reverts()
     check await callEstimateGasError().reverts()
     expect EthersError:
       check await callEthersError().reverts()
@@ -45,12 +48,15 @@ suite "Testing helpers":
   test "reverts with reason only checks ProviderErrors, EstimateGasErrors":
     proc callProviderError() {.async.} =
       raise newException(ProviderError, revertReason)
+    proc callSignerError() {.async.} =
+      raise newException(SignerError, revertReason)
     proc callEstimateGasError() {.async.} =
       raise newException(EstimateGasError, revertReason)
     proc callEthersError() {.async.} =
       raise newException(EthersError, revertReason)
 
     check await callProviderError().reverts(revertReason)
+    check await callSignerError().reverts(revertReason)
     check await callEstimateGasError().reverts(revertReason)
     expect EthersError:
       check await callEthersError().reverts(revertReason)