feat(orders): implement take-sell order flow with LN address support - #75
Conversation
- Wire take_order to Mostro via NIP-59 gift wrap using trade-derived keys - Store per-order trade key index for consistent signing across actions - Subscribe to d-tag K38383 updates after taking to track status changes - Add send_invoice with LN address detection: includes sats amount in PaymentRequest payload when input contains '@' so Mostro can resolve it - Add cancel_order action wired to all cancel buttons in TradeDetailScreen - Remove client-side WrongTradeState guards from send_fiat_sent and release_order — Mostro daemon is the authoritative state validator - Add trade_state_provider: tradeAmountProvider and tradeStatusProvider for reactive status polling in the UI - Update AddLightningInvoiceScreen: submit enabled on non-empty text, NWC auto-invoice flow with manual fallback - Register cancel_order in FRB bridge (funcId 82)
|
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 (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds per-order trade-index signing and subscriptions in Rust, typed Mostro action messages, new cancel_order FFI, Dart providers for trade role/amount/status, extends OrderItem with status/amount, and updates UI flows (take-order, add-invoice, trade detail) to use these providers and APIs. Changes
Sequence DiagramsequenceDiagram
participant UI as TakeOrderScreen
participant DartAPI as orders_api (Dart FRB)
participant RustAPI as rust::api::orders
participant Actions as rust::mostro::actions
participant Relay as Nostr Relay
UI->>DartAPI: takeOrder(orderId, role, fiatAmount)
DartAPI->>RustAPI: take_order(orderId, role, fiatAmount)
RustAPI->>RustAPI: derive_trade_key() → trade_index
RustAPI->>RustAPI: store trade_index in TRADE_KEY_MAP
RustAPI->>Actions: take_buy/take_sell(orderId, trade_index, ...)
Actions->>Actions: build typed Payload/Message/Action
Actions-->>RustAPI: serialized Mostro message
RustAPI->>Relay: publish take action
RustAPI->>RustAPI: subscribe_single_order(orderId)
rect rgba(200,150,255,0.5)
Note over UI, DartAPI: Provider polling (every 2s)
UI->>DartAPI: getOrder(orderId) (tradeStatusProvider / tradeAmountProvider)
DartAPI->>Relay: fetch Kind 38383 events
Relay-->>DartAPI: latest order event
DartAPI-->>UI: OrderStatus + amountSats
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
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)
331-345:⚠️ Potential issue | 🔴 CriticalDon't return success when the take never reached Mostro.
This path reserves/stores
trade_index, then only logs failures from key lookup, action building, or publish, but still returnsOk(trade). The UI can enter a phantom trade even though notake_*event was sent, and all follow-up actions will reuse a signer Mostro never saw. Propagate the error and store the mapping only afterpublish_event_json()succeeds.Also applies to: 352-408
🤖 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 331 - 345, The code currently derives a trade key via derive_trade_key().await and immediately calls store_trade_key_index(&order_id, trade_index) before attempting to build/publish the take event, then returns Ok(trade) even if publish_event_json() failed; change this so you do not persist the mapping until the take event is successfully sent: remove or delay the call to store_trade_key_index(&order_id, trade_index) until after publish_event_json(...) returns success, ensure any errors from key lookup, action building, or publish_event_json are propagated (return Err) instead of logging and returning Ok(trade), and apply the same change for the similar block covering lines 352-408 so the trade_index is stored only after publish_event_json succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/order/providers/trade_state_provider.dart`:
- Around line 9-10: tradeRoleProvider currently holds a volatile Map in Riverpod
(StateProvider<Map<String,bool>>) which is cleared on app restart; persist the
role instead of keeping it only in memory by storing the buyer/seller flag on
the trade record or deriving it from backend/state source and loading it into
the UI layer via Sembast on startup; update usage to read from the trade entity
(or an initialized Sembast-backed store) rather than relying on
tradeRoleProvider alone, and migrate any callers of tradeRoleProvider to fetch
the persisted field (or call the backend) during trade load/rehydration so
reopened trades recover the correct role.
In `@lib/features/order/screens/add_lightning_invoice_screen.dart`:
- Around line 52-67: The submit logic currently falls back to BigInt.one when
_resolvedSats(ref) is null, which causes Lightning Address submissions (invoice
strings containing "@") to be sent with a 1-sat amount; change _submit (and
optionally _isValid) to detect Lightning Addresses by checking
_invoiceController.text.trim().contains("@") and require a non-null
_resolvedSats(ref) for that path—if sats is null for an "@" input, abort
submission and surface a user error (or validation failure) instead of using
BigInt.one; retain the BigInt.one fallback only for bolt11 invoices (non-"@"
inputs) before calling orders_api.sendInvoice.
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 90-109: The default branch in _mapOrderStatus currently returns
TradeStatus.active which causes OrderStatus.pending (the provider's
initial/fallback) to be treated as active; change the default to return a
non-actionable placeholder (e.g., TradeStatus.loading or TradeStatus.unknown) so
unresolved statuses remain read-only, and apply the same change to the other
mapping site referenced (the other mapping around the trade detail code that
currently returns active — the one noted at lines 199-201) so
tradeStatusProvider's initial OrderStatus.pending does not expose active-trade
actions.
In `@rust/src/api/orders.rs`:
- Around line 536-546: The current subscribe_single_order loop sets a fixed
deadline once (`let deadline = tokio::time::Instant::now() +
Duration::from_secs(30 * 60)`) so it times out after 30 minutes total even if
activity continues; update the logic in subscribe_single_order to either (a)
refresh the inactivity timer after each successful receive from `rx.recv()`
(e.g., reset `deadline` or compute `remaining` from a last_activity Instant) or
(b) keep the subscription alive until a terminal order status is observed (check
the received order status and break only on terminal states), ensuring
`timeout(remaining, rx.recv()).await` uses the refreshed inactivity window and
that logs referencing `order_id` remain correct.
- Around line 393-397: The log statement in take_order (log::info! that includes
ln_address_ref) exposes a user Lightning Address; replace detailed logging of
ln_address_ref with a privacy-preserving message that only indicates presence or
redacted metadata (e.g., "ln_address=REDACTED" or "ln_address=present/none") and
keep the rest of the fields (order_id, trade_index) unchanged; update the log
invocation in orders.rs where log::info! is called to avoid printing
ln_address_ref directly.
- Around line 18-40: The in-memory TRADE_KEY_MAP and trade_key_map() must be
replaced with a persistent store: update store_trade_key_index(order_id, index)
to persist the mapping by order_id (use sqlx + SQLite on native and
indexed_db_futures on web per guidelines) and make get_trade_key_index(order_id)
return a Result<u32, Error> (or Option but propagate error) that queries the
persistent store instead of returning 0 on miss; also remove/avoid relying on
the static TRADE_KEY_MAP (or keep as optional cache) and ensure callers handle
the error result rather than assuming a fallback index.
---
Outside diff comments:
In `@rust/src/api/orders.rs`:
- Around line 331-345: The code currently derives a trade key via
derive_trade_key().await and immediately calls store_trade_key_index(&order_id,
trade_index) before attempting to build/publish the take event, then returns
Ok(trade) even if publish_event_json() failed; change this so you do not persist
the mapping until the take event is successfully sent: remove or delay the call
to store_trade_key_index(&order_id, trade_index) until after
publish_event_json(...) returns success, ensure any errors from key lookup,
action building, or publish_event_json are propagated (return Err) instead of
logging and returning Ok(trade), and apply the same change for the similar block
covering lines 352-408 so the trade_index is stored only after
publish_event_json succeeds.
🪄 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: d0a84e46-22ee-43d2-af6d-457d287973ad
📒 Files selected for processing (9)
lib/features/home/providers/home_order_providers.dartlib/features/order/providers/trade_state_provider.dartlib/features/order/screens/add_lightning_invoice_screen.dartlib/features/order/screens/take_order_screen.dartlib/features/trades/screens/trade_detail_screen.dartrust/src/api/orders.rsrust/src/frb_generated.rsrust/src/mostro/actions.rsrust/src/nostr/order_events.rs
| final tradeRoleProvider = | ||
| StateProvider<Map<String, bool>>((ref) => const {}); |
There was a problem hiding this comment.
tradeRoleProvider is too ephemeral for a trade-critical role.
This map is cleared on every app restart, but downstream screens now use it to choose buyer vs seller actions. Reopened trades therefore cannot recover the local role reliably unless some other source repopulates it. Persist the role with the trade record, or derive it from backend state instead of keeping it only in Riverpod memory.
As per coding guidelines, "Use Sembast for UI-layer state management in Dart across all platforms".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/order/providers/trade_state_provider.dart` around lines 9 - 10,
tradeRoleProvider currently holds a volatile Map in Riverpod
(StateProvider<Map<String,bool>>) which is cleared on app restart; persist the
role instead of keeping it only in memory by storing the buyer/seller flag on
the trade record or deriving it from backend/state source and loading it into
the UI layer via Sembast on startup; update usage to read from the trade entity
(or an initialized Sembast-backed store) rather than relying on
tradeRoleProvider alone, and migrate any callers of tradeRoleProvider to fetch
the persisted field (or call the backend) during trade load/rehydration so
reopened trades recover the correct role.
| static TradeStatus _mapOrderStatus(OrderStatus s) { | ||
| switch (s) { | ||
| case OrderStatus.active: | ||
| return TradeStatus.active; | ||
| case OrderStatus.fiatSent: | ||
| return TradeStatus.fiatSent; | ||
| case OrderStatus.settledHoldInvoice: | ||
| case OrderStatus.success: | ||
| case OrderStatus.completedByAdmin: | ||
| case OrderStatus.settledByAdmin: | ||
| return TradeStatus.pendingRating; | ||
| case OrderStatus.canceled: | ||
| case OrderStatus.canceledByAdmin: | ||
| case OrderStatus.expired: | ||
| return TradeStatus.cancelled; | ||
| case OrderStatus.dispute: | ||
| return TradeStatus.disputed; | ||
| default: | ||
| return TradeStatus.active; | ||
| } |
There was a problem hiding this comment.
Don't map the status placeholder to active.
tradeStatusProvider uses OrderStatus.pending as its initial/fallback value, and the default branch here turns that into TradeStatus.active. That exposes active-trade actions before the first real status arrives, and it can stay wrong if the provider never resolves. Keep unresolved status in a loading/read-only state instead.
Also applies to: 199-201
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 90 - 109,
The default branch in _mapOrderStatus currently returns TradeStatus.active which
causes OrderStatus.pending (the provider's initial/fallback) to be treated as
active; change the default to return a non-actionable placeholder (e.g.,
TradeStatus.loading or TradeStatus.unknown) so unresolved statuses remain
read-only, and apply the same change to the other mapping site referenced (the
other mapping around the trade detail code that currently returns active — the
one noted at lines 199-201) so tradeStatusProvider's initial OrderStatus.pending
does not expose active-trade actions.
| /// Maps `order_id` → `trade_key_index` for trades initiated in this session. | ||
| /// Allows subsequent actions (add-invoice, fiat-sent, release) to sign with the | ||
| /// same trade key that was used when taking the order. | ||
| use std::sync::OnceLock; | ||
|
|
||
| static TRADE_KEY_MAP: OnceLock<std::sync::RwLock<HashMap<String, u32>>> = OnceLock::new(); | ||
|
|
||
| fn trade_key_map() -> &'static std::sync::RwLock<HashMap<String, u32>> { | ||
| TRADE_KEY_MAP.get_or_init(|| std::sync::RwLock::new(HashMap::new())) | ||
| } | ||
|
|
||
| fn store_trade_key_index(order_id: &str, index: u32) { | ||
| if let Ok(mut map) = trade_key_map().write() { | ||
| map.insert(order_id.to_string(), index); | ||
| } | ||
| } | ||
|
|
||
| fn get_trade_key_index(order_id: &str) -> u32 { | ||
| trade_key_map() | ||
| .read() | ||
| .ok() | ||
| .and_then(|m| m.get(order_id).copied()) | ||
| .unwrap_or(0) |
There was a problem hiding this comment.
The trade-key lookup isn't actually persisted.
TRADE_KEY_MAP only survives for the current process, and get_trade_key_index() silently returns 0 when the entry is missing. After a restart, taker-side follow-up actions stop using the trade key from take_order, which defeats the signer continuity this PR is trying to preserve. Persist the mapping by order_id and treat a missing taker mapping as an error rather than falling back to 0.
As per coding guidelines, "Use sqlx with SQLite for native platform persistence" and "Use indexed_db_futures for web platform persistence in Rust".
🤖 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 18 - 40, The in-memory TRADE_KEY_MAP and
trade_key_map() must be replaced with a persistent store: update
store_trade_key_index(order_id, index) to persist the mapping by order_id (use
sqlx + SQLite on native and indexed_db_futures on web per guidelines) and make
get_trade_key_index(order_id) return a Result<u32, Error> (or Option but
propagate error) that queries the persistent store instead of returning 0 on
miss; also remove/avoid relying on the static TRADE_KEY_MAP (or keep as optional
cache) and ensure callers handle the error result rather than assuming a
fallback index.
orders.rs — Fix #7: store_trade_key_index before publish Moved store_trade_key_index(&order_id, trade_index) from before the action dispatch to inside the publish-success else branch. A publish failure no longer leaves a stale key mapping. orders.rs — Fix #5: ln_address logged in plain text Replaced ln_address={:?} (which printed the actual address) with ln_address=present/none using if ln_address_ref.is_some(). orders.rs — Fix #4: fixed 30-min deadline regardless of activity Replaced the fixed deadline with a last_activity instant that resets on each matching event. The idle timeout is now 30 minutes of inactivity, not 30 minutes of total uptime. add_lightning_invoice_screen.dart — Fix #2: Lightning Address sent with 1-sat amount _isValid now returns false for @ inputs when _resolvedSats is null. _submit also guards against it and surfaces an error instead of falling through to BigInt.one. trade_detail_screen.dart — Fix #3: OrderStatus.pending mapped to TradeStatus.active Added TradeStatus.loading to the enum and made it the default branch in _mapOrderStatus. Unresolved/initial statuses no longer match the active-trade action button sections.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/features/trades/screens/trade_detail_screen.dart (3)
114-148: Hardcoded strings bypass localization.The dialog title, content, button labels, and SnackBar messages are hardcoded English strings while
AppLocalizationsis imported and used elsewhere in this file (line 575). Consider usingAppLocalizations.of(context)for consistency with the rest of the app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 114 - 148, Replace all hardcoded English strings inside _cancelOrder's dialog and SnackBars with localized strings from AppLocalizations.of(context); specifically, use localized values for the AlertDialog title, content, action labels ('No' and 'Yes, cancel') and the two SnackBar messages ('Cancel request sent' and 'Cancel failed: ...'). Ensure you call AppLocalizations.of(context) inside _cancelOrder (before building the dialog) and interpolate the error into the localized failure message if needed, leaving the orders_api.cancelOrder call and ScaffoldMessenger usage intact.
535-562: Extract duplicate RELEASE button logic.The RELEASE button implementation is identical in both the disputed (lines 535-562) and fiat-sent (lines 619-647) sections. Extract to a private helper method or widget to reduce duplication.
♻️ Proposed refactor
Widget _buildReleaseButton(Color green) { return MostroReactiveButton( label: 'RELEASE', backgroundColor: green, icon: Icons.lock_open, onPressed: () async { final confirmed = await showReleaseConfirmationDialog(context); if (confirmed != true || !context.mounted) return; try { await orders_api.releaseOrder(orderId: widget.orderId); if (context.mounted) { context.push(AppRoute.rateUserPath(widget.orderId)); } } catch (e) { if (!context.mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar(content: Text('Release failed: $e')), ); } }, onError: (e) { if (!mounted) return; ScaffoldMessenger.of(context).showSnackBar( SnackBar(content: Text('Release failed: $e')), ); }, ); }Then replace both occurrences with:
Expanded(child: _buildReleaseButton(green)),Also applies to: 619-647
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 535 - 562, Duplicate RELEASE button logic (two identical MostroReactiveButton instances) should be extracted into a single private helper to remove duplication; create a private method (e.g., Widget _buildReleaseButton(Color green)) that captures the same behavior using widget.orderId, showReleaseConfirmationDialog(context), orders_api.releaseOrder(orderId: widget.orderId), AppRoute.rateUserPath(widget.orderId), and the same onError/snackBar handling that checks mounted/context.mounted, then replace both occurrences (the MostroReactiveButton blocks) with Expanded(child: _buildReleaseButton(green)).
329-346: Consider extracting repeated button styles.The cancel button styling (lines 329-346, 411-427, 515-530, 655-670) and dispute button styling are duplicated multiple times. Consider defining shared
ButtonStyleconstants or a helper widget to reduce repetition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/trades/screens/trade_detail_screen.dart` around lines 329 - 346, The OutlinedButton.icon instances (eg. the cancel button wired to _cancelOrder) duplicate visual configuration (foregroundColor using colors?.destructiveRed, BorderSide color, minimumSize, shape with AppRadius.button); extract these into a shared ButtonStyle constant or a small helper widget (e.g., DestructiveOutlineButton or a function buildDestructiveButton) that constructs an OutlinedButton/OutlinedButton.icon with the shared ButtonStyle and accepts onPressed, icon, and label, then replace all duplicated blocks (the cancel and dispute buttons) to use that shared style/widget so the color reference (colors?.destructiveRed), BorderSide, minimumSize, and RoundedRectangleBorder(AppRadius.button) are defined only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 142-146: The SnackBar catch blocks in trade_detail_screen.dart
(the catch { if (!mounted) return;
ScaffoldMessenger.of(context).showSnackBar(...) } patterns) are showing raw
exceptions to users; change each of these catch handlers to log the full
exception and stacktrace (using your app logger or debugPrint) and display a
generic, user-friendly SnackBar message instead (or, if you must surface
specific info, only use a sanitized e.message when the exception type exposes a
safe message). Ensure you update all occurrences (the handlers currently calling
ScaffoldMessenger.of(context).showSnackBar with 'Cancel failed: $e' and similar
at the noted locations) to perform logging + show the generic message while
preserving the mounted check and existing control flow.
---
Nitpick comments:
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 114-148: Replace all hardcoded English strings inside
_cancelOrder's dialog and SnackBars with localized strings from
AppLocalizations.of(context); specifically, use localized values for the
AlertDialog title, content, action labels ('No' and 'Yes, cancel') and the two
SnackBar messages ('Cancel request sent' and 'Cancel failed: ...'). Ensure you
call AppLocalizations.of(context) inside _cancelOrder (before building the
dialog) and interpolate the error into the localized failure message if needed,
leaving the orders_api.cancelOrder call and ScaffoldMessenger usage intact.
- Around line 535-562: Duplicate RELEASE button logic (two identical
MostroReactiveButton instances) should be extracted into a single private helper
to remove duplication; create a private method (e.g., Widget
_buildReleaseButton(Color green)) that captures the same behavior using
widget.orderId, showReleaseConfirmationDialog(context),
orders_api.releaseOrder(orderId: widget.orderId),
AppRoute.rateUserPath(widget.orderId), and the same onError/snackBar handling
that checks mounted/context.mounted, then replace both occurrences (the
MostroReactiveButton blocks) with Expanded(child: _buildReleaseButton(green)).
- Around line 329-346: The OutlinedButton.icon instances (eg. the cancel button
wired to _cancelOrder) duplicate visual configuration (foregroundColor using
colors?.destructiveRed, BorderSide color, minimumSize, shape with
AppRadius.button); extract these into a shared ButtonStyle constant or a small
helper widget (e.g., DestructiveOutlineButton or a function
buildDestructiveButton) that constructs an OutlinedButton/OutlinedButton.icon
with the shared ButtonStyle and accepts onPressed, icon, and label, then replace
all duplicated blocks (the cancel and dispute buttons) to use that shared
style/widget so the color reference (colors?.destructiveRed), BorderSide,
minimumSize, and RoundedRectangleBorder(AppRadius.button) are defined only once.
🪄 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: a080266b-79d4-4822-a765-bef6e4a35672
📒 Files selected for processing (3)
lib/features/order/screens/add_lightning_invoice_screen.dartlib/features/trades/screens/trade_detail_screen.dartrust/src/api/orders.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/features/order/screens/add_lightning_invoice_screen.dart
- rust/src/api/orders.rs
…ract shared helpers - Replace raw exception SnackBars with debugPrint + generic l10n messages in _cancelOrder, FIAT SENT onError, and both RELEASE button handlers - Localize _cancelOrder dialog title, content, button labels and SnackBars; add 8 new keys across all 5 locale ARB and generated dart files - Extract duplicate RELEASE MostroReactiveButton into _buildReleaseButton helper - Extract repeated destructive OutlinedButton style into _destructiveOutlineStyle helper
Summary by CodeRabbit
New Features
Improvements
Localization