-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: implement ECDH #8
Conversation
ECDH Encryption completed with BabyJubJub
d re ds
ds BREAKING CHANGE: ds re ds
New Improved Circuit, testing vanilla functions
Added Tests for ECDH in Noir, and also made some changes to the ecdh.tests.js. Also trying to test the vanilla circuit. re N
ds BREAKING CHANGE: ds re ds
ECDH Test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
Thanks for submitting this well described PR!
I'd like to wait for @signorecello to review cause I don't write noir myself.
In the meantime could you ensure the style ci check pass?
BREAKING CHANGE: k re k
👍🏽
Re integration tests. I think there're not actual tests actually. You're just generating a proof without verifying it. Looks like there's a function to verify proofs in the Noir tools: https://github.com/noir-lang/noir/blob/master/tooling/noir_js_backend_barretenberg/src/backend.ts#L127 |
l re l
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions and requests, great work @YashBit !
} | ||
|
||
// ECDH Circuit | ||
fn main(private_key1: [u8; 32], private_key2: [u8; 32]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing that this is a lib
but has a main
function, can this be renamed? Something like generate_shared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we need to have a main function and a package of type
bin, don't we @signorecello?
Because we do want to compile something: we need the compilation artifact for the node test.
Or is there another way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we shouldn't name libraries according to their use in tests... For the node test we would actually write a bin
project and compile that one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YashBit saw you resolved this one, but idk what's your opinion
NoirJS Verification, Unit Tests re N
HELLO re n
refactor - .toml, lib.nr and main.nr in ECDH
refactor: package.json
refactor: globals, lib, ecdh.test.ts
refactor: ecdh.test.ts
refactor: yarn fmt
refactor: .gitignore added target
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wanted to know your opinion on the naming, other than that it looks good to go
} | ||
|
||
// ECDH Circuit | ||
fn main(private_key1: [u8; 32], private_key2: [u8; 32]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean we shouldn't name libraries according to their use in tests... For the node test we would actually write a bin
project and compile that one
} | ||
|
||
// ECDH Circuit | ||
fn main(private_key1: [u8; 32], private_key2: [u8; 32]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YashBit saw you resolved this one, but idk what's your opinion
refactor: removed wait times in test.ts
git commit -m "refactor: package.json"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All my comments have be addressed. We now have a cleaner mocha/nargo tests setup. Local scripts are working.
But the CI is broken: the lockfile looks out of date.
@YashBit yarn.lock
is out of date because you modified dependencies by editing manually package.json
It is better to use yarn add
for that purpose as it will always update the yarn.lock
accordingly.
(And I am sure you didn't use yarn add
because the deps in package.json
aren't sorted 😉)
Can you please bump the lockfile (trash it together with node_modules
then re install deps, then push your changes)
EDIT: fixed in 116894c
refactor: yarn.lock, package.json
@signorecello I think what I have done makes sense We have a main.nr file, which is required for the nargo compile, and we have a separate lib.nr file which contains all the functions (which is in fact imported by main.nr) Without a main.nr file + a pub fn main, there will no circuit and no integrated tests. |
Let's merge this PR. Naming and other issues can be part of another PR in case. |
Description
ECDH Implementation with NoirJS for writing tests.
See ecdh.tests.js in the package.
Here is a detailed description for a GitHub Pull Request (PR) that covers the provided code:
Summary
This PR introduces a set of functions and utilities for elliptic curve cryptography using the Baby JubJub curve. It includes functionality to convert byte arrays to field elements, generate public keys from private keys, and compute shared keys for the Elliptic Curve Diffie-Hellman (ECDH) protocol. The code also provides a main function for generating ECDH shared keys and includes initial placeholders for testing.
Details
Function:
field_from_bytes
bytes
: A fixed-size array of 32 bytes that represents the data to be converted.big_endian
: A boolean flag indicating if the byte array is in big-endian format. Iftrue
, the conversion uses big-endian ordering; otherwise, it uses little-endian ordering.Field
element that represents the converted byte array.as_field
andoffset
variables.Function:
generate_public_key
private_key
: AField
element representing the private key.base_point
: APoint
on the Baby JubJub curve used as the base point for the generation of the public key.Point
on the Baby JubJub curve representing the public key.Function:
main
private_key1
andprivate_key2
: Arrays of 32 bytes representing the private keys.Field
elements.Tests
field_from_bytes_correct
: Tests thefield_from_bytes
function to ensure correct conversion of a byte array to a field element.Changes Made
main
function for computing ECDH shared keys and validating their equality.field_from_bytes
andgenerate_public_key
functions.To Do
generate_public_key
function with real inputs.