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

Rework Verifiers to be passed in the evidence #11

Merged
merged 2 commits into from
Mar 2, 2023

Conversation

nick-mobilecoin
Copy link
Collaborator

This also changed the ReportBodyVerifier to use a builder pattern for
the fields to verify.

@codecov
Copy link

codecov bot commented Feb 24, 2023

Codecov Report

Merging #11 (cce9f79) into main (f2c7e14) will decrease coverage by 4.48%.
The diff coverage is 82.27%.

@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
- Coverage   95.15%   90.67%   -4.48%     
==========================================
  Files           2        2              
  Lines         227      236       +9     
==========================================
- Hits          216      214       -2     
- Misses         11       22      +11     
Impacted Files Coverage Δ
verifier/src/lib.rs 85.29% <54.83%> (-8.26%) ⬇️
verifier/src/report_body.rs 98.00% <100.00%> (+0.91%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

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

This mostly looks good to me - changes might not be needed, but there were some comments for your consideration.

verifier/src/lib.rs Show resolved Hide resolved
verifier/src/report_body.rs Outdated Show resolved Hide resolved
verifier/src/report_body.rs Outdated Show resolved Hide resolved
This was referenced Feb 28, 2023
Base automatically changed from nick/no_alloc to main February 28, 2023 18:44
This also changed the `ReportBodyVerifier` to use a builder pattern for
the fields to verify.
Copy link

@NotGyro NotGyro left a comment

Choose a reason for hiding this comment

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

This looks good to me

Copy link
Contributor

@jcape jcape left a comment

Choose a reason for hiding this comment

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

This looks good, but I thought we'd decided we were going to have Verifier always take AttestationEvidence?

verifier/src/lib.rs Show resolved Hide resolved
@nick-mobilecoin
Copy link
Collaborator Author

This looks good, but I thought we'd decided we were going to have Verifier always take AttestationEvidence?

We had originally, however since I got it working with generics I figured it would be better to keep it generic unless we run into issues.
this makes it easier to test, and often easier to test == easier to use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Large PRs
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants