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-Relay): Consolidates the input paths of rln credential, chat2 reads rln credential from file only #1080

Merged
merged 5 commits into from
Aug 18, 2022

Conversation

staheri14
Copy link
Contributor

@staheri14 staheri14 commented Aug 17, 2022

This aim of this PR is to address the issues happened during the rln-relay testnet1 regarding supplying RLN credentials. More details are in #1069.
With the changes included in this PR, there will be only one way to pass Rln credential, that is through rlnCredetnial.txt file (which hopefully soon be replaced with keystone with proper encryption)
For this sake, a new config option is made available named rlnRelayCredPath which is the path to the rlnCredetnial.txt file, if this option is passed, then the credentials are generated and persisted in the file (or retrieved from the file), otherwise, new credentials will be generated for each run but will not be persisted.

The tutorial is also updated to reflect this change.

Note that the RLN credential config options i.e., rln-relay-membership-index, rlnRelayIdCommitmentKey and rlnRelayIdKey are still available (but not recommended in the chat2 tutorial) as we may find use cases for them in the future.

@staheri14 staheri14 mentioned this pull request Aug 17, 2022
15 tasks
@staheri14 staheri14 self-assigned this Aug 17, 2022
@staheri14 staheri14 added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Aug 17, 2022
@staheri14 staheri14 force-pushed the rln-relay/consolidate-rln-credential-input-paths branch from cd9dd8b to e2676da Compare August 17, 2022 23:14
@staheri14 staheri14 changed the title feat(Rln-Relay): Consolidates the input paths of rln credential, accepts credential file only feat(Rln-Relay): Consolidates the input paths of rln credential, chat2 reads rln credential from file only Aug 17, 2022
@staheri14 staheri14 marked this pull request as ready for review August 17, 2022 23:56
@staheri14 staheri14 requested review from jm-clius and s1fr0 August 17, 2022 23:58
@status-im-auto
Copy link
Collaborator

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ bed8601 #1 2022-08-18 04:07:50 ~20 min macos 📦bin

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.

LGTM

Then, the execution command will look like this (inspect the last three config options):
```
./build/chat2 --fleet:test --content-topic:/toy-chat/2/luzhou/proto --rln-relay:true --rln-relay-dynamic:true --eth-mem-contract-address:0x4252105670fe33d2947e8ead304969849e64f2a6 --eth-account-address:your_eth_account --eth-account-privatekey:your_eth_private_key --eth-client-address:your_goerli_node --ports-shift=1 --rln-relay-membership-index:63 --rln-relay-id:xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx --rln-relay-id-commitment:6c6598126ba10d1b70100893b76d7f8d7343eeb8f5ecfd48371b421c5aa6f012
You may pass the `rln-relay-cred-path` config option to specify a path for 1) persisting RLN credential 2) retrieveing persisted RLN credential.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You may pass the `rln-relay-cred-path` config option to specify a path for 1) persisting RLN credential 2) retrieveing persisted RLN credential.
You may pass the `rln-relay-cred-path` config option to specify a path for 1) persisting RLN credential 2) retrieving persisted RLN credential.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 8d6b574

Copy link
Contributor

@s1fr0 s1fr0 left a comment

Choose a reason for hiding this comment

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

LGTM! However, why you allow chosing the file path but not the filename?

This will address (or at least help in addressing) both

Allow re-registration even when the rlncredentials.txt file is present. Maybe via an extra parameter --force-registration=true
Allowing multiple rlnCredentials.txt files for distinct chat2 apps running on the same machine. Potentially prefixing the filenames with the Ethereum account address (good but not sufficient)

by simply passing:

  • a new file name to trigger a new credential registration (if file doesn't exists -> lock it -> registration -> write -> release);
  • an existing filename to load an already generated credential (if file exists -> try load);

@staheri14
Copy link
Contributor Author

staheri14 commented Aug 18, 2022

LGTM! However, why you allow chosing the file path but not the filename?

This will address (or at least help in addressing) both

Allow re-registration even when the rlncredentials.txt file is present. Maybe via an extra parameter --force-registration=true
Allowing multiple rlnCredentials.txt files for distinct chat2 apps running on the same machine. Potentially prefixing the filenames with the Ethereum account address (good but not sufficient)

by simply passing:

  • a new file name to trigger a new credential registration (if file doesn't exists -> lock it -> registration -> write -> release);
  • an existing filename to load an already generated credential (if file exists -> try load);

These are already addressed in this PR. This is possible by changing the path parameter, the file will always have the same name but the enclosing folder can change. Thus, there can be multiple credentials living in different paths.

@staheri14
Copy link
Contributor Author

One additional point about not passing filename is that, this way we will have more control on the file format e.g., being a key-store, db, txt, binary, etc. By limiting it to the path, we make the file format transparent to the user.

@staheri14 staheri14 merged commit 8a6b1cf into master Aug 18, 2022
@staheri14 staheri14 deleted the rln-relay/consolidate-rln-credential-input-paths branch August 18, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants