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

trait FieldElement cleanup #278

Open
2 tasks
tgeoghegan opened this issue Aug 8, 2022 · 4 comments
Open
2 tasks

trait FieldElement cleanup #278

tgeoghegan opened this issue Aug 8, 2022 · 4 comments

Comments

@tgeoghegan
Copy link
Contributor

PR #258 and #277 made some changes to the FieldElement trait. There's still some opportunities for cleanup and improvement here:

  • Hoist some of the method definitions and trait implementations out of the make_field macro and into the FieldElement and FieldElementExt traits. This makes that code easier to read and easier for tools to wrangle. Of course there will be cases where it's easiest to implement something by accessing the private u128 member of the tuple struct that make_field stamps out.
  • Figure out a coherent story for going between various encodings and representations. Right now we have a mishmash of trait impls for serde stuff, prio::codec, a variety of From and TryFroms and some utility methods like byte_slice_into_vec, slice_into_byte_vec, encode_into_bitvector_representation, decode_from_bitvector_representation and valid_integer_try_from. We should strive to harmonize these so they follow common patterns, re-use code as much as possible, and so that their signatures and documentation make it as obvious as possible what they are for.

See also #82.

And on the topic of mod field more generally: we should consider using fast and well-tested finite field and FFT implementations from the Rust cryptography community, like arkworks-rs.

@divergentdave
Copy link
Collaborator

I would like to get rid of FieldElement::slice_into_byte_vec() and FieldElement::byte_slice_into_vec(), as their purpose is better served by Decode or ParameterizedDecode implementations on specific types. These are currently used internally only by Prio2 and Prio2-related tests. Daphne does also use FieldElement::byte_slice_into_vec() as part of its Prio3 aggregate share deserialization. Maybe we could migrate the internal uses and mark them as deprecated.

@cjpatton
Copy link
Collaborator

Maybe we could migrate the internal uses and mark them as deprecated.

This sounds like a safe bet for the next version, but if you do just remove them we should be fine.

@divergentdave
Copy link
Collaborator

Hoist some of the method definitions and trait implementations out of the make_field macro and into the FieldElement and FieldElementExt traits.

The arithmetic-related traits and methods need to stay, as they are tightly coupled to the field's internal representation. I think the implementations of <Vec<u8> as From<F>>, Serialize, and Encode could all be moved out of the macro if we removed them from FieldElement's trait bound, and instead provided a blanket implementation that relied on the impl From<F> for [u8; F::ENCODED_SIZE] conversion. For Decode and Deserialize, we could use the TryFrom<&'a [u8]> implementation.

@cjpatton
Copy link
Collaborator

cjpatton commented Jan 9, 2025

@divergentdave is this still relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants