Skip to content

Feat/create order - #80

Merged
grunch merged 3 commits into
mainfrom
feat/create-order
Apr 3, 2026
Merged

Feat/create order#80
grunch merged 3 commits into
mainfrom
feat/create-order

Conversation

@grunch

@grunch grunch commented Apr 3, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • Bug Fixes
    • Orders now submit reliably to the network and only navigate to the order book after successful creation.
    • Improved validation for fixed SAT amount inputs (prevents invalid or non-positive entries).
    • Submission failures surface clear notifications.
    • Payment method inputs are sanitized and consistently formatted.
    • More robust order publish and resolution behavior to reduce failed or orphaned orders.

grunch added 2 commits April 3, 2026 04:09
if DEFAULT_MOSTRO_PUBKEY is ever malformed, instead of silently swallowing the failure
@coderabbitai

coderabbitai Bot commented Apr 3, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1c578fd7-523c-43fd-ac61-cd610dfa7e2e

📥 Commits

Reviewing files that changed from the base of the PR and between 2e7b4b9 and f2e3357.

📒 Files selected for processing (2)
  • lib/features/order/screens/add_order_screen.dart
  • rust/src/api/orders.rs

Walkthrough

Dart UI now calls the Rust bridge to create orders with improved validation and sanitized payment-method handling; Rust derives per-order trade indices, persists mappings, and exposes a resolver for maker orders while Mostro action construction accepts and uses the trade index.

Changes

Cohort / File(s) Summary
Dart Client: Order Submission
lib/features/order/screens/add_order_screen.dart
Replaced simulated submit with rust_orders.createOrder(params); enhanced validation (require positive fixedSats when not market), build NewOrderParams, sanitize/append custom payment method, show SnackBar on failure, and navigate only after success.
Rust Order API
rust/src/api/orders.rs
create_order derives a fresh trade key (uses derive_trade_key()), selects active sender keys for the derived trade_index, caches order locally, writes pending maker mapping (pubkey → trade_index), and logs/persists mappings after publish. Added resolve_maker_order(order_id, trade_pubkey_hex) to finalize maker mappings. take_order refactored control flow and added explicit Mostro pubkey error logging.
Rust Mostro Actions
rust/src/mostro/actions.rs
new_order signature now accepts trade_index: u32 and uses it (as i64) in Message::new_order instead of a hardcoded 0; payload/wrapping flow otherwise unchanged.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Dart Client
    participant RustAPI as Rust API (orders.rs)
    participant RustActions as Mostro Actions (actions.rs)
    participant Mostro as Mostro Network

    Client->>Client: Validate inputs & sanitize payment method
    Client->>Client: Construct NewOrderParams
    Client->>RustAPI: create_order(params)
    RustAPI->>RustAPI: derive_trade_key() → trade_index
    RustAPI->>RustAPI: select active sender keys for trade_index
    RustAPI->>RustActions: new_order(params, trade_index)
    RustActions->>Mostro: Publish Message::new_order(with trade_index)
    Mostro-->>RustActions: Success (order_id)
    RustActions-->>RustAPI: return order_id
    RustAPI->>RustAPI: store mappings (pending → final)
    RustAPI-->>Client: order_id (success) / error
    Client->>Client: show SnackBar on error / navigate on success
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • phase 6 #55: Modifies the same add-order flow and Rust order APIs, including trade_index usage and Dart→Rust create-order integration.

Poem

🐰 I cleaned the meadow, hopped with glee,

Orders now carry a trade key,
From Dart to Rust the bridge takes flight,
No more zeros — each trade has light,
Hooray! 🥕✨

🚥 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 'Feat/create order' accurately describes the main objective of the pull request, which implements the actual order creation functionality across multiple files.
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 feat/create-order

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: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/features/order/screens/add_order_screen.dart (1)

57-66: ⚠️ Potential issue | 🟠 Major

Make the new fixed-sats validation reactive.

_checkValid reads isMarketPriceProvider and fixedSatsProvider via ref.read (lines 61, 63), but build only watches selectedPaymentMethodsProvider and customPaymentMethodProvider. Toggling between market/fixed price or editing the fixed sats field won't trigger a rebuild, leaving the Submit button validity stale.

Suggested change
-  bool _checkValid(List<String> selectedMethods, String customMethod) {
+  bool _checkValid(
+    List<String> selectedMethods,
+    String customMethod, {
+    required bool isMarket,
+    required String fixedSatsStr,
+  }) {
     final hasPayment = selectedMethods.isNotEmpty || customMethod.isNotEmpty;
     if (!hasPayment) return false;
 
-    final isMarket = ref.read(isMarketPriceProvider);
     if (!isMarket) {
-      final fixedSatsStr = ref.read(fixedSatsProvider);
       final sats = BigInt.tryParse(fixedSatsStr);
       if (sats == null || sats <= BigInt.zero) return false;
     }
final isMarket = ref.watch(isMarketPriceProvider);
final fixedSatsStr = ref.watch(fixedSatsProvider);

final isValid = _checkValid(
  selectedMethods,
  customMethod,
  isMarket: isMarket,
  fixedSatsStr: fixedSatsStr,
);
🤖 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 57 - 66,
_build uses ref.watch only for selectedPaymentMethodsProvider and
customPaymentMethodProvider while _checkValid still calls ref.read on
isMarketPriceProvider and fixedSatsProvider, so toggling market/fixed or editing
fixed sats doesn't rebuild and the Submit button stays stale; fix by making the
market/fixed and fixed-sats values reactive in build (use ref.watch on
isMarketPriceProvider and fixedSatsProvider) and pass them into _checkValid (or
change _checkValid to accept isMarket and fixedSatsStr parameters), then use
those watched values when computing isValid for the Submit button (refer to
_checkValid, isMarketPriceProvider, fixedSatsProvider,
selectedPaymentMethodsProvider, customPaymentMethodProvider and the widget's
build where isValid is computed).
🤖 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 305-309: The code is persisting the maker key under a locally
generated order.id (store_trade_key_index) before the server-assigned ID is
known, so get_trade_key_index(real_order_id) will miss; remove or skip calling
store_trade_key_index in the optimistic local create path (where
Message::new_order(None, ...) is used) and instead persist the mapping once the
real Mostro ID is received—i.e., add the store_trade_key_index(real_order_id,
trade_index) call in the incoming-order handler in rust/src/mostro/actions.rs
(the code that reconciles Message::new_order and replaces the None UUID with the
server ID), or maintain a short-lived local->remote ID mapping and transfer it
when reconciling; keep get_trade_key_index usage unchanged.
- Around line 291-321: The current create_order flow swallows errors from
derive_trade_key, get_active_trade_keys, parsing DEFAULT_MOSTRO_PUBKEY,
actions::new_order, and publish_event_json and still returns Ok(order); change
each error branch (errors from derive_trade_key(),
get_active_trade_keys(trade_index),
nostr_sdk::PublicKey::from_hex(DEFAULT_MOSTRO_PUBKEY), actions::new_order(...),
and publish_event_json(...)) to return an Err(...) instead of logging and
continuing, and only call store_trade_key_index(&order.id, trade_index).await
after a successful publish; also ensure callers clear any optimistic/local state
when Err is returned (or invoke the existing rollback/cleanup helper if one
exists) so failures don’t surface as successful createOrder responses.

---

Outside diff comments:
In `@lib/features/order/screens/add_order_screen.dart`:
- Around line 57-66: _build uses ref.watch only for
selectedPaymentMethodsProvider and customPaymentMethodProvider while _checkValid
still calls ref.read on isMarketPriceProvider and fixedSatsProvider, so toggling
market/fixed or editing fixed sats doesn't rebuild and the Submit button stays
stale; fix by making the market/fixed and fixed-sats values reactive in build
(use ref.watch on isMarketPriceProvider and fixedSatsProvider) and pass them
into _checkValid (or change _checkValid to accept isMarket and fixedSatsStr
parameters), then use those watched values when computing isValid for the Submit
button (refer to _checkValid, isMarketPriceProvider, fixedSatsProvider,
selectedPaymentMethodsProvider, customPaymentMethodProvider and the widget's
build where isValid is computed).
🪄 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: 00256487-d454-49bf-9093-5233be152278

📥 Commits

Reviewing files that changed from the base of the PR and between 1f3b7e5 and 2e7b4b9.

📒 Files selected for processing (3)
  • lib/features/order/screens/add_order_screen.dart
  • rust/src/api/orders.rs
  • rust/src/mostro/actions.rs

Comment thread rust/src/api/orders.rs Outdated
Comment thread rust/src/api/orders.rs Outdated
- Derive a fresh trade key (index ≥ 1) when creating an order instead
  of using the identity key (index 0), fixing the InvalidTradeIndex
  error returned by the Mostro node
- Propagate all dispatch errors so create_order returns Err on failure
  instead of silently swallowing them and returning Ok
- Move optimistic order book upsert to after a successful publish to
  avoid surfacing failed orders in the UI
- Replace the broken store_trade_key_index(local_uuid) call with a
  PENDING_MAKER_KEYS map keyed by trade pubkey; expose
  resolve_maker_order() to transfer the entry once the daemon assigns
  a real order ID
- Watch isMarketPriceProvider and fixedSatsProvider in build() so the
  Submit button re-evaluates reactively when price type or fixed sats
  change
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