fix: Do not delete orders

Instead of deleting orders, I propose to keep them in state `Failed`. That might blow up our database, but we could at any time delete `Failed` orders if we want to.

To be consistent with the existing implementation I am also sending a `DeleteOrder` event to update the prices for the apps.

coordinator/src/orderbook/db/orders.rs

@@ -276,13 +276,20 @@ pub fn set_order_state(
     Ok(OrderbookOrder::from(order))
 }
 
-pub fn set_expired_limit_orders_to_failed(conn: &mut PgConnection) -> QueryResult<usize> {
-    diesel::update(orders::table)
+pub fn set_expired_limit_orders_to_failed(
+    conn: &mut PgConnection,
+) -> QueryResult<Vec<OrderbookOrder>> {
+    let expired_limit_orders: Vec<Order> = diesel::update(orders::table)
         .filter(orders::order_state.eq(OrderState::Open))
         .filter(orders::order_type.eq(OrderType::Limit))
         .filter(orders::expiry.lt(OffsetDateTime::now_utc()))
         .set(orders::order_state.eq(OrderState::Failed))
-        .execute(conn)
+        .get_results(conn)?;
+
+    Ok(expired_limit_orders
+        .into_iter()
+        .map(OrderbookOrder::from)
+        .collect())
 }
 
 /// Returns the order by id
@@ -355,10 +362,3 @@ pub fn get_all_limit_order_filled_matches(
 
     Ok(filled_matches)
 }
-
-/// Returns the number of affected rows: 1.
-pub fn delete_with_id(conn: &mut PgConnection, order_id: Uuid) -> QueryResult<usize> {
-    diesel::delete(orders::table)
-        .filter(orders::trader_order_id.eq(order_id))
-        .execute(conn)
-}

coordinator/src/orderbook/routes.rs

@@ -147,21 +147,6 @@ pub async fn put_order(
     Ok(Json(order))
 }
 
-pub async fn delete_order(
-    Path(order_id): Path<Uuid>,
-    State(state): State<Arc<AppState>>,
-) -> Result<Json<usize>, AppError> {
-    let mut conn = get_db_connection(&state)?;
-    let deleted = orderbook::db::orders::delete_with_id(&mut conn, order_id)
-        .map_err(|e| AppError::InternalServerError(format!("Failed to delete order: {e:#}")))?;
-    if deleted > 0 {
-        let sender = state.tx_price_feed.clone();
-        update_pricefeed(Message::DeleteOrder(order_id), sender);
-    }
-
-    Ok(Json(deleted))
-}
-
 pub async fn websocket_handler(
     ws: WebSocketUpgrade,
     State(state): State<Arc<AppState>>,

coordinator/src/orderbook/tests/sample_test.rs

@@ -36,12 +36,6 @@ async fn crud_test() {
 
     let order = orders::set_is_taken(&mut conn, order.id, true).unwrap();
     assert_eq!(order.order_state, OrderState::Taken);
-
-    let deleted = orders::delete_with_id(&mut conn, order.id).unwrap();
-    assert_eq!(deleted, 1);
-
-    let orders = orders::all(&mut conn, true).unwrap();
-    assert!(orders.is_empty());
 }
 
 #[tokio::test]

coordinator/src/orderbook/trading.rs

@@ -158,7 +158,12 @@ async fn process_new_order(
     // not get matched.
     // TODO(holzeis): orders should probably do not have an expiry, but should either be
     // replaced or deleted if not wanted anymore.
-    orders::set_expired_limit_orders_to_failed(conn)?;
+    let expired_limit_orders = orders::set_expired_limit_orders_to_failed(conn)?;
+    for expired_limit_order in expired_limit_orders {
+        tx_price_feed
+            .send(Message::DeleteOrder(expired_limit_order.id))
+            .map_err(|error| anyhow!("Could not update price feed due to '{error}'"))?;
+    }
 
     let order = orders::insert(conn, new_order.clone(), order_reason)
         .map_err(|e| anyhow!("Failed to insert new order into db: {e:#}"))?;

coordinator/src/routes.rs

@@ -12,7 +12,6 @@ use crate::admin::sign_message;
 use crate::db::user;
 use crate::message::NewUserMessage;
 use crate::node::Node;
-use crate::orderbook::routes::delete_order;
 use crate::orderbook::routes::get_order;
 use crate::orderbook::routes::get_orders;
 use crate::orderbook::routes::post_order;
@@ -117,7 +116,7 @@ pub fn router(
         .route("/api/orderbook/orders", get(get_orders).post(post_order))
         .route(
             "/api/orderbook/orders/:order_id",
-            get(get_order).put(put_order).delete(delete_order),
+            get(get_order).put(put_order),
         )
         .route("/api/orderbook/websocket", get(websocket_handler))
         .route("/api/trade", post(post_trade))

maker/src/trading/mod.rs

@@ -60,10 +60,6 @@ pub async fn run(
             let _ = bitmex_pricefeed_tx.send(ServiceStatus::Online);
             tracing::debug!("Received new quote {quote:?}");
 
-            // Clear stale orders. They should have expired by now.
-            for order in orders.iter() {
-                delete_order(&orderbook_client, orderbook_url, order).await;
-            }
             orders.clear();
 
             for _i in 0..concurrent_orders {
@@ -113,14 +109,3 @@ async fn add_order(
         })
         .ok()
 }
-
-async fn delete_order(
-    orderbook_client: &OrderbookClient,
-    orderbook_url: &Url,
-    last_order: &OrderResponse,
-) {
-    let order_id = last_order.id;
-    if let Err(err) = orderbook_client.delete_order(orderbook_url, order_id).await {
-        tracing::error!("Failed deleting old order `{order_id}` because of {err:#}");
-    }
-}

maker/src/trading/orderbook_http_client.rs

@@ -3,7 +3,6 @@ use anyhow::Result;
 use orderbook_commons::NewOrder;
 use orderbook_commons::OrderResponse;
 use reqwest::Url;
-use uuid::Uuid;
 
 pub struct OrderbookClient {
     client: reqwest::Client,
@@ -32,17 +31,4 @@ impl OrderbookClient {
             bail!("Could not create new order ")
         }
     }
-
-    pub async fn delete_order(&self, url: &Url, order_id: Uuid) -> Result<()> {
-        let url = url.join(format!("/api/orderbook/orders/{order_id}").as_str())?;
-
-        let response = self.client.delete(url).send().await?;
-
-        if response.status().as_u16() == 200 {
-            Ok(())
-        } else {
-            tracing::error!("Could not delete new order");
-            bail!("Could not create new order ")
-        }
-    }
 }