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

fix: checking for keystore file existence #2427

Merged
merged 10 commits into from
Feb 15, 2024

Conversation

gabrielmer
Copy link
Contributor

@gabrielmer gabrielmer commented Feb 13, 2024

Description

Checking for file existence and not only for a configured path when attempting to mount RLN.

This will avoid errors when users run nwaku-compose without having created the RLN credentials, as currently the /keystore/keystore.json path is passed by default and we try to read from a file that doesn't exist.

Changes

  • checking for keystore file existence before attempting to read from it
  • raising exception if path and password are provided but file doesn't exist
  • added test case

Issue

Helps waku-org/nwaku-compose#32

@gabrielmer gabrielmer changed the title fix: checking for file existence and debug print fix: checking for keystore file existence Feb 13, 2024
Copy link

github-actions bot commented Feb 13, 2024

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2427

Built from 9714f74

@gabrielmer gabrielmer self-assigned this Feb 13, 2024
@gabrielmer gabrielmer marked this pull request as ready for review February 13, 2024 14:21
@@ -622,7 +623,7 @@ method init*(g: OnchainGroupManager): Future[void] {.async.} =
g.rlnContract = some(rlnContract)
g.registryContract = some(registryContract)

if g.keystorePath.isSome() and g.keystorePassword.isSome():
if g.keystorePath.isSome() and existsFile(g.keystorePath.get()) and g.keystorePassword.isSome():
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably want to have a specific error log when a path and password is provided but it doesn't exist, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Added it now :)) lmk what you think about the new version

Comment on lines 659 to 660
else:
warn "File provided as keystore path does not exist", path=g.keystorePath.get()
Copy link
Contributor

Choose a reason for hiding this comment

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

probably want to exit here right, this means the node will not be able to generate proofs.
additionally, can we add a test vector to further ensure that it works as intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect - thought we could just continue relaying messages as it's supposed to be optional. But if path and password are passed, I guess that the intention is to be able to generate proofs.

This implies that in nwaku-compose generating the credentials will be mandatory. Or in other words, we won't be able to run a node by simply running docker-compose up -d without generating credentials and without manually changing run_node.sh

Copy link
Contributor

@rymnc rymnc 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 for this!

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.

LGTM! Thanks for it! 🥇

@gabrielmer gabrielmer merged commit 8f487a2 into master Feb 15, 2024
10 checks passed
@gabrielmer gabrielmer deleted the fix-checking-for-keystore-file branch February 15, 2024 15:33
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.

3 participants