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

chore: g & g_lagrange assignment for ParamsKZG #236

Closed
wants to merge 2 commits into from
Closed

Conversation

luxebeng
Copy link

@luxebeng luxebeng commented Dec 4, 2023

g and g_lagrange is swapped for correctness.

Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

Oh wow! This was indeed swapped.

Nice catch!

@CPerezz
Copy link
Member

CPerezz commented Dec 4, 2023

@luxebeng can you fix the actual cloning errors?? I didn't notice that the order swapping would cause those.

@CPerezz CPerezz self-requested a review December 4, 2023 16:32
@luxebeng
Copy link
Author

luxebeng commented Dec 4, 2023

It's fixed. sorry for the inconvenience.
@CPerezz, would you please grant permission once again?

@jonathanpwang
Copy link

Wait, this ordering does not affect correctness: the struct definition does not care about order since the fields are named.
The ordering was done probably to avoid the g.clone()?

@luxebeng
Copy link
Author

luxebeng commented Dec 4, 2023

Wait, this ordering does not affect correctness: the struct definition does not care about order since the fields are named. The ordering was done probably to avoid the g.clone()?

Thank you so much for clarification! this PR will be closed directly.

@luxebeng luxebeng closed this Dec 4, 2023
@CPerezz
Copy link
Member

CPerezz commented Dec 5, 2023

Wait, this ordering does not affect correctness: the struct definition does not care about order since the fields are named.

OMG... I really need to focus more.. Apologies for the inconvenience and thanks for the catch @jonathanpwang

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