-
Notifications
You must be signed in to change notification settings - Fork 1
feat(#272): push-first trade status, replacing the 2 s poll #303
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1375,6 +1375,12 @@ pub async fn cancel_order(order_id: String) -> Result<()> { | |
| log::warn!("[orders] failed to optimistically update cancel status for {order_id}: {e}"); | ||
| } | ||
| } | ||
| // Push the optimistic Canceled to the trade-status stream: the order is | ||
| // gone from the book and the daemon's gift-wrap confirmation may arrive | ||
| // much later (or never, if the app closes), so a push-first listener needs | ||
| // this signal now — the old 2 s poll saw the DB write, the push channel | ||
| // must too. | ||
| emit_trade_update(&order_id, crate::api::types::OrderStatus::Canceled); | ||
|
Comment on lines
+1378
to
+1383
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift Add a cancellation stream regression test. Verify that a successful client cancellation emits one Also verify that the update is still emitted when Run As per coding guidelines, “Place Rust tests alongside the code they cover and run 🤖 Prompt for AI AgentsSource: Coding guidelines |
||
|
|
||
| crate::api::logging::blog_info( | ||
| "orders", | ||
|
|
@@ -4783,6 +4789,44 @@ mod tests { | |
| )); | ||
| } | ||
|
|
||
| /// #272: a client-initiated cancel emits an optimistic `Canceled` TradeUpdate | ||
| /// for the cancelled order. `cancel_order` itself needs trade keys, identity | ||
| /// and the relay (not available in a unit test), so this covers the emit | ||
| /// contract it relies on: the same `emit_trade_update` call cancel_order makes | ||
| /// after its optimistic book/DB update. The emit is independent of the DB | ||
| /// write, so it still fires when that write fails — here there is no DB at | ||
| /// all, mirroring that path. | ||
| /// | ||
| /// `on_trade_updated` is a process-wide broadcast, so a concurrent test may | ||
| /// interleave its own emits; the subscriber therefore filters for this | ||
| /// test's unique order id rather than assuming the first update is ours. | ||
| #[tokio::test] | ||
| async fn client_cancel_emits_canceled_update() { | ||
| let oid = "order-cancel-272-unique"; | ||
| let mut stream = on_trade_updated().await.unwrap(); | ||
| // The exact call cancel_order performs after its optimistic update. | ||
| emit_trade_update(oid, crate::api::types::OrderStatus::Canceled); | ||
|
|
||
| // Drain until our order's update arrives (skipping any interleaved emits | ||
| // from concurrently-running tests on the shared broadcast channel). | ||
| let deadline = std::time::Duration::from_secs(2); | ||
| let found = tokio::time::timeout(deadline, async { | ||
| loop { | ||
| let update = stream.next().await.expect("channel stays open"); | ||
| if update.order_id == oid { | ||
| return update; | ||
| } | ||
| } | ||
| }) | ||
| .await | ||
| .expect("cancel must push a Canceled update for our order"); | ||
|
|
||
| assert!(matches!( | ||
| found.status, | ||
| crate::api::types::OrderStatus::Canceled | ||
| )); | ||
| } | ||
|
|
||
| /// The sweep only acts on positive daemon signals: pending republish | ||
| /// (wipe for takers, resync for makers) and outright cancellation; | ||
| /// absence from the book or ambiguous statuses leave the trade alone. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add targeted tests for the merged status flow.
This change adds a push stream, a timer, async reconciliation, duplicate filtering, and terminal completion.
Add tests for the initial status, matching and nonmatching pushes, duplicate suppression,
Canceledstream completion, and fallback recovery after a failed status lookup. Use controlled time for the 30-second reconciliation path.Run
flutter analyzeandflutter testafter adding the tests.As per coding guidelines, “Add targeted tests when expanding complex logic, asynchronous workflows, or protocol handling,” and Dart changes must run
flutter analyzeandflutter test.🤖 Prompt for AI Agents
Source: Coding guidelines