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

chore: Bump curve25519-dalek to v4, sha3 to v0.10 #946

Closed
wants to merge 2 commits into from

Conversation

Xuanwo
Copy link

@Xuanwo Xuanwo commented Apr 21, 2024

Problem

discussed with Sam earlier and we all agree that we should bump sha3 to the latest but we will need to update code base for some API breaking changes

Summary of Changes

Fixes #647

This PR fix #647 by:

  • Bumping curve25519-dalek to v4
  • Bumping sha3 to v0.10

Follow-up tasks:

  • Bumping ed25519-dalek to v2 so we can remove all depends of curve25519-dalek v3

Signed-off-by: Xuanwo <github@xuanwo.io>
@mergify mergify bot requested a review from a team April 21, 2024 06:21
Signed-off-by: Xuanwo <github@xuanwo.io>
@@ -18,7 +18,7 @@ mod target_arch {
type Error = Curve25519Error;

fn try_from(pod: &PodScalar) -> Result<Self, Self::Error> {
Scalar::from_canonical_bytes(pod.0).ok_or(Curve25519Error::PodConversion)
Option::from(Scalar::from_canonical_bytes(pod.0)).ok_or(Curve25519Error::PodConversion)
Copy link
Author

Choose a reason for hiding this comment

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

API like Scalar::from_canonical_bytes returns CtOption which doesn't have ok_or API. Do you have better ideas?

@@ -64,6 +64,7 @@ mod target_arch {

fn try_from(pod: &PodRistrettoPoint) -> Result<Self, Self::Error> {
CompressedRistretto::from_slice(&pod.0)
.map_err(|_| Curve25519Error::PodConversion)?
Copy link
Author

Choose a reason for hiding this comment

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

It is good to omit the error and returns Curve25519Error::PodConversion directly?

@samkim-crypto samkim-crypto added the CI Pull Request is ready to enter CI label Apr 23, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Apr 23, 2024
@samkim-crypto samkim-crypto self-requested a review May 1, 2024 01:18
@samkim-crypto
Copy link

Thanks so much for the contribution! I am starting to look at this (sorry for the delay 🙏 ), but it seems like there is a duplicate PR #513 that also upgrades the curve25519-dalek version. Since the other one is an ealier PR, let's wait until how it turns out first.

@joncinque
Copy link

Closing since this was done with #1693. Thanks for your contribution anyway!

@joncinque joncinque closed this Oct 28, 2024
@Xuanwo Xuanwo deleted the bump-curve25519 branch October 28, 2024 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

chore: bump sha3 version
4 participants