Reviewed all modules

eth-rpc/client/clientdispatch.nim

@@ -17,8 +17,13 @@ proc newRpcClient*(): RpcClient =
 
 proc call*(self: RpcClient, name: string, params: JsonNode): Future[Response] {.async.} =
   ## Remotely calls the specified RPC method.
+  # REVIEW: is there a reason why a simple counter is not used here?
+  # genOid takes CPU cycles and the output is larger
   let id = $genOid()
   let msg = %{"jsonrpc": %"2.0", "method": %name, "params": params, "id": %id}
+  # REVIEW: it would be more efficient if you append the terminating new line to
+  # the `msg` variable in-place. This way, a copy won't be performed most of the
+  # time because the string is likely to have 2 bytes of unused capacity.
   await self.socket.send($msg & "\c\l")
 
   # Completed by processMessage.
@@ -31,6 +36,9 @@ proc isNull(node: JsonNode): bool = node.kind == JNull
 proc processMessage(self: RpcClient, line: string) =
   let node = parseJson(line)
   
+  # REVIEW: These shouldn't be just asserts. You cannot count
+  # that the other side implements the protocol correctly, so
+  # you must perform validation even in release builds.
   assert node.hasKey("jsonrpc")
   assert node["jsonrpc"].str == "2.0"
   assert node.hasKey("id")
@@ -90,6 +98,7 @@ macro generateCalls: untyped =
   ##   client.web3_clientVersion(params)
   result = newStmtList()
   for callName in ETHEREUM_RPC_CALLS:
+    # REVIEW: `macros.quote` would have worked well here to make the code easier to understand/maintain
     var
       params = newNimNode(nnkFormalParams)
       call = newCall(newDotExpr(ident("client"), ident("call")), newStrLitNode(callName), ident("params"))

eth-rpc/server/asyncutils.nim

@@ -7,9 +7,11 @@ proc sendError*(client: AsyncSocket, code: int, msg: string, id: JsonNode, data:
   ## Send error message to client
   let error = %{"code": %(code), "message": %msg, "data": data}
   ifDebug: echo "Send error json: ", wrapReply(newJNull(), error, id).pretty & "\c\l"
+  # REVIEW: prefer in-place appending instead of string concatenation
+  # (see the similar comment in clientdispatch.nim)
   result = client.send($wrapReply(id, newJNull(), error) & "\c\l")
 
 proc sendJsonError*(state: RpcJsonError, client: AsyncSocket, id: JsonNode, data = newJNull()) {.async.} =
   ## Send client response for invalid json state
   let errMsgs = jsonErrorMessages[state]
-  await client.sendError(errMsgs[0], errMsgs[1], id, data)
+  await client.sendError(errMsgs[0], errMsgs[1], id, data)

eth-rpc/server/cryptoutils.nim

@@ -2,6 +2,7 @@ import nimcrypto
 
 proc k256*(data: string): string =
   # do not convert, assume string is data
+  # REVIEW: Nimcrypto has a one-liner for the code here: sha3_256.digest(data)
   var k = sha3_256()
   k.init
   k.update(cast[ptr uint8](data[0].unsafeaddr), data.len.uint)

eth-rpc/server/private/transportutils.nim

@@ -2,6 +2,8 @@ from asyncdispatch import Port
 
 proc `$`*(port: Port): string = $int(port)
 
+# REVIEW: you can base one of the iterators on the other to avoid the code duplication
+# e.g. implement `bytes` in terms of `bytePairs`.
 iterator bytes*[T: SomeUnsignedInt](value: T): byte {.inline.} =
   ## Traverse the bytes of a value in little endian
   let argSize = sizeOf(T)
@@ -30,6 +32,7 @@ proc encodeQuantity*(value: SomeUnsignedInt): string =
   var hValue = value.toHex.stripLeadingZeros
   result = "0x" & hValue
 
+# REVIEW: I think Mamy has now introduced a similar proc in the `byteutils` package
 proc encodeData*[T: SomeUnsignedInt](values: seq[T]): string =
   ## Translates seq of values to hex string
   let argSize = sizeOf(T)

eth-rpc/server/rpcregistration.nim

@@ -3,6 +3,8 @@ import macros, servertypes
 var rpcCallRefs {.compiletime.} = newSeq[(string)]()
 
 macro rpc*(prc: untyped): untyped =
+  # REVIEW: (IMPORTANT) I think the rpc procs should be async.
+  # they may need to call into other async procs of the VM
   result = prc
   let
     params = prc.findChild(it.kind == nnkFormalParams)

tests/testserverclient.nim

@@ -1,5 +1,7 @@
 import ../eth-rpc / rpcclient, ../eth-rpc / rpcserver, asyncdispatch, json, unittest
 
+# REVIEW: I'd like to see some dummy implementations of RPC calls handled in async fashion.
+
 when isMainModule:
   # create on localhost, default port
   var srv = newRpcServer("")