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

Changes to match other bls12_381 libraries #56

Closed
wants to merge 6 commits into from

Conversation

mikelodder7
Copy link

Added methods to match other bls12-381 libraries (like bls12_381_plus) such as consistent serialization, convenience methods for hex and byte ordering and use const values which is more consistent with Rust-crypto.

@vmx
Copy link
Contributor

vmx commented May 30, 2023

Thanks for the PR, it contains a lot of good changes.

Currently blstrs tries to be a drop-in replacement for the bls12_381 crate. Some of your changes are currently incompatible with that.

You maintain the blst12_381_plus fork. For me, one of the key things about open source is collaboration and trying to do what's best for the community/ecosystem. While sometimes forks are needed, I really try to keep them to a minimum. Have you tried to upstream your changes to bls12_381? I think it would be great if we could get those forks together and then making blstrs compatible to that.

@mikelodder7
Copy link
Author

Thanks for the PR, it contains a lot of good changes.

Currently blstrs tries to be a drop-in replacement for the bls12_381 crate. Some of your changes are currently incompatible with that.

You maintain the blst12_381_plus fork. For me, one of the key things about open source is collaboration and trying to do what's best for the community/ecosystem. While sometimes forks are needed, I really try to keep them to a minimum. Have you tried to upstream your changes to bls12_381? I think it would be great if we could get those forks together and then making blstrs compatible to that.

Yes I have in the past but they are locked on version 1.56 and in order to incorporate the new hash_to_curve interfaces in elliptic-curve 0.13, this restriction has to be lifted. Until they do, I can't raise a PR to them. Happy to keep a fork of this repo until then.

@mikelodder7
Copy link
Author

That being said, bls12_381_plus is 98% compatible with bls12_381.

@mikelodder7
Copy link
Author

Going to close for now. Perhaps once bls12_381_plus is merged then I can raise a PR again.

@vmx
Copy link
Contributor

vmx commented May 31, 2023

Thanks for the information. I had another look. It's really mostly just additions, so it should be possible to keep compatibility where needed. In regards to the hex encoding/decoding functionality, I'd probably leave those outside of the library, it should be easy enough to keep them outside (I prefer keeping the API surface minimal).

So I'd prefer if you could get changes merged upstream first, but if that turns out to not be possible in the nearer term, feel free to re-open this PR and I'll have yet another look.

@daira
Copy link

daira commented May 31, 2023

Yes I have in the past but they are locked on version 1.56 and in order to incorporate the new hash_to_curve interfaces in elliptic-curve 0.13, this restriction has to be lifted. Until they do, I can't raise a PR to them. Happy to keep a fork of this repo until then.

Um, just ask. We only raise the MSRV if there is a reason to, but this would be a reason.

@mikelodder7
Copy link
Author

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.

3 participants