feat(bond): Phase 3 claim slashed bond payout from Mostro#596
Conversation
…yload - Add to the Action enum so incoming gift-wraps stop failing deserialization. - Introduce payload (with an internal mini-model) and register it in . - Expose on (defaults to 15 when the kind-38385 tag is absent). - Add 11 localization keys across en/es/it/de/fr for the upcoming claim screen, banner, badge and submit flow. - Wire the action into the existing notification mapper and item switches as a no-op so exhaustiveness holds.
- Add that publishes the action with a carrying the bolt11; the daemon validates the principal against its persisted counterparty share. - Expose as the public entry point for the upcoming claim screen. - Group with the informational actions in so receipt of the request does not perturb the trade's terminal status. - Update the integration-test to implement the new method.
…-invoice - Add and a route that renders the latest from message history with the localized title, claim message, won-line, deadline and submit form. - Wire to auto-navigate to the screen when an arrives within the claim window; receipts past the deadline are ignored silently. - Add and helpers in a new shared between the screen and the navigation gate.
- Persist the outbound payload after a successful publish so the local history records that the user already responded to the most recent prompt. - Add helper that resolves the latest per order: inbound within the claim window means pending, outbound means already answered. - Override the trade-card status chip in with the label and the pending colour scheme whenever the helper reports a pending claim.
- Render a banner above the trade detail body when the order has an unanswered within the claim window, showing the localized submit line with the counterparty share and the formatted deadline. - Add a action button that pushes so the user can submit their bolt11 without leaving the order detail flow.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements a bond payout claim flow: data models and payload support, MostroInstance claim-window config, claim helpers, service/notifier invoice sending, event routing and route, invoice input screen, trade/list/detail UI updates, localization, and tests/test helper. ChangesBond Payout Claim Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant BondPayoutInvoiceScreen
participant OrderNotifier
participant MostroService
participant Storage
User->>BondPayoutInvoiceScreen: Open screen
BondPayoutInvoiceScreen->>BondPayoutInvoiceScreen: Load BondPayoutRequest & deadline
User->>BondPayoutInvoiceScreen: Enter invoice & submit
BondPayoutInvoiceScreen->>OrderNotifier: sendBondPayoutInvoice(invoice)
OrderNotifier->>MostroService: sendBondPayoutInvoice(orderId, invoice)
MostroService->>Storage: Publish addBondInvoice message
OrderNotifier->>Storage: Persist outbound_addBondInvoice key
BondPayoutInvoiceScreen->>User: Navigate to home on success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (3)
lib/l10n/intl_fr.arb (1)
1554-1597: ⚡ Quick winAlign new bond-claim strings with the
fr.arbconvention for in-progress features.Line 1554 through Line 1597 are fully translated to French, but this repository convention expects new-feature strings in
intl_fr.arbto remain in English placeholders until the follow-up localization pass.Based on learnings: "for new features, use English placeholder text in de.arb and fr.arb, with full translations handled in a follow-up PR."
🤖 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/l10n/intl_fr.arb` around lines 1554 - 1597, The new bond-claim localization entries (keys addBondInvoiceTitle, addBondInvoiceMessage, addBondInvoiceWonLine, addBondInvoiceSubmitLine, addBondInvoiceDeadline, addBondInvoiceInputLabel, addBondInvoiceInputHint, addBondInvoiceButton, addBondInvoiceSubmitButton, addBondInvoiceFailedToSubmit, bondPayoutBadge and their placeholder metadata) were fully translated to French but must follow the repo convention of leaving new-feature strings in English placeholders; revert the French message values to English placeholder text (keeping the same JSON keys and placeholder metadata objects intact) so this file uses English stubs for the new bond-claim feature and the actual French translations can be added in the follow-up localization PR.lib/features/order/screens/bond_payout_invoice_screen.dart (1)
6-7: ⚡ Quick winUse the aggregated models import boundary.
These imports should come through
data/models.dartinstead of individual model files.Proposed refactor
-import 'package:mostro_mobile/data/models/bond_payout_request.dart'; -import 'package:mostro_mobile/data/models/enums/action.dart' as mostro_action; +import 'package:mostro_mobile/data/models.dart' as models;Then update usages, e.g.
models.BondPayoutRequestandmodels.Action.addBondInvoice.As per coding guidelines: "Import models through
data/models.dartinstead of importing individual model files".🤖 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/order/screens/bond_payout_invoice_screen.dart` around lines 6 - 7, Replace the two specific model imports with the aggregated models boundary by importing data/models.dart and update all usages: change BondPayoutRequest -> models.BondPayoutRequest and the aliased mostro_action.Action usages (e.g., mostro_action.addBondInvoice) to the corresponding names under models (e.g., models.Action.addBondInvoice) and remove the old imports; ensure the new import alias (if you choose one) is consistent across bond_payout_invoice_screen.dart.lib/shared/utils/bond_payout_helpers.dart (1)
1-4: ⚡ Quick winUse the model barrel export instead of per-model imports.
Switch model imports to
package:mostro_mobile/data/models.dartfor consistency with project rules.As per coding guidelines, "Import models through
data/models.dartinstead of importing individual model files".🤖 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/shared/utils/bond_payout_helpers.dart` around lines 1 - 4, The file currently imports models individually (BondPayoutRequest, Action, MostroMessage, PaymentRequest); replace those specific imports with the consolidated barrel export by importing package:mostro_mobile/data/models.dart and remove the four per-model imports so all references to BondPayoutRequest, Action, MostroMessage, and PaymentRequest resolve via the models.dart barrel.
🤖 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/data/models/bond_payout_request.dart`:
- Around line 81-117: parseInt currently returns 0 for missing fields which
hides malformed payloads; change parseInt (used when constructing
BondPayoutOrder) to treat missing/null as an error: remove or ignore the
defaultValue behavior and throw FormatException('BondPayoutOrder: missing
$field') when json[field] is null, keep existing parsing for int/String and
throw the existing FormatException for unparsable values; ensure calls that
expect required numbers (amount and fiat_amount in the BondPayoutOrder
constructor) continue to call parseInt so they fail fast on missing/malformed
input.
In `@lib/features/mostro/mostro_instance.dart`:
- Around line 119-121: The current logic reads a tag via
_getOptionalTagValue('bond_payout_claim_window_days') into raw, parses it to int
and returns it even if it's zero or negative; change this so that after parsing
(using int.tryParse on raw) you validate the parsed value is > 0 and only then
return it, otherwise fall back to the default 15; reference the existing raw
variable and the call to _getOptionalTagValue('bond_payout_claim_window_days')
and ensure both parse failures and non-positive parsed values return 15.
- Around line 87-89: The predicate passed to tags?.firstWhere can throw
RangeError when a tag is an empty list because it indexes t[0]; change the
predicate to check emptiness first (e.g., (t) => t.isNotEmpty && t[0] == key) so
you never index an empty tag, keeping the existing orElse and the subsequent tag
length check intact; update the lambda used in firstWhere and keep references to
tags, firstWhere, and the local variable tag.
In `@lib/features/notifications/utils/notification_message_mapper.dart`:
- Around line 97-98: The switch branch for mostro.Action.addBondInvoice (and the
other TODO branches noted around lines 201-202) must return real localization
keys instead of placeholder text; replace the 'TODO: implement title key if
needed' string with the appropriate i18n key (or call to your localization
helper, e.g., S.of(context).yourKey or AppLocalizations.translate('your_key'))
and add the corresponding entries to your localization resource files (ARB/JSON)
for all supported languages, ensuring the same pattern is applied to the other
TODO cases in notification_message_mapper.dart so notifications use real
localized strings.
In `@lib/features/notifications/widgets/notification_item.dart`:
- Around line 135-136: The switch case for mostro_action.Action.addBondInvoice
currently does nothing (break) so taps fall through to the generic trade-detail
handler; update the addBondInvoice case in notification_item.dart to perform the
correct navigation by copying the same logic used for the related trade/invoice
cases (e.g., the handlers for mostro_action.Action.addTrade or
mostro_action.Action.addInvoice) so it opens the order/context for the bond
invoice instead of being a no-op; locate the switch handling inside the
NotificationItem widget (or its _handleAction/_onTap method) and replace the
break with the same navigation invocation and arguments used by the
addTrade/addInvoice cases.
In `@lib/features/order/notifiers/order_notifier.dart`:
- Around line 120-132: sendBondPayoutInvoice currently sends the remote invoice
first then awaits persistence via mostroStorageProvider.addMessage which, if it
throws, will bubble up and treat an already-sent invoice as failed; wrap the
persistence call in a try/catch so persistence failures are non-fatal: catch
exceptions from
ref.read(mostroStorageProvider).addMessage('outbound_addBondInvoice_${orderId}_$timestamp',
outbound), log the error (including orderId, timestamp and exception) and
optionally emit telemetry, but do not rethrow so the remote send
(mostroService.sendBondPayoutInvoice) remains the authoritative success.
In `@lib/features/order/screens/bond_payout_invoice_screen.dart`:
- Around line 84-88: The UI currently renders raw exception text in the error
builder (the closure matching error: (e, _) => Center(child: Text(...))) which
exposes internal details; replace the displayed string with a localized,
user-facing message using S.of(context).<yourErrorKey> (e.g.
S.of(context).failedToLoadInvoice) and remove e.toString() from the Text widget,
and separately log the raw error via your logging facility (e.g. debugPrint,
AppLogger.error, or similar) inside the same error handler so the error is
recorded but not shown to users; ensure you import S and add the new
localization key if it doesn't exist.
In `@lib/features/trades/widgets/mostro_message_detail_widget.dart`:
- Around line 4-5: Replace the two direct model imports
(bond_payout_request.dart and session.dart) in mostro_message_detail_widget.dart
with the models barrel import; remove the individual imports and add a single
import for package:mostro_mobile/data/models.dart so code that references
BondPayoutRequest and Session continues to resolve via the barrel export.
In `@lib/shared/utils/bond_payout_helpers.dart`:
- Around line 24-31: The current hasPendingBondClaim returns based on the first
matching message which assumes list ordering; change it to explicitly find the
latest Action.addBondInvoice message (by a timestamp/createdAt field on the
message or other deterministic ordering) then evaluate its payload: if payload
is BondPayoutRequest call isBondClaimExpired(payload.slashedAt, claimWindowDays)
and return the negated result, if payload is PaymentRequest return false;
reference the function hasPendingBondClaim, Action.addBondInvoice,
BondPayoutRequest, PaymentRequest, slashedAt and claimWindowDays when locating
and updating the logic.
---
Nitpick comments:
In `@lib/features/order/screens/bond_payout_invoice_screen.dart`:
- Around line 6-7: Replace the two specific model imports with the aggregated
models boundary by importing data/models.dart and update all usages: change
BondPayoutRequest -> models.BondPayoutRequest and the aliased
mostro_action.Action usages (e.g., mostro_action.addBondInvoice) to the
corresponding names under models (e.g., models.Action.addBondInvoice) and remove
the old imports; ensure the new import alias (if you choose one) is consistent
across bond_payout_invoice_screen.dart.
In `@lib/l10n/intl_fr.arb`:
- Around line 1554-1597: The new bond-claim localization entries (keys
addBondInvoiceTitle, addBondInvoiceMessage, addBondInvoiceWonLine,
addBondInvoiceSubmitLine, addBondInvoiceDeadline, addBondInvoiceInputLabel,
addBondInvoiceInputHint, addBondInvoiceButton, addBondInvoiceSubmitButton,
addBondInvoiceFailedToSubmit, bondPayoutBadge and their placeholder metadata)
were fully translated to French but must follow the repo convention of leaving
new-feature strings in English placeholders; revert the French message values to
English placeholder text (keeping the same JSON keys and placeholder metadata
objects intact) so this file uses English stubs for the new bond-claim feature
and the actual French translations can be added in the follow-up localization
PR.
In `@lib/shared/utils/bond_payout_helpers.dart`:
- Around line 1-4: The file currently imports models individually
(BondPayoutRequest, Action, MostroMessage, PaymentRequest); replace those
specific imports with the consolidated barrel export by importing
package:mostro_mobile/data/models.dart and remove the four per-model imports so
all references to BondPayoutRequest, Action, MostroMessage, and PaymentRequest
resolve via the models.dart barrel.
🪄 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: a95dd31f-cf6a-411e-b34e-4988130c0aa7
📒 Files selected for processing (24)
integration_test/test_helpers.dartlib/core/app_routes.dartlib/data/models.dartlib/data/models/bond_payout_request.dartlib/data/models/enums/action.dartlib/data/models/payload.dartlib/features/mostro/mostro_instance.dartlib/features/notifications/utils/notification_data_extractor.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/notifiers/order_notifier.dartlib/features/order/screens/bond_payout_invoice_screen.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.arblib/services/mostro_service.dartlib/shared/utils/bond_payout_helpers.dart
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
I see good work here overall, but there are still a few correctness issues that I think should be fixed before merge. Some of the CodeRabbit comments are noise, but a couple are real.
1. hasPendingBondClaim() still depends on implicit message ordering
The helper claims to reason about the latest stored addBondInvoice, but from the current implementation pattern it walks the list and returns on the first match. That is only correct if storage always returns messages in newest-first order for every caller. If ordering changes or differs across providers, the UI can show the wrong state: pending when already answered, or answered when there is a newer inbound claim still pending.
This should explicitly resolve the latest relevant Action.addBondInvoice message by timestamp before deciding between inbound BondPayoutRequest and outbound PaymentRequest. Right now the helper is making a hidden ordering assumption for core UI state.
2. _latestBondPayoutRequest() has the same ordering assumption
BondPayoutInvoiceScreen also walks history and returns the first BondPayoutRequest it sees. If history is not guaranteed newest-first, the screen can render an old request instead of the latest one. This matters because the deadline and amount come from that payload.
This should also select the latest relevant inbound request deterministically instead of relying on list order.
3. Raw exception text is still surfaced directly to users
addBondInvoiceFailedToSubmit(e.toString()) leaks raw exception text into UI. That is not great UX and can expose transport/internal error details verbatim. Log the raw error, but show a user-facing localized failure message instead.
Not blockers for me in this PR
- the TODO notification mapper / no-op notification tap path is intentionally unreachable in this PR because
NotificationDataExtractorreturnsnullforAction.addBondInvoice - the
frtranslation convention discussion is not a correctness issue - the
bond_payout_claim_window_days <= 0and empty-tag fixes are already addressed - catching local persistence failure after remote publish is the right direction here; that should not block this PR anymore
Once the ordering assumptions are made explicit and the raw error text is cleaned up, I think this will be in much better shape.
|
@mostronatorcoder
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/bond_payout_invoice_screen.dart`:
- Line 6: Replace the direct import of the BondPayoutRequest model with the
models barrel: remove the line importing
'package:mostro_mobile/data/models/bond_payout_request.dart' and import
'package:mostro_mobile/data/models.dart' instead so the BondPayoutRequest type
(and any other models used in bond_payout_invoice_screen.dart) come from the
consolidated barrel export; ensure there are no naming conflicts or duplicate
imports after the change.
In `@lib/features/trades/widgets/mostro_message_detail_widget.dart`:
- Around line 258-267: The helper _getBondPayoutMessage currently forces (amount
?? 0).toString() which shows a synthetic "0" when
mostroMessageHistoryProvider(orderId) hasn't produced an amount; change the
function to return a different localized message when amount is null instead of
defaulting to 0: read amount via latestBondPayoutRequest(msgs)?.order.amount and
if amount == null return a dedicated "unknown/amount pending" localized string
(e.g. S.of(context)!.addBondInvoiceSubmitLineUnknown()), otherwise return
S.of(context)!.addBondInvoiceSubmitLine(amount.toString()); if that localization
key doesn't exist, add one so the UI clearly indicates the amount is not
available rather than showing 0.
In `@lib/shared/utils/bond_payout_helpers.dart`:
- Around line 1-4: Replace the four explicit model imports with the models
barrel by removing the imports of bond_payout_request.dart, enums/action.dart,
mostro_message.dart and payment_request.dart and adding a single import
'package:mostro_mobile/data/models.dart'; ensure classes referenced in this file
(BondPayoutRequest, Action, MostroMessage, PaymentRequest) are still resolved
after the change and run analyzer/fix to update any export paths.
🪄 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: 95faf48a-1132-4920-b0c5-73997192bffd
📒 Files selected for processing (8)
lib/features/order/screens/bond_payout_invoice_screen.dartlib/features/trades/widgets/mostro_message_detail_widget.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arblib/shared/utils/bond_payout_helpers.dart
✅ Files skipped from review due to trivial changes (3)
- lib/l10n/intl_it.arb
- lib/l10n/intl_fr.arb
- lib/l10n/intl_de.arb
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/l10n/intl_es.arb
- lib/l10n/intl_en.arb
- Switch the bond payout screen and the shared helper to the and barrels instead of importing individual model and enum files. - In , fall back to the generic text when the payout amount isn't yet available so the description never shows a synthetic 0-sat claim while history loads.
There was a problem hiding this comment.
I re-reviewed the latest revision together with Catrya's reply, and the blockers I raised previously are now addressed.
What changed and why this resolves my concerns:
hasPendingBondClaim()no longer depends on caller-provided ordering. The helper now sorts by timestamp descending before evaluating the latestaddBondInvoice, so the pending/answered decision is explicit and deterministic.latestBondPayoutRequest()was factored into the shared helper with the same ordering rule, and the screen/detail consumers now use that shared logic instead of doing their own first-match scans. That removes the hidden ordering assumption I was worried about.- The submit error path no longer surfaces raw exception text to the user. The raw error is still logged, while the UI now shows a fixed localized retry message, which is the right tradeoff here.
I also checked that the other previously-mentioned items remain in a good state:
- non-positive claim window fallback is handled
- empty tag access is guarded
- post-publish local persistence failure is treated as non-fatal
- the notification TODO/no-op path is still intentionally unreachable in this PR because
NotificationDataExtractordoes not produce notifications forAction.addBondInvoiceyet
CI is green, and with the ordering and error-path concerns resolved, I do not see a remaining correctness issue that should block merge. Approved.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
No blocking functional bug found in the new bond-payout flow, but I would not merge this as-is because the feature is still effectively unverified:
integration_test/test_helpers.dartleavesFakeMostroService.sendBondPayoutInvoice()empty, so the test harness cannot exercise the new submit path.- There is no test coverage for the new
addBondInvoiceparsing / claim-window / navigation branch, which is a lot of stateful surface area for a protocol change.
Please add at least one focused test for the pending-claim detection and one end-to-end test for the submit flow before merge.
| @override | ||
| Future<void> sendInvoice(String orderId, String invoice, int? amount) async {} | ||
| @override | ||
| Future<void> sendBondPayoutInvoice(String orderId, String invoice) async {} |
There was a problem hiding this comment.
This fake is a no-op, so the integration harness cannot exercise the new bond payout submit path. Please mirror the production behavior here (or provide a dedicated test double) so the new flow can actually be covered.
|
Added test/shared/utils/bond_payout_helpers_test.dart with 9 focused I skipped the e2e submit test for this PR: BondPayoutInvoiceScreen is |
There was a problem hiding this comment.
Revisión Hermes
Gracias por los tests y por aclarar el path de notifications. No bloqueo por los TODO/no-op que marcaste como intencionales.
Sigo viendo 1 issue que sí me parece bloqueante antes de mergear: el claim puede publicarse en Mostro y luego fallar la persistencia local sin que el usuario se entere. Eso rompe la fuente de verdad del historial local y puede hacer que el badge / auto-open del claim se comporte como si nunca se hubiera enviado.
| payload: PaymentRequest(lnInvoice: invoice), | ||
| timestamp: timestamp, | ||
| ); | ||
| try { |
There was a problem hiding this comment.
🔴 Blocking: here we publish the bond payout invoice remotely and then swallow any persistence error. If addMessage fails, the app loses the local record of a submitted claim, so the pending badge / auto-open logic can re-fire on restart and the user gets no feedback that durability failed. Please propagate this failure or otherwise make the send + persist step atomic from the caller's point of view.
| @override | ||
| Future<void> sendInvoice(String orderId, String invoice, int? amount) async {} | ||
| @override | ||
| Future<void> sendBondPayoutInvoice(String orderId, String invoice) async {} |
There was a problem hiding this comment.
💡 Suggestion: the fake is still a no-op, so the new bond-payout flow is not exercised end-to-end in integration tests. If you want the new coverage to prove the full path, mirror the production side effect here (store the outbound addBondInvoice) or add a dedicated test double for this flow.
There was a problem hiding this comment.
Revisión Hermes
Volví a revisar el PR con los tests nuevos y tu comentario.
- Los helpers de bond payout ahora están mejor cubiertos y el ordenamiento por timestamp corrige un caso real.
- También acepto como intencional el
TODO/no-op de notifications que marcaste en el comentario.
Sigo viendo 1 bloqueo antes de mergear: si la persistencia local falla después de publicar addBondInvoice, la UI cree que el claim se envió bien y se pierde el rastro local del mensaje. Eso deja el flujo en un estado inconsistente y puede reactivar el badge / auto-open como si nunca se hubiera enviado.
| payload: PaymentRequest(lnInvoice: invoice), | ||
| timestamp: timestamp, | ||
| ); | ||
| await ref.read(mostroStorageProvider).addMessage( |
There was a problem hiding this comment.
🔴 Blocking: this still publishes the bond payout invoice first and then swallows any storage failure. If addMessage fails, the app has already told the user success and navigated away, but the local history does not contain the claim reply. Please make the send + persist step fail visibly to the caller, or otherwise guarantee durability before treating the action as complete.
There was a problem hiding this comment.
This was already addressed in commit f57c075 (currently on the
branch). The try/catch around addMessage was removed — the diff is
+4/-12, full removal of the swallow:
- try {
- await ref.read(mostroStorageProvider).addMessage(...);
- } catch (e, stack) {
- logger.e('Failed to persist outbound add-bond-invoice...', ...);
- }
+ await ref.read(mostroStorageProvider).addMessage(...);
The current behavior is:
- order_notifier.dart:120-133 — sendBondPayoutInvoice awaits both
the publish and the addMessage. No try/catch. A storage failure
throws out of the method. - bond_payout_invoice_screen.dart:209-228 — the caller awaits
sendBondPayoutInvoice inside a try. context.go('/') is inside
that try, after the await. If addMessage throws, the navigation
does not run; the catch fires, the button is re-enabled, and
SnackBarHelper shows addBondInvoiceFailedToSubmit.
So the user is not told "success" and not navigated away when
persistence fails — they stay on the form with a visible error.
Could you point at the specific line where you think the failure is
still being swallowed? My read is that this objection is matching
the pre-fix code; if I'm missing a real path where the persist
failure goes silent, I'd like to see it.
There was a problem hiding this comment.
I reviewed the current head again. I still do not see a remaining correctness issue that should block merge.
What I re-checked on the latest revision:
sendBondPayoutInvoice()now publishes and persists without swallowing storage failure, so the caller can surface durability problems instead of pretending success.- The bond payout helpers are covered with focused unit tests, including timestamp ordering, reply-vs-request resolution, expired vs in-window claims, null timestamps, and post-reply state. That gives good coverage for the logic behind the pending badge / claim CTA behavior.
- The shared helper still makes the latest-message decision explicit rather than relying on provider ordering.
- The submit UI still shows a fixed localized retry message instead of leaking raw exception text.
- CI is green.
The remaining debate about leaving FakeMostroService.sendBondPayoutInvoice() as a no-op in integration_test/test_helpers.dart is not a blocker for me in this PR. I would also prefer a stronger end-to-end harness eventually, but the new helper tests already cover the risky state-resolution logic, and this fake remains consistent with the rest of the existing test helper style.
Approved on the current revision.
1 participant
Implements the mobile side of Phase 3 of the anti-abuse bond spec: the winning counterparty receives
Action::AddBondInvoicewith aBondPayoutRequestpayload and can reply with a bolt11 to claim their share before the forfeit deadline.Action::AddBondInvoiceand theBondPayoutRequestpayload (with its embedded SmallOrder).bondPayoutClaimWindowDaysfrom the kind-38385 info event./bond_payout/:orderIdscreen auto-opened on message receipt within the claim window.Action::AddBondInvoicecarrying aPaymentRequest.Out of scope (follow-up PR): push notifications, badge in the notifications list, anti-spam dedup of daemon retries, session lifecycle extensions for long disputes.
How to test:
[anti_abuse_bond] enabled = true,apply_to = "take",slash_node_share_pct = 0.5,payout_claim_window_days = 1.WaitingPayment/WaitingBuyerInvoice.admin-cancelandBondResolution { slash_buyer: true }(orslash_selleron a buy order — depends on roles).send_paymentroutes successfully and the bond row transitions toSlashed.Summary by CodeRabbit
New Features
Localization
Tests