-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: improve convertion errors and handle invalid ABI #387
base: main
Are you sure you want to change the base?
Conversation
#[derive(Debug, thiserror::Error)] | ||
pub enum ClientError { | ||
#[error("Encountered a request error: {0}")] | ||
ReqwestError(#[from] reqwest::Error), |
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.
From what I've seen of Rust errors, shouldn't this just be Reqwest
? The enum is already named "Error".
#[error("Encountered a request error: {0}")] | ||
ReqwestError(#[from] reqwest::Error), | ||
#[error("Encountered a custom error: {0}")] | ||
Msg(String), |
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.
Custom
would be a better variant name
match self.get_serialized_contract_class() { | ||
Ok(serialized_class) => { | ||
let contract_class = serde_json::from_slice(serialized_class)?; | ||
Ok(contract_class) | ||
} | ||
Err(e) => Err(e), |
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.
I think you can improve this by using Result.and_then
@@ -140,7 +141,8 @@ impl TryFrom<&FlattenedSierraClass> for FlattenedSierraClassWithAbi { | |||
type Error = serde_json::error::Error; | |||
|
|||
fn try_from(sierra_class: &FlattenedSierraClass) -> Result<Self, Self::Error> { | |||
let abi: Option<cairo_lang_starknet_classes::abi::Contract> = serde_json::from_str(&sierra_class.abi)?; | |||
let abi: Option<cairo_lang_starknet_classes::abi::Contract> = | |||
serde_json::from_str(&sierra_class.abi).unwrap_or_default(); |
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.
@whichqua what default would represent here?
Issue Number: N/A
Type
Description
Breaking changes?