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

feat: rln-keystore-generator is now a subcommand #2189

Merged
merged 4 commits into from
Nov 9, 2023

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Nov 7, 2023

Description

As per request from @alrevuelta, rln-keystore-generator is now a subcommand in nwaku.

Changes

  • Removed nim.cfg and config for rln keystore generator
  • included within wakunode2 startup

Pending

  • Remove rln-keystore-generator makefile entry

How to test

  1. make -j16 wakunode2
./build/wakunode2 generateRlnKeystore \
--rln-relay-cred-path:./rlnKeystore.json \
--rln-relay-eth-client-address:wss://eth-sepolia.g.alchemy.com/v2/zzz \
--rln-relay-cred-password:sup3rsecure \
--rln-relay-eth-contract-address:0xF471d71E9b1455bBF4b85d475afb9BB0954A29c4 \
--rln-relay-eth-private-key:0xzzz \
--execute

Copy link

github-actions bot commented Nov 7, 2023

This PR may contain changes to configuration options of one of the apps.

If you are introducing a breaking change (i.e. the set of options in latest release would no longer be applicable) make sure the original option is preserved with a deprecation note for 2 following releases before it is actually removed.

Please also make sure the label release-notes is added to make sure any changes to the user interface are properly announced in changelog and release notes.

Copy link

github-actions bot commented Nov 7, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2189

Built from 33d3bfb

@alrevuelta
Copy link
Contributor

Thanks! Some tests are failing but from a functionality point of view this is exactly what we need.

For context, this is to ship the generateRlnKeystore tool together with nwaku, so that it can be used easily without having to build it. For example, using docker it will be as easy as:

docker run quay.io/wakuorg/nwaku-pr:2189 generateRlnKeystore \
--rln-relay-eth-client-address=wss://sepolia.infura.io/ws/v3/xxx \
--rln-relay-eth-private-key=d9f61e035e233e7baabb7ca806f7e9800cfa68397df2d844bf197c3a728cdcef \
--rln-relay-eth-contract-address=0xxx \
--rln-relay-cred-path=keystore.json \
--rln-relay-cred-password=password \
--execute

Which is the same image as for running nwaku.

@alrevuelta alrevuelta requested a review from jm-clius November 7, 2023 14:59
Copy link
Contributor

@jm-clius jm-clius left a comment

Choose a reason for hiding this comment

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

Good feature!

@Ivansete-status
Copy link
Collaborator

Thanks @alrevuelta @rymnc !
Out of curiosity, what is the drawback of having a separate rln-keystore-generator app?

Copy link
Collaborator

@Ivansete-status Ivansete-status left a comment

Choose a reason for hiding this comment

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

I approve because I don't want to be a blocker on that, but I'd rather have separate binaries with clear separate responsibilities, if that makes sense and is feasible of course :)

@alrevuelta
Copy link
Contributor

Out of curiosity, what is the drawback of having a separate rln-keystore-generator app?

imho, that we need another binary, docker image, etc. Having these kind of tools inside nwaku makes it very easy to use, and requires less cognitive load (less amount of binaries/images). As long as they are light (doesn't make the docker image more heavy) and related to nwaku, I think its a good idea to have them. Even this could be part of it.

@Ivansete-status
Copy link
Collaborator

Thanks for the comment! ❤️
I strongly believe that we can't make nwaku to be a Swiss knife. IMHO, it should only have one big responsibility: be a Waku node :)

I vote for having all the "rln" stuff in separate binaries, as we had before this PR.

I'm kind of having that as a temporary solution but to me, the best is having a clear separate Docker file, binary, etc.

wdyt: @NagyZoltanPeter @SionoiS @gabrielmer @rymnc @jm-clius

@rymnc
Copy link
Contributor Author

rymnc commented Nov 8, 2023

I somehow agree with @Ivansete-status here. the tools can be their own docker image with just one entry point, where nwaku remains on its own. I don't mind either approach, but leaning towards Ivan's suggestion

@alrevuelta
Copy link
Contributor

@rymnc @Ivansete-status I actually got the idea from projects like geth or reth. They offer more than 10 subcommand useful for benchmark, debug, and other misc stuff:

@gabrielmer
Copy link
Contributor

Same here :) I'm also keen on the idea of having separate images for independent tools/services, and creating an unique entry point that can combine them all in an user-friendly way. I think it serves the purpose of the user having everything in one place while being more organized and maintainable

@Ivansete-status
Copy link
Collaborator

@rymnc @Ivansete-status I actually got the idea from projects like geth or reth. They offer more than 10 subcommand useful for benchmark, debug, and other misc stuff

Thanks for sharing that!
Nevertheless, I have the impression that we don't follow that approach in Nimbus

@SionoiS
Copy link
Contributor

SionoiS commented Nov 8, 2023

How many of those "useful" command do we have?

At some point we should separate them but I feel it would be premature in this case.

@NagyZoltanPeter
Copy link
Contributor

@rymnc @Ivansete-status I actually got the idea from projects like geth or reth. They offer more than 10 subcommand useful for benchmark, debug, and other misc stuff:

Personally I more fan of such approach as @alrevuelta. We all working with such tools (git, cmake, even docker just to also name some). But! I feel for nwaku we started to confuse our wished users. In case we have nwaku/go-waku/... and they not matching in some key feature set, one can think we are much distracted and not coordinated team.
So in this particular case I agree with @Ivansete-status that nwaku (wakunode2) shall remain a full node impl. And tooling shall be separate. At some point we might think of having a front-end for them - including tooling (it can be CLI either). And still nwaku has a complex cli interface already.

Copy link
Contributor

@NagyZoltanPeter NagyZoltanPeter left a comment

Choose a reason for hiding this comment

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

Apart from the different viewpoint on overload nwaku with somee tooling it looks fine!

@gabrielmer gabrielmer merged commit 3498a84 into master Nov 9, 2023
10 checks passed
@gabrielmer gabrielmer deleted the rln-keystore-generator-subcommand branch November 9, 2023 09:48
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.

7 participants