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.

Sign up for GitHub

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

Jump to bottom

[Refactor] Adds some tests and contains some documentation suggestions for the fix/cast-lossy-field-to-group PR #2272

Merged
merged 6 commits into from
Jan 6, 2024
Merged

[Refactor] Adds some tests and contains some documentation suggestions for the fix/cast-lossy-field-to-group PR #2272

merged 6 commits into from
Jan 6, 2024

Conversation

d0cd
Copy link
Contributor

@d0cd d0cd commented Jan 2, 2024

^

acoglio
acoglio reviewed Jan 2, 2024
View reviewed changes
circuit/types/group/src/lib.rs
@@ -129,6 +129,18 @@ impl<E: Environment> Group<E> {
// i.e. that it is 4 (= cofactor) times the postulated point on the curve.
double_point.enforce_double(self);
}

/// Returns a `Boolean` indicating if `self` is in the largest prime-order subgroup.
pub fn is_in_group(&self) -> Boolean<E> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest adding here some of the comments about the code that was moved here, namely that we can perform scalar multiplication also on (1) points not in the subgroup (the point may not be in the subgroup) and (2) scalars not in the scalar type (the order does not have scalar type). It wouldn't necessarily be the case for this to be possible, if the group type and operations were defined more restrictively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some comments, please advise if additional ones are needed for clarity.

d0cd and others added 3 commits January 6, 2024 11:21
@d0cd @acoglio
Update circuit/types/group/src/helpers/from_x_coordinate.rs
0990ccd 
Co-authored-by: Alessandro Coglio <coglio@kestrel.edu>
Signed-off-by: d0cd <23022326+d0cd@users.noreply.github.com>
@d0cd @acoglio
Update circuit/types/group/src/helpers/from_x_coordinate.rs
7667c68 
Co-authored-by: Alessandro Coglio <coglio@kestrel.edu>
Signed-off-by: d0cd <23022326+d0cd@users.noreply.github.com>
@d0cd
Address some feedback
c8966f6
acoglio
acoglio approved these changes Jan 6, 2024
View reviewed changes
Copy link
Contributor

@acoglio acoglio left a comment

Choose a reason for hiding this comment

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

Just a couple of additional suggestions.

d0cd and others added 2 commits January 6, 2024 18:01
@d0cd @acoglio
Update circuit/types/group/src/lib.rs
d68d035 
Co-authored-by: Alessandro Coglio <2409151+acoglio@users.noreply.github.com>
Signed-off-by: d0cd <23022326+d0cd@users.noreply.github.com>
@d0cd @acoglio
Update circuit/types/field/src/square_root.rs
8088d62 
Co-authored-by: Alessandro Coglio <2409151+acoglio@users.noreply.github.com>
Signed-off-by: d0cd <23022326+d0cd@users.noreply.github.com>
@d0cd d0cd merged commit 918af38 into fix/cast-lossy-field-to-group Jan 6, 2024
1 check was pending
@d0cd d0cd deleted the refactor/cast-lossy-field-to-group branch January 6, 2024 10:02
@howardwu howardwu mentioned this pull request Jan 9, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acoglio acoglio acoglio approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@d0cd @acoglio