From fc2ffff27984d8c818990f96e744753b04064050 Mon Sep 17 00:00:00 2001 From: Eric <5089238+emizzle@users.noreply.github.com> Date: Mon, 12 Feb 2024 13:15:05 +1100 Subject: [PATCH] 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 | 2 +- ethers/nimshims/hashes.nim | 15 ++++++++----- ethers/providers/jsonrpc.nim | 14 +++++++----- ethers/providers/jsonrpc/subscriptions.nim | 15 +++++-------- ethers/signer.nim | 22 ++++++++++--------- ethers/signers/wallet.nim | 3 ++- ethers/testing.nim | 4 ++-- testmodule/mocks.nim | 3 ++- .../providers/jsonrpc/testConversions.nim | 6 ++--- testmodule/testTesting.nim | 6 +++++ 10 files changed, 51 insertions(+), 39 deletions(-) diff --git a/ethers/erc20.nim b/ethers/erc20.nim index bcbc040..5b49275 100644 --- a/ethers/erc20.nim +++ b/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. diff --git a/ethers/nimshims/hashes.nim b/ethers/nimshims/hashes.nim index ebe2e3f..c691d9e 100644 --- a/ethers/nimshims/hashes.nim +++ b/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 \ No newline at end of file diff --git a/ethers/providers/jsonrpc.nim b/ethers/providers/jsonrpc.nim index f5390ba..9b2b72a 100644 --- a/ethers/providers/jsonrpc.nim +++ b/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" diff --git a/ethers/providers/jsonrpc/subscriptions.nim b/ethers/providers/jsonrpc/subscriptions.nim index a61dfcb..1cf6882 100644 --- a/ethers/providers/jsonrpc/subscriptions.nim +++ b/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 diff --git a/ethers/signer.nim b/ethers/signer.nim index 8240069..cba120c 100644 --- a/ethers/signer.nim +++ b/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: diff --git a/ethers/signers/wallet.nim b/ethers/signers/wallet.nim index 5c70e5a..18a92cd 100644 --- a/ethers/signers/wallet.nim +++ b/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 diff --git a/ethers/testing.nim b/ethers/testing.nim index 8162ab6..2b9ec4a 100644 --- a/ethers/testing.nim +++ b/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 diff --git a/testmodule/mocks.nim b/testmodule/mocks.nim index 7e3d877..eed2c23 100644 --- a/testmodule/mocks.nim +++ b/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 diff --git a/testmodule/providers/jsonrpc/testConversions.nim b/testmodule/providers/jsonrpc/testConversions.nim index 55d7bc6..c29810a 100644 --- a/testmodule/providers/jsonrpc/testConversions.nim +++ b/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 diff --git a/testmodule/testTesting.nim b/testmodule/testTesting.nim index fb62ecc..b5d80bd 100644 --- a/testmodule/testTesting.nim +++ b/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)