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

Pseduo-remove MapToCurve::new, renamed to check_parameters #627

Closed
wants to merge 11 commits into from

Conversation

burdges
Copy link
Contributor

@burdges burdges commented Mar 20, 2023

Description

MapToCurve::new seemingly originates from a more runtime oriented elliptic curve crate: https://github.com/armfazh/redox-ecc/blob/master/src/ellipticcurve.rs#L36

Arguably test_parameters should be inherent methods, invoked by whoever defined the parameters, but maybe not? I left the trait method for now. I suspect the hash-to-curve code would benefit form a wider more idiomatic rewrite btw.


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

¨Jeff added 2 commits March 20, 2023 23:21
MapToCurve::new seemingly originates from a more runtime oriented
elliptic curve crate:
https://github.com/armfazh/redox-ecc/blob/master/src/ellipticcurve.rs#L36

Arguably test_parameters should be inherent methods, invoked by whoever
defined the parameters, but maybe not?  I left the trait method for now.
@burdges burdges requested review from a team as code owners March 20, 2023 23:39
@burdges burdges requested review from Pratyush, mmagician and weikengchen and removed request for a team March 20, 2023 23:39
burdges and others added 5 commits March 21, 2023 01:16
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
Co-authored-by: Pratyush Mishra <pratyushmishra@berkeley.edu>
@burdges burdges changed the title Pseduo-remove MapToCurve::new, renamed to test_parameters Pseduo-remove MapToCurve::new, renamed to check_parameters Mar 21, 2023
¨Jeff and others added 3 commits March 21, 2023 20:31
rustfmt is wrong that f()? should be preferred over f() ?, as errors paths should often be highlighted, but nobody merged that fix yet, so whatever. 
rust-lang/rustfmt#5595
@Pratyush Pratyush added breaking-change This PR contains a breaking change T-refactor Type: cleanup/refactor labels Mar 28, 2023
Copy link
Member

@mmagician mmagician left a comment

Choose a reason for hiding this comment

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

Looks good. @burdges Can you merge latest arkworks master into your branch pls? I can't do it since the PR came from an org

@burdges
Copy link
Contributor Author

burdges commented Apr 23, 2023

Appears @weikengchen did so, no idea how these rules all work.

Just fyi, I've a larger successor master...w3f:arkworks-algebra:xof_h2c approaching readiness, which answers #629 and #630.

@mmagician
Copy link
Member

Seems that the previous merge introduced some conflicts. Maybe that's a good chance for @burdges to add a line to the CHANGELOG :)

@Pratyush
Copy link
Member

I have one question @burdges: the current approach ensures that one can't construct an invalid map and use it, while the new code allow using an invalid mapping. Do you think there's an easy way to preserve the old behaviour?

@burdges
Copy link
Contributor Author

burdges commented Apr 25, 2023

Do you think there's an easy way to preserve the old behaviour?

It could be invoked behind #[cfg(debug_assertions)] further up the stack: https://github.com/arkworks-rs/algebra/compare/master...w3f:arkworks-algebra:xof_h2c?expand=1#diff-d4ed998b41839f70c99ab6471a8c55b448ae708642d0a8e87f826e4b071e79f8R54

I sucked this up into #643 too. I could not parse @mmagician comment but likely it's done there too.

@burdges
Copy link
Contributor Author

burdges commented Sep 8, 2023

I'll close this in favor of #679 which is basically identical, but permits edits by maintainers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This PR contains a breaking change T-refactor Type: cleanup/refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants