feat(bond): support Phase 1.5 anti-abuse bond invoice flow#592
Merged
Conversation
- New PayBondInvoiceScreen at /pay_bond/:orderId with explanatory text, invoice QR, copy/share buttons and cancel confirmation dialog - Route handling in AbstractMostroNotifier for the new action - Status label Awaiting deposit payment in trades list - New l10n keys in en/es/it (de/fr use English placeholders) - Notification mapper and restore manager updated to satisfy exhaustive-switch requirements
- actions map: Status.waitingTakerBond exposes payBondInvoice, cancel, for both buyer and seller roles - MostroMessageDetail: localized message and status label for the bond waiting state - Trade detail switch: render Pay deposit button navigating to pay_bond, and use the bond-specific confirmer dialog when cancelling from the bond waiting state
Contributor
WalkthroughAdds a bond (anti-abuse deposit) payment flow: new enums Action.payBondInvoice and Status.waitingTakerBond, PayBondInvoiceScreen with QR/copy/share/cancel, /pay_bond/:orderId route, event handling, state-machine updates, notification/trade UI wiring, and i18n strings. ChangesBond Payment Feature
Sequence Diagram(s)sequenceDiagram
participant MostroEvent
participant AbstractMostroNotifier
participant SessionNotifier
participant GoRouter
participant PayBondInvoiceScreen
MostroEvent->>AbstractMostroNotifier: receive Action.payBondInvoice (PaymentRequest)
AbstractMostroNotifier->>SessionNotifier: sessionNotifier.saveSession(session)
AbstractMostroNotifier->>GoRouter: navigate('/pay_bond/<event.id>')
GoRouter->>PayBondInvoiceScreen: build(orderId)
PayBondInvoiceScreen->>PayBondInvoiceScreen: render QR, enable copy/share/cancel
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🧹 Nitpick comments (1)
lib/features/notifications/utils/notification_message_mapper.dart (1)
20-21: ⚡ Quick winUse bond-specific notification keys for
payBondInvoice.Right now bond deposit prompts reuse the generic “payment required” copy, which can blur the distinction between normal invoice payment and anti-abuse deposit payment. A dedicated title/message pair would make the notification intent clearer.
💡 Suggested mapping refinement
- case mostro.Action.payInvoice: - case mostro.Action.payBondInvoice: + case mostro.Action.payInvoice: return 'notification_payment_required_title'; + case mostro.Action.payBondInvoice: + return 'notification_bond_payment_required_title';- case mostro.Action.payInvoice: - case mostro.Action.payBondInvoice: + case mostro.Action.payInvoice: return 'notification_payment_required_message'; + case mostro.Action.payBondInvoice: + return 'notification_bond_payment_required_message';Also add corresponding ARB keys for all supported locales.
Also applies to: 123-124
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/features/notifications/utils/notification_message_mapper.dart` around lines 20 - 21, The mapping for mostro.Action.payBondInvoice currently returns the generic 'notification_payment_required_title'; change this to a bond-specific key (e.g., 'notification_bond_payment_required_title') in the notification mapping function (where mostro.Action.payBondInvoice is handled) and update any other payBondInvoice-related returns in the same mapper (the other occurrence mentioned) to use corresponding bond-specific title/message keys (e.g., 'notification_bond_payment_required_message'); then add the new ARB keys and translations for all supported locales so the new bond-specific title/message are available in the app.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/order/screens/pay_bond_invoice_screen.dart`:
- Around line 118-123: The QR widget is rendered even when lnInvoice is empty,
producing a confusing blank QR; update the build logic around QrImageView to
conditionally render a placeholder/loading state (e.g., a spinner, message, or
empty-state widget) whenever lnInvoice is null or an empty string instead of
passing it into QrImageView, and only create QrImageView with data: lnInvoice
when lnInvoice.isNotEmpty; adjust related errorStateBuilder if needed to handle
real QR errors separately.
- Around line 57-59: The navigation happens before the cancellation completes;
change the sequence so you call and await orderNotifier.cancelOrder() first and
only call context.go('/') after it succeeds; wrap the await
orderNotifier.cancelOrder() in a try/catch (or handle the returned result) to
detect failure and show an error (e.g., via ScaffoldMessenger or the notifier's
error state) instead of navigating; update the code around
orderNotifierProvider(orderId).notifier, cancelOrder(), and context.go('/')
accordingly.
---
Nitpick comments:
In `@lib/features/notifications/utils/notification_message_mapper.dart`:
- Around line 20-21: The mapping for mostro.Action.payBondInvoice currently
returns the generic 'notification_payment_required_title'; change this to a
bond-specific key (e.g., 'notification_bond_payment_required_title') in the
notification mapping function (where mostro.Action.payBondInvoice is handled)
and update any other payBondInvoice-related returns in the same mapper (the
other occurrence mentioned) to use corresponding bond-specific title/message
keys (e.g., 'notification_bond_payment_required_message'); then add the new ARB
keys and translations for all supported locales so the new bond-specific
title/message are available in the app.
🪄 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: 713ad311-f306-4fb3-80e4-b710762aa494
📒 Files selected for processing (17)
lib/core/app_routes.dartlib/data/models/enums/action.dartlib/data/models/enums/status.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_item.dartlib/features/order/models/order_state.dartlib/features/order/notifiers/abstract_mostro_notifier.dartlib/features/order/screens/pay_bond_invoice_screen.dartlib/features/restore/restore_manager.dartlib/features/trades/screens/trade_detail_screen.dartlib/features/trades/widgets/mostro_message_detail_widget.dartlib/features/trades/widgets/trades_list_item.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arb
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I reviewed the PR and also checked the existing review discussion before deciding. The previously raised concern about optimistic navigation on cancel is not a blocker here, because Catrya already clarified that this mirrors the existing app-wide cancellation pattern and failures are surfaced asynchronously through the existing cant-do flow. That is a broader architectural cleanup, not something this PR should be forced to solve in isolation.
On the actual change set, the implementation is coherent and matches the Phase 1.5 bond flow well:
Action.payBondInvoiceandStatus.waitingTakerBondare integrated consistently through routing, notification handling, restore, trade detail, and trades list presentation.- The new
/pay_bond/:orderIdscreen covers the key UX actions: explanation, QR, copy, share, and cancel confirmation. OrderStateexposes the correct action set forwaitingTakerBondfor both roles, which keeps the FSM explicit instead of relying on ad-hoc UI conditionals.- Restore support is present, so reopened sessions can land back in the bond flow correctly.
- CI is green.
I do not see a correctness issue here that should block merge. The remaining CodeRabbit points are either copy-level refinements or follow-up polish, not merge blockers for this phase. Approved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/l10n/intl_de.arb`:
- Line 709: The ARB entry "bondPayInvoicePrompt" uses the placeholder {amount}
but lacks the required metadata block; add an `@bondPayInvoicePrompt` metadata
object immediately after the key that defines the "amount" placeholder (include
a brief "description" and a "placeholders" entry specifying "amount" with an
appropriate type/example, e.g., numeric/sats), so Flutter's localization tooling
recognizes the placeholder.
In `@lib/l10n/intl_en.arb`:
- Line 709: The new localization key "bondPayInvoicePrompt" includes a
placeholder {amount} but lacks the required metadata entry
(`@bondPayInvoicePrompt`) describing that placeholder; add an
`@bondPayInvoicePrompt` metadata block (with "description" and "placeholders"
mapping for "amount" and a sample "type" or "example") to intl_en.arb and then
add the equivalent key+metadata entries to intl_es.arb and intl_it.arb (keeping
the same placeholder name {amount}) so all three ARB files stay in sync;
reference the key "bondPayInvoicePrompt" and the metadata marker
"@bondPayInvoicePrompt" when making the changes.
In `@lib/l10n/intl_es.arb`:
- Line 622: Add the missing placeholder metadata for the localization key
bondPayInvoicePrompt by adding an accompanying `@bondPayInvoicePrompt` entry that
declares the {amount} placeholder (e.g., "placeholders": {"amount":
{"type":"String"}} and a short "description") in intl_es.arb; also add the same
key and metadata to intl_en.arb and intl_it.arb so all three ARB files contain
the new localized key and its placeholder metadata.
In `@lib/l10n/intl_fr.arb`:
- Around line 707-712: The FR ARB entries (bondScreenTitle, bondExplanation,
bondPayInvoicePrompt, statusWaitingTakerBond, payBondMessage, payBondButton) are
English; replace each value with accurate French translations so the French
locale contains localized user-facing text (or if translations are unavailable,
revert these keys to the default locale and open a tracking issue to add proper
French strings before release); update the six keys consistently and ensure
punctuation/newlines in bondExplanation are preserved in the translated string.
- Line 709: The ARB entry "bondPayInvoicePrompt" contains a placeholder {amount}
but lacks the required metadata block; add an `@bondPayInvoicePrompt` metadata
object that defines the placeholder "amount" (e.g., include a "placeholders" map
with "amount" and supply a "type" like "String" or "int" and a brief
"description") so the localization tooling knows about the parameter. Ensure the
metadata key is exactly `@bondPayInvoicePrompt` and that it documents the "amount"
placeholder type and purpose.
🪄 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: f2c33af4-e40c-497a-8c11-266032550de0
📒 Files selected for processing (6)
lib/features/order/screens/pay_bond_invoice_screen.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arb
✅ Files skipped from review due to trivial changes (1)
- lib/l10n/intl_it.arb
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/order/screens/pay_bond_invoice_screen.dart
Contributor
There was a problem hiding this comment.
I re-reviewed the latest revision of this PR against the new commits and the current review discussion. I do not see a new correctness issue introduced by the latest changes.
The recent CodeRabbit comments do not change my conclusion:
- The placeholder metadata blocks for
bondPayInvoicePromptare optional in Flutter gen-l10n, and the existing project already uses placeholder keys without metadata. That is a style/consistency cleanup topic, not a merge blocker for this PR. - The English fallback strings in
deandfrare explicitly part of the scoped approach described by the author for these bond keys, and I do not think this should block the functional Phase 1.5 flow.
On the implementation itself, the PR still looks coherent:
waitingTakerBond/payBondInvoiceare integrated consistently across routing, notifier handling, restore mapping, trades list, trade detail, and message detail.- The new bond screen now includes the prompt string with the invoice amount, which improves clarity for the taker flow.
- CI is green.
Given the current scope and the discussion already captured on the PR, I approve the latest revision.
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
2 participants
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.
Adds the full mobile UX for the anti-abuse deposit (bond) that the taker must pay before continuing with a trade. The taker locks a Lightning hold invoice as a deposit; it is released when the trade finishes correctly.
What's included
/pay_bond/<orderId>screen shown when the deposit invoice arrives, with:Test plan
/pay_bond/
Summary by CodeRabbit
New Features
Status / Flow
Localization