From 12f0bfc468fc686504750a128a9199b578c5e2e0 Mon Sep 17 00:00:00 2001 From: grunch Date: Wed, 2 Sep 2026 09:41:38 -0300 Subject: [PATCH 1/2] =?UTF-8?q?feat:=20CancelOrderRequest.pretrade=5Fonly?= =?UTF-8?q?=20=E2=80=94=20refuse=20to=20fall=20through=20to=20the=20disput?= =?UTF-8?q?e=20cancel?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CancelOrder` from the daemon key serves two very different intents: the solver's dispute resolution (cancel + refund seller) and, since #939, the operator's cancel of a still-pending order. A tool that means the latter (`mostro-cli admcancelpending`) had no way to say so: a mistyped id belonging to a dispute the daemon has taken would resolve that dispute and report a "pending order cancelled" success (Codex review on mostro-cli#191). Add `optional bool pretrade_only = 3` to `CancelOrderRequest`. When set the service looks the order up first and refuses anything that is not `pending` / `waiting-taker-bond` with `success = false` and the order's status in `error_message`, before any handler runs. Unset keeps the existing behaviour, so older clients are unaffected. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PWN1jHfoZxfjusDVB9n3GW --- docs/RPC.md | 6 +++ examples/rpc_client.rs | 1 + proto/admin.proto | 5 ++ src/rpc/mod.rs | 2 + src/rpc/service.rs | 109 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+) diff --git a/docs/RPC.md b/docs/RPC.md index d4a086e0..e230a706 100644 --- a/docs/RPC.md +++ b/docs/RPC.md @@ -64,6 +64,12 @@ Any other status is refused with `NotAllowedByStatus`. - `order_id`: UUID of the order to cancel - `request_id`: Optional request identifier +- `pretrade_only`: Optional. When `true` the daemon refuses any order that is + not still `pending` / `waiting-taker-bond` (`success = false` with the + order's status in `error_message`) instead of falling through to the + dispute-resolution cancel. Operator tooling that means "cancel this pending + order" — `mostro-cli admcancelpending` — sets it, so a mistyped id can never + close a dispute. **Response:** diff --git a/examples/rpc_client.rs b/examples/rpc_client.rs index dcc64483..74e07950 100644 --- a/examples/rpc_client.rs +++ b/examples/rpc_client.rs @@ -49,6 +49,7 @@ async fn main() -> Result<(), Box> { let cancel_request = tonic::Request::new(CancelOrderRequest { order_id: "550e8400-e29b-41d4-a716-446655440000".to_string(), request_id: Some("12345".to_string()), + pretrade_only: None, }); match client.cancel_order(cancel_request).await { diff --git a/proto/admin.proto b/proto/admin.proto index 0b74e3c3..22d5bde7 100644 --- a/proto/admin.proto +++ b/proto/admin.proto @@ -36,6 +36,11 @@ service AdminService { message CancelOrderRequest { string order_id = 1; optional string request_id = 2; + // When true the daemon refuses any order that is not still pre-trade + // (pending / waiting-taker-bond) instead of falling through to the + // dispute-resolution cancel. Operator tooling that means "cancel this + // pending order" must set it so a mistyped id can never close a dispute. + optional bool pretrade_only = 3; } // Response for order cancellation diff --git a/src/rpc/mod.rs b/src/rpc/mod.rs index 60f80d72..fadb1f41 100644 --- a/src/rpc/mod.rs +++ b/src/rpc/mod.rs @@ -25,6 +25,7 @@ mod tests { let cancel_request = CancelOrderRequest { order_id: "test-order".to_string(), request_id: Some("test-request".to_string()), + pretrade_only: None, }; assert_eq!(cancel_request.order_id, "test-order"); @@ -45,6 +46,7 @@ mod tests { let _cancel_req = CancelOrderRequest { order_id: "order1".to_string(), request_id: None, + pretrade_only: None, }; let _settle_req = SettleOrderRequest { diff --git a/src/rpc/service.rs b/src/rpc/service.rs index da71d14a..779b1dea 100644 --- a/src/rpc/service.rs +++ b/src/rpc/service.rs @@ -181,6 +181,30 @@ impl AdminServiceImpl { Ok(()) } + /// `CancelOrderRequest.pretrade_only`: refuse anything that is not + /// still `pending` / `waiting-taker-bond`, so operator tooling that + /// means "cancel this pending order" can never resolve a dispute by a + /// mistyped id. Returns the operator-facing reason on refusal. + async fn ensure_pretrade(&self, order_id: &str) -> Result<(), String> { + use mostro_core::db::Crud; + use mostro_core::order::{Order, Status as OrderStatus}; + let id = uuid::Uuid::parse_str(order_id).map_err(|e| format!("invalid order id: {e}"))?; + let order = Order::by_id(self.pool.as_ref(), id) + .await + .map_err(|e| format!("order lookup failed: {e}"))? + .ok_or_else(|| format!("order {order_id} not found"))?; + let pretrade = order.status == OrderStatus::Pending.to_string() + || order.status == OrderStatus::WaitingTakerBond.to_string(); + if pretrade { + Ok(()) + } else { + Err(format!( + "order {order_id} is {} and pretrade_only was requested; use the dispute flow (AdminCancel / AdminSettle) instead", + order.status + )) + } + } + async fn call_admin_settle( &self, order_id: String, @@ -354,6 +378,16 @@ impl AdminService for AdminServiceImpl { let req = request.into_inner(); info!("Received cancel order request for order: {}", req.order_id); + if req.pretrade_only.unwrap_or(false) { + if let Err(msg) = self.ensure_pretrade(&req.order_id).await { + warn!("CancelOrder refused: {msg}"); + return Ok(Response::new(CancelOrderResponse { + success: false, + error_message: Some(msg), + })); + } + } + match self.call_admin_cancel(req.order_id, req.request_id).await { Ok(()) => Ok(Response::new(CancelOrderResponse { success: true, @@ -598,6 +632,7 @@ mod tests { let cancel_req = CancelOrderRequest { order_id: "test-order-id".to_string(), request_id: Some("test-request-id".to_string()), + pretrade_only: None, }; let cancel_resp = CancelOrderResponse { @@ -902,6 +937,7 @@ mod tests { .cancel_order(Request::new(CancelOrderRequest { order_id: "x".into(), request_id: None, + pretrade_only: None, })) .await .unwrap_err() @@ -1025,6 +1061,7 @@ mod tests { .cancel_order(Request::new(CancelOrderRequest { order_id: "not-a-uuid".to_string(), request_id: Some("7".to_string()), + pretrade_only: None, })) .await .expect("RPC surface always answers with a response"); @@ -1040,6 +1077,7 @@ mod tests { .cancel_order(Request::new(CancelOrderRequest { order_id: uuid::Uuid::new_v4().to_string(), request_id: None, + pretrade_only: None, })) .await .expect("RPC surface always answers with a response"); @@ -1182,14 +1220,85 @@ mod tests { let req_with_request_id = CancelOrderRequest { order_id: "order1".to_string(), request_id: Some("req1".to_string()), + pretrade_only: None, }; let req_without_request_id = CancelOrderRequest { order_id: "order2".to_string(), request_id: None, + pretrade_only: None, }; assert!(req_with_request_id.request_id.is_some()); assert!(req_without_request_id.request_id.is_none()); } + + /// `pretrade_only` must never fall through to the dispute cancel: a + /// disputed order is refused with an explanatory message and nothing + /// is touched. + #[tokio::test] + async fn cancel_order_pretrade_only_refuses_a_dispute() { + let service = offline_service().await; + let id = insert_escrowed_order(service.pool.as_ref(), "dispute").await; + let response = service + .cancel_order(Request::new(CancelOrderRequest { + order_id: id.to_string(), + request_id: None, + pretrade_only: Some(true), + })) + .await + .unwrap() + .into_inner(); + assert!(!response.success); + let msg = response.error_message.unwrap_or_default(); + assert!( + msg.contains("is dispute") && msg.contains("pretrade_only"), + "{msg}" + ); + let status: String = sqlx::query_scalar("SELECT status FROM orders WHERE id = ?") + .bind(id) + .fetch_one(service.pool.as_ref()) + .await + .unwrap(); + assert_eq!(status, "dispute"); + } + + /// A pending order passes the guard and reaches the cancel handler + /// (which then fails on the uninitialised Nostr client in this offline + /// test — the point is that the refusal reason is not the guard's). + #[tokio::test] + async fn cancel_order_pretrade_only_lets_a_pending_order_through() { + let service = offline_service().await; + let id = insert_escrowed_order(service.pool.as_ref(), "pending").await; + let response = service + .cancel_order(Request::new(CancelOrderRequest { + order_id: id.to_string(), + request_id: None, + pretrade_only: Some(true), + })) + .await + .unwrap() + .into_inner(); + let msg = response.error_message.unwrap_or_default(); + assert!(!msg.contains("pretrade_only was requested"), "{msg}"); + } + + #[tokio::test] + async fn cancel_order_pretrade_only_reports_unknown_order() { + let service = offline_service().await; + let response = service + .cancel_order(Request::new(CancelOrderRequest { + order_id: uuid::Uuid::new_v4().to_string(), + request_id: None, + pretrade_only: Some(true), + })) + .await + .unwrap() + .into_inner(); + assert!(!response.success); + assert!(response + .error_message + .unwrap_or_default() + .contains("not found")); + } } From 7d3fbd2599030f2cb028de0c883e9e22c16e8077 Mon Sep 17 00:00:00 2001 From: grunch Date: Wed, 2 Sep 2026 10:14:24 -0300 Subject: [PATCH 2/2] fix: pretrade_only refusal hints the dispute flow only for disputes Review follow-ups on the pretrade_only guard: - The "use the dispute flow (AdminCancel / AdminSettle)" hint was appended to every refusal, but it is only actionable when the order is actually in dispute; for active / fiat-sent / success / ... that flow is refused too, so the operator was sent to a second error. Append it only when the status is dispute. - Compare statuses with `Order::check_status` like admin_cancel.rs does instead of matching against `to_string()`. - docs/RPC.md: the client example was missing the new field and no longer compiled with prost; note that an older daemon silently drops the flag. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011V3amicePVbKtJ45jdVVoN --- docs/RPC.md | 6 +++-- src/rpc/service.rs | 55 ++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 11 deletions(-) diff --git a/docs/RPC.md b/docs/RPC.md index e230a706..12ee62e8 100644 --- a/docs/RPC.md +++ b/docs/RPC.md @@ -68,8 +68,9 @@ Any other status is refused with `NotAllowedByStatus`. not still `pending` / `waiting-taker-bond` (`success = false` with the order's status in `error_message`) instead of falling through to the dispute-resolution cancel. Operator tooling that means "cancel this pending - order" — `mostro-cli admcancelpending` — sets it, so a mistyped id can never - close a dispute. + order" — `mostro-cli admcancelpending` — sets it, so a mistyped id is + refused instead of closing a dispute. Unknown fields are dropped on the + wire (proto3), so a daemon older than this field silently ignores it. **Response:** @@ -241,6 +242,7 @@ async fn main() -> Result<(), Box> { let request = tonic::Request::new(CancelOrderRequest { order_id: "550e8400-e29b-41d4-a716-446655440000".to_string(), request_id: Some("12345".to_string()), + pretrade_only: None, }); let response = client.cancel_order(request).await?; diff --git a/src/rpc/service.rs b/src/rpc/service.rs index 779b1dea..cd66dd02 100644 --- a/src/rpc/service.rs +++ b/src/rpc/service.rs @@ -193,16 +193,22 @@ impl AdminServiceImpl { .await .map_err(|e| format!("order lookup failed: {e}"))? .ok_or_else(|| format!("order {order_id} not found"))?; - let pretrade = order.status == OrderStatus::Pending.to_string() - || order.status == OrderStatus::WaitingTakerBond.to_string(); - if pretrade { - Ok(()) - } else { - Err(format!( - "order {order_id} is {} and pretrade_only was requested; use the dispute flow (AdminCancel / AdminSettle) instead", - order.status - )) + if order.check_status(OrderStatus::Pending).is_ok() + || order.check_status(OrderStatus::WaitingTakerBond).is_ok() + { + return Ok(()); } + // The dispute-flow hint is only right when the order really is in + // dispute; for any other status that flow would be refused too. + let hint = if order.check_status(OrderStatus::Dispute).is_ok() { + "; use the dispute flow (AdminCancel / AdminSettle) instead" + } else { + "" + }; + Err(format!( + "order {order_id} is {} and pretrade_only was requested{hint}", + order.status + )) } async fn call_admin_settle( @@ -1263,6 +1269,37 @@ mod tests { assert_eq!(status, "dispute"); } + /// The "use the dispute flow" hint is only right when the order really + /// is in dispute; for any other non-pretrade status that flow would be + /// refused too, so the message must not send the operator there. + #[tokio::test] + async fn cancel_order_pretrade_only_hints_dispute_flow_only_for_disputes() { + let service = offline_service().await; + let disputed = insert_escrowed_order(service.pool.as_ref(), "dispute").await; + let active = insert_escrowed_order(service.pool.as_ref(), "active").await; + let msg_for = |id: uuid::Uuid| { + let service = &service; + async move { + service + .cancel_order(Request::new(CancelOrderRequest { + order_id: id.to_string(), + request_id: None, + pretrade_only: Some(true), + })) + .await + .unwrap() + .into_inner() + .error_message + .unwrap_or_default() + } + }; + let disputed_msg = msg_for(disputed).await; + assert!(disputed_msg.contains("dispute flow"), "{disputed_msg}"); + let active_msg = msg_for(active).await; + assert!(active_msg.contains("is active"), "{active_msg}"); + assert!(!active_msg.contains("dispute flow"), "{active_msg}"); + } + /// A pending order passes the guard and reaches the cancel handler /// (which then fails on the uninitialised Nostr client in this offline /// test — the point is that the refusal reason is not the guard's).