From aebceefe1d400e344ee83dd10ab3fd93f048e0c1 Mon Sep 17 00:00:00 2001 From: mohiiit Date: Tue, 24 Dec 2024 15:21:43 +0530 Subject: [PATCH 1/4] refactor: error handling added for the storage functions --- .../src/data_storage/aws_s3/mod.rs | 34 ++++++++++++++++--- .../src/jobs/state_update_job/mod.rs | 31 +++++++++++------ 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/crates/orchestrator/src/data_storage/aws_s3/mod.rs b/crates/orchestrator/src/data_storage/aws_s3/mod.rs index 6eeda7b0..e342d5ea 100644 --- a/crates/orchestrator/src/data_storage/aws_s3/mod.rs +++ b/crates/orchestrator/src/data_storage/aws_s3/mod.rs @@ -6,6 +6,7 @@ use aws_sdk_s3::primitives::ByteStream; use aws_sdk_s3::types::{BucketLocationConstraint, CreateBucketConfiguration}; use aws_sdk_s3::Client; use bytes::Bytes; +use color_eyre::eyre::Context; use color_eyre::Result; use crate::data_storage::DataStorage; @@ -50,8 +51,20 @@ impl AWSS3 { impl DataStorage for AWSS3 { /// Function to get the data from S3 bucket by Key. async fn get_data(&self, key: &str) -> Result { - let response = self.client.get_object().bucket(&self.bucket).key(key).send().await?; - let data_stream = response.body.collect().await.expect("Failed to convert body into AggregatedBytes."); + let response = self + .client + .get_object() + .bucket(&self.bucket) + .key(key) + .send() + .await + .context(format!("Failed to get object from bucket: {}, key: {}", self.bucket, key))?; + + let data_stream = response.body.collect().await.context(format!( + "Failed to collect body into AggregatedBytes for bucket: {}, key: {}", + self.bucket, key + ))?; + tracing::debug!("DataStorage: Collected response body into data stream from {}, key={}", self.bucket, key); let data_bytes = data_stream.into_bytes(); tracing::debug!( @@ -74,7 +87,8 @@ impl DataStorage for AWSS3 { .body(ByteStream::from(data)) .content_type("application/json") .send() - .await?; + .await + .context(format!("Failed to put object in bucket: {}, key: {}", self.bucket, key))?; tracing::debug!( log_type = "DataStorage", @@ -88,7 +102,12 @@ impl DataStorage for AWSS3 { async fn create_bucket(&self, bucket_name: &str) -> Result<()> { if self.bucket_location_constraint.as_str() == "us-east-1" { - self.client.create_bucket().bucket(bucket_name).send().await?; + self.client + .create_bucket() + .bucket(bucket_name) + .send() + .await + .context(format!("Failed to create bucket: {} in us-east-1", bucket_name))?; return Ok(()); } @@ -101,7 +120,12 @@ impl DataStorage for AWSS3 { .bucket(bucket_name) .set_create_bucket_configuration(bucket_configuration) .send() - .await?; + .await + .context(format!( + "Failed to create bucket: {} in region: {}", + bucket_name, + self.bucket_location_constraint.as_str() + ))?; Ok(()) } diff --git a/crates/orchestrator/src/jobs/state_update_job/mod.rs b/crates/orchestrator/src/jobs/state_update_job/mod.rs index 1ac7bdc9..0856670e 100644 --- a/crates/orchestrator/src/jobs/state_update_job/mod.rs +++ b/crates/orchestrator/src/jobs/state_update_job/mod.rs @@ -131,7 +131,7 @@ impl Job for StateUpdateJob { for block_no in block_numbers.iter() { tracing::debug!(job_id = %job.internal_id, block_no = %block_no, "Processing block"); - let snos = self.fetch_snos_for_block(*block_no, config.clone()).await; + let snos = self.fetch_snos_for_block(*block_no, config.clone()).await?; let txn_hash = self .update_state_for_block(config.clone(), *block_no, snos, nonce) .await @@ -320,7 +320,7 @@ impl StateUpdateJob { .await .map_err(|e| JobError::Other(OtherError(e)))?; - let program_output = self.fetch_program_output_for_block(block_no, config.clone()).await; + let program_output = self.fetch_program_output_for_block(block_no, config.clone()).await?; // TODO : // Fetching nonce before the transaction is run @@ -336,21 +336,30 @@ impl StateUpdateJob { } /// Retrieves the SNOS output for the corresponding block. - async fn fetch_snos_for_block(&self, block_no: u64, config: Arc) -> StarknetOsOutput { + async fn fetch_snos_for_block(&self, block_no: u64, config: Arc) -> Result { let storage_client = config.storage(); let key = block_no.to_string() + "/" + SNOS_OUTPUT_FILE_NAME; - let snos_output_bytes = storage_client.get_data(&key).await.expect("Unable to fetch snos output for block"); - serde_json::from_slice(snos_output_bytes.iter().as_slice()) - .expect("Unable to convert the data into snos output") + + let snos_output_bytes = storage_client.get_data(&key).await.map_err(|e| JobError::Other(OtherError(e)))?; + + serde_json::from_slice(snos_output_bytes.iter().as_slice()).map_err(|e| { + JobError::Other(OtherError(eyre!("Failed to deserialize SNOS output for block {}: {}", block_no, e))) + }) } - async fn fetch_program_output_for_block(&self, block_number: u64, config: Arc) -> Vec<[u8; 32]> { + async fn fetch_program_output_for_block( + &self, + block_number: u64, + config: Arc, + ) -> Result, JobError> { let storage_client = config.storage(); let key = block_number.to_string() + "/" + PROGRAM_OUTPUT_FILE_NAME; - let program_output = storage_client.get_data(&key).await.expect("Unable to fetch snos output for block"); - let decode_data: Vec<[u8; 32]> = - bincode::deserialize(&program_output).expect("Unable to decode the fetched data from storage provider."); - decode_data + + let program_output = storage_client.get_data(&key).await.map_err(|e| JobError::Other(OtherError(e)))?; + + bincode::deserialize(&program_output).map_err(|e| { + JobError::Other(OtherError(eyre!("Failed to deserialize program output for block {}: {}", block_number, e))) + }) } /// Insert the tx hashes into the the metadata for the attempt number - will be used later by From 17c4fa831f54d8573554551925b91165a58a4eab Mon Sep 17 00:00:00 2001 From: mohiiit Date: Tue, 24 Dec 2024 17:37:09 +0530 Subject: [PATCH 2/4] chore: removed expect --- crates/orchestrator/src/jobs/da_job/mod.rs | 46 ++++++++++--------- .../src/jobs/state_update_job/utils.rs | 2 +- crates/orchestrator/src/queue/job_queue.rs | 3 +- crates/orchestrator/src/workers/snos.rs | 12 +++-- .../atlantic-service/src/client.rs | 3 +- crates/settlement-clients/ethereum/src/lib.rs | 20 ++++---- crates/settlement-clients/starknet/src/lib.rs | 4 +- 7 files changed, 49 insertions(+), 41 deletions(-) diff --git a/crates/orchestrator/src/jobs/da_job/mod.rs b/crates/orchestrator/src/jobs/da_job/mod.rs index 54e96655..ab71ab6d 100644 --- a/crates/orchestrator/src/jobs/da_job/mod.rs +++ b/crates/orchestrator/src/jobs/da_job/mod.rs @@ -125,7 +125,11 @@ impl Job for DaJob { let blob_data_biguint = convert_to_biguint(blob_data.clone()); tracing::trace!(job_id = ?job.id, "Converted blob data to BigUint"); - let transformed_data = fft_transformation(blob_data_biguint); + let transformed_data = + fft_transformation(blob_data_biguint).wrap_err("Failed to apply FFT transformation").map_err(|e| { + tracing::error!(job_id = ?job.id, error = ?e, "Failed to apply FFT transformation"); + JobError::Other(OtherError(e)) + })?; // data transformation on the data tracing::trace!(job_id = ?job.id, "Applied FFT transformation"); @@ -204,17 +208,18 @@ impl Job for DaJob { } #[tracing::instrument(skip(elements))] -pub fn fft_transformation(elements: Vec) -> Vec { +pub fn fft_transformation(elements: Vec) -> Result, JobError> { let xs: Vec = (0..*BLOB_LEN) .map(|i| { let bin = format!("{:012b}", i); let bin_rev = bin.chars().rev().collect::(); - GENERATOR.modpow( - &BigUint::from_str_radix(&bin_rev, 2).expect("Not able to convert the parameters into exponent."), - &BLS_MODULUS, - ) + let exponent = BigUint::from_str_radix(&bin_rev, 2) + .wrap_err("Failed to convert binary string to exponent") + .map_err(|e| JobError::Other(OtherError(e)))?; + Ok(GENERATOR.modpow(&exponent, &BLS_MODULUS)) }) - .collect(); + .collect::, JobError>>()?; + let n = elements.len(); let mut transform: Vec = vec![BigUint::zero(); n]; @@ -223,7 +228,7 @@ pub fn fft_transformation(elements: Vec) -> Vec { transform[i] = (transform[i].clone().mul(&xs[i]).add(&elements[j])).rem(&*BLS_MODULUS); } } - transform + Ok(transform) } pub fn convert_to_biguint(elements: Vec) -> Vec { @@ -310,7 +315,7 @@ pub async fn state_update_to_blob_data( nonce = Some(get_current_nonce_result); } - let da_word = da_word(class_flag.is_some(), nonce, storage_entries.len() as u64); + let da_word = da_word(class_flag.is_some(), nonce, storage_entries.len() as u64)?; blob_data.push(address); blob_data.push(da_word); @@ -355,7 +360,7 @@ async fn store_blob_data(blob_data: Vec, block_number: u64, config: Arc /// DA word encoding: /// |---padding---|---class flag---|---new nonce---|---num changes---| /// 127 bits 1 bit 64 bits 64 bits -fn da_word(class_flag: bool, nonce_change: Option, num_changes: u64) -> Felt { +fn da_word(class_flag: bool, nonce_change: Option, num_changes: u64) -> Result { // padding of 127 bits let mut binary_string = "0".repeat(127); @@ -367,13 +372,8 @@ fn da_word(class_flag: bool, nonce_change: Option, num_changes: u64) -> Fe } // checking for nonce here - if let Some(_new_nonce) = nonce_change { - let bytes: [u8; 32] = nonce_change - .expect( - "Not able to convert the nonce_change var into [u8; 32] type. Possible Error : Improper parameter \ - length.", - ) - .to_bytes_be(); + if let Some(new_nonce) = nonce_change { + let bytes: [u8; 32] = new_nonce.to_bytes_be(); let biguint = BigUint::from_bytes_be(&bytes); let binary_string_local = format!("{:b}", biguint); let padded_binary_string = format!("{:0>64}", binary_string_local); @@ -387,12 +387,16 @@ fn da_word(class_flag: bool, nonce_change: Option, num_changes: u64) -> Fe let padded_binary_string = format!("{:0>64}", binary_representation); binary_string += &padded_binary_string; - let biguint = BigUint::from_str_radix(binary_string.as_str(), 2).expect("Invalid binary string"); + let biguint = BigUint::from_str_radix(binary_string.as_str(), 2) + .wrap_err("Failed to convert binary string to BigUint") + .map_err(|e| JobError::Other(OtherError(e)))?; // Now convert the BigUint to a decimal string let decimal_string = biguint.to_str_radix(10); - Felt::from_dec_str(&decimal_string).expect("issue while converting to fieldElement") + Felt::from_dec_str(&decimal_string) + .wrap_err("Failed to convert decimal string to FieldElement") + .map_err(|e| JobError::Other(OtherError(e))) } fn refactor_state_update(state_update: &mut StateDiff) { @@ -453,7 +457,7 @@ pub mod test { #[case] expected: String, ) { let new_nonce = if new_nonce > 0 { Some(Felt::from(new_nonce)) } else { None }; - let da_word = da_word(class_flag, new_nonce, num_changes); + let da_word = da_word(class_flag, new_nonce, num_changes).expect("Failed to create DA word"); let expected = Felt::from_dec_str(expected.as_str()).unwrap(); assert_eq!(da_word, expected); } @@ -562,7 +566,7 @@ pub mod test { // converting the data to its original format let ifft_blob_data = blob::recover(original_blob_data.clone()); // applying the fft function again on the original format - let fft_blob_data = fft_transformation(ifft_blob_data); + let fft_blob_data = fft_transformation(ifft_blob_data).expect("FFT transformation failed during test"); // ideally the data after fft transformation and the data before ifft should be same. assert_eq!(fft_blob_data, original_blob_data); diff --git a/crates/orchestrator/src/jobs/state_update_job/utils.rs b/crates/orchestrator/src/jobs/state_update_job/utils.rs index ab8bb7fb..a6acfe4e 100644 --- a/crates/orchestrator/src/jobs/state_update_job/utils.rs +++ b/crates/orchestrator/src/jobs/state_update_job/utils.rs @@ -59,7 +59,7 @@ pub fn bytes_to_vec_u8(bytes: &[u8]) -> color_eyre::Result> { let trimmed = line.trim(); assert!(!trimmed.is_empty()); - let result = U256::from_str(trimmed).expect("Unable to convert line"); + let result = U256::from_str(trimmed)?; let res_vec = result.to_be_bytes_vec(); let hex = to_padded_hex(res_vec.as_slice()); let vec_hex = hex_string_to_u8_vec(&hex) diff --git a/crates/orchestrator/src/queue/job_queue.rs b/crates/orchestrator/src/queue/job_queue.rs index 1527f493..93d079ff 100644 --- a/crates/orchestrator/src/queue/job_queue.rs +++ b/crates/orchestrator/src/queue/job_queue.rs @@ -231,7 +231,8 @@ fn parse_worker_message(message: &Delivery) -> Result job.internal_id.parse::().expect("Failed to parse job internal ID to u64"), - None => "0".to_string().parse::().expect("Failed to parse '0' to u64"), - }; + let latest_job_id = latest_job_in_db + .map(|job| { + job.internal_id + .parse::() + .wrap_err_with(|| format!("Failed to parse job internal ID: {}", job.internal_id)) + }) + .unwrap_or(Ok(0))?; // To be used when testing in specific block range let block_start = if let Some(min_block_to_process) = config.service_config().min_block_to_process { diff --git a/crates/prover-clients/atlantic-service/src/client.rs b/crates/prover-clients/atlantic-service/src/client.rs index 5b00436d..62ee2487 100644 --- a/crates/prover-clients/atlantic-service/src/client.rs +++ b/crates/prover-clients/atlantic-service/src/client.rs @@ -85,8 +85,7 @@ impl AtlanticClient { ) .send() .await - .map_err(AtlanticError::AddJobFailure) - .expect("Failed to add job"); + .map_err(AtlanticError::AddJobFailure)?; if response.status().is_success() { response.json().await.map_err(AtlanticError::AddJobFailure) diff --git a/crates/settlement-clients/ethereum/src/lib.rs b/crates/settlement-clients/ethereum/src/lib.rs index ce620ae4..4f978348 100644 --- a/crates/settlement-clients/ethereum/src/lib.rs +++ b/crates/settlement-clients/ethereum/src/lib.rs @@ -19,7 +19,7 @@ use alloy::signers::local::PrivateKeySigner; use alloy_primitives::Bytes; use async_trait::async_trait; use c_kzg::{Blob, Bytes32, KzgCommitment, KzgProof, KzgSettings}; -use color_eyre::eyre::{bail, eyre, Ok}; +use color_eyre::eyre::{bail, Ok}; use color_eyre::Result; use conversion::{get_input_data_for_eip_4844, prepare_sidecar}; use settlement_client_interface::{SettlementClient, SettlementVerificationStatus}; @@ -36,6 +36,7 @@ pub mod tests; pub mod types; use alloy::providers::RootProvider; use alloy::transports::http::Http; +use color_eyre::eyre::WrapErr; use lazy_static::lazy_static; use mockall::automock; use reqwest::Client; @@ -164,7 +165,10 @@ impl EthereumSettlementClient { &KZG_SETTINGS, )?; - if !eval { Err(eyre!("ERROR : Assertion failed, not able to verify the proof.")) } else { Ok(kzg_proof) } + if !eval { + bail!("ERROR : Assertion failed, not able to verify the proof."); + } + Ok(kzg_proof) } } @@ -238,14 +242,10 @@ impl SettlementClient for EthereumSettlementClient { // x_0_value : program_output[10] // Updated with starknet 0.13.2 spec - let kzg_proof = Self::build_proof( - state_diff, - Bytes32::from_bytes(program_output[X_0_POINT_OFFSET].as_slice()) - .expect("Not able to get x_0 point params."), - y_0, - ) - .expect("Unable to build KZG proof for given params.") - .to_owned(); + let x_0_point = Bytes32::from_bytes(program_output[X_0_POINT_OFFSET].as_slice()) + .wrap_err("Failed to get x_0 point params")?; + + let kzg_proof = Self::build_proof(state_diff, x_0_point, y_0).wrap_err("Failed to build KZG proof")?.to_owned(); let input_bytes = get_input_data_for_eip_4844(program_output, kzg_proof)?; diff --git a/crates/settlement-clients/starknet/src/lib.rs b/crates/settlement-clients/starknet/src/lib.rs index ade0d17b..b9b5598b 100644 --- a/crates/settlement-clients/starknet/src/lib.rs +++ b/crates/settlement-clients/starknet/src/lib.rs @@ -8,7 +8,7 @@ use std::sync::Arc; use appchain_core_contract_client::clients::StarknetCoreContractClient; use appchain_core_contract_client::interfaces::core_contract::CoreContract; use async_trait::async_trait; -use color_eyre::eyre::eyre; +use color_eyre::eyre::{eyre, WrapErr}; use color_eyre::Result; use crypto_bigint::Encoding; use lazy_static::lazy_static; @@ -250,7 +250,7 @@ impl SettlementClient for StarknetSettlementClient { return Err(eyre!("Could not fetch last block number from core contract.")); } - Ok(u64_from_felt(block_number[0]).expect("Failed to convert to u64")) + u64_from_felt(block_number[0]).wrap_err("Failed to convert block number from Felt to u64") } /// Returns the nonce for the wallet in use. From 5cf92f21a2440709687911b7abb5afec12d1690c Mon Sep 17 00:00:00 2001 From: mohiiit Date: Tue, 24 Dec 2024 18:21:02 +0530 Subject: [PATCH 3/4] chore: changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e8b0acf..421cb5e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## Changed +- refactor: expect removed and added error wraps - refactor: Readme and .env.example - refactor: http_mock version updated - refactor: prover-services renamed to prover-clients From 39696b7057be5125c660e9642551007d60744fcd Mon Sep 17 00:00:00 2001 From: mohiiit Date: Thu, 2 Jan 2025 15:51:19 +0530 Subject: [PATCH 4/4] refactor: http-client refactored to handle panic better --- crates/orchestrator/src/queue/job_queue.rs | 11 +- .../atlantic-service/src/client.rs | 21 ++-- .../atlantic-service/src/error.rs | 14 +++ .../sharp-service/src/client.rs | 5 +- .../prover-clients/sharp-service/src/error.rs | 11 ++ crates/utils/src/http_client.rs | 110 +++++++++++------- 6 files changed, 113 insertions(+), 59 deletions(-) diff --git a/crates/orchestrator/src/queue/job_queue.rs b/crates/orchestrator/src/queue/job_queue.rs index 93d079ff..28161550 100644 --- a/crates/orchestrator/src/queue/job_queue.rs +++ b/crates/orchestrator/src/queue/job_queue.rs @@ -58,8 +58,14 @@ pub struct WorkerTriggerMessage { pub worker: WorkerTriggerType, } +#[derive(Error, Debug)] +pub enum WorkerTriggerTypeError { + #[error("Unknown WorkerTriggerType: {0}")] + UnknownType(String), +} + impl FromStr for WorkerTriggerType { - type Err = String; + type Err = WorkerTriggerTypeError; fn from_str(s: &str) -> Result { match s { @@ -68,7 +74,7 @@ impl FromStr for WorkerTriggerType { "ProofRegistration" => Ok(WorkerTriggerType::ProofRegistration), "DataSubmission" => Ok(WorkerTriggerType::DataSubmission), "UpdateState" => Ok(WorkerTriggerType::UpdateState), - _ => Err(format!("Unknown WorkerTriggerType: {}", s)), + _ => Err(WorkerTriggerTypeError::UnknownType(s.to_string())), } } } @@ -232,6 +238,7 @@ fn parse_worker_message(message: &Delivery) -> Result = match atlantic_params.atlantic_settlement_layer.as_str() { "ethereum" => Box::new(EthereumLayer), "starknet" => Box::new(StarknetLayer), - _ => panic!("proving layer not correct"), + _ => panic!("Invalid settlement layer: {}", atlantic_params.atlantic_settlement_layer), }; Self { client, proving_layer } @@ -66,7 +67,7 @@ impl AtlanticClient { &self, pie_file: &Path, proof_layout: LayoutName, - atlantic_api_key: String, + atlantic_api_key: impl AsRef, ) -> Result { let proof_layout = match proof_layout { LayoutName::dynamic => "dynamic", @@ -76,21 +77,17 @@ impl AtlanticClient { let response = self .proving_layer .customize_request( - self.client - .request() - .method(Method::POST) - .query_param("apiKey", &atlantic_api_key) - .form_file("pieFile", pie_file, "pie.zip") - .form_text("layout", proof_layout), + self.client.request().method(Method::POST).query_param("apiKey", atlantic_api_key.as_ref()), ) + .form_file("pieFile", pie_file, "pie.zip")? + .form_text("layout", proof_layout) .send() .await .map_err(AtlanticError::AddJobFailure)?; - if response.status().is_success() { - response.json().await.map_err(AtlanticError::AddJobFailure) - } else { - Err(AtlanticError::SharpService(response.status())) + match response.status().is_success() { + true => response.json().await.map_err(AtlanticError::AddJobFailure), + false => Err(AtlanticError::SharpService(response.status())), } } diff --git a/crates/prover-clients/atlantic-service/src/error.rs b/crates/prover-clients/atlantic-service/src/error.rs index 07b62b14..bdca1b82 100644 --- a/crates/prover-clients/atlantic-service/src/error.rs +++ b/crates/prover-clients/atlantic-service/src/error.rs @@ -6,26 +6,40 @@ use reqwest::StatusCode; pub enum AtlanticError { #[error("Failed to to add Atlantic job: {0}")] AddJobFailure(#[source] reqwest::Error), + #[error("Failed to to get status of a Atlantic job: {0}")] GetJobStatusFailure(#[source] reqwest::Error), + #[error("Atlantic service returned an error {0}")] SharpService(StatusCode), + + #[error("Failed to read file: {0}")] + FileError(#[from] std::io::Error), + #[error("Failed to parse job key: {0}")] JobKeyParse(uuid::Error), + #[error("Failed to parse fact: {0}")] FactParse(FromHexError), + #[error("Failed to split task id into job key and fact")] TaskIdSplit, + #[error("Failed to encode PIE")] PieEncode(#[source] starknet_os::error::SnOsError), + #[error("Failed to get url as path segment mut. URL is cannot-be-a-base.")] PathSegmentMutFailOnUrl, + #[error("Failed to open file: {0}")] FileOpenError(#[source] tokio::io::Error), + #[error("Failed to create mime string: {0}")] MimeError(#[source] reqwest::Error), + #[error("Failed to read file: {0}")] FileReadError(#[source] tokio::io::Error), + #[error("Other error: {0}")] Other(#[from] color_eyre::eyre::Error), } diff --git a/crates/prover-clients/sharp-service/src/client.rs b/crates/prover-clients/sharp-service/src/client.rs index 58c2b754..802aceb1 100644 --- a/crates/prover-clients/sharp-service/src/client.rs +++ b/crates/prover-clients/sharp-service/src/client.rs @@ -28,7 +28,6 @@ impl SharpClient { /// `cat | base64` pub fn new_with_args(url: Url, sharp_params: &SharpValidatedArgs) -> Self { // Getting the cert files from the .env and then decoding it from base64 - let cert = general_purpose::STANDARD .decode(sharp_params.sharp_user_crt.clone()) .expect("Failed to decode certificate"); @@ -46,11 +45,12 @@ impl SharpClient { let certificate = Certificate::from_pem(server_cert.as_slice()).expect("Failed to add root certificate"); let client = HttpClient::builder(url.as_str()) + .expect("Failed to create HTTP client builder") .identity(identity) .add_root_certificate(certificate) .default_query_param("customer_id", customer_id.as_str()) .build() - .expect("Failed to build the http client"); + .expect("Failed to build HTTP client"); Self { client } } @@ -76,6 +76,7 @@ impl SharpClient { .query_param("offchain_proof", "true") .query_param("proof_layout", proof_layout) .body(encoded_pie) + .map_err(|e| SharpError::SerializationError(e.into()))? .send() .await .map_err(SharpError::AddJobFailure)?; diff --git a/crates/prover-clients/sharp-service/src/error.rs b/crates/prover-clients/sharp-service/src/error.rs index ad41a5ab..94dd2607 100644 --- a/crates/prover-clients/sharp-service/src/error.rs +++ b/crates/prover-clients/sharp-service/src/error.rs @@ -6,20 +6,31 @@ use reqwest::StatusCode; pub enum SharpError { #[error("Failed to to add SHARP job: {0}")] AddJobFailure(#[source] reqwest::Error), + #[error("Failed to to get status of a SHARP job: {0}")] GetJobStatusFailure(#[source] reqwest::Error), + #[error("SHARP service returned an error {0}")] SharpService(StatusCode), + #[error("Failed to parse job key: {0}")] JobKeyParse(uuid::Error), + #[error("Failed to parse fact: {0}")] FactParse(FromHexError), + #[error("Failed to split task id into job key and fact")] TaskIdSplit, + #[error("Failed to encode PIE")] PieEncode(#[source] starknet_os::error::SnOsError), + #[error("Failed to get url as path segment mut. URL is cannot-be-a-base.")] PathSegmentMutFailOnUrl, + + #[error("Failed to serialize body")] + SerializationError(#[source] std::io::Error), + #[error("Other error: {0}")] Other(#[from] color_eyre::eyre::Error), } diff --git a/crates/utils/src/http_client.rs b/crates/utils/src/http_client.rs index b5992b67..2b4c862a 100644 --- a/crates/utils/src/http_client.rs +++ b/crates/utils/src/http_client.rs @@ -34,13 +34,14 @@ //! ``` use std::collections::HashMap; +use std::io; use std::path::Path; use reqwest::header::{HeaderMap, HeaderName, HeaderValue}; use reqwest::multipart::{Form, Part}; use reqwest::{Certificate, Client, ClientBuilder, Identity, Method, Response, Result}; use serde::Serialize; -use url::Url; +use url::{ParseError, Url}; /// Main HTTP client with default configurations. /// @@ -76,14 +77,8 @@ pub struct HttpClientBuilder { impl HttpClient { /// Creates a new builder for constructing an HttpClient. - /// - /// # Arguments - /// * `base_url` - The base URL for all requests made through this client - /// - /// # Panics - /// Panics if the provided base URL is invalid - pub fn builder(base_url: &str) -> HttpClientBuilder { - HttpClientBuilder::new(Url::parse(base_url).expect("Invalid base URL")) + pub fn builder(base_url: &str) -> std::result::Result { + Ok(HttpClientBuilder::new(Url::parse(base_url)?)) } /// Creates a new request builder for making HTTP requests. @@ -280,10 +275,9 @@ impl<'a> RequestBuilder<'a> { /// .method(Method::POST) /// .body(json!({ "key": "value" })); /// ``` - pub fn body(mut self, body: T) -> Self { - let body_string = serde_json::to_string(&body).expect("Failed to serialize body"); - self.body = Some(body_string); - self + pub fn body(mut self, body: T) -> std::result::Result { + self.body = Some(serde_json::to_string(&body)?); + Ok(self) } /// Adds a text part to the multipart form. @@ -297,11 +291,9 @@ impl<'a> RequestBuilder<'a> { } /// Adds a file part to the multipart form. - pub fn form_file(mut self, key: &str, file_path: &Path, file_name: &str) -> Self { - let file_bytes = std::fs::read(file_path).expect("Failed to read file"); - // Convert file_name to owned String + pub fn form_file(mut self, key: &str, file_path: &Path, file_name: &str) -> io::Result { + let file_bytes = std::fs::read(file_path)?; let file_name = file_name.to_string(); - let part = Part::bytes(file_bytes).file_name(file_name); let form = match self.form.take() { @@ -309,7 +301,7 @@ impl<'a> RequestBuilder<'a> { None => Form::new().part(key.to_string(), part), }; self.form = Some(form); - self + Ok(self) } /// Sends the request with all configured parameters. pub async fn send(self) -> Result { @@ -334,7 +326,8 @@ mod http_client_tests { /// and all default values are properly initialized #[test] fn test_builder_basic_initialization() { - let client = HttpClient::builder(TEST_URL).build().unwrap(); + let client = + HttpClient::builder(TEST_URL).expect("Failed to create builder").build().expect("Failed to build client"); assert_eq!(client.base_url.as_str(), format!("{}/", TEST_URL)); assert!(client.default_headers.is_empty()); @@ -346,9 +339,14 @@ mod http_client_tests { /// Ensures the builder properly panics when provided with an invalid URL /// Cases: malformed URLs, invalid schemes, empty URLs #[test] - #[should_panic(expected = "Invalid base URL")] fn test_builder_invalid_url() { - HttpClient::builder("not a url"); + let result = HttpClient::builder("not a url"); + assert!(result.is_err()); + + match result { + Err(e) => assert!(e.to_string().contains("relative URL without a base")), + Ok(_) => panic!("Expected error for invalid URL"), + } } /// Verifies that default headers set during builder phase are: @@ -360,8 +358,11 @@ mod http_client_tests { let header_name = HeaderName::from_static("x-test"); let header_value = HeaderValue::from_static("test-value"); - let client = - HttpClient::builder(TEST_URL).default_header(header_name.clone(), header_value.clone()).build().unwrap(); + let client = HttpClient::builder(TEST_URL) + .expect("Failed to create builder") + .default_header(header_name.clone(), header_value.clone()) + .build() + .expect("Failed to build client"); assert!(client.default_headers.contains_key(&header_name)); assert_eq!(client.default_headers.get(&header_name).unwrap(), &header_value); @@ -374,10 +375,11 @@ mod http_client_tests { #[test] fn test_builder_default_query_params() { let client = HttpClient::builder(TEST_URL) + .expect("Failed to create builder") .default_query_param("key1", "value1") .default_query_param("key2", "value 2") .build() - .unwrap(); + .expect("Failed to build client"); assert_eq!(client.default_query_params.len(), 2); assert_eq!(client.default_query_params.get("key1").unwrap(), "value1"); @@ -390,7 +392,8 @@ mod http_client_tests { /// can be correctly set and are properly sent in requests #[test] fn test_request_builder_method_setting() { - let client = HttpClient::builder(TEST_URL).build().unwrap(); + let client = + HttpClient::builder(TEST_URL).expect("Failed to create builder").build().expect("Failed to build client"); let request = client.request().method(Method::GET); assert_eq!(request.method, Method::GET); @@ -413,7 +416,8 @@ mod http_client_tests { /// - Unicode paths #[test] fn test_request_builder_path_handling() { - let client = HttpClient::builder(TEST_URL).build().unwrap(); + let client = + HttpClient::builder(TEST_URL).expect("Failed to create builder").build().expect("Failed to build client"); // Test absolute path let request = client.request().path("/absolute/path"); @@ -446,7 +450,11 @@ mod http_client_tests { /// - Later parameters override earlier ones #[test] fn test_request_builder_query_params() { - let client = HttpClient::builder(TEST_URL).default_query_param("default", "value").build().unwrap(); + let client = HttpClient::builder(TEST_URL) + .expect("Failed to create builder") + .default_query_param("default", "value") + .build() + .expect("Failed to build client"); let request = client.request().query_param("test", "value").query_param("another", "param"); @@ -463,7 +471,11 @@ mod http_client_tests { fn test_request_builder_headers() { let header_name = HeaderName::from_static("x-test"); let header_value = HeaderValue::from_static("default-value"); - let client = HttpClient::builder(TEST_URL).default_header(header_name.clone(), header_value).build().unwrap(); + let client = HttpClient::builder(TEST_URL) + .expect("Failed to create builder") + .default_header(header_name.clone(), header_value) + .build() + .expect("Failed to build client"); let new_value = HeaderValue::from_static("new-value"); let request = client.request().header(header_name.clone(), new_value.clone()); @@ -480,7 +492,8 @@ mod http_client_tests { /// - Form builder chaining #[test] fn test_multipart_form_text() { - let client = HttpClient::builder(TEST_URL).build().unwrap(); + let client = + HttpClient::builder(TEST_URL).expect("Failed to create builder").build().expect("Failed to build client"); // Test initial state let request = client.request(); @@ -502,14 +515,16 @@ mod http_client_tests { /// - Non-existent file handling #[test] fn test_multipart_form_file() { - let client = HttpClient::builder(TEST_URL).build().unwrap(); + let client = + HttpClient::builder(TEST_URL).expect("Failed to create builder").build().expect("Failed to build client"); let file_path: PathBuf = "../orchestrator/src/tests/artifacts/fibonacci.zip".parse().unwrap(); // Test initial state let request = client.request(); assert!(request.form.is_none()); - let request = client.request().form_file("file", &file_path, "fibonacci.zip"); + let request = + client.request().form_file("file", &file_path, "fibonacci.zip").expect("Failed to add file to form"); assert!(request.form.is_some()); } @@ -520,14 +535,15 @@ mod http_client_tests { /// - Array/Vector serialization #[test] fn test_request_builder_body_serialization() { - let client = HttpClient::builder(TEST_URL).build().unwrap(); + let client = + HttpClient::builder(TEST_URL).expect("Failed to create builder").build().expect("Failed to build client"); // Test string body - let request = client.request().body("test string"); + let request = client.request().body("test string").expect("Failed to serialize string body"); assert_eq!(request.body.unwrap(), r#""test string""#); // Test number body - let request = client.request().body(42); + let request = client.request().body(42).expect("Failed to serialize number body"); assert_eq!(request.body.unwrap(), "42"); // Test struct body @@ -539,12 +555,12 @@ mod http_client_tests { let test_struct = TestStruct { field1: "test".to_string(), field2: 123 }; - let request = client.request().body(test_struct); + let request = client.request().body(test_struct).expect("Failed to serialize struct body"); assert_eq!(request.body.unwrap(), r#"{"field1":"test","field2":123}"#); // Test array/vec body let vec_data = vec![1, 2, 3]; - let request = client.request().body(vec_data); + let request = client.request().body(vec_data).expect("Failed to serialize vector body"); assert_eq!(request.body.unwrap(), "[1,2,3]"); } @@ -572,8 +588,12 @@ mod http_client_tests { Identity::from_pkcs8_pem(&cert, &key).expect("Failed to build the identity from certificate and key"); let certificate = Certificate::from_pem(server_cert.as_slice()).expect("Failed to add root certificate"); - let client = - HttpClient::builder(TEST_URL).identity(identity).add_root_certificate(certificate).build().unwrap(); + let client = HttpClient::builder(TEST_URL) + .expect("Failed to create builder") + .identity(identity) + .add_root_certificate(certificate) + .build() + .expect("Failed to build client"); // Since we can't check the certificates directly, we'll just verify the client was built assert_eq!(client.base_url.as_str(), (TEST_URL.to_owned() + "/")); @@ -587,11 +607,13 @@ mod http_client_tests { async fn test_large_payload_handling() { let mock_server = httpmock::MockServer::start(); - let client = HttpClient::builder(&mock_server.base_url()).build().unwrap(); + let client = HttpClient::builder(&mock_server.base_url()) + .expect("Failed to create builder") + .build() + .expect("Failed to build client"); - // Create a large body string let large_body = "x".repeat(1024 * 1024); // 1MB string - let request = client.request().method(Method::POST).body(&large_body); + let request = client.request().method(Method::POST).body(&large_body).expect("Failed to serialize large body"); assert!(request.body.is_some()); assert_eq!(request.body.unwrap().len(), (1024 * 1024) + 2); @@ -621,10 +643,11 @@ mod http_client_tests { }); let client = HttpClient::builder(&mock_server.base_url()) + .expect("Failed to create builder") .default_header(HeaderName::from_static("authorization"), HeaderValue::from_static("Bearer token")) .default_query_param("version", "v1") .build() - .unwrap(); + .expect("Failed to build client"); #[derive(Serialize)] struct RequestBody { @@ -637,9 +660,10 @@ mod http_client_tests { .path("/api/data") .query_param("id", "123") .body(RequestBody { name: "test".to_string() }) + .expect("Failed to serialize body") .send() .await - .unwrap(); + .expect("Failed to send request"); assert_eq!(response.status(), 200); mock.assert();