feat: RLN proofs as a lightpush service #2768
Conversation
|
You can find the image built from this PR at Built from cbb54d7 |
|
You can find the image built from this PR at Built from cbb54d7 |
shash256
changed the title
There was a problem hiding this comment.
The logic is good but code organisation I don't like.
It seams like the RelayPushHandler no longer has only 1 purpose. I would split the handler in 2 (proofs & relay), updating the logic in protocol.nim in the process.
Also, get familiar with .isOkOr: and .valueOr: those are great line savers.
waku/waku_lightpush/callbacks.nim
Outdated
| proc generateAndValidateRLNProof*(rlnPeer: Option[WakuRLNRelay], message: WakuMessage): Result[WakuMessage, string] = | ||
| # TODO: check and validate if the message already has RLN proof? | ||
| if rlnPeer.isNone(): | ||
| return ok(message) # publishing message without RLN proof | ||
|
|
||
| # generate and append RLN proof | ||
| let | ||
| time = getTime().toUnix() | ||
| senderEpochTime = float64(time) | ||
| var msgWithProof = message | ||
| let appendProofRes = rlnPeer.get().appendRLNProof(msgWithProof, senderEpochTime) | ||
| if appendProofRes.isOk(): | ||
| return ok(msgWithProof) | ||
|
|
||
| return err("RLN proof generation failed: " & appendProofRes.error) |
There was a problem hiding this comment.
This should be a callback in RLNRelay that any protocol can use IMO.
There was a problem hiding this comment.
I think this has specific stuff which is relevant to light push such as checking whether rln mounted and, the appendRLNProof might be something that is more suitable for other protocols to be called in general.
waku/node/waku_node.nim
Outdated
| var pushHandler = | ||
| if node.wakuRelay.isNil: | ||
| debug "mounting lightpush without relay (nil)" | ||
| getNilPushHandler() | ||
| else: | ||
| debug "mounting lightpush with relay" | ||
| let rlnPeer = | ||
| if node.wakuRlnRelay.isNil: | ||
| debug "mounting lightpush without rln-relay" | ||
| none(WakuRLNRelay) | ||
| else: | ||
| debug "mounting lightpush with rln-relay" | ||
| some(node.wakuRlnRelay) | ||
| getRelayPushHandler(node.wakuRelay, rlnPeer) |
There was a problem hiding this comment.
Better than what we had but maybe log only mounting lightpush once and add another log in WakuLightPush.new() for the specific cases.
There was a problem hiding this comment.
I think the challenge in this case would be that we would need wakuRlnRelay (or wakuRelay) in light push which as I understand we would want to generally avoid. Also might be good to have mounting related items in waku_node itself for easy readability ?
There was a problem hiding this comment.
LGTM. Some code organisation can be done before merging, but no need to re-request review.
@SionoiS it will be good to understand which specific comments are blocking approval? Minor code re-organisation can generally be done without requiring another approval or be left for subsequent PRs. I agree with most of your proposals, but think splitting the attach-RLN-proof and publish-RLN handlers is out of scope here: it would imply, among other things, changing the lightpush protocol to accept multiple push handlers with strong dependency on the order in which these handlers are called. Could be done, but I think it needs a proper design and is best left for when we introduce multiple services to the lightpush service node.
| ): Future[WakuLightPushResult[void]] {.async.} = | ||
| return err("no waku relay found") | ||
|
|
||
| proc getRelayPushHandler*( |
There was a problem hiding this comment.
Is it necessary to have proc producing another proc?
Isn't it possible to define the necessary callback proc here and just use it as handler there?
Description
Changes