From 2d03ed6c97941e531640081c78230c29e863a2bf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Adam=20Uhl=C3=AD=C5=99?= <adam@uhlir.dev>
Date: Wed, 27 Nov 2024 09:30:39 +0100
Subject: [PATCH] chore: feedback implementation

---
 ethers/provider.nim                        | 31 ++++++++++++--------
 ethers/providers/jsonrpc/subscriptions.nim | 34 ++++++++++------------
 2 files changed, 35 insertions(+), 30 deletions(-)

diff --git a/ethers/provider.nim b/ethers/provider.nim
index 4f50230..e9372cf 100644
--- a/ethers/provider.nim
+++ b/ethers/provider.nim
@@ -238,7 +238,12 @@ proc confirm*(
   assert confirmations > 0
 
   var blockNumber: UInt256
-  var blockSubscriptionError: ref SubscriptionError
+
+  ## We need initialized succesfull Result, because the first iteration of the `while` loop
+  ## bellow is triggered "manually" by calling `await updateBlockNumber` and not by block
+  ## subscription. If left uninitialized then the Result is in error state and error is raised.
+  ## This result is not used for block value, but for block subscription errors.
+  var blockSubscriptionResult: ?!Block = success(Block(number: UInt256.none, timestamp: 0.u256, hash: BlockHash.none))
   let blockEvent = newAsyncEvent()
 
   proc updateBlockNumber {.async: (raises: []).} =
@@ -252,16 +257,11 @@ proc confirm*(
       discard
 
   proc onBlock(blckResult: ?!Block) =
-    without blck =? blckResult, error:
-        let err = blckResult.error()
+    blockSubscriptionResult = blckResult
 
-        if err of SubscriptionError:
-          blockSubscriptionError = cast[ref SubscriptionError](err)
-        else:
-          echo "What to do now? 😳"
-
-        blockEvent.fire()
-        return
+    if blckResult.isErr:
+      blockEvent.fire()
+      return
 
     # ignore block parameter; hardhat may call this with pending blocks
     asyncSpawn updateBlockNumber()
@@ -276,8 +276,15 @@ proc confirm*(
     await blockEvent.wait()
     blockEvent.clear()
 
-    if not isNil(blockSubscriptionError):
-      raise blockSubscriptionError
+    if blockSubscriptionResult.isErr:
+      let err = blockSubscriptionResult.error()
+
+      if err of SubscriptionError:
+        raise cast[ref SubscriptionError](err)
+      elif err of CancelledError:
+        raise cast[ref CancelledError](err)
+      else:
+        raise err.toErr(ProviderError)
 
     if blockNumber >= finish:
       await subscription.unsubscribe()
diff --git a/ethers/providers/jsonrpc/subscriptions.nim b/ethers/providers/jsonrpc/subscriptions.nim
index 6ce5804..fbb6909 100644
--- a/ethers/providers/jsonrpc/subscriptions.nim
+++ b/ethers/providers/jsonrpc/subscriptions.nim
@@ -80,11 +80,9 @@ proc getCallback(subscriptions: JsonRpcSubscriptions,
                  id: JsonNode): ?SubscriptionCallback  {. raises:[].} =
   try:
     if not id.isNil and id in subscriptions.callbacks:
-        try:
-          return subscriptions.callbacks[id].some
-        except: discard
-    else:
-      return SubscriptionCallback.none
+      try:
+        return subscriptions.callbacks[id].some
+      except: discard
   except KeyError:
     return SubscriptionCallback.none
 
@@ -110,11 +108,11 @@ method subscribeBlocks(subscriptions: WebSocketSubscriptions,
                       {.async, raises: [].} =
   proc callback(id: JsonNode, argumentsResult: ?!JsonNode) {.raises: [].} =
     without arguments =? argumentsResult, error:
-        onBlock(failure(Block, error.toErr(SubscriptionError)))
-        return
+      onBlock(failure(Block, error.toErr(SubscriptionError)))
+      return
 
-    if blck =? Block.fromJson(arguments{"result"}):
-      onBlock(success(blck))
+    let res = Block.fromJson(arguments{"result"}).mapFailure(SubscriptionError)
+    onBlock(res)
 
   let id = await subscriptions.client.eth_subscribe("newHeads")
   subscriptions.callbacks[id] = callback
@@ -207,13 +205,13 @@ proc new*(_: type JsonRpcSubscriptions,
 
   proc poll(id: JsonNode) {.async: (raises: [CancelledError]).} =
     without callback =? subscriptions.getCallback(id):
-        return
+      return
 
     try:
       for change in await getChanges(id):
         callback(id, success(change))
     except CancelledError as e:
-          raise e
+      raise e
     except CatchableError as e:
       callback(id, failure(JsonNode, e))
 
@@ -246,13 +244,13 @@ method subscribeBlocks(subscriptions: PollingSubscriptions,
     except CancelledError as e:
       discard
     except CatchableError as e:
-      let wrappedErr = newException(SubscriptionError, "HTTP polling: There was an exception while getting subscription's block: " & e.msg, e)
-      onBlock(failure(Block, wrappedErr))
+      let err = e.toErr(SubscriptionError, "HTTP polling: There was an exception while getting subscription's block: " & e.msg)
+      onBlock(failure(Block, err))
 
   proc callback(id: JsonNode, changeResult: ?!JsonNode) {.raises:[].} =
-    without change =? changeResult, error:
-        onBlock(failure(Block, error.toErr(SubscriptionError)))
-        return
+    without change =? changeResult, e:
+      onBlock(failure(Block, e.toErr(SubscriptionError)))
+      return
 
     if hash =? BlockHash.fromJson(change):
       asyncSpawn getBlock(hash)
@@ -270,8 +268,8 @@ method subscribeLogs(subscriptions: PollingSubscriptions,
 
   proc callback(id: JsonNode, changeResult: ?!JsonNode) =
     without change =? changeResult, error:
-        onLog(failure(Log, error.toErr(SubscriptionError)))
-        return
+      onLog(failure(Log, error.toErr(SubscriptionError)))
+      return
 
     if log =? Log.fromJson(change):
       onLog(success(log))