Migrate orderbook API from warp to axum #4080
Conversation
This commit migrates the orderbook HTTP API from the warp web framework to axum. The migration includes: - Replace warp filters with axum Router and handlers - Update HTTP status codes to match axum conventions (422 for JSON deserialization errors, proper 400/404 distinction) - Restructure API with centralized AppState shared across handlers - Implement axum-compatible middleware for metrics and tracing - Remove warp dependency and related tracing utilities - Update e2e tests to reflect new HTTP behavior Breaking changes: - Invalid JSON payloads now return 422 (Unprocessable Entity) instead of 400 - Path parameter validation now returns 400 for malformed values - Error response format remains compatible 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fixed critical issues identified in PR review: - Fixed route conflicts by using .merge() for multiple HTTP methods on same paths - Changed get_trades_v2 to use database_read instead of database_write - Reorganized routes alphabetically by prefix using .nest() for better maintainability - Made METRIC_LABELS a module-level const for reusability - Removed convert_json_response helper (too simple to warrant abstraction) - Optimized Arc wrapping by using trait references directly - Applied per-method middleware before merging to ensure correct metric labels - Simplified test code by removing unnecessary verbose match expressions 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the orderbook API from Warp to Axum, modernizing the web framework and improving code organization. The changes include replacing Warp filters with Axum Router, migrating API endpoints, updating dependencies, and adding an E2E test suite. The review focuses on identifying critical and high severity issues, with actionable suggestions for improvement, while adhering to the repository's stricter code review style guide. One comment was modified to explicitly reference a relevant repository rule.
| .route( | ||
| "/:auction_id", | ||
| axum::routing::get(get_solver_competition::get_solver_competition_by_id_handler) | ||
| .layer(middleware::from_fn(with_labelled_metric("v1/solver_competition"))), | ||
| ), | ||
| ) | ||
| // /token/* routes |
There was a problem hiding this comment.
The error function constructs a Json response but uses string literals for error_type and description. It would be more robust and maintainable to use String to avoid lifetime issues and allow for dynamic error messages.
| .route( | |
| "/:auction_id", | |
| axum::routing::get(get_solver_competition::get_solver_competition_by_id_handler) | |
| .layer(middleware::from_fn(with_labelled_metric("v1/solver_competition"))), | |
| ), | |
| ) | |
| // /token/* routes | |
| pub fn error(error_type: &str, description: impl AsRef<str>) -> Json<Error> { | |
| Json(Error { // Construct Json<Error> directly | |
| error_type: error_type.to_string(), // Convert to String | |
| description: description.as_ref().to_string(), // Convert to String | |
| data: None, | |
| }) | |
| } |
References
- Comments must include a clear, actionable suggestion for improvement. (link)
| pub fn error(error_type: &str, description: impl AsRef<str>) -> Json<Error> { | ||
| Json(Error { | ||
| error_type: error_type.to_string(), | ||
| description: description.as_ref().to_string(), | ||
| data: None, | ||
| }) |
There was a problem hiding this comment.
The error function constructs a Json response but uses string literals for error_type and description. It would be more robust and maintainable to use String to avoid lifetime issues and allow for dynamic error messages.
| pub fn error(error_type: &str, description: impl AsRef<str>) -> Json<Error> { | |
| Json(Error { | |
| error_type: error_type.to_string(), | |
| description: description.as_ref().to_string(), | |
| data: None, | |
| }) | |
| pub fn error(error_type: &str, description: impl AsRef<str>) -> Json<Error> { | |
| Json(Error { // Construct Json<Error> directly | |
| error_type: error_type.to_string(), // Convert to String | |
| description: description.as_ref().to_string(), // Convert to String | |
| data: None, | |
| }) | |
| } |
References
- Comments must include a clear, actionable suggestion for improvement. (link)
| .database_read | ||
| .trades_paginated(&trade_filter) | ||
| .await | ||
| .context("get_trades_v2"); |
There was a problem hiding this comment.
The code uses database_read to fetch trades, but the description mentions a bug fix in get_trades_v2 to use database_read instead of database_write. Verify that this handler indeed uses database_read to align with the description and prevent accidental writes.
| match status { | ||
| StatusCode::BAD_REQUEST => match error.error_type { | ||
| StatusCode::BAD_REQUEST => match error.error_type.as_str() { | ||
| "InvalidSignature" => Some(OrderReplacementError::InvalidSignature), | ||
| "OldOrderActivelyBidOn" => Some(OrderReplacementError::OldOrderActivelyBidOn), | ||
| _ => None, |
There was a problem hiding this comment.
The match statement is using error.error_type directly, which is a String. It should be matching against error.error_type.as_str() to compare string slices, which is more efficient and avoids unnecessary allocations.
| match status { | |
| StatusCode::BAD_REQUEST => match error.error_type { | |
| StatusCode::BAD_REQUEST => match error.error_type.as_str() { | |
| "InvalidSignature" => Some(OrderReplacementError::InvalidSignature), | |
| "OldOrderActivelyBidOn" => Some(OrderReplacementError::OldOrderActivelyBidOn), | |
| _ => None, | |
| match status { | |
| StatusCode::BAD_REQUEST => match error.error_type.as_str() { |
References
- Comments must include a clear, actionable suggestion for improvement. (link)
| StatusCode::BAD_REQUEST => match error.error_type.as_str() { | ||
| "InvalidSignature" => Some(OrderCancellationError::InvalidSignature), | ||
| "AlreadyCancelled" => Some(OrderCancellationError::AlreadyCancelled), | ||
| "OrderFullyExecuted" => Some(OrderCancellationError::OrderFullyExecuted), |
There was a problem hiding this comment.
The match statement is using error.error_type directly, which is a String. It should be matching against error.error_type.as_str() to compare string slices, which is more efficient and avoids unnecessary allocations.
match status {
StatusCode::BAD_REQUEST => match error.error_type.as_str() {References
- Comments must include a clear, actionable suggestion for improvement. (link)
Description
This PR migrates the orderbook API from Warp to Axum. The migration modernizes our web framework while maintaining API
compatibility, with improved code organization and reduced complexity (~200 lines removed).
Changes
Breaking Changes:
How to test
Existing tests + staging
I deployed these changes to base staging to ensure metrics were working, here's the graph
Follow up:
Unmarked spots are running
main, the wrong image was labeling endpoints with unknown because it was a fallback.That code has been removed and replaced with the current metric labeling scheme.
That scheme can be improved (read, made less verbose and error prone while being more general) but we need to change the metrics label, which I didn't do to minimize changes.