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

Add AdditiveGroup trait + Field: AdditiveGroup #577

Merged
merged 13 commits into from
May 10, 2023
Merged

Add AdditiveGroup trait + Field: AdditiveGroup #577

merged 13 commits into from
May 10, 2023

Conversation

mmaker
Copy link
Member

@mmaker mmaker commented Jan 13, 2023 •
edited by Pratyush
Loading

Description

closes: #507

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (master)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

Pratyush
Pratyush reviewed Jan 13, 2023
View reviewed changes
ec/src/lib.rs Outdated Show resolved Hide resolved
ec/src/lib.rs Outdated Show resolved Hide resolved
ff/src/fields/mod.rs Outdated Show resolved Hide resolved
ff/src/fields/mod.rs Show resolved Hide resolved
test-templates/src/fields.rs Outdated Show resolved Hide resolved
@mmaker mmaker force-pushed the feature/additive-groups branch 3 times, most recently from 14e2ce7 to 9e68c0d Compare January 19, 2023 16:15
@mmaker
Copy link
Member Author

mmaker commented Jan 19, 2023

Hi @Pratyush ! I updated the PR with the changes you suggested over discord, can you please check it out?

Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
ec/src/lib.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
ec/src/lib.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
ec/src/scalar_mul/wnaf.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
ec/src/scalar_mul/wnaf.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
test-templates/src/msm.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
test-templates/src/msm.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
test-templates/src/msm.rs Outdated Show resolved Hide resolved
Pratyush
Pratyush reviewed Jan 19, 2023
View reviewed changes
test-templates/src/msm.rs Outdated Show resolved Hide resolved
@Pratyush Pratyush changed the title Add AdditiveGroup trait + Field: AdditiveGroup. (#507) Add AdditiveGroup trait + Field: AdditiveGroup Jan 19, 2023
@Pratyush
Copy link
Member

Hi @mmaker this looks mostly good. Could you add a section in the ff README talking about the new trait?

Pratyush
Pratyush approved these changes Jan 21, 2023
View reviewed changes
Copy link
Member

@Pratyush Pratyush left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This looks good now!

As followups to this PR, let's file an additional tracking issue to clean up the trait hierarchy:

  • Make a new ark-group crate,
  • Move AdditiveGroup to ark-group (this might require some shenanigans in that Scalar might no longer be a Field)
  • Let's also make a MultiplicativeGroup trait.
  • Finally, let's impl MultiplicativeGroup for Field and for PairingOutput

@Pratyush Pratyush mentioned this pull request Jan 21, 2023
Open
@mmaker
Copy link
Member Author

mmaker commented Jan 26, 2023

Great, any chance you can merge it? @yuwen01 needs it to work on ark-circom and update it to 0.4.0

@mmaker mmaker mentioned this pull request Jan 26, 2023
Open
3 tasks
@Pratyush Pratyush added T-refactor Type: cleanup/refactor breaking-change This PR contains a breaking change labels Jan 26, 2023
@Pratyush
Copy link
Member

Hm this is a breaking change to 0.4 as we are renaming traits and changing their contents. I don't think we can merge this in this release, and would probably have to wait until the next one. (Thankfully the next breaking release 0.5 should only be a few months away, and not 2 years away =P)

@mmaker
Copy link
Member Author

mmaker commented Jan 26, 2023

agreed 👍

@mmaker mmaker marked this pull request as ready for review May 7, 2023 21:56
@mmaker mmaker requested review from a team as code owners May 7, 2023 21:56
@mmaker mmaker requested review from mmagician and weikengchen and removed request for a team May 7, 2023 21:56
@mmaker mmaker force-pushed the feature/additive-groups branch 2 times, most recently from 49a7b21 to 2537eba Compare May 7, 2023 22:16
@burdges
Copy link
Contributor

burdges commented May 8, 2023

Just curious: Arkworks handles many protocols in which subgroup checks become quite time consuming, so likely people do start doing non-prime order groups eventually. Any thoughts if this new trait improves that story?

@mmaker mmaker merged commit ab13aa0 into arkworks-rs:master May 10, 2023
@mmaker mmaker deleted the feature/additive-groups branch May 10, 2023 08:39
mmaker added a commit to mmaker/ark-r1cs-std that referenced this pull request May 10, 2023
@mmaker
Import AdditiveGroup.
64a6f2e 
See arkworks-rs/algebra#577.
@mmaker mmaker mentioned this pull request May 10, 2023
Merged
6 tasks
@autquis autquis mentioned this pull request May 12, 2023
Merged
6 tasks
@slumber slumber mentioned this pull request Aug 10, 2023
Merged
6 tasks
@z-tech z-tech mentioned this pull request Oct 4, 2023
Open
aleasims pushed a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023
@mmaker @Pratyush @aleasims
Add AdditiveGroup trait + Field: AdditiveGroup (arkworks-rs#577)
137a621 
* Add AdditiveGroup trait + Field: AdditiveGroup. This trait now integrates ZERO, double, double_in_place, and negate. Fix doctests accordingly (arkworks-rs#507)
* Fix disambiguation of what is a ScalarField in bench-templates. Remove `ScalarField` from `CurveGroup`

---------

Co-authored-by: Pratyush Mishra <[email protected]>
aleasims added a commit to NilFoundation/arkworks-algebra that referenced this pull request Oct 18, 2023
@aleasims
Revert "Add AdditiveGroup trait + Field: AdditiveGroup (arkworks-…
c3c59b6 
…rs#577)"

This reverts commit 137a621.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pratyush Pratyush Pratyush approved these changes

@mmagician mmagician Awaiting requested review from mmagician mmagician is a code owner automatically assigned from arkworks-rs/maintainers

@weikengchen weikengchen Awaiting requested review from weikengchen weikengchen is a code owner automatically assigned from arkworks-rs/maintainers

Assignees
No one assigned
Labels
breaking-change This PR contains a breaking change T-refactor Type: cleanup/refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fields are Groups
4 participants
@mmaker @Pratyush @burdges @mmagician