feat(eigen-cllient-extra-features): Address more comments #386
Conversation
| let response = reqwest::get(url) | ||
| .await | ||
| .map_err(|e| VerificationError::LinkError(e.to_string())) | ||
| .unwrap(); |
There was a problem hiding this comment.
this unwrap should not be here
There was a problem hiding this comment.
Removed all unwraps left over on the file, I'm unsure about this one tho:
impl PointFile {
fn path(&self) -> &str {
match self {
PointFile::Temp(file) => file.path().to_str().unwrap_or_default(), // Safe unwrap because NamedTempFile guarantees a valid path
PointFile::Path(path) => path,
}
}
}Should we keep the unwrap with the comment, use unwrap_or_default or do anything different?
There was a problem hiding this comment.
I think it's better to change the function's return type to std::Path since what you want is a path.
| @@ -316,9 +287,9 @@ impl RawEigenClient { | |||
|
|
|||
| match disperser::BlobStatus::try_from(resp.status)? { | |||
| disperser::BlobStatus::Processing | disperser::BlobStatus::Dispersing => Ok(None), | |||
| disperser::BlobStatus::Failed => Err(anyhow::anyhow!("Blob dispatch failed")), | |||
| disperser::BlobStatus::Failed => anyhow::bail!("Blob dispatch failed"), | |||
There was a problem hiding this comment.
we should not use anyhow here.
There was a problem hiding this comment.
I think it is better to have a rich error type that can be matched and handled appropriately by the caller of this module.
Anyhow is more for CLIs, end-user errors.
There was a problem hiding this comment.
I think a good approach would be to apply the same type of error handling as we are doing now with the external client library, which uses thiserror, do you agree?
| @@ -331,17 +302,12 @@ impl RawEigenClient { | |||
| let blob_info = resp.info.context("No blob header in response")?; | |||
| Ok(Some(blob_info)) | |||
| } | |||
|
|
|||
| _ => Err(anyhow::anyhow!("Received unknown blob status")), | |||
| _ => anyhow::bail!("Received unknown blob status"), | |||
|
We should probably add |
|
LGTM , I approved provided you fix the remaining ones |
juan518munoz
merged commit 0575785
into
eigen-client-extra-features
juan518munoz
deleted the
eigen-client-extra-features-address-comments-third
branch
What ❔
Why ❔
Checklist
zkstack dev fmtandzkstack dev lint.