chore(rln-relay): remove all raises and replace with Result types #1321
Conversation
rymnc
added
track:maintenance
track:rln
Jenkins BuildsClick to see older builds (13)
|
| ): RlnRelayResult[bool] {.raises: [Defect, ValueError, IOError, CatchableError, Exception].} = | ||
| waku_rln_relay_mounting_duration_seconds.nanosecondTime: | ||
| let res = mount( | ||
| ): Future[RlnRelayResult[void]] {.async.} = |
There was a problem hiding this comment.
why do we need this change?
There was a problem hiding this comment.
- We don't really need to use bool for a result unless necessary. In this case, we just need to check if it mounted successfully, which is indicated by a success result, i.e ok()
- It is a Future now since we moved from using waitFor to async processing
| return err("could not convert the group key to bytes: " & err.msg) | ||
| return ok(groupKeyPairs) | ||
|
|
||
| proc calcMerkleRoot*(list: seq[IDCommitment]): RlnRelayResult[string] = | ||
| ## returns the root of the Merkle tree that is computed from the supplied list |
There was a problem hiding this comment.
The proc description should change to indicate that the Result output may contain an error if the operation does not go through. This shall be done for all the procs in this PR.
|
|
||
| proc createRLNInstance*(d: int = MerkleTreeDepth): RLNResult {.raises: [Defect, IOError].} = | ||
| proc createRLNInstance*(d: int = MerkleTreeDepth): RLNResult = |
There was a problem hiding this comment.
There seems to be no possibility for errors in this proc, right? then why result type for the output?
There was a problem hiding this comment.
When the rln instance creation fails, etc we do return an error as part of the RlnResult. This would allow appropriate error handling.
| @@ -545,8 +545,8 @@ proc processInput(rfd: AsyncFD) {.async.} = | |||
| echo "You are registered to the rln membership contract, find details of your registration transaction in https://goerli.etherscan.io/tx/0x", txHash | |||
|
|
|||
| echo "rln-relay preparation is in progress..." | |||
| let res = node.mountRlnRelay(conf = conf, spamHandler = some(spamHandler), registrationHandler = some(registrationHandler)) | |||
| if res.isErr: | |||
| let res = await node.mountRlnRelay(conf = conf, spamHandler = some(spamHandler), registrationHandler = some(registrationHandler)) | |||
There was a problem hiding this comment.
Why treat it as an async proc?
There was a problem hiding this comment.
Because the dynamic mounting proc is async. We're trying to avoid use of waitFor unless absolutely necessary (i.e in entrypoints)
There was a problem hiding this comment.
Because the dynamic mounting proc is async. We're trying to avoid use of waitFor unless absolutely necessary (i.e in entrypoints)
Exactly, this is the rule we are following. Only use waitFor at the application level (e.g., chat2 or wakunode2)
The reason is that using waitFor indiscriminately can lead to a live-lock situation where the chronos' futures won't progress. This situation can happen if we wait for a method that (at some point deep in the call hierarchy) also contains a waitFor. The clearest example is asyncTest method: which contains a waitFor keeping the chronos' scheduler polling until all the asynchronous methods/futures have been completed.
|
Merged since this was blocking #1266 |
Should resolve #1189, and allow work to be started on #1245
List of changes -
mountRlnRelayis nowasync: RemovedwaitForProcs that do not raise exceptions, but return their errors with the
RlnRelayResulttype -mountRlnRelayDynamicmountRlnRelayStaticmounttoMembershipKeyPairsmembershipKeyGencreateMembershipListReplaced many
varwithlet, can create a new issue to clean up outliers