Skip to content

fix(trades): sync maker trade status and hold invoice from daemon - #96

Merged
grunch merged 2 commits into
mainfrom
fix/maker-trade-status-sync
Apr 9, 2026
Merged

fix(trades): sync maker trade status and hold invoice from daemon#96
grunch merged 2 commits into
mainfrom
fix/maker-trade-status-sync

Conversation

@grunch

@grunch grunch commented Apr 8, 2026

Copy link
Copy Markdown
Member

The maker's TradeInfo in the DB was never updated after creation:

  • The local UUID was not replaced with the daemon-assigned UUID, so tradeStatusProvider polled with a stale ID and never found the order in the order book cache — status stayed "Pending" forever.

  • Action::PayInvoice gift wraps were unhandled, so the hold invoice was never extracted or saved — the seller never saw the payment screen when a buyer took their order.

  • Kind 38383 status changes were reflected in the order book cache but never persisted to the trades DB, causing stale status on app restart.

Adds update_trade_order_id and update_trade_fields to the Storage trait and wires them into the order subscription loop, single-order subscription, and gift-wrap rumor dispatch for PayInvoice and other status actions.

Summary by CodeRabbit

  • New Features

    • Improved handling of payment requests to capture invoice and amount details and reflect them in order records.
    • Broader automatic synchronization of trade statuses across queued actions and subscriptions, keeping local order views up to date.
  • Bug Fixes

    • Prevents stale order identifiers by updating mappings when daemon IDs replace local IDs.
    • Browser storage backend now explicitly no-ops unsupported updates and logs warnings instead of failing.

The maker's TradeInfo in the DB was never updated after creation:

- The local UUID was not replaced with the daemon-assigned UUID, so
  tradeStatusProvider polled with a stale ID and never found the order
  in the order book cache — status stayed "Pending" forever.

- Action::PayInvoice gift wraps were unhandled, so the hold invoice
  was never extracted or saved — the seller never saw the payment screen
  when a buyer took their order.

- Kind 38383 status changes were reflected in the order book cache but
  never persisted to the trades DB, causing stale status on app restart.

Adds update_trade_order_id and update_trade_fields to the Storage trait
and wires them into the order subscription loop, single-order subscription,
and gift-wrap rumor dispatch for PayInvoice and other status actions.
@coderabbitai

coderabbitai Bot commented Apr 8, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 49f65271-8b50-42c6-9261-6d63db160e3c

📥 Commits

Reviewing files that changed from the base of the PR and between 5ad8a46 and ec2b99e.

📒 Files selected for processing (3)
  • rust/src/api/orders.rs
  • rust/src/db/indexeddb.rs
  • rust/src/db/sqlite.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • rust/src/db/indexeddb.rs
  • rust/src/db/sqlite.rs
  • rust/src/api/orders.rs

Walkthrough

Added two new persistent storage APIs to sync trades (update_trade_order_id, update_trade_fields) and updated the orders API to call them during gift-wrap Action handling and Kind 38383 subscription processing to keep DB trade status and order IDs in sync.

Changes

Cohort / File(s) Summary
Storage Trait
rust/src/db/mod.rs
Added async methods update_trade_order_id(old_order_id, new_order_id) and update_trade_fields(order_id, status, hold_invoice, amount_sats) to Storage trait.
SQLite Storage
rust/src/db/sqlite.rs
Implemented update_trade_order_id (atomic JSON update of $.order.id) and update_trade_fields (single dynamic UPDATE applying JSON mutations and optional denormalized status column).
IndexedDB Storage (stub)
rust/src/db/indexeddb.rs
Added stub update_trade_order_id and update_trade_fields that log a warning and return Ok(()) (no persistence).
Orders API
rust/src/api/orders.rs
Extended gift-wrap Action::PayInvoice handling and broader Action→OrderStatus mapping; best-effort calls to db.update_trade_fields when status/hold invoice/amount available; in Kind 38383 single/global subscriptions, best-effort syncs of trade status and a call to db.update_trade_order_id when replacing pending local UUIDs.

Sequence Diagram(s)

sequenceDiagram
  participant Daemon
  participant OrdersAPI
  participant DB
  participant OrderBook

  Daemon->>OrdersAPI: gift-wrap Action (e.g., PayInvoice / other Actions)
  OrdersAPI->>OrdersAPI: map Action -> Option<OrderStatus>, extract bolt11/amount
  OrdersAPI->>DB: update_trade_fields(order_id, status?, hold_invoice?, amount?)
  DB-->>OrdersAPI: Ok/Err (best-effort)
  OrdersAPI->>OrderBook: upsert/update in-memory order
  Note right of OrdersAPI: For Kind 38383 global subscription\nalso: replace local UUID -> daemon UUID
  OrdersAPI->>DB: update_trade_order_id(local_id, daemon_id)
  DB-->>OrdersAPI: Ok/Err (best-effort)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐇 I hopped through code with twitchy paws,

swapping IDs and fixing claws;
Gifts and invoices now align,
trades updated—how divine! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main fix: syncing maker trade status and hold invoice from the daemon, which is the core objective of the PR.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/maker-trade-status-sync

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 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 972-975: The match arm that builds (bolt11, amount) from
kind.payload for Payload::PaymentRequest currently uses a direct cast (a as u64)
which will wrap negative i64 values; change this to use a checked conversion
(e.g., u64::try_from(a)) and handle the Result by mapping Ok(v) to Some(v) and
treating Err(_) as None (or logging the invalid signed amount) so negative
amounts are not converted into huge unsigned values; update the matching branch
in the code that constructs (bolt11, amount) for Payload::PaymentRequest to
perform this checked conversion and add an appropriate log/error path.

In `@rust/src/db/indexeddb.rs`:
- Around line 111-127: The two no-op methods update_trade_order_id and
update_trade_fields currently return Ok(()) and silently hide missing IndexedDB
persistence; either implement persistence using indexed_db_futures for the web
target (open the DB, start a transaction, update the relevant trade record(s)
and commit) or change both methods to return an explicit error (e.g.,
Err(Error::new("IndexedDB persistence not implemented"))) so callers won’t
assume success; locate these functions in indexeddb.rs and replace the Ok(())
stubs with the proper indexed_db_futures-based persistence logic or a clear Err
until full support is added.

In `@rust/src/db/sqlite.rs`:
- Around line 355-389: The current update_trade_order_id and update_trade_fields
implementations perform a read-modify-write by calling get_trade_by_order_id and
then save_trade, which can cause lost updates under concurrency; change them to
perform atomic updates directly in SQL (use a single UPDATE with json_set to
modify the JSON blob) or run the read/modify/write inside a DB transaction with
row locking (e.g., SELECT ... FOR UPDATE) so concurrent tasks cannot clobber
each other; update the functions update_trade_order_id and update_trade_fields
to execute the atomic UPDATE statement(s) (targeting the JSON path(s) for
order.id, order.status, hold_invoice, order.amount_sats) instead of calling
get_trade_by_order_id/save_trade, or wrap that sequence in a transaction lock to
guarantee serialization.
🪄 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: fa76e07e-6d47-4179-b1af-471a497127d1

📥 Commits

Reviewing files that changed from the base of the PR and between a3e68be and 5ad8a46.

📒 Files selected for processing (4)
  • rust/src/api/orders.rs
  • rust/src/db/indexeddb.rs
  • rust/src/db/mod.rs
  • rust/src/db/sqlite.rs

Comment thread rust/src/api/orders.rs
Comment thread rust/src/db/indexeddb.rs
Comment thread rust/src/db/sqlite.rs
- PayInvoice amount conversion uses u64::try_from instead of bare cast
  to avoid wrapping negative values into huge unsigned amounts.

- SQLite update_trade_order_id and update_trade_fields now use single
  UPDATE statements with json_set instead of read-modify-write to avoid
  lost updates under concurrency.

- IndexedDB stubs log a warning so missing web persistence is visible
  during development, matching the existing save_mostro_node pattern.
@grunch
grunch merged commit 9064e98 into main Apr 9, 2026
1 check passed
@grunch
grunch deleted the fix/maker-trade-status-sync branch April 9, 2026 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant