From d9201488974d3222eb56650c126d754372602d42 Mon Sep 17 00:00:00 2001 From: polydez <155382956+polydez@users.noreply.github.com> Date: Tue, 22 Oct 2024 00:20:19 +0500 Subject: [PATCH] feat(store): improve errors (#518) * feat: improve node errors * docs: update `CHANGELOG.md` --- CHANGELOG.md | 1 + crates/store/src/errors.rs | 16 +++++- crates/store/src/server/api.rs | 89 +++++++++++++--------------------- crates/store/src/state.rs | 4 +- 4 files changed, 51 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ded380..88d847b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - Added `GetAccountProofs` endpoint (#506). - Migrated faucet from actix-web to axum (#511). - Changed the `BlockWitness` to pass the inputs to the VM using only advice provider (#516). +- [BREAKING] Improved store API errors (return "not found" instead of "internal error" status if requested account(s) not found) (#518). ## 0.5.1 (2024-09-12) diff --git a/crates/store/src/errors.rs b/crates/store/src/errors.rs index 2666c138..700c9d3c 100644 --- a/crates/store/src/errors.rs +++ b/crates/store/src/errors.rs @@ -14,6 +14,7 @@ use miden_objects::{ use rusqlite::types::FromSqlError; use thiserror::Error; use tokio::sync::oneshot::error::RecvError; +use tonic::Status; use crate::types::{AccountId, BlockNumber}; @@ -60,8 +61,6 @@ pub enum DatabaseError { InteractError(String), #[error("Deserialization of BLOB data from database failed: {0}")] DeserializationError(DeserializationError), - #[error("Corrupted data: {0}")] - CorruptedData(String), #[error("Invalid Felt: {0}")] InvalidFelt(String), #[error("Block applying was broken because of closed channel on state side: {0}")] @@ -91,6 +90,19 @@ impl From for DatabaseError { } } +impl From for Status { + fn from(err: DatabaseError) -> Self { + match err { + DatabaseError::AccountNotFoundInDb(_) + | DatabaseError::AccountsNotFoundInDb(_) + | DatabaseError::AccountNotOnChain(_) + | DatabaseError::BlockNotFoundInDb(_) => Status::not_found(err.to_string()), + + _ => Status::internal(err.to_string()), + } + } +} + // INITIALIZATION ERRORS // ================================================================================================= diff --git a/crates/store/src/server/api.rs b/crates/store/src/server/api.rs index 38e4bdaf..0018ff16 100644 --- a/crates/store/src/server/api.rs +++ b/crates/store/src/server/api.rs @@ -70,7 +70,7 @@ impl api_server::Api for StoreApi { )] async fn get_block_header_by_number( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { info!(target: COMPONENT, ?request); let request = request.into_inner(); @@ -102,7 +102,7 @@ impl api_server::Api for StoreApi { )] async fn check_nullifiers( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { // Validate the nullifiers and convert them to Digest values. Stop on first error. let request = request.into_inner(); @@ -126,7 +126,7 @@ impl api_server::Api for StoreApi { )] async fn check_nullifiers_by_prefix( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); @@ -137,8 +137,7 @@ impl api_server::Api for StoreApi { let nullifiers = self .state .check_nullifiers_by_prefix(request.prefix_len, request.nullifiers) - .await - .map_err(internal_error)? + .await? .into_iter() .map(|nullifier_info| NullifierUpdate { nullifier: Some(nullifier_info.nullifier.into()), @@ -160,7 +159,7 @@ impl api_server::Api for StoreApi { )] async fn sync_state( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); @@ -224,7 +223,7 @@ impl api_server::Api for StoreApi { )] async fn sync_notes( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); @@ -256,7 +255,7 @@ impl api_server::Api for StoreApi { )] async fn get_notes_by_id( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { info!(target: COMPONENT, ?request); @@ -270,8 +269,7 @@ impl api_server::Api for StoreApi { let notes = self .state .get_notes_by_id(note_ids) - .await - .map_err(internal_error)? + .await? .into_iter() .map(Into::into) .collect(); @@ -289,7 +287,7 @@ impl api_server::Api for StoreApi { )] async fn get_note_authentication_info( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { info!(target: COMPONENT, ?request); @@ -325,7 +323,7 @@ impl api_server::Api for StoreApi { )] async fn get_account_details( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); let account_info = self @@ -333,8 +331,7 @@ impl api_server::Api for StoreApi { .get_account_details( request.account_id.ok_or(invalid_argument("Account missing id"))?.into(), ) - .await - .map_err(internal_error)?; + .await?; Ok(Response::new(GetAccountDetailsResponse { details: Some((&account_info).into()), @@ -354,8 +351,8 @@ impl api_server::Api for StoreApi { )] async fn apply_block( &self, - request: tonic::Request, - ) -> Result, tonic::Status> { + request: Request, + ) -> Result, Status> { let request = request.into_inner(); debug!(target: COMPONENT, ?request); @@ -391,7 +388,7 @@ impl api_server::Api for StoreApi { )] async fn get_block_inputs( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); @@ -417,7 +414,7 @@ impl api_server::Api for StoreApi { )] async fn get_transaction_inputs( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); @@ -430,8 +427,7 @@ impl api_server::Api for StoreApi { let tx_inputs = self .state .get_transaction_inputs(account_id, &nullifiers, unauthenticated_notes) - .await - .map_err(internal_error)?; + .await?; let block_height = self.state.latest_block_num().await; @@ -466,13 +462,13 @@ impl api_server::Api for StoreApi { )] async fn get_block_by_number( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); debug!(target: COMPONENT, ?request); - let block = self.state.load_block(request.block_num).await.map_err(internal_error)?; + let block = self.state.load_block(request.block_num).await?; Ok(Response::new(GetBlockByNumberResponse { block })) } @@ -494,11 +490,8 @@ impl api_server::Api for StoreApi { let account_ids = convert(request.account_ids); let include_headers = request.include_headers.unwrap_or_default(); - let (block_num, infos) = self - .state - .get_account_states(account_ids, include_headers) - .await - .map_err(internal_error)?; + let (block_num, infos) = + self.state.get_account_proofs(account_ids, include_headers).await?; Ok(Response::new(GetAccountProofsResponse { block_num, @@ -515,7 +508,7 @@ impl api_server::Api for StoreApi { )] async fn get_account_state_delta( &self, - request: tonic::Request, + request: Request, ) -> Result, Status> { let request = request.into_inner(); @@ -528,8 +521,7 @@ impl api_server::Api for StoreApi { request.from_block_num, request.to_block_num, ) - .await - .map_err(internal_error)?; + .await?; Ok(Response::new(GetAccountStateDeltaResponse { delta: Some(delta.to_bytes()) })) } @@ -547,9 +539,9 @@ impl api_server::Api for StoreApi { )] async fn list_nullifiers( &self, - _request: tonic::Request, + _request: Request, ) -> Result, Status> { - let raw_nullifiers = self.state.list_nullifiers().await.map_err(internal_error)?; + let raw_nullifiers = self.state.list_nullifiers().await?; let nullifiers = raw_nullifiers .into_iter() .map(|(key, block_num)| SmtLeafEntry { @@ -570,16 +562,9 @@ impl api_server::Api for StoreApi { )] async fn list_notes( &self, - _request: tonic::Request, + _request: Request, ) -> Result, Status> { - let notes = self - .state - .list_notes() - .await - .map_err(internal_error)? - .into_iter() - .map(Into::into) - .collect(); + let notes = self.state.list_notes().await?.into_iter().map(Into::into).collect(); Ok(Response::new(ListNotesResponse { notes })) } @@ -593,16 +578,9 @@ impl api_server::Api for StoreApi { )] async fn list_accounts( &self, - _request: tonic::Request, + _request: Request, ) -> Result, Status> { - let accounts = self - .state - .list_accounts() - .await - .map_err(internal_error)? - .iter() - .map(Into::into) - .collect(); + let accounts = self.state.list_accounts().await?.iter().map(Into::into).collect(); Ok(Response::new(ListAccountsResponse { accounts })) } } @@ -610,13 +588,14 @@ impl api_server::Api for StoreApi { // UTILITIES // ================================================================================================ -/// Formats an error -fn internal_error(err: E) -> Status { - Status::internal(format!("{:?}", err)) +/// Formats an "Internal error" error +fn internal_error(err: E) -> Status { + Status::internal(err.to_string()) } -fn invalid_argument(err: E) -> Status { - Status::invalid_argument(format!("{:?}", err)) +/// Formats an "Invalid argument" error +fn invalid_argument(err: E) -> Status { + Status::invalid_argument(err.to_string()) } #[instrument(target = "miden-store", skip_all, err)] diff --git a/crates/store/src/state.rs b/crates/store/src/state.rs index 44e03425..294f1ebc 100644 --- a/crates/store/src/state.rs +++ b/crates/store/src/state.rs @@ -679,8 +679,8 @@ impl State { self.db.select_account(id).await } - /// Returns account states with details for public accounts. - pub async fn get_account_states( + /// Returns account proofs with optional account and storage headers. + pub async fn get_account_proofs( &self, account_ids: Vec, include_headers: bool,