-
Notifications
You must be signed in to change notification settings - Fork 149
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
Fix negation of coordinates in assert_equal_public_point
#730
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #730 +/- ##
==========================================
- Coverage 85.07% 85.03% -0.05%
==========================================
Files 47 47
Lines 3879 3922 +43
==========================================
+ Hits 3300 3335 +35
- Misses 579 587 +8
Continue to review full report at Codecov.
|
This change also required to change the negation of the constant in `append_equal_constant`. Tests added as well. Resolves #728
b856d7d
to
564ce2a
Compare
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.
LGTM. Some comments on the structure of the test, but if you don't think they make sense feel free to ignore.
tests/ecc.rs
Outdated
// Test: | ||
// GENERATOR != 42 * GENERATOR | ||
{ | ||
let scalar = JubJubScalar::from(42u64); | ||
let point = dusk_jubjub::GENERATOR; | ||
let public = dusk_jubjub::GENERATOR_EXTENDED * &scalar; | ||
let circuit = DummyCircuit::new(point, public.into()); | ||
|
||
prover | ||
.prove(rng, &circuit) | ||
.expect_err("prover should fail because the points are not equal"); | ||
} |
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 it might be better to make these scopes a function that gets called with different points. Something like:
fn prove_equal_points(point: JubJubAffine, public: JubJubAffine) -> Result<(Proof, Vec<BlsScalar>), Error> {
let circuit = DummyCircuit::new();
prover.prove(rng, &circuit)
}
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.
that would mean cloning the prover and verifier for each of these functions. i think it's a bit of an overkill. But it might work with references. I'll give it some thought
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.
naah, it would need two functions per TestCircuit
, one for testing a satisfied circuit and one for testing an unsatisfied circuit and I also want to have custom failure messages for each test case. I think it wouldn't make things easier to read in the end. Also the TestCircuit
itself doesn't exist outside of the test function etc... too complicated
This change also required to change the negation of the constant in
append_equal_constant
.Tests added as well.
Resolves #728