Allow solvers to return custom errors#4232
Allow solvers to return custom errors#4232ashleychandy wants to merge 4 commits intocowprotocol:mainfrom
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism for solvers to return custom errors, which are then propagated to the API. This is a valuable improvement for providing more specific feedback to users. The implementation is solid, with changes across the DTOs, driver, and observability components. I've identified one area in the error handling logic where code duplication can be reduced to improve maintainability.
crates/driver/src/infra/api/error.rs
Outdated
| let (kind, description) = match custom_err { | ||
| solvers_dto::solution::SolverError::TradingOutsideAllowedWindow { message } => { | ||
| ( | ||
| Kind::TradingOutsideAllowedWindow, | ||
| message.clone().unwrap_or_else(|| | ||
| "Token can only be traded during specific time windows".to_string() | ||
| ), | ||
| ) | ||
| } | ||
| solvers_dto::solution::SolverError::TokenTemporarilySuspended { message } => { | ||
| ( | ||
| Kind::TokenTemporarilySuspended, | ||
| message.clone().unwrap_or_else(|| | ||
| "Token is temporarily suspended from trading".to_string() | ||
| ), | ||
| ) | ||
| } | ||
| solvers_dto::solution::SolverError::InsufficientLiquidity { message } => { | ||
| ( | ||
| Kind::InsufficientLiquidity, | ||
| message.clone().unwrap_or_else(|| | ||
| "Insufficient liquidity for the requested trade size".to_string() | ||
| ), | ||
| ) | ||
| } | ||
| solvers_dto::solution::SolverError::UnauthorizedTrader { message } => { | ||
| ( | ||
| Kind::UnauthorizedTrader, | ||
| message.clone().unwrap_or_else(|| | ||
| "Token requires special permissions or whitelisting".to_string() | ||
| ), | ||
| ) | ||
| } | ||
| solvers_dto::solution::SolverError::Other { message } => { | ||
| ( | ||
| Kind::CustomSolverError, | ||
| message.clone().unwrap_or_else(|| | ||
| "Solver returned a custom error".to_string() | ||
| ), | ||
| ) | ||
| } | ||
| }; | ||
| return ( | ||
| axum::http::StatusCode::BAD_REQUEST, | ||
| axum::Json(Error { kind, description }), | ||
| ); |
There was a problem hiding this comment.
This match statement contains significant code duplication for handling the optional message and providing a default description. This can be refactored to reduce complexity and improve maintainability by separating the logic for identifying the error type from the logic for constructing the description string.
let (kind, message, default_description) = match custom_err {
solvers_dto::solution::SolverError::TradingOutsideAllowedWindow { message } => (
Kind::TradingOutsideAllowedWindow,
message,
"Token can only be traded during specific time windows",
),
solvers_dto::solution::SolverError::TokenTemporarilySuspended { message } => (
Kind::TokenTemporarilySuspended,
message,
"Token is temporarily suspended from trading",
),
solvers_dto::solution::SolverError::InsufficientLiquidity { message } => (
Kind::InsufficientLiquidity,
message,
"Insufficient liquidity for the requested trade size",
),
solvers_dto::solution::SolverError::UnauthorizedTrader { message } => (
Kind::UnauthorizedTrader,
message,
"Token requires special permissions or whitelisting",
),
solvers_dto::solution::SolverError::Other { message } => (
Kind::CustomSolverError,
message,
"Solver returned a custom error",
),
};
let description = message.as_deref().unwrap_or(default_description).to_string();
return (
axum::http::StatusCode::BAD_REQUEST,
axum::Json(Error { kind, description }),
);There was a problem hiding this comment.
All message fields are optional, that hints that the modelling is not quite there
There was a problem hiding this comment.
From the code alone I don't see how this change modifies anything in the rest of the code
crates/solvers-dto/src/solution.rs
Outdated
| /// Optional custom error that explains why no solutions could be computed. | ||
| /// When multiple solvers return errors, the system will pick one to return. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub error: Option<SolverError>, |
There was a problem hiding this comment.
If the solution is the payload the solver currently returns, adding an error field is a bit confusing as it's no longer a solution
Feels to me that an enum would be better suited (names up for discussion)
enum SolverResponse { Solution { solutions: Vec<Solution> }, Error { code: SolverError, message: Option<String> }
Separating the message from the code also brings more flexibility
Co-authored-by: José Duarte <duarte.gmj@gmail.com>
|
Thanks for the review! Implemented the changes you suggested - separated error code from message, refactored SolverResponse to a proper enum, and simplified the error mapping. |
jmg-duarte
left a comment
There was a problem hiding this comment.
Personally, I'm still not super convinced about the way the Solutions struct is modeled; my colleagues may have some ideas for this
One thing that is missing is an E2E test for the new errors; writing one from scratch can be cumbersome so I suggest:
- You start by copy pasting another E2E related to the driver
- You ask some AI agent to prepare an E2E test for you, and you review it to check if it makes sense before you push it for our review
Additionally, testing the new solutions & error serialization with unit tests would also be helpful since:
- If in the future we make some change that breaks it, the test will provide us with a warning
- It provides a format example for both human and agentic developers
| #[derive(Debug, Serialize, Deserialize, Default)] | ||
| #[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct Solutions { |
There was a problem hiding this comment.
This breaks compilation
error[E0432]: unresolved import `solvers_dto::solution::Solutions`
--> crates/solvers/src/api/routes/solve/dto/mod.rs:4:41
|
4 | pub use solvers_dto::{auction::Auction, solution::Solutions};
| ^^^^^^^^^^---------
| | |
| | help: a similar name exists in the module: `Solution`
| no `Solutions` in `solution`
|
= help: consider importing this variant instead:
solvers_dto::solution::SolverResponse::Solutions
| pub struct Error { | ||
| kind: Kind, | ||
| description: &'static str, | ||
| description: String, |
There was a problem hiding this comment.
This should rather be Cow<'static, str> which will provide you with the best of both approaches here, saving on allocations for static strings (the already existing errors)
Description
Fixes #4222
Previously, when solvers couldn't compute quotes for RWA tokens, they returned a generic
QuotingFailederror. This made it difficult for the frontend to provide helpful feedback.This change adds support for custom solver errors that can be returned when quoting fails. Solvers can now indicate specific reasons like:
TradingOutsideAllowedWindowTokenTemporarilySuspendedInsufficientLiquidityUnauthorizedTraderOtherWhen a solver returns a custom error, the driver propagates it through the API response so the frontend can display an appropriate error message.
Changes
SolverErrorenum with 5 variants insolvers-dtoerrorfield toSolutionsDTOSolverCustomErrormetric