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

General improvements #56

Merged
merged 5 commits into from
Jan 28, 2022
Merged

General improvements #56

merged 5 commits into from
Jan 28, 2022

Conversation

daxpedda
Copy link
Contributor

@daxpedda daxpedda commented Jan 23, 2022

Builds on #55.

  • Implement all Rust traits for Ristretto255.
  • Remove NonVerifiableServerEvaluateResult as it was holding only one type.
  • Apply warn(missing_debug_implementations) and implement Debug on all public types.
  • Remove leftover copy methods now that we are compiling without alloc.
  • Implement all Rust traits for PreparedEvaluationElement and PreparedTscalar.
  • Introduce more concrete types for complex returned Iterators in the public API.
  • Check for zero scalars during Evaluate according to the spec.
  • Move de/serialization of elements and scalars from GenericArrays to slices.
  • Fix new_with_key would panic if bytes don't have the right length.
  • Fix serde de/serialization to use the checked variants in Group.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 23, 2022
Comment on lines +30 to +31
/// The protocol has failed and can't be completed.
Protocol,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't come up with a better name or description that actually explains the error to a user. I think InverseError doesn't make any sense to a user.

@daxpedda daxpedda marked this pull request as ready for review January 25, 2022 08:06
@daxpedda daxpedda mentioned this pull request Jan 27, 2022
Copy link
Contributor

@kevinlewi kevinlewi 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 overall, will merge!

@kevinlewi kevinlewi merged commit b59b359 into facebook:main Jan 28, 2022
@kevinlewi
Copy link
Contributor

Oh oops, I probably should have re-run the CI before merging this, seems like the main branch now is failing CI. @daxpedda any ideas here?

@daxpedda
Copy link
Contributor Author

Yes ... this is interesting and something I wasn't aware of, depending on pre-releases can automatically update. I thought that pre-releases can not update as they are considered unstable. derive-where is the offender here as it released a breaking change recently, which is fine because it's a pre-release, but downstream has to pin then.

This is addressed by #54, I'm gonna pin pre-release dependencies there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants