Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

feat(17/WAKU-RLN-RELAY): add RLN credentials format #550

Closed
wants to merge 1 commit into from

Conversation

s1fr0
Copy link
Contributor

@s1fr0 s1fr0 commented Oct 26, 2022

This PR proposes a JSON format for RLN credentials that can be easily generalized to different applications. It supports multi-groups for same (id_key, id_commitment) pair and multiple id pairs per application.

This PR addresses #543

@s1fr0 s1fr0 added track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications track:rfc-process RFC process track (RAD) labels Oct 26, 2022
@s1fr0 s1fr0 self-assigned this Oct 26, 2022
- `version`: a unique and progressive integer that can be used to uniquely identify the RLN credential encoding format.

JSON RLN credentials SHOULD be persisted in encrypted form.
We recommend to use the [Web3 Secret Storage](https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition) to password-protect credentials and to allow cross-client support for RLN credentials decryption and decoding.
Copy link
Contributor

Choose a reason for hiding this comment

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

So not extending EIP-2335 instead? Is Web3 Secret Storage the most up to date way done in the Ethereum ecosystem?

Copy link
Contributor Author

@s1fr0 s1fr0 Nov 2, 2022

Choose a reason for hiding this comment

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

It seems so: https://ethereum.org/en/developers/docs/data-structures-and-encoding/web3-secret-storage/#definition last update 27 October 2022.

Even though the document briefly mentions that this format should encrypt "secret keys", it is described generically and it requires only that encryption is the inverse operation of decryption. Ciphertext size is not explicitly mentioned, so our "extension" might be fall already in the Web3SS format.

Furthermore, the encoding format allows for minor non-breaking sub-versions:

In addition to the version field, which should act as a "hard" identifier of version, implementations may also use minorversion to track smaller, non-breaking changes to the format.

Hence for a minor customization like in our case, it is probably not worth a new EIP, since minor changes seems implicitly admitted already and delegated to developers. What do you think?

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, but should be in a different rfc, as discussed :)

Fields are specified as follow:

- `application` : the application name within which credential(s) refer to.
- `appIdentifier` : a unique identifier for the application. In RLN-RELAY, this SHOULD corresond to the application-specific [RLN identifier](https://rfc.vac.dev/spec/32/#terminology).
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
- `appIdentifier` : a unique identifier for the application. In RLN-RELAY, this SHOULD corresond to the application-specific [RLN identifier](https://rfc.vac.dev/spec/32/#terminology).
- `appIdentifier` : a unique identifier for the application. In RLN-RELAY, this SHOULD correspond to the application-specific [RLN identifier](https://rfc.vac.dev/spec/32/#terminology).

Copy link
Contributor

@staheri14 staheri14 left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good, and I agree having a universal format is a helpful feature for the reusability of the cred files. I left my suggestions below.


```
{
"application": string,
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 contribute to the proof generation?

"application": string,
"appIdentifier": string,
"credentials": [{
"key": string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe Poseidon hash parameters would be good to include.

- `membershipGroups`: the array containing information for membership groups registrations for the pair (`key`, `commitment`). Each node consists of:
- `chainId`: the integer corresponding to the [unique chain-id](https://github.com/ethereum-lists/chains) identifying the chain where the membership contract is deployed.
- `contract`: the smart contract address of the membership contract. It is encoded as a 40-characters left 0-padded hex string with a leading "0x" prefix (42 chars in total).
- `treeIndex`: the Merkle tree index assigned when registering `commitment` to `contract`. It is encoded as a 64-characters left 0-padded hex string with a leading `0x` prefix (10 chars in total).
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it correspond to the leaf index? if yes, I'd explicitly mention it here

Comment on lines +210 to +211
- `application` : the application name within which credential(s) refer to.
- `appIdentifier` : a unique identifier for the application. In RLN-RELAY, this SHOULD corresond to the application-specific [RLN identifier](https://rfc.vac.dev/spec/32/#terminology).
Copy link
Contributor

@staheri14 staheri14 Nov 9, 2022

Choose a reason for hiding this comment

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

I am not sure whether we really need to persist the application and application identifier in this file. That can be provided by the application layer, given that the membership info is not tied to that.

To me, there are two important pieces of info to maintain: The RLN group info (G), and the membership info (M)within that RLN group. They have an n:n relationship, that is, a user can have multiple accounts (M) in a given group (G) hence n:1. Or the same account M in multiple groups Gs i.e., 1:n. Since the former is more likely i.e., n:1, I think it would be more efficient to have the group information as the key and an array of accounts as the value. WDYT?

- `membershipGroups`: the array containing information for membership groups registrations for the pair (`key`, `commitment`). Each node consists of:
- `chainId`: the integer corresponding to the [unique chain-id](https://github.com/ethereum-lists/chains) identifying the chain where the membership contract is deployed.
- `contract`: the smart contract address of the membership contract. It is encoded as a 40-characters left 0-padded hex string with a leading "0x" prefix (42 chars in total).
- `treeIndex`: the Merkle tree index assigned when registering `commitment` to `contract`. It is encoded as a 64-characters left 0-padded hex string with a leading `0x` prefix (10 chars in total).
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest keeping this as part of credentials (following my last comment)

```
Fields are specified as follow:

- `application` : the application name within which credential(s) refer to.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to tie in the fqdn of the web app if relevant ? Similar to the name field in EIP-712?
Do we want to add

Should this be possible, the usage of the credentials in a web app SHOULD be checked against the application, treating the string as the webapp's domain.

@jimstir
Copy link
Contributor

jimstir commented Feb 28, 2024

Continue Discussion : vacp2p/rfc-index#12

@jimstir jimstir closed this Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
track:rfc-process RFC process track (RAD) 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.

5 participants