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

New bytes_from_field_elements util #234

Merged
merged 15 commits into from
Apr 14, 2023
Merged

New bytes_from_field_elements util #234

merged 15 commits into from
Apr 14, 2023

Conversation

ggutoski
Copy link
Contributor

@ggutoski ggutoski commented Apr 13, 2023 •
edited
Loading

Description

closes: #227

  • add bytes_from_field_elements() to invert bytes_to_field_elements()
  • modify bytes_to_field_elements() to encode bytes length in the first field element for use in inversion
  • remove unneeded bytes copy in bytes_to_field_elements and other tidying
  • generalize both functions from PrimeField to Field
  • TODO: used String as error type for bytes_from_field_elements---should we do something else instead?
  • minimal tests only

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 (main)
  • 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

mrain
mrain previously approved these changes Apr 13, 2023
View reviewed changes
Copy link
Contributor

@mrain mrain left a comment

Choose a reason for hiding this comment

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

LGTM

chancharles92
chancharles92 reviewed Apr 13, 2023
View reviewed changes
Copy link
Contributor

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

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

LGTM, a few minor comments. There is also a minor clippy warning.

utilities/src/conversion.rs Outdated Show resolved Hide resolved
utilities/src/conversion.rs Show resolved Hide resolved
utilities/src/conversion.rs Show resolved Hide resolved
@ggutoski
Copy link
Contributor Author

LGTM, a few minor comments. There is also a minor clippy warning.

Found the clippy warning and fixed it. It was in plonk, so not my fault. 😉

@ggutoski ggutoski merged commit c730140 into main Apr 14, 2023
@ggutoski ggutoski deleted the bytes-to-field branch April 14, 2023 12:39
alxiong
alxiong reviewed Apr 28, 2023
View reviewed changes
utilities/src/conversion.rs Show resolved Hide resolved
utilities/src/conversion.rs Show resolved Hide resolved
ggutoski added a commit that referenced this pull request May 11, 2023
@ggutoski
refactor compile-time checks as per #234 (comment)
333cd27
ggutoski added a commit that referenced this pull request May 11, 2023
@ggutoski
refactor compile-time checks as per #234 (comment)
7e46a86
@ggutoski ggutoski mentioned this pull request May 11, 2023
Merged
6 tasks
alxiong pushed a commit that referenced this pull request May 12, 2023
@ggutoski
chore: bytes-from-field cleanup (#255)
7323d81 
* implement #250 (comment)

* even more rusty than #250 (comment)

* refactor compile-time checks as per #234 (comment)

* tests for extension fields

* pretty rustdoc as requested in #255 (review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrain mrain mrain left review comments

@alxiong alxiong alxiong left review comments

@chancharles92 chancharles92 chancharles92 approved these changes

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

Successfully merging this pull request may close these issues.

Need field_elements_to_bytes util
4 participants
@ggutoski @mrain @chancharles92 @alxiong