-
Notifications
You must be signed in to change notification settings - Fork 14
Conversation
…cs/rfcs/71/README.md
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
Co-authored-by: Aaryamann Challani <[email protected]>
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.) |
@jimstir see my comment here: #629 Also, you named this document waku-keystore.md. It should be XX/README.md |
@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. |
hey @jimstir let me know if this is ready for another pass! thanks |
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.
Thanks, left a few comments
content/docs/rfcs/72/README.md
Outdated
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, | ||
]) | ||
) |
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.
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, | |
]) | |
) |
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.
Does the identityCredential
belong in the decryption section? I recently moved it from the input section. @rymnc
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.
its part of the input actually, it isn't used to decrypt the keystore.
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.
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.
Added some comments ! thanks
content/docs/rfcs/72/README.md
Outdated
@@ -202,11 +205,14 @@ Hashing function used: Poseidon Hash, as described in [Poseidon Paper](https://e | |||
|
|||
`version`: "0.2" | |||
|
|||
`hash_function`: "poseidonHash" | |||
`hashFunction`: "poseidonHash" |
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.
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.
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.
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, minor changes, but looks good to merge after they have been resolved. Thanks @jimstir !!
content/docs/rfcs/72/README.md
Outdated
|
||
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. |
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.
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. |
content/docs/rfcs/72/README.md
Outdated
- 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 |
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.
- 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` |
content/docs/rfcs/72/README.md
Outdated
|
||
The `contractId` SHOULD be the blockchain identifier used for `membershipcontract`. | ||
The `chainId` SHOULD be the blockchain identifier used for `membershipcontract`. |
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.
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 |
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.
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. Ready for merge.
Continue discussion: waku-org/specs#2. The RFC process has been changed separating raw specs and the draft/stable specs into different repositories. |
New RFC for 72/WAKU-RLN-KEYSTORE