feat(bond): show the taker when their anti-abuse bond is slashed. Phase 4 #604
feat(bond): show the taker when their anti-abuse bond is slashed. Phase 4 #604Catrya wants to merge 9 commits into
Conversation
- Add Action.bondSlashed so the slash forfeiture notice parses instead of being dropped in MostroMessage.fromJson - order_state.dart: its SmallOrder has a null status and bond-sized amount; preserve the tracked order's real status amount instead of overwriting - notifications: add bondSlashed as no-op to the exhaustive switches, grouped with the other not-yet-wired bond actions
- Extractor builds the notification data for bondSlashed from the payload (bond amount, order id, fiat, payment method) - Add localized title/message keys (en/es/it/de/fr) and a bond amount label - Map bondSlashed to the cancellation notification type - List shows a short message; tapping opens a dialog with the full order details and a close button - NotificationDetails renders nothing instead of an empty box when an action has no detail rows
- canceled handler: if the order had a bond, defer deleting the session 60s so a trailing bond-slashed notice can still be received and decrypted; otherwise delete immediately as before - Add bondSlashed handler: cancels the deferred deletion and releases the session once the notice arrives - Out-of-order safe: if bond-slashed arrives first, the later canceled finds no session and no-ops
- Add OrderState.updateWith regression tests for bondSlashed - Assert the current order status is preserved (not reset to pending by the null-status SmallOrder payload) - Assert the tracked trade order is kept instead of the bond-sized SmallOrder
- Replace the bordered detail rows with a single prose sentence including the order id, bond amount, fiat and payment method - Add parameterized notification_bond_slashed_detail string (en/es/it/de/fr); drop the now-unused bond amount label - Close button uses the standard green ElevatedButton style instead of a plain text button
|
Warning Review limit reached
More reviews will be available in 6 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds Action.bondSlashed end-to-end: enum, notification extraction and localization keys, UI dialog/details, order-state preservation, deferred session-deletion timers in notifier with cancel-on-bondSlashed, multi-language strings, and tests. ChangesBond-slashed action implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/notifications/widgets/notification_item.dart (1)
112-137:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBond-slashed dialog is triggered for unrelated notification actions due to switch fallthrough.
_showBondSlashedDialog(context)currently runs for every action in this case group (e.g.,cantDo,newOrder,takeSell, etc.), not justbondSlashed. This changes tap behavior incorrectly for many notification types.Proposed fix
case mostro_action.Action.cantDo: case mostro_action.Action.rateReceived: case mostro_action.Action.newOrder: case mostro_action.Action.takeSell: case mostro_action.Action.takeBuy: case mostro_action.Action.fiatSent: case mostro_action.Action.release: case mostro_action.Action.cancel: case mostro_action.Action.cooperativeCancelAccepted: case mostro_action.Action.buyerInvoiceAccepted: case mostro_action.Action.holdInvoicePaymentCanceled: case mostro_action.Action.rateUser: case mostro_action.Action.dispute: case mostro_action.Action.adminCancel: case mostro_action.Action.adminSettle: case mostro_action.Action.adminAddSolver: case mostro_action.Action.adminTakeDispute: case mostro_action.Action.adminTookDispute: case mostro_action.Action.invoiceUpdated: case mostro_action.Action.tradePubkey: case mostro_action.Action.restore: case mostro_action.Action.orders: case mostro_action.Action.lastTradeIndex: + break; case mostro_action.Action.bondSlashed: _showBondSlashedDialog(context); break;🤖 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/widgets/notification_item.dart` around lines 112 - 137, The switch grouping in notification_item.dart incorrectly falls through so _showBondSlashedDialog(context) is called for many actions; update the switch in the widget (the cases around mostro_action.Action.bondSlashed and the surrounding cases) so only the single case mostro_action.Action.bondSlashed invokes _showBondSlashedDialog(context) — either remove mostro_action.Action.bondSlashed from the multi-case group and place it in its own case that calls _showBondSlashedDialog(context) with a break, or add explicit breaks/returns after each unrelated case to prevent fallthrough.
🤖 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.
Outside diff comments:
In `@lib/features/notifications/widgets/notification_item.dart`:
- Around line 112-137: The switch grouping in notification_item.dart incorrectly
falls through so _showBondSlashedDialog(context) is called for many actions;
update the switch in the widget (the cases around
mostro_action.Action.bondSlashed and the surrounding cases) so only the single
case mostro_action.Action.bondSlashed invokes _showBondSlashedDialog(context) —
either remove mostro_action.Action.bondSlashed from the multi-case group and
place it in its own case that calls _showBondSlashedDialog(context) with a
break, or add explicit breaks/returns after each unrelated case to prevent
fallthrough.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8707975f-62dd-4e55-9e2d-80d9ab57a80c
📒 Files selected for processing (14)
lib/data/models/enums/action.dartlib/data/models/notification.dartlib/features/notifications/utils/notification_data_extractor.dartlib/features/notifications/utils/notification_message_mapper.dartlib/features/notifications/widgets/notification_details.dartlib/features/notifications/widgets/notification_item.dartlib/features/order/models/order_state.dartlib/features/order/notifiers/abstract_mostro_notifier.dartlib/l10n/intl_de.arblib/l10n/intl_en.arblib/l10n/intl_es.arblib/l10n/intl_fr.arblib/l10n/intl_it.arbtest/features/order/models/order_state_bond_slashed_test.dart
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good direction overall, and I agree with the OrderState protection for bond-sized SmallOrder payloads. But I do see one real behavior regression that should be fixed before merge.
Blocking issue:
- In
NotificationItem, the tap handler now routesAction.bondSlashedinto_showBondSlashedDialog(context)from the same switch group that previously justbreaked for many unrelated actions (cantDo,newOrder,takeSell,adminCanceled,settleBondedHoldInvoice, etc.). With the current structure, those unrelated actions will also open the bond-slashed dialog on tap instead of preserving their old no-op/detail behavior. That is a user-visible behavior regression outside the scope of this feature.
Please make the dialog path exclusive to Action.bondSlashed and keep the previous handling for the other actions intact.
Aside from that, the rest looks reasonable to me on this revision: the deferred session deletion logic is coherent, the out-of-order note makes sense, and the OrderState regression tests cover the payload-overwrite risk well.
- bondSlashed shared the no-op group's empty case labels, so unrelated actions (cantDo, newOrder, etc.) fell through into the bond-slashed dialog on tap - Move it to its own case after the no-op group's break - Add a widget test asserting bondSlashed opens the dialog and a no-op action does not
There was a problem hiding this comment.
Re-reviewed the latest head together with the follow-up fix. The blocker I raised is resolved on this revision.
What changed and why this clears the regression:
Action.bondSlashednow has its own dedicated switch branch inNotificationItem, instead of sharing the old no-op group. That makes the dialog path exclusive to the forfeiture notification and restores the previous tap behavior for unrelated actions.- You also added a focused widget regression test covering both sides of the dispatch:
bondSlashedopens the dialog, while a no-op action from the same former group (cantDo) does not. That is exactly the guard this area needed.
I re-checked the rest of the PR and still think the other pieces are reasonable: the OrderState protection against bond-sized SmallOrder payloads is correct, the deferred session deletion logic is coherent, and CI is green.
Approved on the current revision.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/features/notifications/widgets/notification_item_tap_test.dart`:
- Line 7: Replace the direct import of NotificationModel from
data/models/notification.dart with the barrel export by changing the import to
package:mostro_mobile/data/models.dart; locate the import line that mentions
NotificationModel and update it so the test file uses the re-exported model
(NotificationModel) from models.dart, ensuring all references to
NotificationModel in notification_item_tap_test.dart continue to compile
unchanged.
🪄 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: f2f60dfa-0144-4220-add6-7cf0f968044f
📒 Files selected for processing (2)
lib/features/notifications/widgets/notification_item.darttest/features/notifications/widgets/notification_item_tap_test.dart
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment.
The PR looks solid overall: the bond-slashed flow is wired end-to-end, the state preservation change is correct, and the regression tests cover the main path.
One minor suggestion:
lib/features/order/notifiers/abstract_mostro_notifier.dart:747— the deferred cleanup timer callsdeleteSession(orderId)without awaiting the returnedFuture. That means async failures will bypass thetry/catch, and the success log can fire before the delete actually completes. Consider awaiting the call or handling the future explicitly.
- The deferred bond-cancel deletion relied on an in-memory timer that
was lost if the app closed within the 60s grace window, leaving the
session orphaned in My Trades and blocking retaking.
- Add reconcileCanceledBondedSession(), called from OrderNotifier.sync()
when state rebuilds as canceled: delete now if the grace window elapsed
or bond-slashed already arrived; re-arm the timer otherwise.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/features/order/notifiers/abstract_mostro_notifier.dart (1)
766-796: 🏗️ Heavy liftCancel bond-cancel grace timer on notifier disposal (defensive), though current lifecycle makes the stale-ref scenario unlikely
orderNotifierProviderinlib/features/order/providers/order_notifier_provider.dartis a non-autoDisposeStateNotifierProvider.family, and there are noref.invalidate/refreshusages fororderNotifierProviderin the codebase. So the specific “provider auto-disposed/recreated mid-grace → staleref→ reconcile can’t recover due to_bondCancelDeletionTimerskey” path doesn’t appear to apply.Still,
_startBondCancelDeletioncaptures the notifierrefinside a staticTimer; adding cleanup todispose()(or usingref.onDispose(timer.cancel)) prevents any potential stale-refexecution if lifecycle changes (or the container is disposed during the grace window):`@override` void dispose() { subscription?.close(); _sessionTimeouts[orderId]?.cancel(); _sessionTimeouts.remove(orderId); _bondCancelDeletionTimers[orderId]?.cancel(); _bondCancelDeletionTimers.remove(orderId); super.dispose(); }🤖 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/notifiers/abstract_mostro_notifier.dart` around lines 766 - 796, The notifier leaves bond-cancel timers in _bondCancelDeletionTimers and captures ref in a Timer started by _startBondCancelDeletion; ensure timers are cleaned up on notifier disposal by cancelling and removing _bondCancelDeletionTimers[orderId] in dispose() (or register timer.cancel with ref.onDispose when creating the Timer in _startBondCancelDeletion) so stale timers won't run against a disposed/stale ref; update dispose() (or _startBondCancelDeletion) to cancel and remove the entry for orderId and call super.dispose().
🤖 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.
Nitpick comments:
In `@lib/features/order/notifiers/abstract_mostro_notifier.dart`:
- Around line 766-796: The notifier leaves bond-cancel timers in
_bondCancelDeletionTimers and captures ref in a Timer started by
_startBondCancelDeletion; ensure timers are cleaned up on notifier disposal by
cancelling and removing _bondCancelDeletionTimers[orderId] in dispose() (or
register timer.cancel with ref.onDispose when creating the Timer in
_startBondCancelDeletion) so stale timers won't run against a disposed/stale
ref; update dispose() (or _startBondCancelDeletion) to cancel and remove the
entry for orderId and call super.dispose().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9668c3c3-74bc-46da-ab9b-f11e92d0ad4c
📒 Files selected for processing (2)
lib/features/order/notifiers/abstract_mostro_notifier.dartlib/features/order/notifiers/order_notifier.dart
- A voluntary cancel returns the taker's bond (no slash), so the daemon
never sends bond-slashed; deferring deletion 60s was pure downside and
risked an orphaned session if the app closed in that window.
- Track user-initiated cancels in memory (markUserInitiatedCancel from
cancelOrder) and delete the session immediately on the canceled
response; keep the 60s defer only for non-user-initiated bonded cancels
(the real timeout-slash case).
- Not persisted on purpose: Action.cancel resolves to Status.canceled,
so a stored marker would corrupt sync()-rebuilt state on a rejected or
cooperative cancel. The restart case is handled by the reconcile path.
…n retake
- clearBondCancelDeletion(orderId) on retake (takeSell/takeBuyOrder) cancels
any pending grace timer + clears the user-cancel flag, so retaking within
the 60s window can't delete the new session.
- Extract decision logic to bond_cancel_helpers.dart and add 13 regression
tests (defer truth table, reconcile none/deleteNow/rearm, window boundary).
There was a problem hiding this comment.
I re-reviewed the current head. The restart/reconcile work is a good improvement, but I do see one remaining correctness issue that I would fix before merge.
Blocking issue:
OrderNotifier.cancelOrder()marks the order as user-initiated after awaitingmostroService.cancelOrder(orderId). That creates a race with the incomingAction.canceledevent: if the daemon responds quickly and the event is processed before thatawaitreturns, thecanceledhandler will not see the user-initiated flag yet. In that case it can wrongly take the timeout-slash path and defer session deletion for 60 seconds on a voluntary cancel, which is exactly the case this patch is trying to avoid.
I think the intent here is right, but the marker needs to be set before the async send (and then cleared/reverted if the send itself fails) so the event path can observe it reliably.
The rest of the new direction looks reasonable to me: extracting the decision logic into pure helpers was a good move, the retake cleanup guard makes sense, and the added tests improve confidence. But I would still block on the race above because it changes cancel behavior in a real timing-dependent way.
I don't think the race is reachable, and moving the marker before the send would actually reintroduce the bug this phase fixes. There are two sources of Action::Canceled to the taker (confirmed in the daemon):
For a real voluntary cancel, the Canceled is the daemon's response to the event we publish, so it's causally after publishEvent. markUserInitiatedCancel runs in the microtask continuation right after that await (no await between), and incoming relay events are I/O macrotasks that can't preempt the microtask drain, so the flag is always set first. The "voluntary cancel wrongly deferred" path isn't reachable. The only window where the flag isn't set yet is when an autonomous timeout-slash Canceled overlaps a user cancel, and there, deferring is correct, because a bond-slashed is coming:
revert on failure doesn't help, since the cancel send succeeds — the daemon just slashed first. The current ordering is the correct one; I can add a comment documenting why the marker is set after the send. |
There was a problem hiding this comment.
I re-read your explanation carefully, and I agree the blocker I raised does not hold given the actual event sources and ordering you described.
What changed my mind:
- For a voluntary cancel, the taker's
Action.canceledis causally downstream of the cancel event we publish, so the relevant question is whether an incoming relay callback can run before the post-awaitcontinuation that sets the marker. Your explanation about Dart scheduling here is reasonable:markUserInitiatedCancel(orderId)runs in the microtask continuation immediately after the awaited publish returns, while the inbound relay event arrives on a later I/O task. Under that model, the specific "voluntary cancel gets misclassified because canceled arrived before the marker" path is not actually reachable. - More importantly, setting the marker before the send would indeed blur the autonomous timeout-slash case. In the overlap scenario you described, pre-marking would make the slash-driven
canceledlook user-initiated and could cause immediate session deletion before the trailingbondSlashedarrives, which is worse than the race I was worried about.
So I’m retracting that blocker. I still think a short comment in cancelOrder() explaining why the marker is intentionally set after the awaited send would be helpful for the next reviewer, but I no longer consider it merge-blocking.
Given that, and since the earlier tap-handler regression was already fixed, I’m approving the current revision.
1 participant
Handles the
bond-slashednotice that Mostro sends to a taker whose anti-abuse bond is slashed on a waiting-state timeout, and surfaces it to the user.bond-slashedaction so the message no longer fails to parse and gets dropped; its SmallOrder (bondamount, null status) never overwrites the tracked order/status.
in prose and a green close button.
and decrypted; release the session as soon as it arrives. Out-of-order safe.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Localization
Tests