-
Notifications
You must be signed in to change notification settings - Fork 57
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
Conversation
You can find the image built from this PR at
Built from 9714f74 |
@@ -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(): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
else: | ||
warn "File provided as keystore path does not exist", path=g.keystorePath.get() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
fee49b0
to
ea608d7
Compare
There was a problem hiding this 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!
There was a problem hiding this 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! 🥇
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
Issue
Helps waku-org/nwaku-compose#32