-
Notifications
You must be signed in to change notification settings - Fork 310
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
Switch from assert to expect #39
Conversation
@@ -59,7 +59,7 @@ impl KeyGenFirstMsg { | |||
ec_context, | |||
&BigInt::sample_below(&EC::get_q().div_floor(&BigInt::from(3))), | |||
); | |||
assert!(pk.mul_assign(ec_context, &sk).is_ok()); | |||
pk.mul_assign(ec_context, &sk).expect("Assignment expected"); |
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 seems like we can benefit from a cryptographic helper function that would call pk.mul_assign(ec_context, &sk).expect("Failed to multiply and assign")
- so the protocol here could use it without adding an .expect(...)
at each call-site.
IIUC, the only way mul_assign
can fail is if the secret key is invalid.
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.
tests/signing_integ_test.rs
Outdated
assert!(party_two_proof_result.d_log_proof_result.is_ok()); | ||
party_two_proof_result | ||
.d_log_proof_result | ||
.expect("Party two DLog proved."); |
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.
Since the expect(...)
message is used to describe the error context, consider rephrasing it to a "negative" form:
"Party # DLog proved" -> "Incorrect party # DLog proof"
"Correct signature" -> "Invalid signature"
...
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.
Interesting, It sound counter intuitive to me. I read it "expect party dlog proved" instaed of "expect Incorrect party..". Nevermind, you are correct, I will change that.
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.
Thanks for the fix!
A few suggestions inside.
tests/keygen_integ_test.rs
Outdated
|
||
party_one_second_message | ||
.d_log_proof_result | ||
.expect("Party one DLog proved."); |
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.
Consider returning a Result from verify_and_decommit
, so the user won't be able to "forget" to check d_log_proof_result
for success -> #33.
Otherwise, the protocol may continue while the verification failed.
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
BTW, should we use Result
in case of proof/signature errors (instead of panicking)?
If so - I can recommend using the error-chain crate :)
I will create a specific task for error-chain. |
No description provided.