Better error handling with Results from interactions with swap-contracts-module (#393)

* Fix balance JSON resp

* Resultify swap contract calls for error handling

* More resultify and fixup tests

* More results

* resultify waku swap handling

* re-enable waku swap test

should not crash now, even if it fails to settle (tested separately)

tests/all_tests_v2.nim

@@ -7,7 +7,7 @@ import
   ./v2/test_waku_filter,
   ./v2/test_waku_pagination,
   ./v2/test_waku_payload,
-#  ./v2/test_waku_swap,
+  ./v2/test_waku_swap,
   ./v2/test_message_store,
   ./v2/test_jsonrpc_waku,
   ./v2/test_peer_manager,

tests/v2/test_waku_swap_contracts.nim

@@ -19,13 +19,20 @@ procSuite "Basic balance test":
 
   test "Get balance from running node":
     # NOTE: This corresponds to the first default account in Hardhat
-    let balance = waku_swap_contracts.getBalance("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266")
+    let balRes = waku_swap_contracts.getBalance("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266")
+    var balance: float
+    if balRes.isOk():
+      let json = balRes[]
+      let balanceStr = json["balance"].getStr()
+      balance = parseFloat(balanceStr)
 
     check:
-      contains(balance, "ETH")
+      balRes.isOk()
+      balance > 0
 
   test "Setup Swap":
-    let json = waku_swap_contracts.setupSwap()
+    let res = waku_swap_contracts.setupSwap()
+    let json = res[]
 
     var aliceAddress = json["aliceAddress"].getStr()
     aliceSwapAddress = json["aliceSwapAddress"].getStr()
@@ -38,26 +45,31 @@ procSuite "Basic balance test":
       contains(aliceAddress, "0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266")
 
   test "Sign Cheque":
-    signature = waku_swap_contracts.signCheque(aliceSwapAddress)
+    var sigRes = waku_swap_contracts.signCheque(aliceSwapAddress)
+    if sigRes.isOk():
+      let json = sigRes[]
+      signature = json["signature"].getStr()
 
     check:
+      sigRes.isOk()
       contains(signature, "0x")
 
   test "Get ERC20 Balances":
-    let json = getERC20Balances(erc20address)
+    let res = waku_swap_contracts.getERC20Balances(erc20address)
 
     check:
-      json["bobBalance"].getInt() == 10000
+      res.isOk()
+      res[]["bobBalance"].getInt() == 10000
 
   test "Redeem cheque and check balance":
-    let json = waku_swap_contracts.redeemCheque(aliceSwapAddress, signature)
-    var resp = json["resp"].getStr()
-    debug "json", json
+    let redeemRes = waku_swap_contracts.redeemCheque(aliceSwapAddress, signature)
+    var resp = redeemRes[]["resp"].getStr()
+    debug "Redeem resp", resp
 
-    debug "Get balances"
-    let json2 = getERC20Balances(erc20address)
-    debug "json", json2
+    let balRes = getERC20Balances(erc20address)
 
     # Balance for Bob has now increased
     check:
-      json2["bobBalance"].getInt() == 10500
+      redeemRes.isOk()
+      balRes.isOk()
+      balRes[]["bobBalance"].getInt() == 10500

waku/v2/protocol/waku_swap/waku_swap.nim

@@ -22,7 +22,7 @@
 ##
 
 import
-  std/[tables, options],
+  std/[tables, options, json],
   bearssl,
   chronos, chronicles, metrics, stew/results,
   libp2p/crypto/crypto,
@@ -118,9 +118,18 @@ proc sendCheque*(ws: WakuSwap) {.async.} =
   # TODO We get this from the setup of swap setup, dynamic, should be part of setup
   # TODO Add beneficiary, etc
   var aliceSwapAddress = "0x6C3d502f1a97d4470b881015b83D9Dd1062172e1"
-  let signature = waku_swap_contracts.signCheque(aliceSwapAddress)
-  info "Signed Cheque", swapAddress = aliceSwapAddress, signature = signature
+  var signature: string
 
+  var res = waku_swap_contracts.signCheque(aliceSwapAddress)
+  if res.isOk():
+    echo "signCheque ", res[]
+    let json = res[]
+    signature = json["signature"].getStr()
+  else:
+    # To test code paths, this should look different in a production setting
+    warn "Something went wrong when signing cheque, sending anyway"
+
+  info "Signed Cheque", swapAddress = aliceSwapAddress, signature = signature
   let sigBytes = cast[seq[byte]](signature)
   await connOpt.get().writeLP(Cheque(amount: 1, signature: sigBytes).encode().buffer)
 

waku/v2/protocol/waku_swap/waku_swap_contracts.nim

@@ -2,64 +2,86 @@
 #
 # Assumes swap-contracts-module node is running.
 #
+
+{.push raises: [Defect].}
+
 import
   std/[osproc, strutils, json],
-  chronicles
+  chronicles, stew/results
 
 logScope:
   topics = "wakuswapcontracts"
 
-# XXX In general this is not a good API, more a collection of hacky glue code for PoC.
-#
-# TODO Error handling
+# TODO Richer error types than string, overkill for now...
+type NodeTaskJsonResult = Result[JsonNode, string]
+
+# XXX In general this is not a great API, more a collection of hacky glue code for PoC.
 
 # Interacts with node in sibling path and interacts with a local Hardhat node.
 const taskPrelude = "npx hardhat --network localhost "
 const cmdPrelude = "cd ../swap-contracts-module; " & taskPrelude
 
-proc execNodeTask(taskStr: string): tuple[output: TaintedString, exitCode: int] =
+# proc execNodeTask(taskStr: string): tuple[output: TaintedString, exitCode: int] =
+#   let cmdString = $cmdPrelude & $taskStr
+#   debug "execNodeTask", cmdString
+#   return osproc.execCmdEx(cmdString)
+
+proc execNodeTaskJson(taskStr: string): NodeTaskJsonResult =
   let cmdString = $cmdPrelude & $taskStr
   debug "execNodeTask", cmdString
-  return osproc.execCmdEx(cmdString)
 
-# TODO JSON?
-proc getBalance*(accountAddress: string): string =
+  try:
+    let (output, errC) = osproc.execCmdEx(cmdString)
+    if errC>0:
+      error "Error executing node task", output
+      return err(output)
+
+    debug "Command executed", output
+
+    try:
+      let json = parseJson(output)
+      return ok(json)
+    except JsonParsingError:
+      return err("Unable to parse JSON:" & $output)
+    except Exception:
+      return err("Unable to parse JSON:" & $output)
+
+  except OSError:
+    return err("Unable to execute command, OSError:" & $taskStr)
+  except Exception:
+    return err("Unable to execute command:" & $taskStr)
+
+proc getBalance*(accountAddress: string): NodeTaskJsonResult =
     let task = "balance --account " & $accountAddress
-    let (output, errC) = execNodeTask(task)
-    debug "getBalance", output
-    return output
+    let res = execNodeTaskJson(task)
+    return res
 
-proc setupSwap*(): JsonNode =
+proc setupSwap*(): NodeTaskJsonResult =
     let task = "setupSwap"
-    let (output, errC) = execNodeTask(task)
-
-    # XXX Assume succeeds
-    let json = parseJson(output)
-    return json
+    let res = execNodeTaskJson(task)
+    return res
  
-# TODO Signature
-proc signCheque*(swapAddress: string): string =
+proc signCheque*(swapAddress: string): NodeTaskJsonResult =
   let task = "signCheque --swapaddress '" & $swapAddress & "'"
-  let (output, errC) = execNodeTask(task)
+  var res = execNodeTaskJson(task)
+  return res
 
- # XXX Assume succeeds
-  let json = parseJson(output)
-  let signature = json["signature"].getStr()
-  debug "signCheque", json=json, signature=signature
-  return signature
-
-proc getERC20Balances*(erc20address: string): JsonNode =
+proc getERC20Balances*(erc20address: string): NodeTaskJsonResult =
     let task = "getBalances --erc20address '" & $erc20address & "'"
-    let (output, errC) = execNodeTask(task)
+    let res = execNodeTaskJson(task)
+    debug "getERC20Balances", res
+    return res
 
-    # XXX Assume succeeds
-    let json = parseJson(output)
-    return json
-
-proc redeemCheque*(swapAddress: string, signature: string): JsonNode =
+proc redeemCheque*(swapAddress: string, signature: string): NodeTaskJsonResult =
   let task = "redeemCheque --swapaddress '" & $swapAddress & "' --signature '" & $signature & "'"
-  let (output, errC) = execNodeTask(task)
+  let res = execNodeTaskJson(task)
+  return res
 
-  # XXX Assume succeeds
-  let json = parseJson(output)
-  return json
+when isMainModule:
+  var aliceSwapAddress = "0x6C3d502f1a97d4470b881015b83D9Dd1062172e1"
+  var sigRes = signCheque(aliceSwapAddress)
+  if sigRes.isOk():
+    echo "All good"
+    echo "Signature ", sigRes[]
+  else:
+    echo sigRes