Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(rln-relay): rpc handler to support waku rln relay #1852

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Jul 15, 2023

  • Modifies the endpoint post_waku_v2_relay_v1_message so that when rln is enabled in the node the proof is included automatically in the message that is published.
  • Before this PR, it wasn't possible to publish messages containing RLN proofs via the RLN.

@alrevuelta
Copy link
Contributor

should we modify the rpc endpoint post_waku_v2_relay_v1_message to include something like useRln? The behaviour would be:

  • If useRln=true but rln is not enabled in the node, return error.
  • If useRln=true and rln is enabled in the node, publish the msg including an rln proof
  • If useRln=false do not include the rln proof (legacy behaviour)

would also require changing the spec

@vpavlin
Copy link
Member

vpavlin commented Aug 7, 2023

Would this introduce unnecessary level of complexity? As in if the node has RLN membership info configured and RLN relay enabled, always attach the proof, otherwise, do not

Is there particular reason/need for not sending proof when RLN is enabled on the node?

@rymnc
Copy link
Contributor Author

rymnc commented Aug 7, 2023 via email

@alrevuelta
Copy link
Contributor

@vpavlin yeap agree with the complexity here. The main reason for adding this complexity would be to have always the same input, same output. Without the useRln we have different behavours for the same input.

But well, as an advocate of simplicity im actually leaning towards your suggestion. Same api call but proof is attached or not depending on how the node is configured.

@jm-clius
Copy link
Contributor

Unless there's some hidden complexity, my suggestion would be to leave the API untouched and simply attach proof based on whether node is RLN-configured/compiled or not.

@Ivansete-status
Copy link
Collaborator

I agree with @vpavlin's suggestion.
On the other hand, should we do that in the REST handler? I thought we were moving out from rpc and the idea is to only have REST.

@alrevuelta
Copy link
Contributor

@rymnc Seems we have quorum. Mind rebasing and implementing the changes? Ideally we can have it for the REST api but I'm fine with leaving it for another PR.

Thanks!

@github-actions
Copy link

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1852

let success = node.wakuRlnRelay.appendRLNProof(message,
float64(getTime().toUnix()))
if not success:
raise newException(ValueError, "Failed to append RLN proof to message")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will the appendRLNProof print out any more specific error log? This seems to be too ambiguous for users to know what is going on:)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, we're working on it on the zerokit side where we return an error enum atleast so the error code is known

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, just one comment/question

@alrevuelta alrevuelta merged commit 8bcb0ac into master Aug 16, 2023
12 of 13 checks passed
@alrevuelta alrevuelta deleted the waku-sim-rln-rpc-test branch August 16, 2023 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants