fix: improve convertion errors and handle invalid ABI (#387)

* fix: improve convertion errors to provide more context

* fix: better error handling and fix for invalid abi

* fix: add tests for blocks, and minor fixes based on review

* fix: handle contract commitments for contract without data items (#388)

* fix: handle contract commitments for contract without data items

Problem: reexecution panics when a contract doesnt have data items

Solution: handle the edge case to prevent panics

* fix: only one commitment return

* Revert "fix: handle contract commitments for contract without data items (#388)" (#394)

This reverts commit 0c13f1c.

---------

Co-authored-by: Herman Obst Demaestri <[email protected]>

crates/bin/prove_block/src/rpc_utils.rs

@@ -3,6 +3,7 @@ use std::collections::HashMap;
 use blockifier::transaction::objects::TransactionExecutionInfo;
 use cairo_vm::Felt252;
 use num_bigint::BigInt;
+use rpc_client::pathfinder::client::ClientError;
 use rpc_client::pathfinder::proofs::{
     ContractData, EdgePath, PathfinderClassProof, PathfinderProof, ProofVerificationError, TrieNode,
 };
@@ -25,7 +26,7 @@ async fn fetch_storage_proof_for_contract(
     contract_address: Felt,
     keys: &[Felt],
     block_number: u64,
-) -> Result<PathfinderProof, reqwest::Error> {
+) -> Result<PathfinderProof, ClientError> {
     let storage_proof = if keys.is_empty() {
         rpc_client.pathfinder_rpc().get_proof(block_number, contract_address, &[]).await?
     } else {
@@ -50,7 +51,7 @@ async fn get_storage_proof_for_contract<KeyIter: Iterator<Item = StorageKey>>(
     contract_address: ContractAddress,
     storage_keys: KeyIter,
     block_number: u64,
-) -> Result<PathfinderProof, reqwest::Error> {
+) -> Result<PathfinderProof, ClientError> {
     let contract_address_felt = *contract_address.key();
     let keys: Vec<_> = storage_keys.map(|storage_key| *storage_key.key()).collect();
 
@@ -113,7 +114,7 @@ pub(crate) async fn get_storage_proofs(
     block_number: u64,
     tx_execution_infos: &[TransactionExecutionInfo],
     old_block_number: Felt,
-) -> Result<HashMap<Felt, PathfinderProof>, reqwest::Error> {
+) -> Result<HashMap<Felt, PathfinderProof>, ClientError> {
     let accessed_keys_by_address = {
         let mut keys = get_all_accessed_keys(tx_execution_infos);
         // We need to fetch the storage proof for the block hash contract
@@ -124,11 +125,8 @@ pub(crate) async fn get_storage_proofs(
     let mut storage_proofs = HashMap::new();
 
     log::info!("Contracts we're fetching proofs for:");
-    for contract_address in accessed_keys_by_address.keys() {
-        log::info!("    {}", contract_address.to_string());
-    }
-
     for (contract_address, storage_keys) in accessed_keys_by_address {
+        log::info!("    Fetching proof for {}", contract_address.to_string());
         let contract_address_felt = *contract_address.key();
         let storage_proof =
             get_storage_proof_for_contract(client, contract_address, storage_keys.into_iter(), block_number).await?;
@@ -196,7 +194,7 @@ pub(crate) async fn get_class_proofs(
     rpc_client: &RpcClient,
     block_number: u64,
     class_hashes: &[&Felt],
-) -> Result<HashMap<Felt252, PathfinderClassProof>, reqwest::Error> {
+) -> Result<HashMap<Felt252, PathfinderClassProof>, ClientError> {
     let mut proofs: HashMap<Felt252, PathfinderClassProof> = HashMap::with_capacity(class_hashes.len());
     for class_hash in class_hashes {
         let proof = rpc_client.pathfinder_rpc().get_class_proof(block_number, class_hash).await?;

crates/bin/prove_block/tests/prove_block.rs

@@ -47,6 +47,8 @@ use rstest::rstest;
 #[case::dest_ptr_not_a_relocatable_2(155830)]
 #[case::inconsistent_cairo0_class_hash_0(30000)]
 #[case::inconsistent_cairo0_class_hash_1(204936)]
+#[case::no_possible_convertion_1(155007)]
+#[case::no_possible_convertion_2(155029)]
 #[case::reference_pie_with_full_output_enabled(173404)]
 #[case::inconsistent_cairo0_class_hash_2(159674)]
 #[case::inconsistent_cairo0_class_hash_3(164180)]

crates/rpc-client/src/pathfinder/client.rs

@@ -1,10 +1,19 @@
+use reqwest::{Response, StatusCode};
 use serde::de::DeserializeOwned;
 use serde::Deserialize;
 use serde_json::json;
 use starknet_types_core::felt::Felt;
 
 use crate::pathfinder::proofs::{PathfinderClassProof, PathfinderProof};
 
+#[derive(Debug, thiserror::Error)]
+pub enum ClientError {
+    #[error("Encountered a request error: {0}")]
+    ReqwestError(#[from] reqwest::Error),
+    #[error("Encountered a custom error: {0}")]
+    CustomError(String),
+}
+
 fn jsonrpc_request(method: &str, params: serde_json::Value) -> serde_json::Value {
     json!({
         "jsonrpc": "2.0",
@@ -19,7 +28,7 @@ async fn post_jsonrpc_request<T: DeserializeOwned>(
     rpc_provider: &str,
     method: &str,
     params: serde_json::Value,
-) -> Result<T, reqwest::Error> {
+) -> Result<T, ClientError> {
     let request = jsonrpc_request(method, params);
     let response = client.post(format!("{}/rpc/pathfinder/v0.1", rpc_provider)).json(&request).send().await?;
 
@@ -28,12 +37,21 @@ async fn post_jsonrpc_request<T: DeserializeOwned>(
         result: T,
     }
 
-    let response_text = response.text().await?;
-    let response: TransactionReceiptResponse<T> =
-        serde_json::from_str(&response_text).unwrap_or_else(|_| panic!("Error: {}", response_text));
+    let response: TransactionReceiptResponse<T> = handle_error(response).await?;
+
     Ok(response.result)
 }
 
+async fn handle_error<T: DeserializeOwned>(response: Response) -> Result<T, ClientError> {
+    match response.status() {
+        StatusCode::OK => Ok(response.json().await?),
+        s => {
+            let error = response.text().await?;
+            Err(ClientError::CustomError(format!("Received response: {s:?} Error: {error}")))
+        }
+    }
+}
+
 pub struct PathfinderRpcClient {
     /// A raw client to access endpoints not covered by starknet-rs.
     http_client: reqwest::Client,
@@ -56,7 +74,7 @@ impl PathfinderRpcClient {
         block_number: u64,
         contract_address: Felt,
         keys: &[Felt],
-    ) -> Result<PathfinderProof, reqwest::Error> {
+    ) -> Result<PathfinderProof, ClientError> {
         post_jsonrpc_request(
             &self.http_client,
             &self.rpc_base_url,
@@ -70,7 +88,7 @@ impl PathfinderRpcClient {
         &self,
         block_number: u64,
         class_hash: &Felt,
-    ) -> Result<PathfinderClassProof, reqwest::Error> {
+    ) -> Result<PathfinderClassProof, ClientError> {
         log::debug!("querying pathfinder_getClassProof for {:x}", class_hash);
         post_jsonrpc_request(
             &self.http_client,

crates/starknet-os-types/src/casm_contract_class.rs

@@ -3,7 +3,7 @@ use std::sync::Arc;
 
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
-use crate::error::ContractClassError;
+use crate::error::{ContractClassError, ConversionError};
 use crate::hash::GenericClassHash;
 
 pub type CairoLangCasmClass = cairo_lang_starknet_classes::casm_contract_class::CasmContractClass;
@@ -26,8 +26,9 @@ pub struct GenericCasmContractClass {
 fn blockifier_contract_class_from_cairo_lang_class(
     cairo_lang_class: CairoLangCasmClass,
 ) -> Result<BlockifierCasmClass, ContractClassError> {
-    let blockifier_class: BlockifierCasmClass =
-        cairo_lang_class.try_into().map_err(|_| ContractClassError::BlockifierConversionError)?;
+    let blockifier_class: BlockifierCasmClass = cairo_lang_class
+        .try_into()
+        .map_err(|e| ContractClassError::ConversionError(ConversionError::BlockifierError(Box::new(e))))?;
     Ok(blockifier_class)
 }
 
@@ -52,7 +53,7 @@ impl GenericCasmContractClass {
             return Ok(contract_class);
         }
 
-        Err(ContractClassError::NoPossibleConversion)
+        Err(ContractClassError::ConversionError(ConversionError::CairoLangClassMissing))
     }
 
     fn build_blockifier_class(&self) -> Result<BlockifierCasmClass, ContractClassError> {
@@ -68,7 +69,7 @@ impl GenericCasmContractClass {
             return blockifier_contract_class_from_cairo_lang_class(cairo_lang_class);
         }
 
-        Err(ContractClassError::NoPossibleConversion)
+        Err(ContractClassError::ConversionError(ConversionError::BlockifierClassMissing))
     }
     pub fn get_cairo_lang_contract_class(&self) -> Result<&CairoLangCasmClass, ContractClassError> {
         self.cairo_lang_contract_class

crates/starknet-os-types/src/deprecated_compiled_class.rs

@@ -4,7 +4,7 @@ use std::sync::Arc;
 use pathfinder_gateway_types::class_hash::compute_class_hash;
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
-use crate::error::ContractClassError;
+use crate::error::{ContractClassError, ConversionError};
 use crate::hash::GenericClassHash;
 use crate::starknet_core_addons::{decompress_starknet_core_contract_class, LegacyContractDecompressionError};
 
@@ -45,7 +45,7 @@ impl GenericDeprecatedCompiledClass {
             return Ok(contract_class);
         }
 
-        Err(ContractClassError::NoPossibleConversion)
+        Err(ContractClassError::ConversionError(ConversionError::StarknetClassMissing))
     }
 
     fn build_blockifier_class(&self) -> Result<BlockifierDeprecatedClass, ContractClassError> {

crates/starknet-os-types/src/error.rs

@@ -1,12 +1,11 @@
+use std::error::Error;
+
 use cairo_lang_starknet_classes::casm_contract_class::StarknetSierraCompilationError;
 
 #[derive(thiserror::Error, Debug)]
 pub enum ContractClassError {
-    #[error("Internal error: no type conversion is possible to generate the desired effect.")]
-    NoPossibleConversion,
-
-    #[error("Could not build Blockifier contract class")]
-    BlockifierConversionError,
+    #[error(transparent)]
+    ConversionError(#[from] ConversionError),
 
     #[error(transparent)]
     SerdeError(#[from] serde_json::Error),
@@ -17,3 +16,18 @@ pub enum ContractClassError {
     #[error(transparent)]
     CompilationError(#[from] StarknetSierraCompilationError),
 }
+
+#[derive(thiserror::Error, Debug)]
+pub enum ConversionError {
+    #[error("Could not build Blockifier contract class: {0}")]
+    BlockifierError(Box<dyn Error + 'static>),
+
+    #[error("Missing Starknet serialized class")]
+    StarknetClassMissing,
+
+    #[error("Missing Blockifier serialized class")]
+    BlockifierClassMissing,
+
+    #[error("Missing CairoLang serialized class")]
+    CairoLangClassMissing,
+}

crates/starknet-os-types/src/sierra_contract_class.rs

@@ -40,12 +40,10 @@ impl GenericSierraContractClass {
     }
 
     fn build_cairo_lang_class(&self) -> Result<CairoLangSierraContractClass, ContractClassError> {
-        if let Ok(serialized_class) = self.get_serialized_contract_class() {
-            let contract_class = serde_json::from_slice(serialized_class)?;
-            return Ok(contract_class);
-        }
-
-        Err(ContractClassError::NoPossibleConversion)
+        self.get_serialized_contract_class().and_then(|res| {
+            let contract_class = serde_json::from_slice(res)?;
+            Ok(contract_class)
+        })
     }
 
     pub fn get_serialized_contract_class(&self) -> Result<&Vec<u8>, ContractClassError> {
@@ -140,7 +138,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();
 
         Ok(Self {
             sierra_program: sierra_class.sierra_program.clone(),