Merge #1697

1697: Simplify error handling in http routes r=thomaseizinger a=thomaseizinger

By making better use of anyhow and thiserror, we can question-mark
away many of the errors. We still have control of the error returned to the user through the custom recover function of warp.

Fixes #1683.

Co-authored-by: Thomas Eizinger <[email protected]>

Author: bors[bot]

cnd/src/http_api/action.rs

@@ -1,5 +1,7 @@
 use crate::{
-    http_api::{ethereum_network, problem, Http},
+    http_api::{
+        ethereum_network, problem, Http, MissingQueryParameters, UnexpectedQueryParameters,
+    },
     swap_protocols::{
         actions::{
             bitcoin::{SendToAddress, SpendOutput},
@@ -8,6 +10,7 @@ use crate::{
         ledger, SwapId, Timestamp,
     },
 };
+use anyhow::Context;
 use blockchain_contracts::bitcoin::witness;
 use http::StatusCode;
 use http_api_problem::HttpApiProblem;
@@ -94,20 +97,20 @@ pub trait IntoResponsePayload {
     fn into_response_payload(
         self,
         parameters: ActionExecutionParameters,
-    ) -> Result<ActionResponseBody, HttpApiProblem>;
+    ) -> anyhow::Result<ActionResponseBody>;
 }
 
 impl IntoResponsePayload for SendToAddress {
     fn into_response_payload(
         self,
         query_params: ActionExecutionParameters,
-    ) -> Result<ActionResponseBody, HttpApiProblem> {
+    ) -> anyhow::Result<ActionResponseBody> {
         match query_params {
             ActionExecutionParameters::None {} => Ok(self.into()),
-            _ => Err(problem::unexpected_query_parameters(
-                "bitcoin::SendToAddress",
-                vec!["address".into(), "fee_per_wu".into()],
-            )),
+            _ => Err(anyhow::Error::from(UnexpectedQueryParameters {
+                action: "bitcoin::SendToAddress",
+                parameters: &["address", "fee_per_wu"],
+            })),
         }
     }
 }
@@ -137,13 +140,13 @@ impl IntoResponsePayload for SpendOutput {
     fn into_response_payload(
         self,
         query_params: ActionExecutionParameters,
-    ) -> Result<ActionResponseBody, HttpApiProblem> {
+    ) -> anyhow::Result<ActionResponseBody> {
         match query_params {
             ActionExecutionParameters::BitcoinAddressAndFee {
                 address,
                 fee_per_wu,
             } => {
-                let fee_per_wu = fee_per_wu.parse::<usize>().map_err(|_| {
+                let fee_per_wu = fee_per_wu.parse::<usize>().with_context(|| {
                     HttpApiProblem::new("Invalid query parameter.")
                         .set_status(StatusCode::BAD_REQUEST)
                         .set_detail("Query parameter fee-per-byte is not a valid unsigned integer.")
@@ -178,22 +181,22 @@ impl IntoResponsePayload for SpendOutput {
                     network,
                 ))
             }
-            _ => Err(problem::missing_query_parameters(
-                "bitcoin::SpendOutput",
-                vec![
-                    &problem::MissingQueryParameter {
+            _ => Err(anyhow::Error::from(MissingQueryParameters {
+                action: "bitcoin::SpendOutput",
+                parameters: &[
+                    problem::MissingQueryParameter {
                         name: "address",
                         data_type: "string",
                         description: "The bitcoin address to where the funds should be sent.",
                     },
-                    &problem::MissingQueryParameter {
+                    problem::MissingQueryParameter {
                         name: "fee_per_wu",
                         data_type: "uint",
                         description:
                         "The fee per weight unit you want to pay for the transaction in satoshis.",
                     },
-                ],
-            )),
+                ]
+            }))
         }
     }
 }
@@ -229,7 +232,7 @@ impl IntoResponsePayload for ethereum::DeployContract {
     fn into_response_payload(
         self,
         query_params: ActionExecutionParameters,
-    ) -> Result<ActionResponseBody, HttpApiProblem> {
+    ) -> anyhow::Result<ActionResponseBody> {
         let ethereum::DeployContract {
             data,
             amount,
@@ -244,10 +247,10 @@ impl IntoResponsePayload for ethereum::DeployContract {
                 chain_id,
                 network: chain_id.into(),
             }),
-            _ => Err(problem::unexpected_query_parameters(
-                "ethereum::ContractDeploy",
-                vec!["address".into(), "fee_per_wu".into()],
-            )),
+            _ => Err(anyhow::Error::from(UnexpectedQueryParameters {
+                action: "ethereum::ContractDeploy",
+                parameters: &["address", "fee_per_wu"],
+            })),
         }
     }
 }
@@ -262,7 +265,7 @@ impl IntoResponsePayload for ethereum::CallContract {
     fn into_response_payload(
         self,
         query_params: ActionExecutionParameters,
-    ) -> Result<ActionResponseBody, HttpApiProblem> {
+    ) -> anyhow::Result<ActionResponseBody> {
         let ethereum::CallContract {
             to,
             data,
@@ -279,10 +282,10 @@ impl IntoResponsePayload for ethereum::CallContract {
                 network: chain_id.into(),
                 min_block_timestamp,
             }),
-            _ => Err(problem::unexpected_query_parameters(
-                "ethereum::SendTransaction",
-                vec!["address".into(), "fee_per_wu".into()],
-            )),
+            _ => Err(anyhow::Error::from(UnexpectedQueryParameters {
+                action: "ethereum::SendTransaction",
+                parameters: &["address", "fee_per_wu"],
+            })),
         }
     }
 }
@@ -303,7 +306,7 @@ impl IntoResponsePayload for Infallible {
     fn into_response_payload(
         self,
         _: ActionExecutionParameters,
-    ) -> Result<ActionResponseBody, HttpApiProblem> {
+    ) -> anyhow::Result<ActionResponseBody> {
         unreachable!("how did you manage to construct Infallible?")
     }
 }

cnd/src/http_api/problem.rs

@@ -1,149 +1,118 @@
 use crate::{
     db,
-    swap_protocols::rfc003::{self, actions::ActionKind, state_store},
+    http_api::routes::rfc003::handlers::{
+        post_swap::{MalformedRequest, UnsupportedSwap},
+        InvalidAction, InvalidActionInvocation,
+    },
 };
 use http::StatusCode;
 use http_api_problem::HttpApiProblem;
-use libp2p_comit::frame::Response;
-use serde::Serialize;
 use warp::{Rejection, Reply};
 
-#[derive(Debug, Serialize)]
+#[derive(Debug, thiserror::Error)]
+#[error("Missing GET parameters for a {} action type. Expected: {:?}", action, parameters.iter().map(|parameter| parameter.name).collect::<Vec<&str>>())]
+pub struct MissingQueryParameters {
+    pub action: &'static str,
+    pub parameters: &'static [MissingQueryParameter],
+}
+
+#[derive(Debug, serde::Serialize)]
 pub struct MissingQueryParameter {
     pub name: &'static str,
     pub data_type: &'static str,
     pub description: &'static str,
 }
 
+#[derive(Debug, thiserror::Error)]
+#[error("unexpected GET parameters {parameters:?} for a {action} action type, expected: none")]
+pub struct UnexpectedQueryParameters {
+    pub action: &'static str,
+    pub parameters: &'static [&'static str],
+}
+
 pub fn from_anyhow(e: anyhow::Error) -> HttpApiProblem {
+    let e = match e.downcast::<HttpApiProblem>() {
+        Ok(problem) => return problem,
+        Err(e) => e,
+    };
+
     if let Some(db::Error::SwapNotFound) = e.downcast_ref::<db::Error>() {
-        return swap_not_found();
+        return HttpApiProblem::new("Swap not found.").set_status(StatusCode::NOT_FOUND);
     }
 
-    internal_error(e)
-}
+    if let Some(e) = e.downcast_ref::<UnexpectedQueryParameters>() {
+        log::error!("{}", e);
 
-pub fn internal_error(e: anyhow::Error) -> HttpApiProblem {
-    log::error!("internal error occurred: {:?}", e);
-    HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
-}
+        let mut problem = HttpApiProblem::new("Unexpected query parameter(s).")
+            .set_status(StatusCode::BAD_REQUEST)
+            .set_detail("This action does not take any query parameters.");
 
-pub fn missing_channel() -> HttpApiProblem {
-    log::error!("Channel for swap was not found in hash map");
-    HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
-}
+        problem
+            .set_value("unexpected_parameters", &e.parameters)
+            .expect("parameters will never fail to serialize");
 
-pub fn send_over_channel(_e: Response) -> HttpApiProblem {
-    log::error!("Sending response over channel failed");
-    HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
-}
+        return problem;
+    }
 
-pub fn state_store() -> HttpApiProblem {
-    log::error!("State store didn't have state in it despite swap being in database");
-    HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
-}
+    if let Some(e) = e.downcast_ref::<MissingQueryParameters>() {
+        log::error!("{}", e);
 
-pub fn swap_not_found() -> HttpApiProblem {
-    HttpApiProblem::new("Swap not found.").set_status(StatusCode::NOT_FOUND)
-}
+        let mut problem = HttpApiProblem::new("Missing query parameter(s).")
+            .set_status(StatusCode::BAD_REQUEST)
+            .set_detail("This action requires additional query parameters.");
 
-pub fn unsupported() -> HttpApiProblem {
-    HttpApiProblem::new("Swap not supported.")
-        .set_status(StatusCode::BAD_REQUEST)
-        .set_detail("The requested combination of ledgers and assets is not supported.")
-}
+        problem
+            .set_value("missing_parameters", &e.parameters)
+            .expect("parameters will never fail to serialize");
 
-pub fn deserialize(e: serde_json::Error) -> HttpApiProblem {
-    log::error!("Failed to deserialize body: {:?}", e);
-    HttpApiProblem::new("Invalid body.")
-        .set_status(StatusCode::BAD_REQUEST)
-        .set_detail("Failed to deserialize given body.")
-}
+        return problem;
+    }
 
-pub fn serialize(e: serde_json::Error) -> HttpApiProblem {
-    log::error!("Failed to serialize body: {:?}", e);
-    HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
-}
+    if e.is::<serde_json::Error>() {
+        log::error!("deserialization error: {:?}", e);
 
-pub fn not_yet_implemented(feature: &str) -> HttpApiProblem {
-    log::error!("{} not yet implemented", feature);
-    HttpApiProblem::new("Feature not yet implemented.")
-        .set_status(StatusCode::INTERNAL_SERVER_ERROR)
-        .set_detail(format!("{} is not yet implemented! Sorry :(", feature))
-}
+        return HttpApiProblem::new("Invalid body.")
+            .set_status(StatusCode::BAD_REQUEST)
+            .set_detail("Failed to deserialize given body.");
+    }
 
-pub fn action_already_done(action: ActionKind) -> HttpApiProblem {
-    log::error!("{} action has already been done", action);
-    HttpApiProblem::new("Action already done.").set_status(StatusCode::GONE)
-}
+    if e.is::<InvalidActionInvocation>() {
+        log::warn!("{:?}", e);
 
-pub fn invalid_action(action: ActionKind) -> HttpApiProblem {
-    log::error!("{} action is invalid for this swap", action);
-    HttpApiProblem::new("Invalid action.")
-        .set_status(StatusCode::CONFLICT)
-        .set_detail("Cannot perform requested action for this swap.")
-}
+        return HttpApiProblem::new("Invalid action invocation")
+            .set_status(http::StatusCode::METHOD_NOT_ALLOWED);
+    }
 
-pub fn unexpected_query_parameters(action: &str, parameters: Vec<String>) -> HttpApiProblem {
-    log::error!(
-        "Unexpected GET parameters {:?} for a {} action type. Expected: none",
-        parameters,
-        action
-    );
-    let mut problem = HttpApiProblem::new("Unexpected query parameter(s).")
-        .set_status(StatusCode::BAD_REQUEST)
-        .set_detail("This action does not take any query parameters.");
-
-    problem
-        .set_value("unexpected_parameters", &parameters)
-        .expect("parameters will never fail to serialize");
-
-    problem
-}
+    if e.is::<InvalidAction>() {
+        log::warn!("{:?}", e);
 
-pub fn missing_query_parameters(
-    action: &str,
-    parameters: Vec<&MissingQueryParameter>,
-) -> HttpApiProblem {
-    log::error!(
-        "Missing GET parameters for a {} action type. Expected: {:?}",
-        action,
-        parameters
-            .iter()
-            .map(|parameter| parameter.name)
-            .collect::<Vec<&str>>()
-    );
-
-    let mut problem = HttpApiProblem::new("Missing query parameter(s).")
-        .set_status(StatusCode::BAD_REQUEST)
-        .set_detail("This action requires additional query parameters.");
-
-    problem
-        .set_value("missing_parameters", &parameters)
-        .expect("parameters will never fail to serialize");
-
-    problem
-}
+        return HttpApiProblem::new("Invalid action.")
+            .set_status(StatusCode::CONFLICT)
+            .set_detail("Cannot perform requested action for this swap.");
+    }
+
+    if e.is::<UnsupportedSwap>() {
+        log::warn!("{:?}", e);
 
-impl From<state_store::Error> for HttpApiProblem {
-    fn from(e: state_store::Error) -> Self {
-        log::error!("Storage layer failure: {:?}", e);
-        HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
+        return HttpApiProblem::new("Swap not supported.")
+            .set_status(StatusCode::BAD_REQUEST)
+            .set_detail("The requested combination of ledgers and assets is not supported.");
     }
-}
 
-impl From<rfc003::state_machine::Error> for HttpApiProblem {
-    fn from(e: rfc003::state_machine::Error) -> Self {
-        log::error!("Protocol execution error: {:?}", e);
-        HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
-            .set_title("Protocol execution error.")
+    if e.is::<MalformedRequest>() {
+        log::warn!("{:?}", e);
+
+        return HttpApiProblem::with_title_and_type_from_status(StatusCode::BAD_REQUEST)
+            .set_detail("The request body was malformed.");
     }
+
+    log::error!("internal error occurred: {:?}", e);
+
+    HttpApiProblem::with_title_and_type_from_status(StatusCode::INTERNAL_SERVER_ERROR)
 }
 
 pub fn unpack_problem(rejection: Rejection) -> Result<impl Reply, Rejection> {
     if let Some(problem) = rejection.find_cause::<HttpApiProblem>() {
-        log::debug!(target: "http-api", "HTTP request got rejected, returning HttpApiProblem response: {:?}", problem);
-
         let code = problem.status.unwrap_or(StatusCode::INTERNAL_SERVER_ERROR);
 
         let reply = warp::reply::json(problem);