Merge #585

585: Fix/only update position if order succeeded r=klochowicz a=klochowicz

Improved version of #567 . Made sure I did the least amount of changes needed, and addressed issues raised by `@da-kami` and `@holzeis` in previous review.

Co-authored-by: Mariusz Klochowicz <mariusz@klochowicz.com>

coordinator/src/orderbook/db/orders.rs

@@ -164,7 +164,11 @@ pub fn insert(conn: &mut PgConnection, order: OrderbookNewOrder) -> QueryResult<
 }
 
 /// Returns the number of affected rows: 1.
-pub fn taken(conn: &mut PgConnection, id: Uuid, is_taken: bool) -> QueryResult<OrderbookOrder> {
+pub fn set_is_taken(
+    conn: &mut PgConnection,
+    id: Uuid,
+    is_taken: bool,
+) -> QueryResult<OrderbookOrder> {
     let order: Order = diesel::update(orders::table)
         .filter(orders::trader_order_id.eq(id))
         .set(orders::taken.eq(is_taken))

coordinator/src/orderbook/routes.rs

@@ -158,7 +158,7 @@ pub async fn post_order(
             notify_traders(matched_orders, authenticated_users.clone()).await;
 
             for order_id in orders_to_set_taken {
-                if let Err(err) = db::orders::taken(&mut conn, order_id, true) {
+                if let Err(err) = db::orders::set_is_taken(&mut conn, order_id, true) {
                     let order_id = order_id.to_string();
                     tracing::error!(order_id, "Could not set order to taken {err:#}");
                 }
@@ -192,7 +192,7 @@ pub async fn put_order(
     Json(updated_order): Json<UpdateOrder>,
 ) -> Result<Json<Order>, AppError> {
     let mut conn = get_db_connection(&state)?;
-    let order = orderbook::db::orders::taken(&mut conn, order_id, updated_order.taken)
+    let order = orderbook::db::orders::set_is_taken(&mut conn, order_id, updated_order.taken)
         .map_err(|e| AppError::InternalServerError(format!("Failed to update order: {e:#}")))?;
     let sender = state.tx_pricefeed.clone();
     update_pricefeed(OrderbookMsg::Update(order.clone()), sender);

coordinator/src/orderbook/tests/sample_test.rs

@@ -31,7 +31,7 @@ async fn crud_test() {
     )
     .unwrap();
 
-    let order = orders::taken(&mut conn, order.id, true).unwrap();
+    let order = orders::set_is_taken(&mut conn, order.id, true).unwrap();
     assert!(order.taken);
 
     let deleted = orders::delete_with_id(&mut conn, order.id).unwrap();

mobile/native/src/api.rs

@@ -157,8 +157,7 @@ pub async fn get_orders() -> Result<Vec<Order>> {
 
 #[tokio::main(flavor = "current_thread")]
 pub async fn get_positions() -> Result<Vec<Position>> {
-    let positions = position::handler::get_positions()
-        .await?
+    let positions = position::handler::get_positions()?
         .into_iter()
         .map(|order| order.into())
         .collect::<Vec<Position>>();

mobile/native/src/trade/order/handler.rs

@@ -7,6 +7,7 @@ use crate::trade::order::FailureReason;
 use crate::trade::order::Order;
 use crate::trade::order::OrderState;
 use crate::trade::position;
+use crate::trade::position::handler::update_position_after_order_submitted;
 use anyhow::bail;
 use anyhow::Context;
 use anyhow::Result;
@@ -17,7 +18,7 @@ pub async fn submit_order(order: Order) -> Result<()> {
     let url = format!("http://{}", config::get_http_endpoint());
     let orderbook_client = OrderbookClient::new(Url::parse(&url)?);
 
-    if let Err(e) = position::handler::update_position_after_order_submitted(order) {
+    if let Err(e) = position::handler::get_position_matching_order(&order) {
         order_failed(Some(order.id), FailureReason::OrderNotAcceptable, e)?;
         bail!("Could not submit order because extending/reducing the position is not part of the MVP scope");
     }
@@ -35,6 +36,7 @@ pub async fn submit_order(order: Order) -> Result<()> {
     }
 
     update_order_state_in_db_and_ui(order.id, OrderState::Open)?;
+    update_position_after_order_submitted(&order)?;
 
     Ok(())
 }

mobile/native/src/trade/position/handler.rs

@@ -9,7 +9,6 @@ use crate::trade::order::OrderState;
 use crate::trade::order::OrderType;
 use crate::trade::position::Position;
 use crate::trade::position::PositionState;
-use anyhow::bail;
 use anyhow::ensure;
 use anyhow::Context;
 use anyhow::Result;
@@ -58,32 +57,36 @@ pub async fn trade(filled: FilledWith) -> Result<()> {
 }
 
 /// Fetch the positions from the database
-pub async fn get_positions() -> Result<Vec<Position>> {
+pub fn get_positions() -> Result<Vec<Position>> {
     db::get_positions()
 }
 
 /// Update the position once an order was submitted
 ///
 /// If the new order submitted is an order that closes the current position, then the position will
 /// be updated to `Closing` state.
-pub fn update_position_after_order_submitted(submitted_order: Order) -> Result<()> {
-    if let Some(position) = db::get_positions()?.first() {
-        // closing the position
-        if position.direction == submitted_order.direction.opposite()
-            && position.quantity == submitted_order.quantity
-        {
-            db::update_position_state(position.contract_symbol, PositionState::Closing)?;
-            let mut position = position.clone();
-            position.position_state = PositionState::Closing;
-            event::publish(&EventInternal::PositionUpdateNotification(position));
-        } else {
-            bail!("Currently not possible to extend or reduce a position, you can only close the position with a counter-order");
-        }
+pub fn update_position_after_order_submitted(submitted_order: &Order) -> Result<()> {
+    if let Some(position) = get_position_matching_order(submitted_order)? {
+        db::update_position_state(position.contract_symbol, PositionState::Closing)?;
+        let mut position = position;
+        position.position_state = PositionState::Closing;
+        event::publish(&EventInternal::PositionUpdateNotification(position));
     }
-
     Ok(())
 }
 
+/// Returns the position that would be closed by submitted, if there is any
+pub fn get_position_matching_order(order: &Order) -> Result<Option<Position>> {
+    Ok(if let Some(position) = db::get_positions()?.first() {
+        // closing the position
+        ensure!(position.direction == order.direction.opposite()
+                && position.quantity == order.quantity, "Currently not possible to extend or reduce a position, you can only close the position with a counter-order");
+        Some(position.clone())
+    } else {
+        None
+    })
+}
+
 /// Resets the position to open again
 ///
 /// This should be called if a went in a dirty state, e.g. the position is currently in
@@ -169,8 +172,7 @@ pub fn price_update(prices: Prices) -> Result<()> {
 // a close position. In order to fixup the ui, we are
 // creating an order here and store it to the database.
 pub async fn close_position() -> Result<()> {
-    let positions = get_positions()
-        .await?
+    let positions = get_positions()?
         .into_iter()
         .filter(|p| p.position_state == PositionState::Open)
         .map(Position::from)