Generic convert_response for API reponse handling

[bh2smith]

Based on the suggestion here

We write a generic method convert_response which is used in place of all the *_response(result: Result<XXX>) methods where appropriate.

This method expects the Err( . ) route to implement WarpReplyConverting. Additional work will have to go into generically handling the Ok( . ) route - which is still lingering around for some reply conversions and will be addressed in a follow up PR.

Test Plan

No logical changes - existing CI.

[vkgnosis]

which is used in place of all the

Is that the plan as what is supposed be in this PR already? Because we're only changing one method here.

[bh2smith]

Is that the plan as what is supposed be in this PR already? Because we're only changing one method here.

It wasn't exactly the plan for this PR but I do agree it would make sense to include here or soon after. It will probably be a more involved change and whether or not its done here depends on your suggestion.

[vkgnosis]

The idea in this PR seems to be fine but it's hard to tell how useful the abstraction is when only convert one use. If you think it will fit then I'm fine with it.

[vkgnosis]

Seems fine to me. Although I would

[nlordell]

I don't think the PR implements my suggestion linked in the description.

Specifically:

• In general, for Result<T, E>, we T: Serialize because we convert it to JSON, so I don't really understand why we need the trait bound T: WarpReplyConverting - instead I suggest we should use warp::reply::json(ok_value).
• The suggestion was about using the E: WarpReplyConverting for converting errors into warp replies. For this, we already have a few error types that implement this (PartialValidationError, ValidationError, FeeError). Furthermore, you could easily impl WarpReplyConverting for anyhow::Error where it returns a 500 error.

In the end, if we had any result where the Ok type implements Serialize (i.e. can be converted into JSON), then the convert_response would just convert into JSON for us, and then the Error type would just need to implement WarpReplyConverting and the convert_response could be used everywhere.

In general, my suggestion was aiming at deduplicate methods like:

pub fn response<T: Serialize>(result: Result<T, OrderQuoteError>) -> impl Reply {
    match result {
        Ok(response) => reply::with_status(reply::json(&response), StatusCode::OK),
        Err(err) => {
            let (reply, status) = err.convert_to_reply();
            reply::with_status(reply, status)
        }
    }
}

Where instead of using OrderQuoteError, you make it generic on the error type E where E: WarpReplyConverting.

edit: Some other methods that could be de-deduplicated by making this generic on the error type:

• orderbook::api::get_user_orders::response
• orderbook::api::get_trades::get_trades_response
• orderbook::api::get_solvable_orders::get_solvable_orders_response
• orderbook::api::get_solvable_orders_v2::get_solvable_orders_response
• orderbook::api::get_orders::get_orders_response
• orderbook::api::get_orders_by_tx::get_orders_by_tx_response
• Maybe more.

[bh2smith]

Thanks @nlordell for clarifying here. I will attempt to implement your suggested change, but I would point out that in several cases we are actually converting the Ok(ResultEnumContainingBothErrorAndOk) with a large match statement and not just Status code of Ok. Of course there are more places where we handle the error in this fashion as well. I would argue that we must deal with both cases simultaneously. However, I agree that there are more candidates for de-duplication with what you're suggesting so I will aim to handle that first.

UPDATE: I would argue that both <T, E> in this context should be WarpReplyConverting here are a few examples of why:

In each of the examples to follow, we aim to demonstrate why Ok(response) => reply::with_status(reply::json(&response), StatusCode::OK) isn't enough

gp-v2-services/orderbook/src/api/get_solvable_orders_v2.rs

Lines 11 to 22 in 863f899

	fn get_solvable_orders_response(result: Result<SolvableOrders>) -> impl Reply {
	match result {
	Ok(orders) => Ok(reply::with_status(
	reply::json(&model::SolvableOrders {
	orders: orders.orders,
	latest_settlement_block: orders.latest_settlement_block,
	}),
	StatusCode::OK,
	)),
	Err(err) => Ok(convert_get_orders_error_to_reply(err)),
	}
	}

gp-v2-services/orderbook/src/api/get_order_by_uid.rs

Lines 12 to 26 in 863f899

	pub fn get_order_by_uid_response(result: Result<Option<Order>>) -> impl Reply {
	let order = match result {
	Ok(order) => order,
	Err(err) => {
	return Ok(convert_get_orders_error_to_reply(err));
	}
	};
	Ok(match order {
	Some(order) => reply::with_status(reply::json(&order), StatusCode::OK),
	None => reply::with_status(
	super::error("NotFound", "Order was not found"),
	StatusCode::NOT_FOUND,
	),
	})
	}

gp-v2-services/orderbook/src/api/get_solvable_orders.rs

Lines 11 to 19 in 863f899

	fn get_solvable_orders_response(result: Result<SolvableOrders>) -> impl Reply {
	match result {
	Ok(orders) => Ok(reply::with_status(
	reply::json(&orders.orders),
	StatusCode::OK,
	)),
	Err(err) => Ok(convert_get_orders_error_to_reply(err)),
	}
	}

gp-v2-services/orderbook/src/api/create_order.rs

Lines 17 to 32 in 863f899

	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),
	Ok(AddOrderResult::OrderValidation(err)) => err.into_warp_reply(),
	Ok(AddOrderResult::UnsupportedSignature) => (
	super::error("UnsupportedSignature", "signing scheme is not supported"),
	StatusCode::BAD_REQUEST,
	),
	Ok(AddOrderResult::DuplicatedOrder) => (
	super::error("DuplicatedOrder", "order already exists"),
	StatusCode::BAD_REQUEST,
	),
	Err(_) => (super::internal_error(), StatusCode::INTERNAL_SERVER_ERROR),
	};
	warp::reply::with_status(body, status_code)
	}

gp-v2-services/orderbook/src/api/cancel_order.rs

Lines 32 to 69 in 863f899

	pub fn cancel_order_response(result: Result<OrderCancellationResult>) -> impl Reply {
	let (body, status_code) = match result {
	Ok(OrderCancellationResult::Cancelled) => (warp::reply::json(&"Cancelled"), StatusCode::OK),
	Ok(OrderCancellationResult::InvalidSignature) => (
	super::error("InvalidSignature", "Likely malformed signature"),
	StatusCode::BAD_REQUEST,
	),
	Ok(OrderCancellationResult::AlreadyCancelled) => (
	super::error("AlreadyCancelled", "Order is already cancelled"),
	StatusCode::BAD_REQUEST,
	),
	Ok(OrderCancellationResult::OrderFullyExecuted) => (
	super::error("OrderFullyExecuted", "Order is fully executed"),
	StatusCode::BAD_REQUEST,
	),
	Ok(OrderCancellationResult::OrderExpired) => (
	super::error("OrderExpired", "Order is expired"),
	StatusCode::BAD_REQUEST,
	),
	Ok(OrderCancellationResult::OrderNotFound) => (
	super::error("OrderNotFound", "Order not located in database"),
	StatusCode::NOT_FOUND,
	),
	Ok(OrderCancellationResult::WrongOwner) => (
	super::error(
	"WrongOwner",
	"Signature recovery's owner doesn't match order's",
	),
	StatusCode::UNAUTHORIZED,
	),
	Ok(OrderCancellationResult::OnChainOrder) => (
	super::error("OnChainOrder", "On-chain orders must be cancelled on-chain"),
	StatusCode::BAD_REQUEST,
	),
	Err(_) => (super::internal_error(), StatusCode::INTERNAL_SERVER_ERROR),
	};
	warp::reply::with_status(body, status_code)
	}

gp-v2-services/orderbook/src/api/get_fee_info.rs

Lines 37 to 53 in 863f899

	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))
	}
	}
	}

gp-v2-services/orderbook/src/api/get_markets.rs

Lines 69 to 86 in 863f899

	fn get_amount_estimate_response(
	result: Result<price_estimation::Estimate, PriceEstimationError>,
	query: AmountEstimateQuery,
	) -> impl Reply {
	match result {
	Ok(estimate) => reply::with_status(
	reply::json(&AmountEstimateResult {
	amount: estimate.out_amount,
	token: query.market.quote_token,
	}),
	StatusCode::OK,
	),
	Err(err) => {
	let (json, status_code) = price_estimation_error_to_warp_reply(err);
	reply::with_status(json, status_code)
	}
	}
	}

The only methods that fit naturally into the generic structure you propose are

• get_orders_response
• get_orders_by_tx_response
• get_trades_response

So I am seeing 7/10 that don't fit the description.

Anyway, I will continue attempting to fit this together.

[codecov-commenter]

Codecov Report

Merging #1292 (1ec4687) into main (2c0b991) will decrease coverage by 0.08%.
The diff coverage is 38.19%.

@@            Coverage Diff             @@
##             main    #1292      +/-   ##
==========================================
- Coverage   67.73%   67.65%   -0.09%     
==========================================
  Files         138      138              
  Lines       28936    28853      -83     
==========================================
- Hits        19601    19521      -80     
+ Misses       9335     9332       -3     

[bh2smith]

I have now implemented a combination of both possibilities with convert_response_ok and convert_response_err. I am looking for a clean way to combine these ideas, but I can only imagine this would somehow mean separating the enum patters into errors and oks respectively (although I think this will get quite messy). I noticed that we won't simply be able to do a generic status code of ok, because for example when creating an order the positive status code is CREATED.

[nlordell]

So I am seeing 7/10 that don't fit the description.

Hmm, I'm not sure I agree. In all of those examples, I see a:

Ok(reply::with_status(reply::json(...), StatusCode::OK))

Just to pick a single example:

gp-v2-services/orderbook/src/api/get_markets.rs

Lines 69 to 86 in 863f899

	fn get_amount_estimate_response(
	result: Result<price_estimation::Estimate, PriceEstimationError>,
	query: AmountEstimateQuery,
	) -> impl Reply {
	match result {
	Ok(estimate) => reply::with_status(
	reply::json(&AmountEstimateResult {
	amount: estimate.out_amount,
	token: query.market.quote_token,
	}),
	StatusCode::OK,
	),
	Err(err) => {
	let (json, status_code) = price_estimation_error_to_warp_reply(err);
	reply::with_status(json, status_code)
	}
	}
	}

With the proposed convert_response it can be implemented:

convert_response(result.map(|estimate| AmountEstimateResult { 
    amount: estimate.out_amount, 
    token: query.market.quote_token, 
}))

I only see two that are problematic and those are create_order and cancel_order that use Ok(OrderXResult::Y) to signal errors. In those cases, I would recommend not changing them to use Ok variant only for successes.

[bh2smith]

Nice, I see that this little thingy

convert_response(result.map(|estimate| AmountEstimateResult { 
    amount: estimate.out_amount, 
    token: query.market.quote_token, 
}))

is the key to adapting this in all places. thanks.

However, in this place we see that the patter can also not be generalized since we are matching an option here

gp-v2-services/orderbook/src/api/get_order_by_uid.rs

Lines 12 to 26 in 863f899

	pub fn get_order_by_uid_response(result: Result<Option<Order>>) -> impl Reply {
	let order = match result {
	Ok(order) => order,
	Err(err) => {
	return Ok(convert_get_orders_error_to_reply(err));
	}
	};
	Ok(match order {
	Some(order) => reply::with_status(reply::json(&order), StatusCode::OK),
	None => reply::with_status(
	super::error("NotFound", "Order was not found"),
	StatusCode::NOT_FOUND,
	),
	})
	}