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

Bug fixes and extra features? #7

Open
nanaknihal opened this issue Jun 11, 2023 · 3 comments
Open

Bug fixes and extra features? #7

nanaknihal opened this issue Jun 11, 2023 · 3 comments

Comments

@nanaknihal
Copy link

I've been using this library for a couple projects and have added various features and bug fixes in a fork: https://github.com/nanaknihal/babyjubjub-rs
If you're interested, I will make a PR. First I'd have to refactor some code because I made some unnecessary breaking changes (e.g., using a different hash) that need to be changed back before I make the PR. Please let me know if that would be helpful and I will make the PR.

List of features I have added

  • Point.on_curve(&self) -> bool and Point.in_subgroup() -> bool methods to check that Points are safe to operate on for certain constructions, e.g. ElGamal decryption.
  • ElGamal encryption and decryption
  • DLEQ proofs
  • Method for taking negative of point,
  • Fl : same as Fr but uses the subgroup's order instead of r
  • Serialization and Deserialization of Fr and Fl into JSON or BigInts. These aren't the most elegant or efficient (using strings as intermediate representation) but they work

List of fixes I can make

  • Fixing bug in Point.mul_scar by only allowing a BigUInt. Currently, it takes BigInt, and actually gives the wrong answer when the inputs is <0. Cleanest way might be to force BigUInt as input which would require breaking changes. Easiest way without breaking would be just to panic if the input is negative.
  • Fixing bug in ORDER being stored as an Fr, when it is larger than r. It should probably rather be stored as a BigInt.

I'm happy to merge these features, and let me know if so which bug fixes I should add as well. The Point.mul_scalar can be fixed number of different ways; if we're fine with panicking that would be an easy fix!

@arnaucube
Copy link
Owner

Sounds good! The initial objective of this repo was to have a rust implementation compatible with circom's EdDSA over BabyJubJub version from circomlib. Also this was done in a moment where arkworks was not a thing yet (nowadays I would use directly arkworks). So, as long as we keep compatibility with circom lib, all improvements and new features are more than welcome ^^

@nanaknihal
Copy link
Author

Finally done in #8 (except the mul_scalar negative input check). Sorry for the delay -- I had changed the blake hash function because I was having trouble compiling on my M1 mac, and couldn't merge until I had a chance to get the original codebase to run. Turns, out I just was missing the flag --features aarch64 for cargo build|test|run which should hopefully help for anyone else running into issues on an Apple Silicon.

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

No branches or pull requests

3 participants
@nanaknihal @arnaucube and others