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

Verifier: Refactor errors in cca module #326

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kartikjoshi21
Copy link
Contributor

Fixes: #231

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Thanks for the great work of replacing anyhow to thiserror. I love this. Let's make it better

attestation-service/verifier/Cargo.toml Show resolved Hide resolved
attestation-service/verifier/Cargo.toml Show resolved Hide resolved
#[error("Failed to discover the verification endpoint details")]
VerificationEndpoint(),
#[error("anyhow error: {0}")]
Anyhow(#[from] anyhow::Error),
Copy link
Member

Choose a reason for hiding this comment

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

I am worried about this kind of error. Other types have determined semantics while this not. Could you try to classify the source of this error as one of the above and use map_err?

attestation-service/verifier/src/cca/mod.rs Show resolved Hide resolved
@@ -80,13 +119,13 @@ impl Verifier for CCA {
expected_init_data_hash: &InitDataHash,
) -> Result<TeeEvidenceParsedClaim> {
let ReportData::Value(expected_report_data) = expected_report_data else {
bail!("CCA verifier must provide report data field!");
return Err(CCAError::ReportDataMissing.into());
Copy link
Member

Choose a reason for hiding this comment

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

I notice that severval .into() are used in this function to convert Result<_, CCAError> to Result<_, anyhow::Error>.

One way to avoid this is to have an inner non-pub function cca_evaluate() -> Result<_, CCAError> to be wrapped by this evaluate(). In cca_evaluate() can delete all the .into()s. And in evaluate() just call cca_evaluate() to finish the error conversion.

Another way is to add another overall error type VerifierError under verifier. One enum is named CCA. And change the signature of evaluate(..)->anyhow::Result<TeeEvidenceParsedClaim> to evaluate(..)->Result<TeeEvidenceParsedClaim, VerifierError>. In this function different CCAErrors will finally be mapped into VerifierError::CCA.

It's up to you : )

Comment on lines +88 to +89
#[error("Serde Json Error")]
SerdeJson(#[from] serde_json::Error),
Copy link
Member

Choose a reason for hiding this comment

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

Could we add some extra information to this kind of error? I mean if this error is raised, the user will only see Serde Json Error and he will not know what is serded

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding error types with source as serde::json error and whole context for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Attestation-Service: replacing anyhow with explicit error types
2 participants