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: use identity credentials, and expose hash, bulk insert and delete members functions #58

Merged
merged 3 commits into from
May 8, 2023

Conversation

richard-ramos
Copy link
Member

@richard-ramos richard-ramos force-pushed the identity-credentials branch 2 times, most recently from 15c0794 to 2bc0ec2 Compare May 1, 2023 19:36
@richard-ramos richard-ramos marked this pull request as ready for review May 1, 2023 19:37
@richard-ramos
Copy link
Member Author

@rymnc,
While testing these changes, I wasnt able to generate/validate proofs with latest zerokit master. This commit vacp2p/zerokit@5eb98d4 ended being the culprit, because once I git revert it locally, my tests passed. I believe that there are some parts of the code that is using usize byte length instead of 8 bytes. I'll try to figure out tomorrow where that is happening.

Also, something slightly concerning is that executing all the test units is taking a lot of time. The whole test suite takes now 4 minutes.

Chrome Headless 101.0.4950.0 (Linux x86_64): Executed 13 of 13 SUCCESS (4 mins 13.007 secs / 4 mins 12.974 secs)

@richard-ramos richard-ramos requested review from rymnc and weboko May 1, 2023 19:42
@rymnc
Copy link

rymnc commented May 2, 2023

@richard-ramos we could revert the offending commit. Are the tests slow with a working version of zerokit?

@richard-ramos
Copy link
Member Author

@richard-ramos we could revert the offending commit. Are the tests slow with a working version of zerokit?

This PR vacp2p/zerokit#153 reverts the specific lines causing the issue. I'll try to identify which PR slows down the tests

@richard-ramos
Copy link
Member Author

@rymnc,
The following commit introduce slowness into js-rln: vacp2p/zerokit@8cd4bab
it takes a single test from 3 seconds to ~23s

To test this out I did the following:

  1. Checkout this PR, and execute npm install
  2. In a terminal, execute the following, replacing the path in the last line by the location where js-rln was cloned
git clone https://github.com/vacp2p/zerokit
cd zerokit
git checkout 8cd4baba8ac73b0ef71e2fb6049409a8a349347f
git revert 5eb98d4b330e1107ed18c22856707110302263e4
cd rln-wasm
cargo make build
cp pkg/* /path/to/js-rln/node_modules/@waku/zerokit-rln-wasm/.
  1. In the folder where js-rln is located npm run test

To speed up the testing process, you can remove all .spec.ts files except for index.spec.ts

…te members functions

Also update the resources and karma.conf because proof generation is
taking longer than usual
@richard-ramos
Copy link
Member Author

This PR is ready for reviewing now! :)

Copy link
Contributor

@weboko weboko left a comment

Choose a reason for hiding this comment

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

Looks good, only thing is that we'll need to follow up by updating the example.

Copy link
Contributor

@fryorcraken fryorcraken left a comment

Choose a reason for hiding this comment

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

Looks good.

README.md Outdated
@@ -70,24 +70,24 @@ import * as rln from "@waku/rln";
const rlnInstance = await rln.create();
```

### Generating RLN Membership Keypair
#### Generating RLN membership credentials
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting nitpick

Suggested change
#### Generating RLN membership credentials
#### Generating RLN Membership Credentials

example/index.js Outdated
@@ -1,7 +1,7 @@
import * as rln from "@waku/rln";

rln.create().then(async rlnInstance => {
let memKeys = rlnInstance.generateMembershipKey();
let credentials = rlnInstance.generateIdentityCredentials();
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
let credentials = rlnInstance.generateIdentityCredentials();
const credentials = rlnInstance.generateIdentityCredentials();

no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, didn't you/we said we could remove this example? cc @weboko

Copy link
Member Author

Choose a reason for hiding this comment

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

That was on js-noise, although I think we can remove it here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

Before we were talking about noise's example.
We can remove that one but I'd keep it since it helps to test changes in one repo.

@@ -1,6 +1,6 @@
{
"name": "@waku/rln",
"version": "0.0.14",
"version": "0.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Big bump :)
Probably time to sort out automated release management?

Copy link
Contributor

Choose a reason for hiding this comment

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

Was thinking about it as well. I can duplicate from js-wakuto other repos or (just saying) we can move all waku libs to js-waku repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think until we expect someone to use RLN in prod we leave it out. We can then reconsider when RLN is used beyond POCs

this.zkRLN,
seedBytes
);
return MembershipKey.fromBytes(memKeys);
return IdentityCredential.fromBytes(memKeys);
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a difference between Identity Credentials and Credentials?

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally MembershipKey only had IDKey and IDCommitment but in waku-org/nwaku#1466 this structure was renamed to IdentityCredential and added 2 new attributes to it (IDTrapdoor and IDNullifier) and renamed IDKey to IDSecretHash. For the sake of making it easier to compare js-rln with nwaku rln implementation I decided to rename it as well.

Since this is a breaking change I ended up creating a new minor version instead of just bumping up the revision

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.

4 participants