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

Removes FromMont and ToMont from field.Element api #288

Merged
merged 3 commits into from
Dec 19, 2022
Merged

Conversation

gbotrel
Copy link
Collaborator

@gbotrel gbotrel commented Dec 16, 2022

See #276

@gbotrel gbotrel requested a review from Tabaie December 16, 2022 21:29
@gbotrel gbotrel linked an issue Dec 16, 2022 that may be closed by this pull request
@@ -9,6 +9,7 @@ import (
func TestG1IsogenyVectors(t *testing.T) {
t.Parallel()

// TODO @gbotrel fix me test vectors shouldn't set words directly
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, if these come from the standard docs they should be available in decimal or hex form.

@samlaf
Copy link

samlaf commented Oct 2, 2023

Trying to upgrade our version of gnark-crypto from 0.8 to 0.9.
We were using

config := ecc.MultiExpConfig{ScalarsMont: true}

whereas now MultiExpConfig does not have a ScalarsMont argument.

I see that the default was set to false in 0.8. Can I just remove that field and everything will work fine? Or will this lead to breaking changes?

Disclaimer: I don't know what montgomery form is.

@gbotrel
Copy link
Collaborator Author

gbotrel commented Oct 2, 2023

That was actually an overlook on my sight, pointed by other users in the past;

there is actually a subtle breaking change here..

the default is now effectively ecc.MultiExpConfig{ScalarsMont: true} so if you used that everywhere, you should be fine.

But callers without an explicit value would have had the default at: ecc.MultiExpConfig{ScalarsMont: false} and the upgrade would be a silent change.. --> TLDR; all the APIs are now consistant and don't expose the underlying form / encoding of the field elements.

@samlaf
Copy link

samlaf commented Oct 2, 2023

That was actually an overlook on my sight, pointed by other users in the past;

there is actually a subtle breaking change here..

the default is now effectively ecc.MultiExpConfig{ScalarsMont: true} so if you used that everywhere, you should be fine.

But callers without an explicit value would have had the default at: ecc.MultiExpConfig{ScalarsMont: false} and the upgrade would be a silent change.. --> TLDR; all the APIs are now consistant and don't expose the underlying form / encoding of the field elements.

Awesome thanks! Small note: would have been useful if you guys added a Breaking Changes section to the release notes.
Otherwise is there a good reference that you would recommend to understand these montgomery forms and how they are used in this library?

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.

What are the usecases for ToBigInt method?
3 participants