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: store contract address in rlnCredentials.txt #1238

Closed
richard-ramos opened this issue Oct 5, 2022 · 12 comments
Closed

feat: store contract address in rlnCredentials.txt #1238

richard-ramos opened this issue Oct 5, 2022 · 12 comments

Comments

@richard-ramos
Copy link
Member

in the rlnCredentials.txt file we should also add the contract address that belongs to those credentials. Credentials are unique per contract, so it probably makes sense to store the contract address in the file, and that way we can avoid having to write the --rln-relay-eth-contract flag if it's already available in the credentials file

@richard-ramos richard-ramos added the track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications label Oct 5, 2022
@staheri14 staheri14 moved this to Later/Icebox in Vac Research Oct 6, 2022
@staheri14 staheri14 removed the status in Vac Research Oct 6, 2022
@rymnc
Copy link
Contributor

rymnc commented Oct 13, 2022

Additionally, we should store the chainId as well

@rymnc
Copy link
Contributor

rymnc commented Oct 13, 2022

Although, one could have the same credentials for different membership sets.
Currently, the rlnCredentials format is as follows -

{
  "membershipKeyPair": {
    "idKey": uint8[],
    "idCommitment": uint8[]
  },
  "rlnIndex": uint256
}

We could have the schema extended in the following manner (if we consider that membership groups exist on only Ethereum smart contracts) -

{
  "version": uint8
  "membershipKeyPair": {
    "idKey": uint8[],
    "idCommitment": uint8[]
  },
  "membershipGroups": {
    "chainId": {
      "contractAddress": {
        "rlnIndex": uint8
      }
    }
  }
}

For example -

{
  "version": 2,
  "membershipKeyPair": {},
  "membershipGroups": {
    "1": {
      "0x000000000": {
        "rlnIndex": 123
      }
    },
    "5": {
      "0x4252105670fe33d2947e8ead304969849e64f2a6": {
         "rlnIndex": 125
      }
    }
  }
}

We should also track these schemas in their own spec with versioning for backwards compatibility.

wdyt? cc: @staheri14 @richard-ramos @jm-clius @s1fr0

@rymnc
Copy link
Contributor

rymnc commented Oct 13, 2022

In favor of anonymity, we should not be using the same idCommitment across different membership groups. One membershipKeyPair should only be used in one membership group.

@staheri14
Copy link
Contributor

In favor of anonymity, we should not be using the same idCommitment across different membership groups.

Right, also (as discussed in our call), not only for anonymity but also for the sake of simplicity it is better to keep one group info for each key pair inside the credential file. Otherwise, we need to give explicit choices to the user to reuse some old persisted credentials on a new chain/group. One may argue it is beneficial for key management i.e., one key pair instead of multiple key pairs across different groups/chains, however that comes with the anonymity cost.

@s1fr0
Copy link
Contributor

s1fr0 commented Oct 13, 2022

I agree that we need to enrich credentials, and probably further allow storage of multiple credentials (why not allowing users that stake double the membership fee to message at double rate?). Depending on this detail (allowing multiple credentials or not) then i would have a contract field and a sequence of membership keypairs. Otherwise if we don't want the same commitment among different membership groups (we cannot enforce it on the smart contract though), the first solution

{
  "version": uint8
  "membershipKeyPair": {
    "idKey": uint8[],
    "idCommitment": uint8[]
  },
  "membershipGroups": {
    "chainId": {
      "contractAddress": {
        "rlnIndex": uint8
      }
    }
  }
}

seems to be the most suitable one

@s1fr0
Copy link
Contributor

s1fr0 commented Oct 13, 2022

As a side comment:membership fees are paid with an eth account so changing idcommitment but using same eth account to pay them would invalidate anonymity in same way.

@staheri14
Copy link
Contributor

staheri14 commented Oct 13, 2022

why not allowing users that stake double the membership fee to message at double rate

@s1fr0 That is a good suggestion (here is the tracking issue #1264), but I think it is not captured by the first solution, what you suggested is an n-to-1 relationship between the credential and the group, whereas the first solution covers the 1-to-n relationship i.e., one credential and multiple groups.

Nevertheless, supporting a higher messaging rate with multiple valid rln credentials requires some careful changes at the protocol level as well. We shall attempt those first before proceeding with any relevant update on the rln credential file.

@staheri14
Copy link
Contributor

As a side comment:membership fees are paid with an eth account so changing idcommitment but using same eth account to pay them would invalidate anonymity in same way.

Right, worth adding a security recommendation to the RFC about this.

@s1fr0
Copy link
Contributor

s1fr0 commented Oct 13, 2022

In a 1-to-n relation I would also probably specify the application using the credential: the same membership contract can be used by different applications (e.g. we allow to do X to all users that subscribed to RLN). This might be of help in case we standardize the credential schema so that can be imported by different applications that will then be immediately be able to check if there is a valid credential for what they need a credential for.

In this sense we should probably not have the keypair as separate fields (since are specific to RLN) but should rather go under application's "credentials" so that we have something similar to the following structure that remains general but allows us to enrich it with new fields without invalidating consistency of previous ones:

application1
    |------ version
    |------ credentials
    |          |------- idkey1, idcommitment1
    |          |            |------- membershipgroups
    |          |                         |------- group1, idx1
    |          |                         |------- group2, idx2
    |          | 
    |          |------- idkey2, idcommitment2
    |                       |------- membershipgroups
    |                                    |------- group1, idx3
    |                                    |------- group3, idx4
    |
    |------ possibly extended with other useful fields
    
application2
    |----- ...

@rymnc
Copy link
Contributor

rymnc commented Oct 14, 2022

Yes, this would however, add complexity to the end user, and may result in a lot more user-prone errors, i.e choosing the wrong credential index. Furthermore, if we were to increase the messaging rate by having multiple membershipKeyPair's on one membershipGroup, the schema should be altered as follows -

{
  "version": uint8
  "membershipGroups": {
    "chainId": {
      "contractAddress": {
        "rlnIndex": uint8,
        "membershipKeyPairs": MembershipKeyPair[]
      }
    }
  }
}

Which would allow us to enforce the relation that each membership group can have one or more membershipKeyPairs, but not the converse.

@s1fr0
Copy link
Contributor

s1fr0 commented Oct 14, 2022

"rlnIndex": uint8,
"membershipKeyPairs": MembershipKeyPair[]

The rlnIndex is per membership key pair, not the same for all of them.

Also, it should be the group that is associated to a credential pairs (that can be reused among multiple application - think about that the pair can be generated from your eth wallet waku-org/js-rln#28) and not the opposite where you'll have many entries replicating the same credential pairs.

@staheri14 staheri14 moved this to New in Vac Research Oct 14, 2022
@staheri14 staheri14 moved this from New to Next/Backlog in Vac Research Oct 14, 2022
@rymnc rymnc moved this from Next/Backlog to Later/Icebox in Vac Research Jan 18, 2023
@rymnc rymnc added track:rlnp2p and removed track:rln RLN Track (Secure Messaging/Applied ZK), e.g. relay and applications labels Jan 19, 2023
@s1fr0
Copy link
Contributor

s1fr0 commented Feb 8, 2023

This issue has been addressed by #1466, which implements #1238 (comment).

@s1fr0 s1fr0 closed this as completed Feb 8, 2023
@github-project-automation github-project-automation bot moved this from Later/Icebox to Done in Vac Research Feb 8, 2023
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

No branches or pull requests

4 participants