-
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 1 commit
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", | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -221,7 +221,12 @@ TradeUpdate { | |||||||||||||||||||||||||||
| ``` | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ### on_order_status_changed(order_id: String) → Stream<OrderStatus> | ||||||||||||||||||||||||||||
| Emits when a specific order's status changes. | ||||||||||||||||||||||||||||
| **Superseded by `on_trade_updated()`.** A separate per-order status | ||||||||||||||||||||||||||||
| stream is not implemented: `on_trade_updated()` already emits a | ||||||||||||||||||||||||||||
| `TradeUpdate { order_id, status }` on every daemon-driven status | ||||||||||||||||||||||||||||
| transition, and clients filter by `order_id`. `tradeStatusProvider` | ||||||||||||||||||||||||||||
| consumes that push channel directly (with a low-frequency reconnection | ||||||||||||||||||||||||||||
| fallback), so a dedicated single-order status stream would duplicate it. | ||||||||||||||||||||||||||||
|
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. 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win Document the optimistic cancellation update. Line 226 limits Document that clients can receive an optimistic Proposed contract update-`TradeUpdate { order_id, status }` on every daemon-driven status
-transition, and clients filter by `order_id`.
+`TradeUpdate { order_id, status }` on daemon-driven status transitions and
+on client-initiated optimistic cancellation. Clients filter by `order_id`;
+an optimistic `Canceled` update can arrive before daemon confirmation.As per coding guidelines, “Update the matching specification or contract whenever behavior or an API contract changes.” 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| ### on_trade_step_changed() → Stream<TradeInfo> | ||||||||||||||||||||||||||||
| Emits when the active trade's step changes. Used to update the | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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