Generic convert_response for API reponse handling (#1292)

* implement generic convert_response method for api in most cases

* put tracing log into internal error method

orderbook/src/api.rs

@@ -16,9 +16,10 @@ pub mod post_quote;
 use crate::{
     api::post_quote::OrderQuoter, database::trades::TradeRetrieving, orderbook::Orderbook,
 };
-use anyhow::Error as anyhowError;
+use anyhow::{Error as anyhowError, Result};
 use serde::{de::DeserializeOwned, Serialize};
 use shared::{metrics::get_metric_storage_registry, price_estimation::PriceEstimationError};
+use std::fmt::Debug;
 use std::{convert::Infallible, sync::Arc};
 use warp::{
     hyper::StatusCode,
@@ -117,44 +118,54 @@ fn error(error_type: &str, description: impl AsRef<str>) -> Json {
     })
 }
 
-fn internal_error() -> Json {
+fn internal_error(error: anyhowError) -> Json {
+    tracing::error!(?error, "internal server error");
     json(&Error {
         error_type: "InternalServerError",
         description: "",
     })
 }
 
-pub trait WarpReplyConverting {
-    fn into_warp_reply(self) -> (Json, StatusCode);
+pub fn convert_json_response<T, E>(result: Result<T, E>) -> WithStatus<Json>
+where
+    T: Serialize,
+    E: IntoWarpReply + Debug,
+{
+    match result {
+        Ok(response) => with_status(warp::reply::json(&response), StatusCode::OK),
+        Err(err) => err.into_warp_reply(),
+    }
 }
 
-pub fn convert_get_orders_error_to_reply(err: anyhowError) -> WithStatus<Json> {
-    tracing::error!(?err, "get_orders error");
-    with_status(internal_error(), StatusCode::INTERNAL_SERVER_ERROR)
+pub trait IntoWarpReply {
+    fn into_warp_reply(self) -> WithStatus<Json>;
 }
 
-pub fn convert_get_trades_error_to_reply(err: anyhowError) -> WithStatus<Json> {
-    tracing::error!(?err, "get_trades error");
-    with_status(internal_error(), StatusCode::INTERNAL_SERVER_ERROR)
+impl IntoWarpReply for anyhowError {
+    fn into_warp_reply(self) -> WithStatus<Json> {
+        with_status(internal_error(self), StatusCode::INTERNAL_SERVER_ERROR)
+    }
 }
 
-pub fn price_estimation_error_to_warp_reply(err: PriceEstimationError) -> (Json, StatusCode) {
-    match err {
-        PriceEstimationError::UnsupportedToken(token) => (
-            error("UnsupportedToken", format!("Token address {:?}", token)),
-            StatusCode::BAD_REQUEST,
-        ),
-        PriceEstimationError::NoLiquidity => (
-            error("NoLiquidity", "not enough liquidity"),
-            StatusCode::NOT_FOUND,
-        ),
-        PriceEstimationError::ZeroAmount => (
-            error("ZeroAmount", "Please use non-zero amount field"),
-            StatusCode::BAD_REQUEST,
-        ),
-        PriceEstimationError::Other(err) => {
-            tracing::error!(?err, "get_market error");
-            (internal_error(), StatusCode::INTERNAL_SERVER_ERROR)
+impl IntoWarpReply for PriceEstimationError {
+    fn into_warp_reply(self) -> WithStatus<Json> {
+        match self {
+            Self::UnsupportedToken(token) => with_status(
+                error("UnsupportedToken", format!("Token address {:?}", token)),
+                StatusCode::BAD_REQUEST,
+            ),
+            Self::NoLiquidity => with_status(
+                error("NoLiquidity", "not enough liquidity"),
+                StatusCode::NOT_FOUND,
+            ),
+            Self::ZeroAmount => with_status(
+                error("ZeroAmount", "Please use non-zero amount field"),
+                StatusCode::BAD_REQUEST,
+            ),
+            Self::Other(err) => with_status(
+                internal_error(err.context("price_estimation")),
+                StatusCode::INTERNAL_SERVER_ERROR,
+            ),
         }
     }
 }

orderbook/src/api/cancel_order.rs

@@ -1,6 +1,6 @@
 use crate::api::extract_payload;
 use crate::orderbook::{OrderCancellationResult, Orderbook};
-use anyhow::Result;
+use anyhow::{Context, Result};
 use model::signature::EcdsaSignature;
 use model::{
     order::{OrderCancellation, OrderUid},
@@ -63,7 +63,10 @@ pub fn cancel_order_response(result: Result<OrderCancellationResult>) -> impl Re
             super::error("OnChainOrder", "On-chain orders must be cancelled on-chain"),
             StatusCode::BAD_REQUEST,
         ),
-        Err(_) => (super::internal_error(), StatusCode::INTERNAL_SERVER_ERROR),
+        Err(err) => (
+            super::internal_error(err.context("cancel_order")),
+            StatusCode::INTERNAL_SERVER_ERROR,
+        ),
     };
     warp::reply::with_status(body, status_code)
 }
@@ -74,10 +77,7 @@ pub fn cancel_order(
     cancel_order_request().and_then(move |order| {
         let orderbook = orderbook.clone();
         async move {
-            let result = orderbook.cancel_order(order).await;
-            if let Err(err) = &result {
-                tracing::error!(?err, ?order, "cancel_order error");
-            }
+            let result = orderbook.cancel_order(order).await.context("cancel_order");
             Result::<_, Infallible>::Ok(cancel_order_response(result))
         }
     })

orderbook/src/api/create_order.rs

@@ -1,10 +1,11 @@
 use crate::{
-    api::{extract_payload, WarpReplyConverting},
+    api::{extract_payload, IntoWarpReply},
     orderbook::{AddOrderResult, Orderbook},
 };
 use anyhow::Result;
 use model::order::OrderCreationPayload;
 use std::{convert::Infallible, sync::Arc};
+use warp::reply::with_status;
 use warp::{hyper::StatusCode, Filter, Rejection, Reply};
 
 pub fn create_order_request(
@@ -15,20 +16,22 @@ pub fn create_order_request(
 }
 
 pub fn create_order_response(result: Result<AddOrderResult>) -> impl Reply {
-    let (body, status_code) = match result {
-        Ok(AddOrderResult::Added(uid)) => (warp::reply::json(&uid), StatusCode::CREATED),
+    match result {
+        Ok(AddOrderResult::Added(uid)) => with_status(warp::reply::json(&uid), StatusCode::CREATED),
         Ok(AddOrderResult::OrderValidation(err)) => err.into_warp_reply(),
-        Ok(AddOrderResult::UnsupportedSignature) => (
+        Ok(AddOrderResult::UnsupportedSignature) => with_status(
             super::error("UnsupportedSignature", "signing scheme is not supported"),
             StatusCode::BAD_REQUEST,
         ),
-        Ok(AddOrderResult::DuplicatedOrder) => (
+        Ok(AddOrderResult::DuplicatedOrder) => with_status(
             super::error("DuplicatedOrder", "order already exists"),
             StatusCode::BAD_REQUEST,
         ),
-        Err(_) => (super::internal_error(), StatusCode::INTERNAL_SERVER_ERROR),
-    };
-    warp::reply::with_status(body, status_code)
+        Err(err) => with_status(
+            super::internal_error(err.context("create_order")),
+            StatusCode::INTERNAL_SERVER_ERROR,
+        ),
+    }
 }
 
 pub fn create_order(
@@ -39,6 +42,10 @@ pub fn create_order(
         async move {
             let order_payload_clone = order_payload.clone();
             let result = orderbook.add_order(order_payload).await;
+            // TODO - This is one place where the error log is more rich than the
+            //  generic error inside internal_error (i.e. doesn't include order_payload).
+            //  Perhaps this should be a warning and the real alert comes from the internal error.
+            //  Otherwise we should just resort to using this error logging style everywhere.
             if let Err(err) = &result {
                 tracing::error!(?err, ?order_payload_clone, "add_order error");
             }

orderbook/src/api/get_fee_and_quote.rs

@@ -1,5 +1,6 @@
-use crate::api::post_quote::{
-    response, OrderQuoteRequest, OrderQuoteResponse, OrderQuoteSide, OrderQuoter, SellAmount,
+use crate::api::{
+    convert_json_response,
+    post_quote::{OrderQuoteRequest, OrderQuoteResponse, OrderQuoteSide, OrderQuoter, SellAmount},
 };
 use anyhow::Result;
 use chrono::{DateTime, Utc};
@@ -120,7 +121,7 @@ pub fn get_fee_and_quote_sell(
     sell_request().and_then(move |query: SellQuery| {
         let quoter = quoter.clone();
         async move {
-            Result::<_, Infallible>::Ok(response(
+            Result::<_, Infallible>::Ok(convert_json_response(
                 quoter
                     .calculate_quote(&query.into())
                     .await
@@ -136,7 +137,7 @@ pub fn get_fee_and_quote_buy(
     buy_request().and_then(move |query: BuyQuery| {
         let quoter = quoter.clone();
         async move {
-            Result::<_, Infallible>::Ok(response(
+            Result::<_, Infallible>::Ok(convert_json_response(
                 quoter
                     .calculate_quote(&query.into())
                     .await

orderbook/src/api/get_fee_info.rs

@@ -1,14 +1,13 @@
-use crate::api::price_estimation_error_to_warp_reply;
+use crate::api::convert_json_response;
 use crate::fee::MinFeeCalculating;
 use anyhow::Result;
 use chrono::{DateTime, Utc};
 use model::{order::OrderKind, u256_decimal};
 use primitive_types::{H160, U256};
 use serde::{Deserialize, Serialize};
-use shared::price_estimation::PriceEstimationError;
 use std::convert::Infallible;
 use std::sync::Arc;
-use warp::{hyper::StatusCode, reply, Filter, Rejection, Reply};
+use warp::{Filter, Rejection, Reply};
 
 #[derive(Debug, Serialize, Deserialize)]
 #[serde(rename_all = "camelCase")]
@@ -34,41 +33,27 @@ fn get_fee_info_request() -> impl Filter<Extract = (Query,), Error = Rejection>
         .and(warp::query::<Query>())
 }
 
-pub fn get_fee_info_response(
-    result: Result<(U256, DateTime<Utc>), PriceEstimationError>,
-) -> impl Reply {
-    match result {
-        Ok((amount, expiration_date)) => {
-            let fee_info = FeeInfo {
-                expiration_date,
-                amount,
-            };
-            Ok(reply::with_status(reply::json(&fee_info), StatusCode::OK))
-        }
-        Err(err) => {
-            let (json, status_code) = price_estimation_error_to_warp_reply(err);
-            Ok(reply::with_status(json, status_code))
-        }
-    }
-}
-
 pub fn get_fee_info(
     fee_calculator: Arc<dyn MinFeeCalculating>,
 ) -> impl Filter<Extract = (impl Reply,), Error = Rejection> + Clone {
     get_fee_info_request().and_then(move |query: Query| {
         let fee_calculator = fee_calculator.clone();
         async move {
-            Result::<_, Infallible>::Ok(get_fee_info_response(
-                fee_calculator
-                    .compute_subsidized_min_fee(
-                        query.sell_token,
-                        Some(query.buy_token),
-                        Some(query.amount),
-                        Some(query.kind),
-                        None,
-                    )
-                    .await,
-            ))
+            let result = fee_calculator
+                .compute_subsidized_min_fee(
+                    query.sell_token,
+                    Some(query.buy_token),
+                    Some(query.amount),
+                    Some(query.kind),
+                    None,
+                )
+                .await;
+            Result::<_, Infallible>::Ok(convert_json_response(result.map(
+                |(amount, expiration_date)| FeeInfo {
+                    expiration_date,
+                    amount,
+                },
+            )))
         }
     })
 }
@@ -78,6 +63,8 @@ mod tests {
     use super::*;
     use crate::api::response_body;
     use chrono::FixedOffset;
+    use shared::price_estimation::PriceEstimationError;
+    use warp::hyper::StatusCode;
     use warp::test::request;
 
     #[tokio::test]
@@ -101,9 +88,11 @@ mod tests {
 
     #[tokio::test]
     async fn get_fee_info_response_() {
-        let response =
-            get_fee_info_response(Ok((U256::zero(), Utc::now() + FixedOffset::east(10))))
-                .into_response();
+        let result = Ok(FeeInfo {
+            expiration_date: Utc::now() + FixedOffset::east(10),
+            amount: U256::zero(),
+        });
+        let response = convert_json_response::<_, PriceEstimationError>(result).into_response();
         assert_eq!(response.status(), StatusCode::OK);
         let body = response_body(response).await;
         let body: FeeInfo = serde_json::from_slice(body.as_slice()).unwrap();