-
Notifications
You must be signed in to change notification settings - Fork 70
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
Verifier #173
Verifier #173
Conversation
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.
Lookin good, I left some comments mostly around naming. Will re-review after changes as discussed in discord regarding label order/removing block semantics
785d267
to
26e4ac1
Compare
@th4s , all sinu's comments have been addressed. I will not be adding any new commits. |
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.
Nice work 🚀
I think it would be good to have some more unit tests for this crate, because there are some modules without any tests. We should also fix the warnings from cargo check
when compiling.
All feedback was addressed. I had to break down the document into more testable chunks and make mpc-core a dev dependency.
|
I opened the issue for rs_merkle antouhou/rs-merkle#20
|
@sinui0 , adding you to review since some changes happened since your last review. |
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, couple small things
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.
🚀 Nice work, ack
closing, superseded by #232 |
The Verifier of the notarization document.
See tests/integration_test.rs for an end-to-end test.
I did not write tests for verification failure conditions and for document validation errors. All other tests are present. I will add the missing tests as a separate commit once this PR is ACKed.