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

chore(rln-relay): use the only key from keystore if only 1 exists #1984

Merged
merged 2 commits into from
Sep 4, 2023

Conversation

rymnc
Copy link
Contributor

@rymnc rymnc commented Sep 4, 2023

Description

from the perspective of an end user, i will most likely have only one membership per chainId-contractAddress pair.
To account for this, we are removing rln-relay-membership-index as a compulsory argument to wakunode2, and instead, using the only available key in the keystore

Changes

  • Changes to the keystore to account for this
  • Updated tests
  • Updated config of wakunode2 and chat2 to make it an Option type
  • Updated OnchainGroupManager init process to set the membershipIndex appropriately

How to test

  1. Create a fresh keystore using rln-keystore-generator - contractAddress: 0x0A988fd9CA5BAebDf098b8A73621b2AaDa6492E8
  2. Run wakunode2 with the following args
./build/wakunode2 \
--rln-relay=true \
--rln-relay-dynamic=true \
--rln-relay-cred-password=$KEYSTORE_PASSWORD \
--rln-relay-cred-path=rlnKeystore.json \
--rln-relay-tree-path=rln_tree_new_1.db \
--rln-relay-eth-contract-address=0x0A988fd9CA5BAebDf098b8A73621b2AaDa6492E8  \
--rln-relay-eth-client-address=$SEPOLIA_URL \
--nodekey=161c2836197e8918d7eb1039758423795f0dc697a018ed55d5931aa9b0e5efb3 \
--log-level=debug

You should see that it is able to fetch the membership index from the decrypted keystore. Additionally, the rpc api is able to append proofs to messages as well.

Issue

Addresses a part of #1906

@rymnc rymnc self-assigned this Sep 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 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.

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:1984

@rymnc rymnc marked this pull request as ready for review September 4, 2023 07:54
Comment on lines 193 to 195
if keystoreCredentials.len == 1:
for v in keystoreCredentials.getFields().values():
keystoreCredential = v
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is quite ugly - the compiler was throwing errors when i tried doing this -

keystoreCredential = keystoreCredentials.getFields().values()[0]

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that because values() returns iterator, which means you'd have to turn it into seq first before you can access the Nth element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, addressed in 99b3fe6!

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm! thanks!

@rymnc rymnc merged commit a14c326 into master Sep 4, 2023
2 checks passed
@rymnc rymnc deleted the keystore-1-key-usage branch September 4, 2023 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants