-
Notifications
You must be signed in to change notification settings - Fork 25
fix(restore): show new-invoice prompt for failed payout in settled-hold-invoice #623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -798,23 +798,57 @@ class RestoreService { | |
| } | ||
| } | ||
|
|
||
| // Create regular order message with Order payload | ||
| final mostroMessage = MostroMessage<Order>( | ||
| id: orderDetail.id, | ||
| action: action, | ||
| payload: order, | ||
| timestamp: | ||
| orderDetail.createdAt ?? | ||
| DateTime.now().millisecondsSinceEpoch, | ||
| ); | ||
| // `settled-hold-invoice` is overloaded for the buyer (see issue | ||
| // #615): it covers both "hold settled, sats in flight" and "payout | ||
| // failed, awaiting a new invoice". The protocol distinguishes them | ||
| // only via the action (`payment-failed` / `add-invoice`), so a | ||
| // status-based rebuild always lands on the "paying sats" screen. | ||
| // On restore the daemon re-sends `add-invoice` to the buyer's trade | ||
| // key when the payout failed (mostro#754). Detect that substate and | ||
| // replay `payment-failed` then `add-invoice` so the order lands on | ||
| // `payment-failed` + `add-invoice` (the new-invoice prompt) instead. | ||
| List<Action> actionsToApply = [action]; | ||
| final orderSession = ref | ||
| .read(sessionNotifierProvider.notifier) | ||
| .getSessionByOrderId(orderDetail.id); | ||
| if (order.status == Status.settledHoldInvoice && | ||
| orderSession?.role == Role.buyer) { | ||
| final messages = await storage.getAllMessagesForOrderId( | ||
| orderDetail.id, | ||
| ); | ||
| if (restoreHasFailedPayoutSignal(messages)) { | ||
| actionsToApply = [Action.paymentFailed, Action.addInvoice]; | ||
| logger.i( | ||
| 'Restore: detected failed payout for order ${orderDetail.id}, ' | ||
| 'restoring payment-failed substate instead of "paying sats"', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Save order message to storage | ||
| final key = | ||
| '${orderDetail.id}_restore_${action.value}_${DateTime.now().millisecondsSinceEpoch}'; | ||
| await storage.addMessage(key, mostroMessage); | ||
| // Create and apply the regular order message(s) with Order payload. | ||
| // When more than one action is replayed, stagger their timestamps so | ||
| // a later sync() replays them in the same order and converges to the | ||
| // same final state. | ||
| final baseTimestamp = | ||
| orderDetail.createdAt ?? DateTime.now().millisecondsSinceEpoch; | ||
| for (var i = 0; i < actionsToApply.length; i++) { | ||
| final replayAction = actionsToApply[i]; | ||
| final mostroMessage = MostroMessage<Order>( | ||
| id: orderDetail.id, | ||
| action: replayAction, | ||
| payload: order, | ||
| timestamp: baseTimestamp + i, | ||
| ); | ||
|
|
||
| // Save order message to storage | ||
| final key = | ||
| '${orderDetail.id}_restore_${replayAction.value}_' | ||
| '${DateTime.now().millisecondsSinceEpoch}_$i'; | ||
| await storage.addMessage(key, mostroMessage); | ||
|
|
||
| // Update state with order message | ||
| notifier.updateStateFromMessage(mostroMessage); | ||
| // Update state with order message | ||
| notifier.updateStateFromMessage(mostroMessage); | ||
| } | ||
| } | ||
| } catch (e, stack) { | ||
| logger.e( | ||
|
|
@@ -1071,6 +1105,45 @@ class RestoreService { | |
| } | ||
| } | ||
|
|
||
| /// Detects the "payout failed, awaiting a new invoice" substate of an order | ||
| /// that Mostro reports as `settled-hold-invoice`. See issue #615. | ||
| /// | ||
| /// `settled-hold-invoice` is overloaded for the buyer: it covers both "hold | ||
| /// settled, sats in flight" and "payout failed". The protocol distinguishes | ||
| /// them only via the action, so the snapshot status alone cannot recover the | ||
| /// failed substate. On restore the daemon re-sends `add-invoice` (and may also | ||
| /// re-send `payment-failed`) to the buyer's trade key when the payout failed | ||
| /// (mostro#754). | ||
| /// | ||
| /// `payment-failed` only ever occurs after a failed payout, so it is a | ||
| /// definitive signal on its own. `add-invoice` also appears early in the happy | ||
| /// flow (waiting-buyer-invoice), so it is only treated as a failed-payout signal | ||
| /// when it arrives after the hold was released/settled. Restore clears storage | ||
| /// before re-subscribing, so in practice only freshly re-sent messages are | ||
| /// present, but the ordering check keeps this correct even if the daemon | ||
| /// re-sends the full message history. | ||
| bool restoreHasFailedPayoutSignal(List<MostroMessage> messages) { | ||
| final sorted = [...messages] | ||
| ..sort((a, b) => (a.timestamp ?? 0).compareTo(b.timestamp ?? 0)); | ||
|
|
||
| int releaseIndex = -1; | ||
| for (var i = 0; i < sorted.length; i++) { | ||
| final action = sorted[i].action; | ||
| if (action == Action.release || | ||
| action == Action.released || | ||
| action == Action.holdInvoicePaymentSettled) { | ||
| releaseIndex = i; | ||
| } | ||
| } | ||
|
|
||
| for (var i = 0; i < sorted.length; i++) { | ||
| final action = sorted[i].action; | ||
| if (action == Action.paymentFailed) return true; | ||
| if (action == Action.addInvoice && i > releaseIndex) return true; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With Useful? React with 👍 / 👎. |
||
| } | ||
| return false; | ||
| } | ||
|
|
||
| /// Thrown when Mostro responds with cant-do: invalid_trade_index to | ||
| /// Action.lastTradeIndex during the restore flow. | ||
| class RestoreInvalidTradeIndexException implements Exception { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,115 @@ | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:mostro_mobile/data/models.dart'; | ||
| import 'package:mostro_mobile/data/enums.dart'; | ||
| import 'package:mostro_mobile/features/order/models/order_state.dart'; | ||
| import 'package:mostro_mobile/features/restore/restore_manager.dart'; | ||
|
|
||
| /// Regression coverage for issue #615: restoring an order that Mostro reports | ||
| /// as `settled-hold-invoice` after a failed Lightning payout must land on the | ||
| /// new-invoice prompt (`add-invoice` + `payment-failed`), not "paying sats" | ||
| /// (`released` + `settled-hold-invoice`). | ||
|
|
||
| MostroMessage<Order> _msg(Action action, {int? timestamp}) => | ||
| MostroMessage<Order>( | ||
| action: action, | ||
| id: 'order-1', | ||
| timestamp: timestamp, | ||
| payload: const Order( | ||
| id: 'order-1', | ||
| kind: OrderType.buy, | ||
| status: Status.settledHoldInvoice, | ||
| amount: 500, | ||
| fiatCode: 'USD', | ||
| fiatAmount: 50, | ||
| paymentMethod: 'Cash', | ||
| ), | ||
| ); | ||
|
|
||
| /// Replays a sequence of actions onto a fresh order state, mirroring how | ||
| /// RestoreService.restore() applies snapshot-derived messages via | ||
| /// OrderNotifier.updateStateFromMessage(). | ||
| OrderState _replay(List<Action> actions) { | ||
| OrderState state = OrderState( | ||
| action: Action.newOrder, | ||
| status: Status.pending, | ||
| order: null, | ||
| ); | ||
| for (final action in actions) { | ||
| state = state.updateWith(_msg(action)); | ||
| } | ||
| return state; | ||
| } | ||
|
|
||
| void main() { | ||
| group('restoreHasFailedPayoutSignal', () { | ||
| test('returns false for empty history', () { | ||
| expect(restoreHasFailedPayoutSignal([]), isFalse); | ||
| }); | ||
|
|
||
| test('returns true when payment-failed is present', () { | ||
| expect( | ||
| restoreHasFailedPayoutSignal([_msg(Action.paymentFailed, timestamp: 1)]), | ||
| isTrue, | ||
| ); | ||
| }); | ||
|
|
||
| test('returns true for a re-sent add-invoice (storage cleared on restore)', | ||
| () { | ||
| expect( | ||
| restoreHasFailedPayoutSignal([_msg(Action.addInvoice, timestamp: 1)]), | ||
| isTrue, | ||
| ); | ||
| }); | ||
|
|
||
| test('returns true when add-invoice arrives after the hold was released', | ||
| () { | ||
| expect( | ||
| restoreHasFailedPayoutSignal([ | ||
| _msg(Action.released, timestamp: 1), | ||
| _msg(Action.addInvoice, timestamp: 2), | ||
| ]), | ||
| isTrue, | ||
| ); | ||
| }); | ||
|
|
||
| test('returns false for an early add-invoice that precedes the release', () { | ||
| // Happy path where the daemon re-sends full history: the only add-invoice | ||
| // is the early waiting-buyer-invoice one, before the release. | ||
| expect( | ||
| restoreHasFailedPayoutSignal([ | ||
| _msg(Action.addInvoice, timestamp: 1), | ||
| _msg(Action.released, timestamp: 2), | ||
| ]), | ||
| isFalse, | ||
| ); | ||
| }); | ||
|
|
||
| test('returns false for a settled order with no failed-payout signal', () { | ||
| expect( | ||
| restoreHasFailedPayoutSignal([ | ||
| _msg(Action.holdInvoicePaymentSettled, timestamp: 1), | ||
| ]), | ||
| isFalse, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| group('restore state rebuild for settled-hold-invoice + buyer', () { | ||
| test( | ||
| 'failed payout replays to payment-failed + add-invoice (new-invoice prompt)', | ||
| () { | ||
| final state = _replay([Action.paymentFailed, Action.addInvoice]); | ||
|
|
||
| expect(state.status, equals(Status.paymentFailed)); | ||
| expect(state.action, equals(Action.addInvoice)); | ||
| }); | ||
|
|
||
| test('happy path replays to settled-hold-invoice + released (paying sats)', | ||
| () { | ||
| final state = _replay([Action.released]); | ||
|
|
||
| expect(state.status, equals(Status.settledHoldInvoice)); | ||
| expect(state.action, equals(Action.released)); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When historical messages were saved during the 10-second restore window, these synthetic
payment-failed/add-invoicemessages are stored with the order creation time, so the nextOrderNotifier.sync()(which replays storage sorted by timestamp) can replay older historical actions likereleasedafter the failed-payout pair and move the buyer back tosettled-hold-invoice. This makes the fix non-persistent across provider recreation/app restart; the replayed actions need timestamps after the restored history, e.g. after the max stored timestamp.Useful? React with 👍 / 👎.