Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/RPC.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,13 @@ 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 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:**

Expand Down Expand Up @@ -235,6 +242,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
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?;
Expand Down
1 change: 1 addition & 0 deletions examples/rpc_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
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 {
Expand Down
5 changes: 5 additions & 0 deletions proto/admin.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/rpc/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand All @@ -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 {
Expand Down
146 changes: 146 additions & 0 deletions src/rpc/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,36 @@ 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"))?;
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(
&self,
order_id: String,
Expand Down Expand Up @@ -354,6 +384,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 {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Enforce pretrade-only inside the cancel handler

When this check accepts a pending order, its result becomes stale before call_admin_cancel re-reads the row: normal Nostr actions run concurrently with the RPC server, and the call can also wait for the RPC Lightning mutex. If the order advances and enters dispute in that interval, admin_cancel_action follows its dispute branch and can cancel the escrow despite pretrade_only = true, defeating the safety guarantee and potentially refunding the seller unintentionally. Pass the restriction into the handler and apply it to the same fetched order used to choose the cancellation branch, or otherwise make the check and branch atomic.

Useful? React with 👍 / 👎.

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,
Expand Down Expand Up @@ -598,6 +638,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 {
Expand Down Expand Up @@ -902,6 +943,7 @@ mod tests {
.cancel_order(Request::new(CancelOrderRequest {
order_id: "x".into(),
request_id: None,
pretrade_only: None,
}))
.await
.unwrap_err()
Expand Down Expand Up @@ -1025,6 +1067,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");
Expand All @@ -1040,6 +1083,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");
Expand Down Expand Up @@ -1182,14 +1226,116 @@ 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");
}

/// 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).
#[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"));
}
}