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

72/WAKU-RLN-KEYSTORE: add new RFC #631

Closed
wants to merge 68 commits into from

Conversation

jimstir
Copy link
Contributor

@jimstir jimstir commented Nov 10, 2023

New RFC for 72/WAKU-RLN-KEYSTORE

@kaiserd kaiserd marked this pull request as draft November 10, 2023 08:37
@kaiserd
Copy link
Contributor

kaiserd commented Nov 10, 2023

Thank you :). Converting this to a draft PR for now. Feel free to open it for review, once you feel it is ready for a first review round.

(Just glancing over it, I saw a few spots that do not follow sembr.)

@kaiserd
Copy link
Contributor

kaiserd commented Nov 10, 2023

@jimstir see my comment here: #629
The directory structure is mixed up.

Also, you named this document waku-keystore.md. It should be XX/README.md
As discussed, feel free to allocate the next new RFC number, and also name this PR accordingly please.
In this case, it would be 72/
Generally, just check the existing RFCs, issues and PRs, and assign the next free number.
If deemed necessary in the future, we might come up with a different number allocation process, but for now, this is the simplest and straightforward.

@jimstir
Copy link
Contributor Author

jimstir commented Nov 10, 2023

@kaiserd Created this draft in the old repo and did not notice. I deleted the keystore file from the push notification repo and changed the name in this repo.

@rymnc
Copy link
Contributor

rymnc commented Jan 4, 2024

hey @jimstir let me know if this is ready for another pass! thanks

@jimstir
Copy link
Contributor Author

jimstir commented Jan 4, 2024

hey @jimstir let me know if this is ready for another pass! thanks

@rymnc I am ready for another round of feedback, thank you.

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.

Thanks, left a few comments

content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
Comment on lines 230 to 236
IDCommitmentBigInt: buildBigIntFromUint8Array(
new Uint8Array([
112, 216, 27, 89, 188, 135, 203, 19, 168, 211, 117, 13, 231, 135, 229,
58, 94, 20, 246, 8, 33, 65, 238, 37, 112, 97, 65, 241, 255, 93, 171,
15,
])
)
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
IDCommitmentBigInt: buildBigIntFromUint8Array(
new Uint8Array([
112, 216, 27, 89, 188, 135, 203, 19, 168, 211, 117, 13, 231, 135, 229,
58, 94, 20, 246, 8, 33, 65, 238, 37, 112, 97, 65, 241, 255, 93, 171,
15,
])
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the identityCredential belong in the decryption section? I recently moved it from the input section. @rymnc

Copy link
Contributor

Choose a reason for hiding this comment

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

its part of the input actually, it isn't used to decrypt the keystore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rymc Yes, I see now, so I forgot to add contractId to the membershipHash creation, which made me a little confused. I changed it to include the four attributes, instead of three previously, here and here

content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
@rymnc rymnc self-requested a review January 12, 2024 05:19
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.

Added some comments ! thanks

content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
@@ -202,11 +205,14 @@ Hashing function used: Poseidon Hash, as described in [Poseidon Paper](https://e

`version`: "0.2"

`hash_function`: "poseidonHash"
`hashFunction`: "poseidonHash"
Copy link
Contributor

Choose a reason for hiding this comment

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

does this hashFunction refer to the hash function used by the rln protocol, or by the keystore? it is necessary to differentiate between the two because we use sha256 to generate the keys for the multiple credentials in the keystore, and poseidon in the protocol which will not change anytime soon. if we do decide to move from poseidon to a different hash function, that will be reflected in the keystore version field, which we will update and ensure the rfc has the reference to it for other implementers as well.

Copy link
Contributor Author

@jimstir jimstir Jan 12, 2024

Choose a reason for hiding this comment

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

@rymnc clarified where poseidon is being used in the test vector section. Also clarified for identity_commitment here.

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, minor changes, but looks good to merge after they have been resolved. Thanks @jimstir !!


The `WakuCredential` will store values used for encrytion and decrypting user's credentials.
The `WakuCredential` will store values used for encryting and decrypting user's keystores.
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
The `WakuCredential` will store values used for encryting and decrypting user's keystores.
The `WakuCredential` will store values used for encrypting and decrypting user's keystores.

- it MUST be used for password verification.
- it MUST follow [EIP-2335](https://eips.ethereum.org/EIPS/eip-2335)
- it MUST follow [EIP-2335](https://eips.ethereum.org/EIPS/eip-2335)
- it MAY use [SHA256](https://www.rfc-editor.org/rfc/rfc4634.txt) as the hash function
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
- it MAY use [SHA256](https://www.rfc-editor.org/rfc/rfc4634.txt) as the hash function
- it SHOULD use [SHA256](https://www.rfc-editor.org/rfc/rfc4634.txt) as the hash function to derive the `membershipHash`


The `contractId` SHOULD be the blockchain identifier used for `membershipcontract`.
The `chainId` SHOULD be the blockchain identifier used for `membershipcontract`.
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
The `chainId` SHOULD be the blockchain identifier used for `membershipcontract`.
The `chainId` SHOULD be the blockchain identifier used for `membershipcontract`. It uniquely defines the chain upon which the registration has occurred.


The `contractId` SHOULD be the blockchain identifier used for `membershipcontract`.
The `chainId` SHOULD be the blockchain identifier used for `membershipcontract`.
- it MUST be a string
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

@kaiserd kaiserd 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. Ready for merge.

content/docs/rfcs/72/README.md Outdated Show resolved Hide resolved
@jimstir
Copy link
Contributor Author

jimstir commented Mar 5, 2024

Continue discussion: waku-org/specs#2. The RFC process has been changed separating raw specs and the draft/stable specs into different repositories.

@jimstir jimstir closed this Mar 5, 2024
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.

None yet

3 participants