Fix/gift wrap status sync - #98
Merged
Merged
Conversation
- Add global Kind 1059 subscription for all known trade keys so gift-wrap events are captured even after app restart - Replace local UUID with daemon UUID on NewOrder gift-wrap so subsequent actions (WaitingBuyerInvoice, etc.) update the correct order book entry - Update in-memory order book alongside DB on every gift-wrap status change so tradeStatusProvider reflects real status without waiting for Kind 38383 - Fix FiatSentOk mapping: was Active, now correctly maps to FiatSent - Handle Rate/RateReceived/PaymentFailed actions explicitly - Add bridge_log() to bypass log crate when another logger is installed, forward Rust logs to Flutter debugPrint via _forwardRustLogs() - Add detailed gift-wrap decryption logging ([gift-wrap] tag) - Deduplicate gift-wrap processing across per-trade and global subscriptions - Handle CantDo action: remove rejected order from order book, notify UI via OrderEvent stream, show snackbar with rejection reason - Add WaitingInvoice/WaitingPayment as distinct TradeStatusFilter values with spec-correct colors (#7C2D12/#FED7AA) - Remove noisy [orderBook] update log
Replace fire-and-forget order creation with a synchronous wait for the daemon's response via a oneshot channel. On CantDo rejection, create_order returns an error to Dart — no phantom pending order is created locally. - Add PENDING_CONFIRMATIONS oneshot channel map keyed by trade_pubkey - create_order registers channel before subscribing/publishing, waits up to 5s for NewOrder (confirmed) or CantDo (rejected) from daemon - On Confirmed: create order book + DB entry with daemon UUID - On Rejected: return Err(message) — Dart catch block shows snackbar - On timeout: create locally with local UUID (optimistic fallback) - Remove OrderEvent/OrderEventStream/on_order_event broadcast machinery - Remove PENDING_LOCAL_BY_TRADE map (no longer needed) - Remove rootScaffoldMessengerKey and _listenOrderEvents from Dart - Persist trade_key_index after successful order creation
- Use regex capture group for AnyhowException stripping so non-Anyhow errors are not corrupted - Wrap trade_key_index persistence in try-catch so failures don't mask successful order creation - Remove duplicate doc comment and inaccurate filtering mention - Fix timeout log message: 15s → 5s to match actual value
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
WalkthroughOrder creation now awaits daemon confirmations and deduplicates gift-wraps; trade-status filters were expanded; a Rust→Flutter logging bridge was added; identity trade-key index is persisted after order creation; FFI dispatch table updated to include a new OrderBook.update_order_status wire handler. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Flutter Client
participant OrderBook as OrderBook (Rust)
participant Daemon as Daemon / Relays
participant LogBridge as Rust Log Bridge
Client->>OrderBook: create_order(order)
activate OrderBook
OrderBook->>OrderBook: register confirmation channel (PENDING_CONFIRMATIONS)
OrderBook->>Daemon: subscribe to gift-wraps (Kind1059/...subscriptions)
OrderBook->>Daemon: publish order request
OrderBook->>OrderBook: await confirmation (up to 5s)
rect rgba(100, 150, 255, 0.5)
Note over Daemon,OrderBook: Confirmation Path
Daemon->>OrderBook: gift-wrap with daemon UUID (Action::Pending)
OrderBook->>OrderBook: replace local UUID with daemon UUID
end
rect rgba(255, 150, 100, 0.5)
Note over Daemon,OrderBook: Rejection Path
Daemon->>OrderBook: gift-wrap rejection (Action::CantDo)
OrderBook->>OrderBook: signal Rejected to requester
end
rect rgba(150, 255, 100, 0.5)
Note over OrderBook: Timeout Path
OrderBook->>OrderBook: use local UUID on 5s timeout
end
OrderBook->>Client: return order result
deactivate OrderBook
sequenceDiagram
participant Rust as Rust Runtime
participant LogBridge as Logging Bridge (logging.rs)
participant Flutter as Flutter App (main.dart)
participant Console as Debug Console
Rust->>LogBridge: blog_info/blog_warn/blog_debug(tag, msg)
activate LogBridge
LogBridge->>LogBridge: forward_log to Flutter stream
LogBridge->>Console: print to stderr
deactivate LogBridge
Flutter->>LogBridge: subscribe onLogEntry() stream
activate Flutter
LogBridge->>Flutter: yield log entry
Flutter->>Console: debugPrint([rust/<tag>] msg)
deactivate Flutter
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/api/orders.rs (1)
481-511:⚠️ Potential issue | 🟠 MajorRollback the pending state when publish fails.
The early
publish_event_json(&event_json).await?can return after you've already populatedTRADE_KEY_MAP,PENDING_MAKER_KEYS,PENDING_LOCAL_IDS, andPENDING_CONFIRMATIONS. A retry with the same fingerprint can then inherit stale ownership/ID state.Suggested fix
- publish_event_json(&event_json).await?; + if let Err(e) = publish_event_json(&event_json).await { + if let Ok(mut map) = pending_confirmations().lock() { + map.remove(&trade_pk_hex); + } + if let Ok(mut map) = pending_maker_keys().write() { + map.remove(&trade_pk_hex); + } + let _ = take_pending_local_id(&ck); + if let Ok(mut map) = trade_key_map().write() { + map.remove(&order.id); + map.remove(&ck); + } + return Err(e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/orders.rs` around lines 481 - 511, You must roll back the in-memory pending state if publishing fails: after creating event_json, inserting into TRADE_KEY_MAP via store_trade_key_index(&order.id, ..) and store_trade_key_index(&ck,..), calling store_pending_maker_key(&trade_pk_hex,..), store_pending_local_id(&ck,..), inserting conf_tx into pending_confirmations() and subscribing with subscribe_gift_wraps(...), wrap publish_event_json(&event_json).await in a match/Result handling block and on Err perform cleanup by removing the two trade-key indexes, removing the pending maker key and pending local id, removing the entry from pending_confirmations() (map.remove(&trade_pk_hex)), and undoing the subscription (unsubscribe_gift_wraps or equivalent) before propagating the error; use the existing symbols (store_trade_key_index, store_pending_maker_key, store_pending_local_id, pending_confirmations, subscribe_gift_wraps, publish_event_json, trade_pk_hex, conf_tx) to locate and implement the rollback.
🧹 Nitpick comments (1)
lib/features/order/screens/add_order_screen.dart (1)
133-138: Don’t swallow persistence failures silently.Line 138 currently hides why
tradeKeyIndexpersistence fails, which makes field diagnosis harder.Suggested tweak
try { final identity = await identity_api.getIdentity(); if (identity != null) { await IdentityService.saveTradeKeyIndex(identity.tradeKeyIndex); } - } catch (_) {} + } catch (e, st) { + debugPrint('[add-order] saveTradeKeyIndex failed: $e\n$st'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/order/screens/add_order_screen.dart` around lines 133 - 138, The try/catch around identity_api.getIdentity() and IdentityService.saveTradeKeyIndex currently swallows errors (catch (_) {}); update it to catch the exception and log the error (and stack) or rethrow so persistence failures are visible. Specifically, modify the catch to catch (e, s) and call your app logger or debugPrint with a clear message referencing identity_api.getIdentity and IdentityService.saveTradeKeyIndex (e.g., "Failed saving tradeKeyIndex: $e"), or rethrow after logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust/src/api/orders.rs`:
- Around line 1065-1070: The NewOrder branch currently only logs when an ack
arrives after create_order timed out, leaving orders persisted under the local
UUID and causing future updates to use an unknown ID; in the NewOrder handling
code update the mapping in PENDING_LOCAL_IDS (lookup by content key) to replace
the local UUID with the daemon UUID when the daemon confirms the order, and also
emit or call the same reconciliation path used by create_order success (so
status/cancel flows use the daemon ID). Specifically, modify the NewOrder branch
that logs the late ack to: 1) retrieve the local ID from PENDING_LOCAL_IDS using
the content key, 2) swap the stored key/value to the confirmed daemon_id (or
remove the stale local entry and insert the daemon mapping), and 3) trigger the
existing order-confirmation handler used by create_order so persisted orders and
subsequent handlers reference the daemon UUID.
- Around line 992-1042: The logs currently leak sensitive decrypted data and
full keys: remove/redact any verbatim logging of rumor_json and content and
avoid printing full trade_pubkey_hex or raw content on errors; specifically
delete the blog_debug("gift-wrap", ... rumor_json) call, delete the blog_info
that prints decrypted content, and remove the blog_warn that prints "raw content
was: {content}" in the serde_json::from_str(&content) Err branch. Keep the
high-level blog_info about action/kind but ensure trade_pubkey_hex is truncated
(keep &trade_pubkey_hex[..8]) and ensure payload_desc for PaymentRequest does
not include the bolt11 string (only use pr.len() or mask the invoice), and
preserve only non-sensitive fields in payload_desc/msg/kind logging.
- Around line 499-509: The current race is that subscribe_gift_wraps(...) spawns
a detached task (using tokio::spawn) and returns immediately, so
client.notifications() / client.subscribe() happen inside that spawned task and
may not be active before the subsequent publish — causing fast Kind 1059 events
to be missed; fix by moving the subscription work out of the detached task so
the caller can await it before publish: either (A) refactor subscribe_gift_wraps
to perform client.notifications() and client.subscribe() synchronously (i.e.,
remove internal tokio::spawn) and return a future the caller can await, or (B)
add a new subscribe_gift_wraps_sync (or change the existing function signature)
that performs and awaits client.notifications() and client.subscribe() before
returning, and have the caller call/await that before publishing, leaving
spawning to the caller if needed; reference subscribe_gift_wraps,
client.notifications(), client.subscribe(), and the tokio::spawn usage for
locating the changes.
---
Outside diff comments:
In `@rust/src/api/orders.rs`:
- Around line 481-511: You must roll back the in-memory pending state if
publishing fails: after creating event_json, inserting into TRADE_KEY_MAP via
store_trade_key_index(&order.id, ..) and store_trade_key_index(&ck,..), calling
store_pending_maker_key(&trade_pk_hex,..), store_pending_local_id(&ck,..),
inserting conf_tx into pending_confirmations() and subscribing with
subscribe_gift_wraps(...), wrap publish_event_json(&event_json).await in a
match/Result handling block and on Err perform cleanup by removing the two
trade-key indexes, removing the pending maker key and pending local id, removing
the entry from pending_confirmations() (map.remove(&trade_pk_hex)), and undoing
the subscription (unsubscribe_gift_wraps or equivalent) before propagating the
error; use the existing symbols (store_trade_key_index, store_pending_maker_key,
store_pending_local_id, pending_confirmations, subscribe_gift_wraps,
publish_event_json, trade_pk_hex, conf_tx) to locate and implement the rollback.
---
Nitpick comments:
In `@lib/features/order/screens/add_order_screen.dart`:
- Around line 133-138: The try/catch around identity_api.getIdentity() and
IdentityService.saveTradeKeyIndex currently swallows errors (catch (_) {});
update it to catch the exception and log the error (and stack) or rethrow so
persistence failures are visible. Specifically, modify the catch to catch (e, s)
and call your app logger or debugPrint with a clear message referencing
identity_api.getIdentity and IdentityService.saveTradeKeyIndex (e.g., "Failed
saving tradeKeyIndex: $e"), or rethrow after logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 158b95f7-aad8-4cdd-81e0-40ce68e483a3
📒 Files selected for processing (8)
lib/features/home/providers/home_order_providers.dartlib/features/order/screens/add_order_screen.dartlib/features/trades/providers/trades_providers.dartlib/features/trades/widgets/trades_list_item.dartlib/main.dartrust/src/api/logging.rsrust/src/api/orders.rsrust/src/frb_generated.rs
💤 Files with no reviewable changes (1)
- lib/features/home/providers/home_order_providers.dart
…ubscription race, publish rollback - NewOrder handler reconciles local UUID with daemon UUID when no caller is waiting (timeout/cold-start scenario) using PENDING_LOCAL_IDS lookup - Remove raw rumor JSON, decrypted content, and raw error content from gift-wrap logs — keep only action/kind with truncated trade pubkey - Await relay subscription setup synchronously in subscribe_gift_wraps before returning, then spawn event loop — prevents race where fast daemon response arrives before subscription is active - Rollback TRADE_KEY_MAP, PENDING_MAKER_KEYS, PENDING_LOCAL_IDS, and PENDING_CONFIRMATIONS entries if publish_event_json fails - Log identity persistence failures instead of silently swallowing
This was referenced Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores