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

Update to RLN v2 #21

Merged
merged 3 commits into from
May 24, 2024
Merged

Update to RLN v2 #21

merged 3 commits into from
May 24, 2024

Conversation

@alrevuelta alrevuelta marked this pull request as ready for review May 23, 2024 16:21
@alrevuelta alrevuelta requested review from richard-ramos and rymnc May 23, 2024 16:21
rln/rln.go Outdated
func (r *RLN) MembershipKeyGen() (*IdentityCredential, error) {
// Accepts an optional parameter that sets the user message limit which defaults
// to DEFAULT_USER_MESSAGE_LIMIT
func (r *RLN) MembershipKeyGen(params ...uint32) (*IdentityCredential, error) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it wouldnt be better to rename params to userMessageLimit`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed 5bd2a08

rln/rln.go Outdated
Comment on lines 231 to 232
if len(proofBytes) != 288 {
return nil, fmt.Errorf("invalid proof generated. size: %d expected: 288",
Copy link
Member

@richard-ramos richard-ramos May 23, 2024

Choose a reason for hiding this comment

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

Since 288 is used both in this and next line, it's probably better to extract to a constant and then the error should be fmt.Errorf("invalid proof generated. size: %d expected: %d", len(proofBytes), yourConstant)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right! 5bd2a08

Copy link
Member

@richard-ramos richard-ramos left a comment

Choose a reason for hiding this comment

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

LGTM. Just left a couple of nitpicks that shouldnt stop merging this PR

Copy link

@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, after Richard's comments are addressed!

@alrevuelta alrevuelta merged commit 54bb48f into master May 24, 2024
4 checks passed
@alrevuelta alrevuelta deleted the bump-rln-v2 branch May 24, 2024 08:17
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.

3 participants