feat: distinguish custom errors from internal errors

Introduce a custom variant in various error enums
Keep anyhow errors mapped to the internal variants

CHANGELOG.md

@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Changed
+
+- RPC errors now only include the root cause if white-listed as non-sensitive
+
 ### Fixed
 
 - JSON-RPC 0.5 transaction traces now have the required `type` property.

crates/executor/src/error.rs

@@ -10,6 +10,7 @@ pub enum CallError {
     InvalidMessageSelector,
     Reverted(String),
     Internal(anyhow::Error),
+    Custom(anyhow::Error),
 }
 
 impl From<TransactionExecutionError> for CallError {

crates/gateway-types/src/error.rs

@@ -10,7 +10,7 @@ pub enum SequencerError {
     /// Errors directly coming from reqwest
     #[error(transparent)]
     ReqwestError(#[from] reqwest::Error),
-    /// Custom errors that we fidded with because the original error was either
+    /// Custom errors that we fiddled with because the original error was either
     /// not informative enough or bloated
     #[error("error decoding response body: invalid error variant")]
     InvalidStarknetErrorVariant,

crates/rpc/src/error.rs

@@ -78,8 +78,15 @@ pub enum ApplicationError {
     ProofLimitExceeded { limit: u32, requested: u32 },
     #[error("Internal error")]
     GatewayError(starknet_gateway_types::error::StarknetError),
+    /// Internal errors are errors whose details we don't want to show to the end user.
+    /// These are logged, and a simple "internal error" message is shown to the end
+    /// user.
     #[error("Internal error")]
     Internal(anyhow::Error),
+    /// Custom errors are mostly treated as internal errors with the big difference that the error
+    /// details aren't logged and are eventually displayed to the end user.
+    #[error("Internal error")]
+    Custom(anyhow::Error),
 }
 
 impl ApplicationError {
@@ -120,7 +127,9 @@ impl ApplicationError {
             // doc/rpc/pathfinder_rpc_api.json
             ApplicationError::ProofLimitExceeded { .. } => 10000,
             // https://www.jsonrpc.org/specification#error_object
-            ApplicationError::GatewayError(_) | ApplicationError::Internal(_) => -32603,
+            ApplicationError::GatewayError(_)
+            | ApplicationError::Internal(_)
+            | ApplicationError::Custom(_) => -32603,
         }
     }
 
@@ -158,13 +167,14 @@ impl ApplicationError {
             ApplicationError::GatewayError(error) => Some(json!({
                 "error": error,
             })),
-            ApplicationError::Internal(error) => {
-                let error = error.to_string();
-                if error.is_empty() {
+            ApplicationError::Internal(_) => None,
+            ApplicationError::Custom(cause) => {
+                let cause = cause.to_string();
+                if cause.is_empty() {
                     None
                 } else {
                     Some(json!({
-                        "error": error.to_string(),
+                        "error": cause.to_string(),
                     }))
                 }
             }
@@ -200,18 +210,17 @@ impl ApplicationError {
 /// generate_rpc_error_subset!(<enum_name>: <variant a>, <variant b>, <variant N>);
 /// ```
 /// Note that the variants __must__ match the [ApplicationError] variant names and that [ApplicationError::Internal]
-/// is always included by default (and therefore should not be part of macro input).
+/// and [ApplicationError::Custom] are always included by default (and therefore should not be part of macro input).
 ///
 /// An `Internal` only variant can be generated using `generate_rpc_error_subset!(<enum_name>)`.
 ///
 /// ## Specifics
 /// This macro generates the following:
 ///
 /// 1. New enum definition with `#[derive(Debug)]`
-/// 2. `impl From<NewEnum> for RpcError`
-/// 3. `impl From<anyhow::Error> for NewEnum`
-///
-/// It always includes the `Internal(anyhow::Error)` variant.
+/// 2. `Internal(anyhow::Error)` and `Custom(anyhow::Error)` variants
+/// 3. `impl From<NewEnum> for RpcError`
+/// 4. `impl From<anyhow::Error> for NewEnum`, mapping to the `Internal` variant
 ///
 /// ## Example with expansion
 /// This macro invocation:
@@ -224,7 +233,10 @@ impl ApplicationError {
 /// pub enum MyError {
 ///     BlockNotFound,
 ///     NoBlocks,
+///     /// See [`crate::error::ApplicationError::Internal`]
 ///     Internal(anyhow::Error),
+///     /// See [`crate::error::ApplicationError::Custom`]
+///     Custom(anyhow::Error),
 /// }
 ///
 /// impl From<MyError> for RpcError {
@@ -274,7 +286,10 @@ macro_rules! generate_rpc_error_subset {
     (@enum_def, $enum_name:ident, $($subset:tt),*) => {
         #[derive(Debug)]
         pub enum $enum_name {
+            /// See [`crate::error::ApplicationError::Internal`]
             Internal(anyhow::Error),
+            /// See [`crate::error::ApplicationError::Custom`]
+            Custom(anyhow::Error),
             $($subset),*
         }
     };
@@ -312,6 +327,7 @@ macro_rules! generate_rpc_error_subset {
         match $var {
             $($arms)*
             $enum_name::Internal(internal) => Self::Internal(internal),
+            $enum_name::Custom(error) => Self::Custom(error),
         }
     };
     // Special case for single variant. This could probably be folded into one of the other

crates/rpc/src/jsonrpc/error.rs

@@ -46,8 +46,6 @@ impl RpcError {
             RpcError::InvalidRequest => "Invalid Request".into(),
             RpcError::MethodNotFound { .. } => "Method not found".into(),
             RpcError::InvalidParams => "Invalid params".into(),
-            // TODO: this is not necessarily a good idea. All internal errors are returned here, even
-            // ones that we probably should not disclose.
             RpcError::InternalError(_) => "Internal error".into(),
             RpcError::ApplicationError(e) => e.to_string().into(),
             RpcError::WebsocketSubscriptionClosed { .. } => "Websocket subscription closed".into(),
@@ -64,16 +62,7 @@ impl RpcError {
                 "reason": reason,
             })),
             RpcError::ApplicationError(e) => e.data(),
-            RpcError::InternalError(error) => {
-                let error = error.to_string();
-                if error.is_empty() {
-                    None
-                } else {
-                    Some(json!({
-                        "error": error.to_string(),
-                    }))
-                }
-            }
+            RpcError::InternalError(_) => None,
             RpcError::ParseError => None,
             RpcError::InvalidRequest => None,
             RpcError::MethodNotFound => None,

crates/rpc/src/jsonrpc/response.rs

@@ -1,3 +1,4 @@
+use crate::error::ApplicationError;
 use axum::response::IntoResponse;
 use serde::Serialize;
 use serde_json::Value;
@@ -67,6 +68,18 @@ impl Serialize for RpcResponse<'_> {
 
 impl IntoResponse for RpcResponse<'_> {
     fn into_response(self) -> axum::response::Response {
+        // Log internal errors.
+        match &self.output {
+            Err(RpcError::InternalError(e))
+            | Err(RpcError::ApplicationError(ApplicationError::Internal(e))) => {
+                tracing::warn!(backtrace = ?e, "Internal error");
+            }
+            Err(RpcError::ApplicationError(ApplicationError::Custom(e))) => {
+                tracing::debug!(backtrace = ?e, "Custom error");
+            }
+            _ => {}
+        }
+
         serde_json::to_vec(&self).unwrap().into_response()
     }
 }

crates/rpc/src/jsonrpc/router.rs

@@ -93,8 +93,9 @@ impl RpcRouter {
             Ok(output) => output,
             Err(e) => {
                 tracing::warn!(method=%request.method, backtrace=?e, "RPC method panic'd");
-                // No error message so that the caller cannot learn to abuse this.
-                Err(RpcError::InternalError(anyhow::anyhow!("")))
+                Err(RpcError::InternalError(anyhow::anyhow!(
+                    "RPC method panic'd"
+                )))
             }
         };
 

crates/rpc/src/pathfinder/methods/get_proof.rs

@@ -22,11 +22,13 @@ pub enum GetProofError {
     BlockNotFound,
     ProofLimitExceeded { limit: u32, requested: u32 },
 }
+
 impl From<anyhow::Error> for GetProofError {
     fn from(e: anyhow::Error) -> Self {
         Self::Internal(e)
     }
 }
+
 impl From<GetProofError> for crate::error::ApplicationError {
     fn from(x: GetProofError) -> Self {
         match x {

crates/rpc/src/v02/method/add_declare_transaction.rs

@@ -13,6 +13,7 @@ pub enum AddDeclareTransactionError {
     InvalidContractClass,
     GatewayError(StarknetError),
     Internal(anyhow::Error),
+    Custom(anyhow::Error),
 }
 
 impl From<AddDeclareTransactionError> for crate::error::ApplicationError {
@@ -21,6 +22,7 @@ impl From<AddDeclareTransactionError> for crate::error::ApplicationError {
             AddDeclareTransactionError::InvalidContractClass => Self::InvalidContractClass,
             AddDeclareTransactionError::GatewayError(x) => Self::GatewayError(x),
             AddDeclareTransactionError::Internal(x) => Self::Internal(x),
+            AddDeclareTransactionError::Custom(x) => Self::Custom(x),
         }
     }
 }
@@ -80,7 +82,7 @@ pub async fn add_declare_transaction(
 ) -> Result<AddDeclareTransactionOutput, AddDeclareTransactionError> {
     match input.declare_transaction {
         Transaction::Declare(BroadcastedDeclareTransaction::V0(_)) => {
-            Err(AddDeclareTransactionError::Internal(anyhow::anyhow!(
+            Err(AddDeclareTransactionError::Custom(anyhow::anyhow!(
                 "Declare v0 transactions are not allowed"
             )))
         }

crates/rpc/src/v02/method/add_deploy_account_transaction.rs

@@ -44,12 +44,6 @@ impl From<AddDeployAccountTransactionError> for crate::error::ApplicationError {
     }
 }
 
-impl From<anyhow::Error> for AddDeployAccountTransactionError {
-    fn from(value: anyhow::Error) -> Self {
-        AddDeployAccountTransactionError::Internal(value)
-    }
-}
-
 pub async fn add_deploy_account_transaction(
     context: RpcContext,
     input: AddDeployAccountTransactionInput,

crates/rpc/src/v02/method/add_invoke_transaction.rs

@@ -40,12 +40,6 @@ impl From<AddInvokeTransactionError> for crate::error::ApplicationError {
     }
 }
 
-impl From<anyhow::Error> for AddInvokeTransactionError {
-    fn from(value: anyhow::Error) -> Self {
-        AddInvokeTransactionError::Internal(value)
-    }
-}
-
 pub async fn add_invoke_transaction(
     context: RpcContext,
     input: AddInvokeTransactionInput,

crates/rpc/src/v02/method/call.rs

@@ -10,8 +10,9 @@ impl From<crate::v05::method::call::CallError> for CallError {
             crate::v05::method::call::CallError::BlockNotFound => Self::BlockNotFound,
             crate::v05::method::call::CallError::ContractNotFound => Self::ContractNotFound,
             crate::v05::method::call::CallError::ContractErrorV05 { revert_error } => {
-                Self::Internal(anyhow::anyhow!("Transaction reverted: {}", revert_error))
+                Self::Custom(anyhow::anyhow!("Transaction reverted: {}", revert_error))
             }
+            crate::v05::method::call::CallError::Custom(e) => Self::Custom(e),
         }
     }
 }

crates/rpc/src/v03/method/estimate_fee.rs

@@ -17,8 +17,9 @@ impl From<crate::v05::method::estimate_fee::EstimateFeeError> for EstimateFeeErr
             BlockNotFound => Self::BlockNotFound,
             ContractNotFound => Self::ContractNotFound,
             ContractErrorV05 { revert_error } => {
-                Self::Internal(anyhow::anyhow!("Transaction reverted: {}", revert_error))
+                Self::Custom(anyhow::anyhow!("Transaction reverted: {}", revert_error))
             }
+            Custom(e) => Self::Custom(e),
         }
     }
 }

crates/rpc/src/v03/method/estimate_message_fee.rs

@@ -29,6 +29,7 @@ impl From<crate::v05::method::estimate_message_fee::EstimateMessageFeeError>
             ContractErrorV05 { revert_error } => {
                 Self::Internal(anyhow::anyhow!("Transaction reverted: {}", revert_error))
             }
+            Custom(error) => Self::Custom(error),
         }
     }
 }
@@ -255,7 +256,7 @@ mod tests {
         let rpc = setup(Setup::Full).await.expect("RPC context");
         assert_matches::assert_matches!(
             estimate_message_fee(rpc, input).await,
-            Err(EstimateMessageFeeError::Internal(_))
+            Err(EstimateMessageFeeError::Custom(_))
         );
     }
 }

crates/rpc/src/v03/method/get_events.rs

@@ -12,6 +12,7 @@ use tokio::task::JoinHandle;
 #[derive(Debug)]
 pub enum GetEventsError {
     Internal(anyhow::Error),
+    Custom(anyhow::Error),
     BlockNotFound,
     PageSizeTooBig,
     InvalidContinuationToken,
@@ -28,6 +29,7 @@ impl From<GetEventsError> for crate::error::ApplicationError {
     fn from(e: GetEventsError) -> Self {
         match e {
             GetEventsError::Internal(internal) => Self::Internal(internal),
+            GetEventsError::Custom(internal) => Self::Custom(internal),
             GetEventsError::BlockNotFound => Self::BlockNotFound,
             GetEventsError::PageSizeTooBig => Self::PageSizeTooBig,
             GetEventsError::InvalidContinuationToken => Self::InvalidContinuationToken,
@@ -162,15 +164,11 @@ pub async fn get_events(
         // We don't add context here, because [StarknetEventsTable::get_events] adds its
         // own context to the errors. This way we get meaningful error information
         // for errors related to query parameters.
-        let page = transaction.events(&filter).map_err(|e| {
-            if let Some(event_filter_error) = e.downcast_ref::<EventFilterError>() {
-                match event_filter_error {
-                    EventFilterError::PageSizeTooBig(_) => GetEventsError::PageSizeTooBig,
-                    EventFilterError::TooManyMatches => GetEventsError::from(e),
-                }
-            } else {
-                GetEventsError::from(e)
-            }
+        let page = transaction.events(&filter).map_err(|e| match e {
+            EventFilterError::PageSizeTooBig(_) => GetEventsError::PageSizeTooBig,
+            EventFilterError::TooManyMatches => GetEventsError::Custom(e.into()),
+            EventFilterError::Internal(e) => GetEventsError::Internal(e),
+            EventFilterError::PageSizeTooSmall => GetEventsError::Custom(e.into()),
         })?;
 
         let new_continuation_token = match page.is_last_page {

crates/rpc/src/v03/method/simulate_transaction.rs

@@ -32,9 +32,10 @@ impl From<CallError> for SimulateTransactionError {
             ContractNotFound => Self::ContractNotFound,
             InvalidMessageSelector => Self::ContractError,
             Reverted(revert_error) => {
-                Self::Internal(anyhow::anyhow!("Transaction reverted: {}", revert_error))
+                Self::Custom(anyhow::anyhow!("Transaction reverted: {}", revert_error))
             }
             Internal(e) => Self::Internal(e),
+            Custom(e) => Self::Custom(e),
         }
     }
 }

crates/rpc/src/v04/method/add_deploy_account_transaction.rs

@@ -57,12 +57,6 @@ impl From<AddDeployAccountTransactionError> for crate::error::ApplicationError {
     }
 }
 
-impl From<anyhow::Error> for AddDeployAccountTransactionError {
-    fn from(value: anyhow::Error) -> Self {
-        AddDeployAccountTransactionError::UnexpectedError(value.to_string())
-    }
-}
-
 impl From<SequencerError> for AddDeployAccountTransactionError {
     fn from(e: SequencerError) -> Self {
         use starknet_gateway_types::error::KnownStarknetErrorCode::{

crates/rpc/src/v04/method/add_invoke_transaction.rs

@@ -54,12 +54,6 @@ impl From<AddInvokeTransactionError> for crate::error::ApplicationError {
     }
 }
 
-impl From<anyhow::Error> for AddInvokeTransactionError {
-    fn from(value: anyhow::Error) -> Self {
-        AddInvokeTransactionError::UnexpectedError(value.to_string())
-    }
-}
-
 impl From<SequencerError> for AddInvokeTransactionError {
     fn from(e: SequencerError) -> Self {
         use starknet_gateway_types::error::KnownStarknetErrorCode::{

crates/rpc/src/v04/method/estimate_message_fee.rs

@@ -20,6 +20,7 @@ impl From<crate::v05::method::estimate_message_fee::EstimateMessageFeeError>
             ContractErrorV05 { revert_error } => {
                 Self::Internal(anyhow::anyhow!("Transaction reverted: {}", revert_error))
             }
+            Custom(error) => Self::Custom(error),
         }
     }
 }

crates/rpc/src/v04/method/simulate_transactions.rs

@@ -34,9 +34,10 @@ impl From<CallError> for SimulateTransactionError {
             ContractNotFound => Self::ContractNotFound,
             InvalidMessageSelector => Self::ContractError,
             Reverted(revert_error) => {
-                Self::Internal(anyhow::anyhow!("Transaction reverted: {}", revert_error))
+                Self::Custom(anyhow::anyhow!("Transaction reverted: {}", revert_error))
             }
             Internal(e) => Self::Internal(e),
+            Custom(e) => Self::Custom(e),
         }
     }
 }