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

Pin curve25519-dalek dependency #147

Closed

Conversation

thomaseizinger
Copy link

Pre-releases are compared lexicographically. Pre-releases are also allowed to make breaking changes. Depending on a pre-release without pinning is likely to cause breakages downstream.

Pin the dependency until this issue is resolved.

Fixes #146.

@nickray
Copy link

nickray commented Jan 30, 2023

Would probably be better to pin on the release candidate than the prerelease, and simply remove the &?

@thomaseizinger
Copy link
Author

Would probably be better to pin on the release candidate than the prerelease, and simply remove the &?

If that is backwards-compatible, yes. I don't know the public API-surface of this crate and whether curve25519-dalek is exposed or not.

Remember that we need to release as a patch-release to unblock downstream users like CI pipelines that will download the latest compatible version.

@BlackHoleFox
Copy link
Contributor

If that is backwards-compatible, yes. I don't know the public API-surface of this crate and whether curve25519-dalek is exposed or not.

I'm not the crate author but afaict it's not exposed anywhere. Every method that uses it in parameters is not public or ends up working with &[u8] anyway. See also the commit (b581997) which added the dep.

@tarcieri
Copy link
Contributor

FYI: I opened #148 which updates to rc.0

@thomaseizinger
Copy link
Author

If that is backwards-compatible, yes. I don't know the public API-surface of this crate and whether curve25519-dalek is exposed or not.

I'm not the crate author but afaict it's not exposed anywhere. Every method that uses it in parameters is not public or ends up working with &[u8] anyway. See also the commit (b581997) which added the dep.

I am happy with either. I only opened this because I hoped it would be the least controversial and thus the fastest to merge and release so we can unblock downstream users that currently have failing builds.

@mcginty
Copy link
Owner

mcginty commented Jan 31, 2023

Closing in favor of the @tarcieri's pinning to rc.0, thanks for opening this and sorry for the lag in publishing a fixed version.

@mcginty mcginty closed this Jan 31, 2023
@thomaseizinger
Copy link
Author

Closing in favor of the @tarcieri's pinning to rc.0, thanks for opening this and sorry for the lag in publishing a fixed version.

All good, thank you for handling this! :)

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.

cannot multiply &&EdwardsBasepointTable by &Scalar
6 participants