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

Remove ToBytes and FromBytes #417

Merged
merged 7 commits into from
Jul 6, 2022
Merged

Remove ToBytes and FromBytes #417

merged 7 commits into from
Jul 6, 2022

Conversation

Pratyush
Copy link
Member

@Pratyush Pratyush commented May 12, 2022

Description

As per title. These traits are insecure, often incorrect, and have been superseded by CanonicalSerialize and CanonicalDeserialize. Closes #390


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

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.

The CI job will complain for now, until corresponding fixes are made afterwards in curves, so should be good to go 👍🏼

@weikengchen
Copy link
Member

A reason why we previously keep them is that they are, at least expected, to match the constraint counterparts, ToBytesGadget, which would be useful for certain serialization.

What do you think?

@Pratyush
Copy link
Member Author

Pratyush commented Jun 1, 2022

I think for usage in constraint world, we need a to_bits variant anyway, as that is more efficient in circuit. Another option is to just bite the bullet and use canonical serialization inside circuits. So, e.g., we make a CanonicalSerializeGadget trait, and use serialize_uncompressed in circuit.

@Pratyush Pratyush merged commit 80857c9 into master Jul 6, 2022
@Pratyush Pratyush deleted the remove-to-from-bytes branch July 6, 2022 02:43
@Pratyush Pratyush linked an issue Sep 2, 2022 that may be closed by this pull request
redshiftzero added a commit to penumbra-zone/decaf377 that referenced this pull request Apr 20, 2023
These traits no longer exist upstream. Instead we should be using
the `CanonicalDeserialize` or `CanonicalSerialize` traits:

arkworks-rs/algebra#417
redshiftzero added a commit to penumbra-zone/decaf377 that referenced this pull request Apr 25, 2023
These traits no longer exist upstream. Instead we should be using
the `CanonicalDeserialize` or `CanonicalSerialize` traits:

arkworks-rs/algebra#417
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.

Projective Coordinates Leak Remove ToBytes and FromBytes traits
3 participants