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

Code review #2

Open
wants to merge 57 commits into
base: code-review
Choose a base branch
from
Open

Code review #2

wants to merge 57 commits into from

Conversation

jspada
Copy link
Owner

@jspada jspada commented Nov 18, 2021

No description provided.

    * Implemented schnorr signature algorithm
    * Got crypto primatives working
    * Added a couple unit test stubs
    * Added network_id to signer context
    * Implemented roinput add_field
    * Started on blinding hash implementation
    * Implemented loading keypair from secret key hex
    * Implemented address generation
    * Added keypair unit tests
    * Work in progress on ROInput add_scalar
    * Created field and scalar helpers and unit tests
    * Simplified keypair deserialization, added unit tests
    * Implemented add_scalar, add_bit and add_bytes for ROInput
    * Implemented some ROInput unit tests
    * Moved Transaction and Notarization to tests module
    * Implemented ROInput add_u32 and add_u64
    * Finished ROInput bits unit tests
    * Added scalar to_string method
    * Implemented ROInput to_bytes
    * Added some ROInput::to_bytes unit tests
    * Implemented ROInput::to_fields serialization
    * Renamed add_ to append_
    * Added simple from_bytes for field elements
    * Created to_field serialization unit tests
    * Added compressed private key struct and helpers
    * Added mina transaction struct for unit testing
    * Implemented random oracle input serialization for transaction struct
    * Added new payment transaction API (with builder pattern)
    * Added pubkey module
    * Moved Mina address creation into PubKeyHelper
    * Simplified double-sha256 calls
    * Implemented safe memo_str()
    * Added memo unit tests
    * Implemented CompressedPubKey decompression
    * Implemented PubKey::from_address(), Mina address -> PubKey
    * Added PubKey::from_address() unit tests
    * Added CompressedPubKey::to_address()
    * Added Signature::to_string()
    * Bugfix to transaction memo (don't forget prefix)
    * Updated memo unit tests
    * Switch from from 64- to 32-byte blake2b output
    * Check whether field element is odd in a better way
    * Added domain_prefix to Input interface
    * Implemented prefix hashing for domain separation (initial poseidon sponge state)
    * Added support for testing delegation transaction signing
    * Implemented equality and debug for Signature struct
    * Imported all payment and delegation signature unit tests
      from other signer implementations
    * Cleanup debug code
    * Make signer context reusable for multiple signatures
    * Simplify Input interface
    * Added verify to signer interface
    * Implemented Mina Schnorr verifier
    * Added verification unit tests
Copy link

@mimoo mimoo left a comment

Choose a reason for hiding this comment

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

Nice work :) very quick first pass on shallow stuff. Can you run cargo fmt and cargo clippy and make sure they return clean?

Cargo.toml Outdated Show resolved Hide resolved
tests/transaction.rs Outdated Show resolved Hide resolved
src/keypair.rs Outdated Show resolved Hide resolved
src/keypair.rs Outdated Show resolved Hide resolved
src/keypair.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/roinput.rs Outdated Show resolved Hide resolved
src/roinput.rs Outdated Show resolved Hide resolved
@jspada
Copy link
Owner Author

jspada commented Nov 22, 2021

Thanks David for awesome code review!

@jspada
Copy link
Owner Author

jspada commented Nov 22, 2021

Do you think FieldHelpers and ScalarHelpers traits (e.g. for to_hex and from_hex) could be handy addition to proof systems generally? I think it would help when it comes to adding test vectors to various parts of the system. Of course we may wish to name them differently! :-)

@mimoo
Copy link

mimoo commented Nov 22, 2021

yeah I think these would be nice to have in o1-utils as an extension trait for Field :o, or perhaps for anything that implements CanonicalSerialize/CanonicalDeserialize.

@jspada
Copy link
Owner Author

jspada commented Nov 22, 2021

yeah I think these would be nice to have in o1-utils as an extension trait for Field :o, or perhaps for anything that implements CanonicalSerialize/CanonicalDeserialize.

Cool - I will try to do that as part of the merge over to proof-systems

    * This really doesn't belong in the FieldHelpers which
      should be agnostic to this
    * Update all the unit tests
…nt types

    * Remove ScalarHelpers and made FieldHelpers generic
    * Renamed library with a more specific name
    * Hashable is for code that only wants to hash
    * Added comments and documentation
    * This provides flexibility to extend things in future and useful customization
    * Disable Debug and Display for SecKey
    * Keypair Debug and Display only show public key part
    * Merged PubKeyHelpers into PubKey implementation
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.

2 participants