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

revisit BFieldElement's to / from native endian bytes #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jan-ferdinand
Copy link
Member

This PR fixes the following issues with current conversion from “native” endian bytes to base field elements:

  • Make the function infallible. Before, if the passed-in slice was of the incorrect length, the function would panic – see .copy_from_slice(). This behavior was not documented.
  • Add a reciprocal method to get a “native” endianess byte array from a base field element.
  • Test the behavior.

All that said, I think there are potential footguns regarding cross-platform support lingering here. I would argue we should drop “native” endianness in favor of either little or big endian representation, or maybe drop all of that since we already have from_raw_bytes() and .raw_bytes().

Also, introduce `to_ne_bytes`, the logical reciprocal.
@coveralls
Copy link

Coverage Status

coverage: 97.618% (+0.05%) from 97.573%
when pulling 2748b52 on bfe_ne_bytes
into b6f4ceb on master.

@Sword-Smith
Copy link
Member

I totally agree that we shouldn't even have the option of encoding from/to native endianness. I also remarked that in an issue, as it seems we're actually using native endianness at least one place in our code base: #205

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.

3 participants