004 mostro p2p client - #59
Merged
Merged
Conversation
…tion Rust: - orders.rs: release_order(order_id) with FiatSent state validation Dart: - PayLightningInvoiceScreen: QR code via qr_flutter, copy/share buttons, cancel button, dev simulate-payment button - NwcPaymentWidget: "Pay with Wallet" button stub (Phase 14) - PayLightningInvoiceWidget: reusable QR + copy + share component - ReleaseConfirmationDialog: centered modal with info icon, "Release Bitcoin" title, No/Yes buttons - TradeDetailScreen: extended for seller flow — Active state shows CLOSE/CANCEL/DISPUTE/CONTACT; Fiat Sent shows RELEASE button with confirmation dialog → navigate to rate screen; updated _getInstructionText for seller-specific messages - app_routes: wired PayLightningInvoiceScreen at /pay_invoice/:orderId
QR accessibility - pay_lightning_invoice_widget: remove unused onSubmit/onCancel callbacks; await Clipboard.setData with mounted check - pay_lightning_invoice_screen: await Clipboard.setData with mounted check; add semanticsLabel to QR code
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 20 minutes and 50 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 (1)
WalkthroughReplaces a route stub with a real PayLightningInvoice screen, adds UI/widgets for Lightning/NWC payments and a release confirmation dialog, extends trade-detail seller flows, adds Mostro fiat_sent/release action builders and a Rust release_order endpoint, updates specs, and registers/share+url plugins and dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Seller as Seller (UI)
participant TradeDetail as TradeDetailScreen
participant ReleaseDialog as ReleaseConfirmationDialog
participant RustAPI as Release API (Rust)
participant Rating as RatingScreen
Seller->>TradeDetail: Tap RELEASE (seller, fiatSent)
TradeDetail->>ReleaseDialog: showReleaseConfirmationDialog()
ReleaseDialog->>Seller: Display modal (Yes/No)
Seller->>ReleaseDialog: Confirm (Yes)
ReleaseDialog-->>TradeDetail: return true
TradeDetail->>RustAPI: call release_order(orderId)
RustAPI-->>TradeDetail: Result<()>
alt success
TradeDetail->>Rating: Navigate to /rate_user/:orderId
else failure
TradeDetail->>Seller: Show error SnackBar
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 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 |
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/trades/screens/trade_detail_screen.dart (1)
46-50:⚠️ Potential issue | 🔴 CriticalThe new seller branches are dead code right now.
_isBuyeris hard-coded totrueand_statusis still a local mock, so the seller-onlyactive/fiatSentbranches below never render from real trade data. As-is the screen always boots into the buyer flow, which blocks the RELEASE UI entirely.🤖 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 46 - 50, The screen currently hardcodes TradeStatus _status = TradeStatus.active and bool _isBuyer = true which makes all seller-only branches dead; update TradeDetailScreen to source real state instead of mocks by wiring _status and _isBuyer to the actual trade provider / Rust bridge data (e.g., read from the Trade model or provider inside initState/build or subscribe to the provider) and remove the temporary hard-coded defaults/ignore comment so the UI uses the real TradeStatus and buyer/seller flag (references: _status, _isBuyer, TradeStatus, and the seller-specific branches in TradeDetailScreen) ensuring seller active/fiatSent branches can render.
🤖 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/pay_lightning_invoice_screen.dart`:
- Around line 26-39: The screen is using a hardcoded _mockInvoice and only
advances via _simulatePaymentDetected, so replace the mock flow with real
invoice loading and real payment detection: fetch the invoice for widget.orderId
when the screen initializes (replace uses of _mockInvoice in QR/copy/share with
the fetched invoice string), implement payment confirmation by subscribing to
the real payment signal or polling the order/payment status and call
context.go(AppRoute.tradeDetailPath(widget.orderId)) only after a verified
payment, and keep _simulatePaymentDetected/_waiting behind a debug flag or
remove it entirely; update any UI bindings that reference _mockInvoice or
_waiting to use the new invoice and confirmed-payment state.
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 403-415: The onPressed handler currently awaits
showReleaseConfirmationDialog then waits a fixed 500ms and navigates to
AppRoute.rateUserPath(widget.orderId) without calling the Rust release_order()
API or waiting for the SettledHoldInvoice/Success stream; replace the fixed
delay with an actual call to the Rust bridge release_order(orderId) (or the
appropriate method used in your bridge), await its result or subscribe to the
streamed SettledHoldInvoice/Success event and only navigate when the release is
confirmed successful, keeping the context.mounted guard before calling
context.push(AppRoute.rateUserPath(widget.orderId)); ensure errors from
release_order() are handled (show error/abort navigation) and remove the TODO
and artificial delay.
In `@lib/shared/widgets/nwc_payment_widget.dart`:
- Around line 32-47: The _pay() implementation always falls back to manual;
change it so after calling the NWC payment API (replace the placeholder
Future.delayed with the actual nwc_api.pay_invoice(bolt11) Rust bridge call
inside _pay()), check the result and on successful payment invoke
widget.onPaymentSuccess() (or the appropriate success callback) instead of
widget.onFallbackToManual(), and only call widget.onFallbackToManual() in the
error branch or when the API indicates failure; keep the existing mounted checks
and the finally block that sets _paying back to false.
In `@lib/shared/widgets/pay_lightning_invoice_widget.dart`:
- Around line 78-84: The Share button in the FilledButton.icon currently only
shows a "coming soon" Snackbar; replace this no-op with a real system share
invocation (or hide/disable the button until implemented). Locate the
FilledButton.icon in pay_lightning_invoice_widget.dart (inside the
PayLightningInvoiceWidget / build method) and update its onPressed to call the
platform share API (e.g., use the share_plus package and call
Share.share(invoiceText) asynchronously), include necessary import and error
handling (try/catch and a Snackbar on failure), and ensure the button is
disabled or hidden when there is no invoice string available.
In `@rust/src/api/orders.rs`:
- Around line 347-365: The release_order function currently aborts after
validating order.status == OrderStatus::FiatSent by returning a hard
NotImplemented error; replace that with real dispatch: construct the Release
MostroMessage for the given order_id, wrap it according to NIP-59, publish/send
it to the Mostro daemon using the existing messaging/publish helper used
elsewhere in the repo, and then return Ok(()) (or handle/propagate publish
errors as Err). Locate release_order, the order_book() call and
OrderStatus::FiatSent check to insert the message construction/wrapping and the
publish call so the seller release flow actually sends the Release message and
allows the daemon to perform the SettledHoldInvoice → Success transition.
In `@specs/004-mostro-p2p-client/tasks.md`:
- Around line 211-218: The review points out T061–T068 are still stubbed: remove
dev mocks and wire real flows — replace the mock invoice and dev-bypass in
pay_lightning_invoice_screen.dart so it fetches the real invoice for :orderId
and uses the real decision path (use NwcPaymentWidget when NWC configured,
otherwise show pay_lightning_invoice_widget); implement NwcPaymentWidget
(lib/shared/widgets/nwc_payment_widget.dart) to call
nwc_api.pay_invoice(bolt11), show a loading spinner, and call onPaymentSuccess
or onFallbackToManual based on the Rust result; implement
pay_lightning_invoice_widget
(lib/shared/widgets/pay_lightning_invoice_widget.dart) to perform actual
copy/share and call onSubmit/onCancel rather than stubbing; implement
release_order(order_id) in rust/src/api/orders.rs to send the Release
MostroMessage, transition trade status to SettledHoldInvoice→Success and emit
on_trade_updated(order_id); and update the RELEASE button handler in
trade_detail_screen.dart to call release_order, show reactive loading, handle
success by navigating to /rate_user/:orderId, and surface errors instead of
using fixed delays or NotImplemented stubs.
---
Outside diff comments:
In `@lib/features/trades/screens/trade_detail_screen.dart`:
- Around line 46-50: The screen currently hardcodes TradeStatus _status =
TradeStatus.active and bool _isBuyer = true which makes all seller-only branches
dead; update TradeDetailScreen to source real state instead of mocks by wiring
_status and _isBuyer to the actual trade provider / Rust bridge data (e.g., read
from the Trade model or provider inside initState/build or subscribe to the
provider) and remove the temporary hard-coded defaults/ignore comment so the UI
uses the real TradeStatus and buyer/seller flag (references: _status, _isBuyer,
TradeStatus, and the seller-specific branches in TradeDetailScreen) ensuring
seller active/fiatSent branches can render.
🪄 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: ce971e4c-051c-4c2f-9b3d-84b9c9d99ee9
📒 Files selected for processing (8)
lib/core/app_routes.dartlib/features/order/screens/pay_lightning_invoice_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/features/trades/widgets/release_confirmation_dialog.dartlib/shared/widgets/nwc_payment_widget.dartlib/shared/widgets/pay_lightning_invoice_widget.dartrust/src/api/orders.rsspecs/004-mostro-p2p-client/tasks.md
Comment on lines
+403
to
+415
| onPressed: () async { | ||
| final confirmed = | ||
| await showReleaseConfirmationDialog(context); | ||
| if (confirmed != true || !context.mounted) return; | ||
| // TODO: Call release_order() via Rust bridge. | ||
| await Future.delayed( | ||
| const Duration(milliseconds: 500), | ||
| ); | ||
| if (context.mounted) { | ||
| context.push( | ||
| AppRoute.rateUserPath(widget.orderId), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t navigate to rating before the release actually succeeds.
This handler waits 500 ms and pushes /rate_user/:orderId without calling the Rust release_order() API or waiting for a streamed SettledHoldInvoice/Success update. That lets the UI report a completed release even when no sats were released.
🤖 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 403 - 415,
The onPressed handler currently awaits showReleaseConfirmationDialog then waits
a fixed 500ms and navigates to AppRoute.rateUserPath(widget.orderId) without
calling the Rust release_order() API or waiting for the
SettledHoldInvoice/Success stream; replace the fixed delay with an actual call
to the Rust bridge release_order(orderId) (or the appropriate method used in
your bridge), await its result or subscribe to the streamed
SettledHoldInvoice/Success event and only navigate when the release is confirmed
successful, keeping the context.mounted guard before calling
context.push(AppRoute.rateUserPath(widget.orderId)); ensure errors from
release_order() are handled (show error/abort navigation) and remove the TODO
and artificial delay.
Comment on lines
+32
to
+47
| Future<void> _pay() async { | ||
| setState(() => _paying = true); | ||
| try { | ||
| // TODO: Call nwc_api.pay_invoice(bolt11) via Rust bridge. | ||
| await Future.delayed(const Duration(seconds: 1)); | ||
|
|
||
| if (!mounted) return; | ||
| // Placeholder — fall back to manual until NWC is wired. | ||
| widget.onFallbackToManual(); | ||
| } catch (e) { | ||
| debugPrint('NWC payment failed: $e'); | ||
| if (!mounted) return; | ||
| widget.onFallbackToManual(); | ||
| } finally { | ||
| if (mounted) setState(() => _paying = false); | ||
| } |
Contributor
There was a problem hiding this comment.
onPaymentSuccess can never fire from this widget.
Even on the no-error path, _pay() waits and then calls onFallbackToManual(). Once this is wired into a screen, it will always force the manual path instead of ever surfacing an NWC payment success.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/shared/widgets/nwc_payment_widget.dart` around lines 32 - 47, The _pay()
implementation always falls back to manual; change it so after calling the NWC
payment API (replace the placeholder Future.delayed with the actual
nwc_api.pay_invoice(bolt11) Rust bridge call inside _pay()), check the result
and on successful payment invoke widget.onPaymentSuccess() (or the appropriate
success callback) instead of widget.onFallbackToManual(), and only call
widget.onFallbackToManual() in the error branch or when the API indicates
failure; keep the existing mounted checks and the finally block that sets
_paying back to false.
…dling Add release() and fiat_sent() action builders to mostro/actions.rs following the existing take_buy/take_sell pattern. Wire release_order() and send_fiat_sent() in orders.rs to build NIP-59 wrapped messages instead of returning NotImplemented errors. Add share_plus dependency and replace "coming soon" share button stubs with real system share sheet invocations in both invoice widgets. Wrap RELEASE button handler in try/catch with error snackbar. Clarify bridge TODOs for mock invoice, trade state, and NWC payment paths. Update T061–T068 task descriptions with partial-wiring annotations.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specs/004-mostro-p2p-client/tasks.md (1)
211-218: Consider using a distinct marker for partial implementations.The Partial and Note annotations clarify the implementation state, which is an improvement. However, using
[x](complete) alongside "Partial" can be ambiguous for tracking purposes.A clearer convention might be:
[x]for fully complete tasks[~]or[partial]for tasks with known gaps[ ]for not startedThis would make it easier to filter and identify work that still needs attention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/004-mostro-p2p-client/tasks.md` around lines 211 - 218, The checklist uses [x] for items that are still partial/annotated which is ambiguous; update the task markers for any entries that include "Partial" or "Note" (e.g., T061, T062, T063, T064, T065, T066, T067, T068) from [x] to a distinct partial marker like [~] or [partial], and add or update a legend at the top of the tasks document explaining the three states ([x]=done, [~]=partial, [ ]=not started); ensure the textual "Partial"/"Note" annotations remain in the task descriptions so reviewers can see why it’s partial and, if you have any parsing scripts that read the checklist, adjust them to recognize the new [~]/[partial] marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@specs/004-mostro-p2p-client/tasks.md`:
- Around line 211-218: The checklist uses [x] for items that are still
partial/annotated which is ambiguous; update the task markers for any entries
that include "Partial" or "Note" (e.g., T061, T062, T063, T064, T065, T066,
T067, T068) from [x] to a distinct partial marker like [~] or [partial], and add
or update a legend at the top of the tasks document explaining the three states
([x]=done, [~]=partial, [ ]=not started); ensure the textual "Partial"/"Note"
annotations remain in the task descriptions so reviewers can see why it’s
partial and, if you have any parsing scripts that read the checklist, adjust
them to recognize the new [~]/[partial] marker.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a2472a4-9299-49c8-a528-a751a7f33691
📒 Files selected for processing (13)
lib/features/order/screens/pay_lightning_invoice_screen.dartlib/features/trades/screens/trade_detail_screen.dartlib/shared/widgets/nwc_payment_widget.dartlib/shared/widgets/pay_lightning_invoice_widget.dartlinux/flutter/generated_plugin_registrant.cclinux/flutter/generated_plugins.cmakemacos/Flutter/GeneratedPluginRegistrant.swiftpubspec.yamlrust/src/api/orders.rsrust/src/mostro/actions.rsspecs/004-mostro-p2p-client/tasks.mdwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugins.cmake
✅ Files skipped from review due to trivial changes (7)
- macos/Flutter/GeneratedPluginRegistrant.swift
- pubspec.yaml
- linux/flutter/generated_plugins.cmake
- linux/flutter/generated_plugin_registrant.cc
- windows/flutter/generated_plugins.cmake
- windows/flutter/generated_plugin_registrant.cc
- lib/features/trades/screens/trade_detail_screen.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/shared/widgets/pay_lightning_invoice_widget.dart
- lib/shared/widgets/nwc_payment_widget.dart
- lib/features/order/screens/pay_lightning_invoice_screen.dart
- rust/src/api/orders.rs
Introduce a three-state legend ([ ]=not started, [~]=partial, [x]=done) near the top of tasks.md. Change T061, T062, T066, T067, T068 from [x] to [~] since they carry Partial annotations in their descriptions. T063, T064, T065 remain [x] as fully implemented.
This was referenced Mar 30, 2026
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.
Summary by CodeRabbit