phase 6 - #55
Merged
Merged
Conversation
…ble FAB Rust: - mostro/fsm.rs: 15-state protocol FSM with next_status(status, action, role) and 5 unit tests covering happy paths + invalid transitions - mostro/actions.rs: new_order, take_buy, take_sell dispatch functions that build MostroMessage JSON + NIP-59 Gift Wrap - api/orders.rs: create_order with full param validation (fiat_amount XOR range, fiat_code/payment_method non-empty, range min > 0 < max) - api/types.rs: NewOrderParams struct Dart: - AddOrderButton: expandable FAB with Buy/Sell sub-buttons, dark overlay, animated rotation + scale transitions - AddOrderScreen: 4-card form (type+amount+currency, payment methods, price type, premium slider), Cancel/Submit bottom bar with validation - CurrencySection: tappable selector with search dialog from fiat.json - PaymentMethodSection: multi-select chips + custom text field - PriceSection: Market/Fixed toggle, purple premium slider with editable field, fixed sats input - app_routes: wired AddOrderScreen with ?type= query parameter - home_screen: replaced simple FAB with AddOrderButton
…cycle, dialog context, FSM safety, fiat_amount validation Rust: - actions.rs: remove unused imports (anyhow!, OrderInfo); fix doc - orders.rs: validate fixed fiat_amount > 0 and is_finite - fsm.rs: restrict NewOrder to Pending only; add cooperative cancel role enforcement comment Dart: - add_order_screen: ref.watch() for reactive Submit button - price_section: ConsumerStatefulWidget with managed controller - payment_method_section: ConsumerStatefulWidget with managed controller - currency_section + payment_method_section: dialog context for pop
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 42 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR implements order creation functionality across the Flutter frontend and Rust backend. It introduces a new Changes
Sequence DiagramsequenceDiagram
actor User
participant AddOrderScreen as AddOrderScreen<br/>(Flutter)
participant API as orders API<br/>(Rust)
participant FSM as Mostro FSM<br/>(Rust)
participant Cache as Order Cache<br/>(In-Memory)
User->>AddOrderScreen: Fill order form & submit
AddOrderScreen->>AddOrderScreen: Validate inputs
AddOrderScreen->>API: create_order(params)
API->>API: Validate params (XOR, ranges, non-empty)
API->>FSM: Check initial action allowed
FSM-->>API: Status transition: Pending
API->>Cache: upsert_order(OrderInfo)
Cache-->>API: Order persisted
API-->>AddOrderScreen: OrderInfo returned
AddOrderScreen->>AddOrderScreen: Set loading=false
AddOrderScreen->>User: Navigate to orderBook
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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
🧹 Nitpick comments (5)
lib/features/order/widgets/price_section.dart (1)
51-57: Consider usingaddListenerinstead of syncing inbuild.Writing to
_premiumController.textinsidebuild()works but mixes concerns. A cleaner approach would be to use aref.listenininitState(viaWidgetsBinding.instance.addPostFrameCallback) or in aref.listencallback to update the controller when the provider changes.That said, the current approach is functional and the
_editingPremiumguard prevents feedback loops.Also applies to: 170-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/order/widgets/price_section.dart` around lines 51 - 57, The build() method is mutating _premiumController.text to reflect provider changes which mixes concerns; instead, stop writing to _premiumController in build and move the sync logic into a lifecycle/listener: set up a ref.listen (or a provider listener) in initState (or via WidgetsBinding.instance.addPostFrameCallback) to observe the provider value and update _premiumController.text only when !_editingPremium and the text differs. Update both places that write in build (the block around _premiumController.text and the similar block at lines 170-179) to remove that mutation and rely on the new ref.listen handler; keep the _editingPremium guard and ensure you cancel/unsubscribe the listener in dispose if needed.rust/src/mostro/actions.rs (2)
12-13: Consider consolidatingMOSTRO_DM_KINDwithKIND_ORDER.
rust/src/nostr/order_events.rsalready definespub const KIND_ORDER: u16 = 38383. Using that constant here would avoid duplication.-/// Kind used for Mostro direct messages (NIP-59 inner rumor). -const MOSTRO_DM_KIND: u16 = 38383; +use crate::nostr::order_events::KIND_ORDER;Then replace
MOSTRO_DM_KINDwithKIND_ORDERin thegift_wrap::wrapcalls.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/mostro/actions.rs` around lines 12 - 13, Remove the duplicated const MOSTRO_DM_KIND and replace its usages in gift_wrap::wrap calls with the existing KIND_ORDER constant; update the imports or qualify the reference (e.g., use crate::nostr::order_events::KIND_ORDER or refer to order_events::KIND_ORDER) so gift_wrap::wrap(...) passes KIND_ORDER instead of MOSTRO_DM_KIND and delete the now-unused MOSTRO_DM_KIND definition.
56-112:take_buyandtake_sellhave nearly identical implementations.These two functions differ only in the
"action"string value. Consider extracting a shared helper to reduce duplication.♻️ Suggested refactor
async fn take_order_impl( sender_keys: &Keys, mostro_pubkey: &PublicKey, order_id: &str, amount: Option<f64>, action: &str, ) -> Result<String> { let mut content = json!({ "id": order_id }); if let Some(amt) = amount { content["amount"] = json!(amt); } let payload = json!({ "order": { "version": 1, "action": action, "content": content, } }); gift_wrap::wrap( sender_keys, mostro_pubkey, &payload.to_string(), Kind::from(MOSTRO_DM_KIND), ) .await } pub async fn take_buy( sender_keys: &Keys, mostro_pubkey: &PublicKey, order_id: &str, amount: Option<f64>, ) -> Result<String> { take_order_impl(sender_keys, mostro_pubkey, order_id, amount, "take-buy").await } pub async fn take_sell( sender_keys: &Keys, mostro_pubkey: &PublicKey, order_id: &str, amount: Option<f64>, ) -> Result<String> { take_order_impl(sender_keys, mostro_pubkey, order_id, amount, "take-sell").await }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/mostro/actions.rs` around lines 56 - 112, Both take_buy and take_sell duplicate the same payload-building and wrapping logic; extract a single helper (e.g. take_order_impl) that accepts sender_keys: &Keys, mostro_pubkey: &PublicKey, order_id: &str, amount: Option<f64>, and action: &str, builds the JSON payload (including version, action, and content with optional "amount"), calls gift_wrap::wrap(..., Kind::from(MOSTRO_DM_KIND)).await and returns Result<String>, then have take_buy and take_sell simply call that helper with "take-buy" and "take-sell" respectively.rust/src/api/types.rs (1)
211-229: DuplicateNewOrderParamsstruct exists inmostro/actions.rs.This struct has an identical definition in
rust/src/mostro/actions.rs(lines 16-26). This creates a maintenance burden and potential for drift. Consider:
- Using this single definition from
api::typesinmostro::actions, or- Adding a
Fromimpl if the structs need to remain separate for layering reasons.The relevant snippet from
rust/src/mostro/actions.rsconfirms field-for-field equivalence.♻️ Suggested approach: reuse api::types::NewOrderParams
In
rust/src/mostro/actions.rs, replace the local struct with an import:-/// Parameters for creating a new order. -#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] -pub struct NewOrderParams { - pub kind: OrderKind, - pub fiat_amount: Option<f64>, - pub fiat_amount_min: Option<f64>, - pub fiat_amount_max: Option<f64>, - pub fiat_code: String, - pub payment_method: String, - pub premium: f64, - pub amount_sats: Option<u64>, -} +use crate::api::types::NewOrderParams;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/api/types.rs` around lines 211 - 229, There is a duplicate NewOrderParams struct defined in api::types and mostro::actions; remove the copy in mostro::actions and reuse api::types::NewOrderParams by importing it (use crate::api::types::NewOrderParams) or, if you need distinct types for layering, keep the local MostroNewOrderParams and add a From<api::types::NewOrderParams> (and/or Into) impl to convert between them; update any references in functions (e.g., those constructing or accepting NewOrderParams in mostro::actions) to use the chosen single definition or conversion helpers to eliminate drift.rust/src/mostro/fsm.rs (1)
105-108: Consider extendingbuy_order_happy_pathtest coverage.This test only verifies the initial
TakeSelltransition. For consistency withsell_order_happy_path, consider extending it through the full flow:WaitingPayment → Active → FiatSent → SettledHoldInvoice.✨ Extended test
#[test] fn buy_order_happy_path() { assert_eq!(next_status(&OrderStatus::Pending, Action::TakeSell, TradeRole::Seller), Some(OrderStatus::WaitingPayment)); + // Continue through the flow - seller pays invoice + assert_eq!(next_status(&OrderStatus::WaitingPayment, Action::PayInvoice, TradeRole::Seller), Some(OrderStatus::Active)); + // Buyer marks fiat sent + assert_eq!(next_status(&OrderStatus::Active, Action::FiatSent, TradeRole::Buyer), Some(OrderStatus::FiatSent)); + // Seller releases + assert_eq!(next_status(&OrderStatus::FiatSent, Action::Release, TradeRole::Seller), Some(OrderStatus::SettledHoldInvoice)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/mostro/fsm.rs` around lines 105 - 108, Extend the buy_order_happy_path test to assert the full lifecycle: after the existing assert that TakeSell from Pending (TradeRole::Seller) yields WaitingPayment, call next_status repeatedly to verify WaitingPayment → Active → FiatSent → SettledHoldInvoice, using the same Action variants and TradeRole::Buyer/TradeRole::Seller roles used in the sell_order_happy_path test; add asserts for each intermediate state (e.g., assert_eq!(next_status(&OrderStatus::WaitingPayment, <appropriate Action>, <role>), Some(OrderStatus::Active)), then for Active → FiatSent and FiatSent → SettledHoldInvoice) so the test covers the entire happy path.
🤖 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/screens/add_order_screen.dart`:
- Around line 24-39: The screen's global provider state is not cleared on entry,
so in _AddOrderScreenState.initState you should reset the listed providers
(selectedPaymentMethodsProvider, customPaymentMethodProvider,
selectedFiatCodeProvider, isMarketPriceProvider, premiumValueProvider,
fixedSatsProvider) to their defaults via their notifiers/readers (e.g., call
ref.read(<provider>.notifier).reset() or assign the default value with
ref.read(<provider>.notifier).state = <default>) before any UI uses them; add
this reset logic inside initState of _AddOrderScreenState so each new
AddOrderScreen starts with fresh/default provider state.
In `@lib/features/order/widgets/price_section.dart`:
- Around line 142-154: The onTapOutside handler currently sets _editingPremium =
false without triggering a rebuild, so the TextField won't refresh to reflect
slider/premiumValueProvider changes; update the onTapOutside callback in the
widget containing _editingPremium (the same place as onTap/onSubmitted) to call
setState(() { _editingPremium = false; }); and also ensure after setState you
sync the TextEditingController text from ref.read(premiumValueProvider) (or
trigger the same update logic used when slider changes) so the field shows the
latest clamped double value.
In `@rust/src/api/orders.rs`:
- Around line 165-173: The range validation block (when has_range is true) for
fiat_amount uses params.fiat_amount_min.unwrap() and fiat_amount_max.unwrap()
but doesn't check for non-finite values; update the validation in the same scope
(the block that reads min and max) to first verify both min.is_finite() and
max.is_finite() and return an Err(anyhow::anyhow!(...)) with a clear message if
either is not finite, then continue with the existing min <= 0.0 || min >= max
check; this change should be applied where has_range, params.fiat_amount_min,
and params.fiat_amount_max are referenced in rust/src/api/orders.rs.
---
Nitpick comments:
In `@lib/features/order/widgets/price_section.dart`:
- Around line 51-57: The build() method is mutating _premiumController.text to
reflect provider changes which mixes concerns; instead, stop writing to
_premiumController in build and move the sync logic into a lifecycle/listener:
set up a ref.listen (or a provider listener) in initState (or via
WidgetsBinding.instance.addPostFrameCallback) to observe the provider value and
update _premiumController.text only when !_editingPremium and the text differs.
Update both places that write in build (the block around _premiumController.text
and the similar block at lines 170-179) to remove that mutation and rely on the
new ref.listen handler; keep the _editingPremium guard and ensure you
cancel/unsubscribe the listener in dispose if needed.
In `@rust/src/api/types.rs`:
- Around line 211-229: There is a duplicate NewOrderParams struct defined in
api::types and mostro::actions; remove the copy in mostro::actions and reuse
api::types::NewOrderParams by importing it (use
crate::api::types::NewOrderParams) or, if you need distinct types for layering,
keep the local MostroNewOrderParams and add a From<api::types::NewOrderParams>
(and/or Into) impl to convert between them; update any references in functions
(e.g., those constructing or accepting NewOrderParams in mostro::actions) to use
the chosen single definition or conversion helpers to eliminate drift.
In `@rust/src/mostro/actions.rs`:
- Around line 12-13: Remove the duplicated const MOSTRO_DM_KIND and replace its
usages in gift_wrap::wrap calls with the existing KIND_ORDER constant; update
the imports or qualify the reference (e.g., use
crate::nostr::order_events::KIND_ORDER or refer to order_events::KIND_ORDER) so
gift_wrap::wrap(...) passes KIND_ORDER instead of MOSTRO_DM_KIND and delete the
now-unused MOSTRO_DM_KIND definition.
- Around line 56-112: Both take_buy and take_sell duplicate the same
payload-building and wrapping logic; extract a single helper (e.g.
take_order_impl) that accepts sender_keys: &Keys, mostro_pubkey: &PublicKey,
order_id: &str, amount: Option<f64>, and action: &str, builds the JSON payload
(including version, action, and content with optional "amount"), calls
gift_wrap::wrap(..., Kind::from(MOSTRO_DM_KIND)).await and returns
Result<String>, then have take_buy and take_sell simply call that helper with
"take-buy" and "take-sell" respectively.
In `@rust/src/mostro/fsm.rs`:
- Around line 105-108: Extend the buy_order_happy_path test to assert the full
lifecycle: after the existing assert that TakeSell from Pending
(TradeRole::Seller) yields WaitingPayment, call next_status repeatedly to verify
WaitingPayment → Active → FiatSent → SettledHoldInvoice, using the same Action
variants and TradeRole::Buyer/TradeRole::Seller roles used in the
sell_order_happy_path test; add asserts for each intermediate state (e.g.,
assert_eq!(next_status(&OrderStatus::WaitingPayment, <appropriate Action>,
<role>), Some(OrderStatus::Active)), then for Active → FiatSent and FiatSent →
SettledHoldInvoice) so the test covers the entire happy path.
🪄 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: 4052927e-0291-4623-a56a-09051c1d4c94
📒 Files selected for processing (13)
lib/core/app_routes.dartlib/features/home/screens/home_screen.dartlib/features/order/screens/add_order_screen.dartlib/features/order/widgets/currency_section.dartlib/features/order/widgets/payment_method_section.dartlib/features/order/widgets/price_section.dartlib/shared/widgets/add_order_button.dartrust/src/api/orders.rsrust/src/api/types.rsrust/src/mostro/actions.rsrust/src/mostro/fsm.rsrust/src/mostro/mod.rsspecs/004-mostro-p2p-client/tasks.md
…ync, range validation, dedup, test coverage Rust: - orders.rs: add is_finite check for range fiat_amount_min/max - actions.rs: remove duplicate NewOrderParams (use api::types), replace MOSTRO_DM_KIND with KIND_ORDER from order_events, extract take_order_impl helper to deduplicate take_buy/take_sell - fsm.rs: extend buy_order_happy_path test to cover full lifecycle (WaitingPayment → Active → FiatSent → SettledHoldInvoice) Dart: - add_order_screen: reset all form providers in initState via Future.microtask so each screen starts fresh - price_section: use ref.listen for controller sync instead of mutating in build; wrap _editingPremium changes in setState; sync controller text in onTapOutside
This was referenced Mar 30, 2026
Merged
Merged
Merged
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.
Rust:
role) and 5 unit tests covering happy paths + invalid transitions
that build MostroMessage JSON + NIP-59 Gift Wrap
XOR range, fiat_code/payment_method non-empty, range min > 0 < max)
Dart:
overlay, animated rotation + scale transitions
price type, premium slider), Cancel/Submit bottom bar with validation
editable field, fixed sats input
Summary by CodeRabbit
New Features
Refactor