From 897bbba427a0547c0e3ea28ea7e690767f04b2b0 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Sat, 12 Sep 2026 21:48:17 -0600 Subject: [PATCH 1/4] fix(trades): a public pending is never a take's status tradeStatusProvider reads the order book first, and the book holds the order's public view. The trade screen showed that view as the trade's own status, so a take left Canceled by the old optimistic cancel (#434) read as the user's pending order once the daemon put the order back in the book, with a Cancel the daemon refuses (IsNotYourOrder). The My Trades list already kept an ended row's status, so the list said cancelled and the screen said pending. Both now go through one rule, shownTradeStatus. The row wins when it has ended, and when it is a take and the book says pending: a public pending means nobody holds the order, which also covers a take parked at WaitingTakerBond, whose order is still pending in public. Every other live status still wins over an open row. In the trade screen the rule applies wherever the live status was read: the status, the post-cancel decision, the cancel dialog's copy, and the step nudge, so a book change the row outranks is not a step. --- .../order/providers/trade_state_provider.dart | 34 ++++- .../trades/providers/trade_rows_provider.dart | 25 ++-- .../trades/screens/trade_detail_screen.dart | 35 ++++- .../004-mostro-p2p-client/contracts/orders.md | 14 ++ .../providers/trade_state_provider_test.dart | 60 +++++++++ .../providers/trade_rows_provider_test.dart | 23 ++++ .../trades/trade_detail_screen_test.dart | 127 ++++++++++++++++++ 7 files changed, 296 insertions(+), 22 deletions(-) diff --git a/lib/features/order/providers/trade_state_provider.dart b/lib/features/order/providers/trade_state_provider.dart index 811e8816a..78583e2f2 100644 --- a/lib/features/order/providers/trade_state_provider.dart +++ b/lib/features/order/providers/trade_state_provider.dart @@ -88,7 +88,7 @@ final tradeStatusProvider = StreamProvider.family final status = await lookup(orderId); if (status != null) { yield status; - if (_isTerminal(status)) return; + if (isTerminalTradeStatus(status)) return; } await Future.delayed(const Duration(seconds: 2)); } @@ -111,8 +111,9 @@ final tradeUpdatesProvider = StreamProvider.autoDispose(( } }); -/// Whether the UI can stop polling. Escrow settlement still awaits payout. -bool _isTerminal(OrderStatus s) => const { +/// Whether a trade in [s] has ended: nothing moves it out again, so the UI +/// can stop polling. Escrow settlement still awaits payout. +bool isTerminalTradeStatus(OrderStatus s) => const { OrderStatus.success, OrderStatus.settledByAdmin, OrderStatus.completedByAdmin, @@ -122,6 +123,33 @@ bool _isTerminal(OrderStatus s) => const { OrderStatus.canceledByAdmin, }.contains(s); +/// The status a trade shows: its [row]'s persisted one, or the [live] one +/// from [tradeStatusProvider], which reads the order book first. +/// +/// The row wins in two cases: +/// * **It has ended** ([isTerminalTradeStatus]). Whatever the book says about +/// the order later is no longer this trade. The one way such a row can be +/// wrong is the cancel's optimistic write on an active trade: a cooperative +/// cancel the peer never accepts, on a trade that then completes. +/// * **It is a take ([isTake]) and the book says `pending`.** A public +/// `pending` means nobody holds the order, so it is never a take's status. +/// Older builds marked a take `Canceled` as soon as its cancel went out, +/// even before it went active; once the daemon put the order back in the +/// book, that `pending` read as the user's own order, with a Cancel the +/// daemon refuses (`IsNotYourOrder`). A take parked at `WaitingTakerBond` +/// is another: publicly its order is still `pending`. +/// +/// Otherwise the live status, or the row's while there is none yet. +OrderStatus shownTradeStatus({ + required OrderStatus row, + required OrderStatus? live, + required bool isTake, +}) { + if (live == null || isTerminalTradeStatus(row)) return row; + if (isTake && live == OrderStatus.pending) return row; + return live; +} + /// Loads the buyer/seller role for a trade from the persistent DB. /// /// Returns `true` when the local user is the buyer, `false` for seller, or diff --git a/lib/features/trades/providers/trade_rows_provider.dart b/lib/features/trades/providers/trade_rows_provider.dart index b12138907..a63a52d40 100644 --- a/lib/features/trades/providers/trade_rows_provider.dart +++ b/lib/features/trades/providers/trade_rows_provider.dart @@ -34,7 +34,8 @@ class TradeRow { final String orderId; - /// Live when the order is still moving, else the persisted status. + /// Live when the order is still moving, else the persisted status — see + /// [shownTradeStatus], which the trade screen shares. final rust_types.OrderStatus status; final TradeRowState state; final bool isSelling; @@ -59,16 +60,6 @@ class TradeRow { final String? peerHandle; } -const _terminal = { - rust_types.OrderStatus.success, - rust_types.OrderStatus.settledByAdmin, - rust_types.OrderStatus.completedByAdmin, - rust_types.OrderStatus.canceled, - rust_types.OrderStatus.expired, - rust_types.OrderStatus.cooperativelyCanceled, - rust_types.OrderStatus.canceledByAdmin, -}; - const _successes = { rust_types.OrderStatus.success, rust_types.OrderStatus.settledByAdmin, @@ -89,10 +80,14 @@ final tradeRowsProvider = Provider>>((ref) { TradeRow _row(Ref ref, rust_types.TradeInfo trade, {required bool canRate}) { final order = trade.order; final persisted = order.status; - final status = - _terminal.contains(persisted) - ? persisted - : ref.watch(tradeStatusProvider(order.id)).valueOrNull ?? persisted; + final status = shownTradeStatus( + row: persisted, + live: + isTerminalTradeStatus(persisted) + ? null + : ref.watch(tradeStatusProvider(order.id)).valueOrNull, + isTake: !order.isMine, + ); final ratedByMe = trade.ratedAt != null || (_successes.contains(status) && ref.watch(ratedByMeProvider(order.id))); diff --git a/lib/features/trades/screens/trade_detail_screen.dart b/lib/features/trades/screens/trade_detail_screen.dart index 4abaaa761..07cbf4e70 100644 --- a/lib/features/trades/screens/trade_detail_screen.dart +++ b/lib/features/trades/screens/trade_detail_screen.dart @@ -36,6 +36,7 @@ import 'package:mostro/shared/widgets/mostro_reactive_button.dart'; import 'package:mostro/src/rust/api/disputes.dart' as disputes_api; import 'package:mostro/src/rust/api/orders.dart' as orders_api; import 'package:mostro/src/rust/api/reputation.dart' as reputation_api; +import 'package:mostro/src/rust/api/types.dart' show TradeInfo; export 'package:mostro/features/trades/models/trade_status.dart'; @@ -189,9 +190,23 @@ class _TradeDetailScreenState extends ConsumerState { /// await: the seller's payment can land while the cancel dialog is open. TradeStatus _liveStatus(TradeStatus fallback) { final live = ref.read(tradeStatusProvider(widget.orderId)).valueOrNull; - return live == null ? fallback : tradeStatusFromOrderStatus(live); + if (live == null) return fallback; + final trade = ref.read(tradeInfoProvider(widget.orderId)).valueOrNull; + return tradeStatusFromOrderStatus(_shown(live, trade)); } + /// What [live] shows for this trade once its [trade] row is read in + /// ([shownTradeStatus]); [live] itself while the row has not loaded, or + /// when there is none. + static OrderStatus _shown(OrderStatus live, TradeInfo? trade) => + trade == null + ? live + : shownTradeStatus( + row: trade.order.status, + live: live, + isTake: !trade.order.isMine, + ); + Future _cancelOrder(TradeStatus status) async { final l10n = AppLocalizations.of(context); final confirmed = await showDialog( @@ -207,8 +222,14 @@ class _TradeDetailScreenState extends ConsumerState { dialogRef .watch(tradeStatusProvider(widget.orderId)) .valueOrNull; + final trade = + dialogRef + .watch(tradeInfoProvider(widget.orderId)) + .valueOrNull; final now = - live == null ? status : tradeStatusFromOrderStatus(live); + live == null + ? status + : tradeStatusFromOrderStatus(_shown(live, trade)); return Text(_cancelDialogContent(l10n, now)); }, ), @@ -405,7 +426,8 @@ class _TradeDetailScreenState extends ConsumerState { debugPrint('[TradeDetailScreen] trade status failed: ${live.error}'); } if (!live.hasValue) return TradeStatus.loading; - final status = tradeStatusFromOrderStatus(live.value!); + final trade = ref.watch(tradeInfoProvider(widget.orderId)).valueOrNull; + final status = tradeStatusFromOrderStatus(_shown(live.value!, trade)); if (status != TradeStatus.pendingRating) return status; final rating = ref.watch(tradeRatingProvider(widget.orderId)); if (rating.isLoading && !rating.hasValue) return TradeStatus.loading; @@ -439,13 +461,18 @@ class _TradeDetailScreenState extends ConsumerState { // A step advanced by the relay: a nudge, the crossfades below, and a // fresh deadline — every step has its own expiration, owned by a // different party, so the clock loaded for the previous step is stale. + // Judged on what the screen shows: a book change the row outranks is + // not a step of this trade. ref.listen>(tradeStatusProvider(widget.orderId), ( previous, next, ) { + final trade = ref.read(tradeInfoProvider(widget.orderId)).valueOrNull; final before = previous?.valueOrNull; final after = next.valueOrNull; - if (before != null && after != null && before != after) { + if (before != null && + after != null && + _shown(before, trade) != _shown(after, trade)) { HapticFeedback.mediumImpact(); _loadExpiresAt(); } diff --git a/specs/004-mostro-p2p-client/contracts/orders.md b/specs/004-mostro-p2p-client/contracts/orders.md index 0a356ea16..f8d4148f2 100644 --- a/specs/004-mostro-p2p-client/contracts/orders.md +++ b/specs/004-mostro-p2p-client/contracts/orders.md @@ -749,3 +749,17 @@ The seller pay-invoice flow uses two complementary providers from advancing past the pay-invoice screen; the NWC widget's local `onPaymentSuccess` callback only flips a spinner flag and does not navigate. + +`tradeStatusProvider` reads the order book first, and the book holds the +order's public view. So the My Trades list and `TradeDetailScreen` show a +trade through `shownTradeStatus`, where the trade row wins in two cases: + +- **The row has ended** (success or a cancelled family): whatever the book + says about the order afterwards is no longer this trade. +- **A take, and the book says `pending`**: a public `pending` means nobody + holds the order, so it is never a take's status. That covers a take left + `Canceled` by builds that wrote the status before the daemon answered + (the daemon later put the order back in the book), and a take parked at + `WaitingTakerBond`, whose order is still `pending` in public. + +Every other live status wins over an open row. diff --git a/test/features/order/providers/trade_state_provider_test.dart b/test/features/order/providers/trade_state_provider_test.dart index cb44d0b44..327eed89a 100644 --- a/test/features/order/providers/trade_state_provider_test.dart +++ b/test/features/order/providers/trade_state_provider_test.dart @@ -47,4 +47,64 @@ void main() { expect(lookups, 2); expect(requestedOrders, ['order-payout', 'order-payout']); }); + + group('shownTradeStatus', () { + test('an ended row wins over any live status', () { + for (final row in OrderStatus.values.where(isTerminalTradeStatus)) { + for (final live in OrderStatus.values) { + for (final isTake in [true, false]) { + expect( + shownTradeStatus(row: row, live: live, isTake: isTake), + row, + reason: 'row=$row live=$live isTake=$isTake', + ); + } + } + } + }); + + test('a public pending never shows over a take that is still open', () { + final open = OrderStatus.values.where((s) => !isTerminalTradeStatus(s)); + for (final row in open) { + expect( + shownTradeStatus(row: row, live: OrderStatus.pending, isTake: true), + row, + reason: 'row=$row', + ); + // The maker's own order is `pending` for real. + expect( + shownTradeStatus(row: row, live: OrderStatus.pending, isTake: false), + OrderStatus.pending, + reason: 'row=$row', + ); + } + }); + + test('any other live status wins over an open row', () { + final open = OrderStatus.values.where((s) => !isTerminalTradeStatus(s)); + for (final row in open) { + for (final live in OrderStatus.values) { + if (live == OrderStatus.pending) continue; + for (final isTake in [true, false]) { + expect( + shownTradeStatus(row: row, live: live, isTake: isTake), + live, + reason: 'row=$row live=$live isTake=$isTake', + ); + } + } + } + }); + + test('the row stands in while there is no live status yet', () { + expect( + shownTradeStatus( + row: OrderStatus.waitingPayment, + live: null, + isTake: true, + ), + OrderStatus.waitingPayment, + ); + }); + }); } diff --git a/test/features/trades/providers/trade_rows_provider_test.dart b/test/features/trades/providers/trade_rows_provider_test.dart index 575e5ea62..ff6e3b0b8 100644 --- a/test/features/trades/providers/trade_rows_provider_test.dart +++ b/test/features/trades/providers/trade_rows_provider_test.dart @@ -69,6 +69,29 @@ void main() { expect((await _rows(c)).single.status, OrderStatus.canceled); }); + test('a take keeps its own status over a public pending', () async { + // A take parked at its bond: publicly the order is still `pending`. + final c = _container( + [fakeTrade(id: 'a', status: OrderStatus.waitingTakerBond)], + live: {'order-a': OrderStatus.pending}, + ); + expect((await _rows(c)).single.status, OrderStatus.waitingTakerBond); + }); + + test("a maker's pending order still follows the book", () async { + final c = _container( + [ + fakeTrade( + id: 'a', + status: OrderStatus.waitingBuyerInvoice, + isMine: true, + ), + ], + live: {'order-a': OrderStatus.pending}, + ); + expect((await _rows(c)).single.status, OrderStatus.pending); + }); + test('a durable rating marker closes a successful trade', () async { final rated = fakeTrade(id: 'a', status: OrderStatus.success); final c = _container([ diff --git a/test/features/trades/trade_detail_screen_test.dart b/test/features/trades/trade_detail_screen_test.dart index 609d58421..02debdc54 100644 --- a/test/features/trades/trade_detail_screen_test.dart +++ b/test/features/trades/trade_detail_screen_test.dart @@ -1,6 +1,7 @@ import 'dart:async'; import 'package:flutter/material.dart'; +import 'package:flutter/services.dart'; import 'package:flutter_localizations/flutter_localizations.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; @@ -1267,4 +1268,130 @@ void main() { ); }); }); + + group('the trade row outranks what the book says about the order', () { + const orderId = 'order-row'; + + testWidgets('a take left Canceled reads cancelled over a public pending', ( + tester, + ) async { + // Older builds marked a take Canceled as soon as its cancel went out; + // the daemon then put the order back in the book, where it reads + // `pending`. Shown as is, that was the user's own order with a Cancel + // the daemon refuses (IsNotYourOrder). + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.pending, + loadTrades: + () async => [fakeTrade(id: 'row', status: OrderStatus.canceled)], + book: [fakeOrder(id: orderId)], + ); + await _finishPageTransition(tester); + + expect(find.byType(TradeDetailScreen), findsOneWidget); + expect(find.text(_en.tradeHeadlineCancelled), findsOneWidget); + expect(find.text(_en.tradeHeadlinePending), findsNothing); + expect(_cancelButton(), findsNothing); + }); + + testWidgets('an ended take stays ended when someone else completes it', ( + tester, + ) async { + // The same leftover row, its order since taken and completed by + // someone else: the book's `success` is not this user's trade. The + // My Trades list reads it the same way. + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.success, + loadTrades: + () async => [fakeTrade(id: 'row', status: OrderStatus.canceled)], + ); + await _finishPageTransition(tester); + + expect(find.text(_en.tradeHeadlineCancelled), findsOneWidget); + expect(find.byType(TradeCompletedCard), findsNothing); + }); + + testWidgets('an open take keeps its own step over a public pending', ( + tester, + ) async { + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.pending, + loadTrades: + () async => [ + fakeTrade(id: 'row', status: OrderStatus.waitingPayment), + ], + book: [fakeOrder(id: orderId)], + ); + await _finishPageTransition(tester); + + expect(find.text(_en.tradeHeadlineWaitingPaymentBuyer), findsOneWidget); + expect(find.text(_en.tradeHeadlinePending), findsNothing); + }); + + testWidgets("a maker's order still reads pending from the book", ( + tester, + ) async { + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.pending, + loadTrades: + () async => [ + fakeTrade( + id: 'row', + status: OrderStatus.waitingPayment, + isMine: true, + ), + ], + book: [fakeOrder(id: orderId, isMine: true)], + ); + await _finishPageTransition(tester); + + expect(find.text(_en.tradeHeadlinePending), findsOneWidget); + }); + + testWidgets('a book change the row outranks is not a step: no nudge', ( + tester, + ) async { + final nudges = []; + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'HapticFeedback.vibrate') nudges.add('$call'); + return null; + }, + ); + addTearDown( + () => tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + null, + ), + ); + final book = StreamController(); + addTearDown(() => unawaited(book.close())); + book.add(OrderStatus.pending); + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.pending, + statusUpdates: book.stream, + loadTrades: + () async => [fakeTrade(id: 'row', status: OrderStatus.canceled)], + ); + await _finishPageTransition(tester); + + // Someone else takes the order: the book moves, this trade does not. + book.add(OrderStatus.inProgress); + await tester.pump(); + await tester.pump(); + + expect(nudges, isEmpty); + expect(find.text(_en.tradeHeadlineCancelled), findsOneWidget); + }); + }); } From c0aa804e5a2476ce82b7b54787f1b20be61b65d2 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Sat, 12 Sep 2026 22:55:11 -0600 Subject: [PATCH 2/4] fix(order): a take that already ended no longer blocks taking the order The take screen sends a user who already takes part in the order to the trade instead of offering to take it again, and it counted any trade row on the order as taking part. A take left Canceled by the old optimistic cancel (#434) therefore held the order forever: once the daemon put it back in the book, opening it landed on that dead trade, and the take button did the same, so the user could never take it again. tradeRoleLookupProvider now answers through participatingRole: a take whose row has ended does not count. Once its order can be taken again, such a row can only be what a take that never went active left behind, since an order whose trade truly ended never goes back to Pending, and take_order already replaces it with the new take's row. A maker's row always counts, and so does any row still open. Every row of the order is read, because a database from before takes replaced their order's earlier row can hold two. --- .../order/providers/trade_state_provider.dart | 42 ++++++++- .../004-mostro-p2p-client/contracts/orders.md | 7 ++ .../providers/trade_state_provider_test.dart | 70 +++++++++++++++ .../order/screens/take_order_screen_test.dart | 88 +++++++++++++++++-- test/support/fake_trades.dart | 6 +- 5 files changed, 202 insertions(+), 11 deletions(-) diff --git a/lib/features/order/providers/trade_state_provider.dart b/lib/features/order/providers/trade_state_provider.dart index 78583e2f2..e23b4fa28 100644 --- a/lib/features/order/providers/trade_state_provider.dart +++ b/lib/features/order/providers/trade_state_provider.dart @@ -41,13 +41,47 @@ final tradeStatusLookupProvider = }, ); -/// Reads the local user's role in a trade through the bridge; injectable so -/// screens that must know whether they already participate can be tested -/// without the Rust side. +/// The local user's role in an order they still take part in +/// ([participatingRole]), read through the bridge; injectable so screens that +/// must know whether they already participate can be tested without the Rust +/// side. final tradeRoleLookupProvider = Provider Function(String)>( - (ref) => (orderId) => orders_api.getTradeRole(orderId: orderId), + (ref) => + (orderId) async => + participatingRole(await orders_api.listTrades(), orderId), ); +/// The role of the user's trade on [orderId] among [trades], or null when +/// they no longer take part in it: no row at all, or only a take that has +/// ended ([isEndedTake]). +/// +/// Every row for the order is read, not just one: a database from before +/// takes replaced their order's earlier row can hold two, and a live one +/// among them still makes the user a participant. +TradeRole? participatingRole(Iterable trades, String orderId) { + for (final trade in trades) { + if (trade.order.id == orderId && !isEndedTake(trade)) return trade.role; + } + return null; +} + +/// Whether [trade] is a take whose row has ended, which leaves the user +/// nothing to follow on its order. +/// +/// A trade that truly ended leaves its order in a status mostrod never takes +/// it out of: a take needs `Pending`, and only a waiting state goes back to +/// it. So once the order can be taken again, such a row is what a take that +/// never went active left behind: older builds marked it `Canceled` as soon +/// as its cancel went out. Holding on to it sent the user to that dead trade +/// instead of letting them take the order again (#434). Rust already takes +/// over such a row: the confirmed take replaces every earlier row of its +/// order. +/// +/// Never a maker's row: its order is theirs, and the take screen is not +/// where they manage it. +bool isEndedTake(TradeInfo trade) => + !trade.order.isMine && isTerminalTradeStatus(trade.order.status); + /// Takes an order through the bridge; injectable so the take screen's /// outcomes (loading, already taken, rejected) can be tested without Rust. final takeOrderActionProvider = Provider< diff --git a/specs/004-mostro-p2p-client/contracts/orders.md b/specs/004-mostro-p2p-client/contracts/orders.md index f8d4148f2..f65ffce6b 100644 --- a/specs/004-mostro-p2p-client/contracts/orders.md +++ b/specs/004-mostro-p2p-client/contracts/orders.md @@ -763,3 +763,10 @@ trade through `shownTradeStatus`, where the trade row wins in two cases: `WaitingTakerBond`, whose order is still `pending` in public. Every other live status wins over an open row. + +The take screen sends a user who already takes part in the order to the +trade instead of offering to take it again (`tradeRoleLookupProvider`, +`participatingRole`). A take whose row has ended does not count: once its +order can be taken again, such a row can only be what a take that never +went active left behind, and `take_order` replaces it with the new take's +row. A maker's row always counts, and so does any row still open. diff --git a/test/features/order/providers/trade_state_provider_test.dart b/test/features/order/providers/trade_state_provider_test.dart index 327eed89a..cd0e5be8a 100644 --- a/test/features/order/providers/trade_state_provider_test.dart +++ b/test/features/order/providers/trade_state_provider_test.dart @@ -4,6 +4,7 @@ import 'package:flutter_test/flutter_test.dart'; import 'package:mostro/features/order/providers/trade_state_provider.dart'; import 'package:mostro/src/rust/api/types.dart'; +import '../../../support/fake_trades.dart'; import '../../../support/provider_harness.dart'; void main() { @@ -107,4 +108,73 @@ void main() { ); }); }); + + group('participatingRole', () { + const orderId = 'order-x'; + + test('no row on the order is no role', () { + expect(participatingRole(const [], orderId), isNull); + expect( + participatingRole([fakeTrade(id: 'other')], orderId), + isNull, + reason: 'another order\'s row', + ); + }); + + test('a take still open is a participant', () { + for (final status in OrderStatus.values.where( + (s) => !isTerminalTradeStatus(s), + )) { + expect( + participatingRole([ + fakeTrade( + id: 'x', + orderId: orderId, + status: status, + role: TradeRole.seller, + ), + ], orderId), + TradeRole.seller, + reason: '$status', + ); + } + }); + + test('a take that has ended is not', () { + for (final status in OrderStatus.values.where(isTerminalTradeStatus)) { + final ended = fakeTrade(id: 'x', orderId: orderId, status: status); + expect(isEndedTake(ended), isTrue, reason: '$status'); + expect(participatingRole([ended], orderId), isNull, reason: '$status'); + } + }); + + test("a maker's row always is, ended or not", () { + final maker = fakeTrade( + id: 'x', + orderId: orderId, + status: OrderStatus.canceled, + isMine: true, + ); + expect(isEndedTake(maker), isFalse); + expect(participatingRole([maker], orderId), TradeRole.buyer); + }); + + test('an open row wins over an ended one on the same order', () { + // Databases from before a take replaced its order's earlier row can + // hold both, in either order. + final ended = fakeTrade( + id: 'ended', + orderId: orderId, + status: OrderStatus.canceled, + ); + final open = fakeTrade( + id: 'open', + orderId: orderId, + status: OrderStatus.active, + role: TradeRole.seller, + ); + expect(participatingRole([ended, open], orderId), TradeRole.seller); + expect(participatingRole([open, ended], orderId), TradeRole.seller); + }); + }); } diff --git a/test/features/order/screens/take_order_screen_test.dart b/test/features/order/screens/take_order_screen_test.dart index ebd89c409..07bb015d5 100644 --- a/test/features/order/screens/take_order_screen_test.dart +++ b/test/features/order/screens/take_order_screen_test.dart @@ -4,6 +4,8 @@ import 'package:clock/clock.dart'; import 'package:flutter/material.dart'; import 'package:flutter_riverpod/flutter_riverpod.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:go_router/go_router.dart'; +import 'package:mostro/core/app_routes.dart'; import 'package:mostro/core/app_theme.dart'; import 'package:mostro/core/automation/automation_id.dart'; import 'package:mostro/core/automation/automation_ids.dart'; @@ -18,6 +20,7 @@ import 'package:mostro/src/rust/api/types.dart'; import 'package:mostro/shared/utils/fiat_currencies.dart'; import '../../../support/fake_orders.dart'; +import '../../../support/fake_trades.dart'; import '../../../support/provider_harness.dart'; const _id = '09150348-1a2b-4c3d-8e9f-0a1b2c3d99b5'; @@ -25,8 +28,9 @@ const _dark = OrderDetailPalette.dark; const _book = OrderBookPalette.dark; /// Pumps the take-order screen over a book fed by [books], with the node's -/// rate and the (absent) trade role stubbed. 1 000 ARS is 1 000 sats at -/// the stubbed rate, so the estimates are easy to read. +/// rate stubbed and the user's trade rows being [trades] (none by default). +/// 1 000 ARS is 1 000 sats at the stubbed rate, so the estimates are easy to +/// read. typedef _Take = Future Function({ required String orderId, required TradeRole role, @@ -39,6 +43,7 @@ Future>> _pump( bool isBuying = true, double? rate = 100000000, _Take? take, + List trades = const [], }) async { tester.view.physicalSize = const Size(360, 760); tester.view.devicePixelRatio = 1.0; @@ -51,7 +56,9 @@ Future>> _pump( yield [order]; yield* books.stream; }), - tradeRoleLookupProvider.overrideWithValue((_) async => null), + tradeRoleLookupProvider.overrideWithValue( + (orderId) async => participatingRole(trades, orderId), + ), if (take != null) takeOrderActionProvider.overrideWithValue(take), exchangeRateProvider.overrideWith((ref, code) async => rate), fiatCurrenciesProvider.overrideWith( @@ -61,15 +68,32 @@ Future>> _pump( ), ], ); + // Under a router, so a redirect to the trade is observable: it lands on a + // stand-in reading `trade`. + final router = GoRouter( + initialLocation: + isBuying ? AppRoute.takeSellPath(_id) : AppRoute.takeBuyPath(_id), + routes: [ + GoRoute( + path: isBuying ? AppRoute.takeSell : AppRoute.takeBuy, + builder: (_, __) => TakeOrderScreen(orderId: _id, isBuying: isBuying), + ), + GoRoute( + path: AppRoute.tradeDetail, + builder: (_, __) => const Scaffold(body: Text('trade')), + ), + ], + ); + addTearDown(router.dispose); await tester.pumpWidget( UncontrolledProviderScope( container: container, - child: MaterialApp( + child: MaterialApp.router( theme: buildDarkTheme(), locale: const Locale('en'), localizationsDelegates: AppLocalizations.localizationsDelegates, supportedLocales: AppLocalizations.supportedLocales, - home: TakeOrderScreen(orderId: _id, isBuying: isBuying), + routerConfig: router, ), ), ); @@ -349,4 +373,58 @@ void main() { }); }); }); + + group('TakeOrderScreen and a trade the user already has on the order', () { + testWidgets('a take still open on the order lands on its trade', ( + tester, + ) async { + await withClock(Clock.fixed(kFakeNow), () async { + await _pump( + tester, + order: _order(), + trades: [ + fakeTrade( + id: 'open', + orderId: _id, + status: OrderStatus.waitingBuyerInvoice, + ), + ], + ); + + expect(find.byType(TakeOrderScreen), findsNothing); + expect(find.text('trade'), findsOneWidget); + }); + }); + + testWidgets('a take that already ended leaves the order takeable', ( + tester, + ) async { + // What older builds left behind: a take cancelled before it went + // active, marked Canceled, its order since back in the book. Sent to + // that dead trade, the user could never take the order again (#434). + await withClock(Clock.fixed(kFakeNow), () async { + final taken = []; + await _pump( + tester, + order: _order(), + trades: [ + fakeTrade(id: 'ended', orderId: _id, status: OrderStatus.canceled), + ], + take: ({required orderId, required role, fiatAmount}) { + taken.add(orderId); + // Left unanswered: only the dispatch is under test. + return Completer().future; + }, + ); + expect(find.byType(TakeOrderScreen), findsOneWidget); + + await tester.tap(find.text('Take order')); + await tester.pump(); + await tester.pump(); + + expect(taken, [_id]); + expect(find.text('trade'), findsNothing); + }); + }); + }); } diff --git a/test/support/fake_trades.dart b/test/support/fake_trades.dart index 87c24c882..ddabd7306 100644 --- a/test/support/fake_trades.dart +++ b/test/support/fake_trades.dart @@ -2,9 +2,11 @@ import 'package:mostro/src/rust/api/types.dart'; /// Builds a [TradeInfo] exposing only the fields the trades-list mapping reads. /// [startedAt] is the newest-first sort key. `currentStep` is unread by the -/// mapping, so it takes an arbitrary value. +/// mapping, so it takes an arbitrary value. The order id is `order-$id` +/// unless [orderId] names one. TradeInfo fakeTrade({ String id = 'trade-1', + String? orderId, OrderStatus status = OrderStatus.active, TradeRole role = TradeRole.buyer, String fiatCode = 'USD', @@ -18,7 +20,7 @@ TradeInfo fakeTrade({ int? peerDays, }) { final order = OrderInfo( - id: 'order-$id', + id: orderId ?? 'order-$id', kind: OrderKind.sell, status: status, amountSats: amountSats, From 5e8e5d2d901caa29b07ea5be903f207704e110ca Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:02:56 -0600 Subject: [PATCH 3/4] fix(trades): hold the trade screen until the row can judge a pending MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The row decides whether a public pending is the trade's status or a leftover take's (#434), but it arrives later than everything else the screen waits for: it is a full trades read, while the role comes from an indexed lookup. Until then _shown had no row and let the book's pending through, so a leftover take opened cold — from a notification, the chat header, or after a restart — still rendered the maker's view for a few frames, Cancel included. That is the cancel the daemon refuses with IsNotYourOrder. The screen now holds loading while a live pending has no row yet, and the step nudge stays quiet for the same reason. Only the first read waits: the trades cache is invalidated on every trade update, and a refresh keeps the previous row, so waiting for those would flash loading on every update. --- .../trades/screens/trade_detail_screen.dart | 22 ++++++- .../trades/trade_detail_screen_test.dart | 63 +++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/lib/features/trades/screens/trade_detail_screen.dart b/lib/features/trades/screens/trade_detail_screen.dart index 329c8b9f4..0013a481e 100644 --- a/lib/features/trades/screens/trade_detail_screen.dart +++ b/lib/features/trades/screens/trade_detail_screen.dart @@ -475,8 +475,20 @@ class _TradeDetailScreenState extends ConsumerState { debugPrint('[TradeDetailScreen] trade status failed: ${live.error}'); } if (!live.hasValue) return TradeStatus.loading; - final trade = ref.watch(tradeInfoProvider(widget.orderId)).valueOrNull; - final status = tradeStatusFromOrderStatus(_shown(live.value!, trade)); + final tradeAsync = ref.watch(tradeInfoProvider(widget.orderId)); + // A public `pending` may be a take's leftover, and only the row tells + // them apart (#434). Until it answers, hold `loading` rather than offer + // the maker's view with a Cancel the daemon refuses. First read only: a + // refresh keeps the previous row, and the trades cache is invalidated on + // every trade update, so waiting for those would flash `loading`. + if (live.value == OrderStatus.pending && + tradeAsync.isLoading && + !tradeAsync.hasValue) { + return TradeStatus.loading; + } + final status = tradeStatusFromOrderStatus( + _shown(live.value!, tradeAsync.valueOrNull), + ); if (status != TradeStatus.pendingRating) return status; final rating = ref.watch(tradeRatingProvider(widget.orderId)); if (rating.isLoading && !rating.hasValue) return TradeStatus.loading; @@ -516,7 +528,11 @@ class _TradeDetailScreenState extends ConsumerState { previous, next, ) { - final trade = ref.read(tradeInfoProvider(widget.orderId)).valueOrNull; + final row = ref.read(tradeInfoProvider(widget.orderId)); + // Without the row a public `pending` cannot be told from the trade's + // own status, so a change into or out of it says nothing yet. + if (row.isLoading && !row.hasValue) return; + final trade = row.valueOrNull; final before = previous?.valueOrNull; final after = next.valueOrNull; if (before != null && diff --git a/test/features/trades/trade_detail_screen_test.dart b/test/features/trades/trade_detail_screen_test.dart index 3e4d25b83..1a548a9d1 100644 --- a/test/features/trades/trade_detail_screen_test.dart +++ b/test/features/trades/trade_detail_screen_test.dart @@ -1376,6 +1376,69 @@ void main() { expect(_cancelButton(), findsNothing); }); + testWidgets('a public pending waits for the row before it is shown', ( + tester, + ) async { + // The row is a full trades read, while the role comes from an indexed + // lookup, so opening a leftover take cold (a notification, the chat + // header, a restart) resolves the role first. Shown as is, the book's + // `pending` offered the maker's view with a Cancel the daemon refuses. + final trades = Completer>(); + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.pending, + loadTrades: () => trades.future, + book: [fakeOrder(id: orderId)], + ); + await _finishPageTransition(tester); + + expect(find.text(_en.tradeHeadlinePending), findsNothing); + expect(_cancelButton(), findsNothing); + + trades.complete([fakeTrade(id: 'row', status: OrderStatus.canceled)]); + await _finishPageTransition(tester); + + expect(find.text(_en.tradeHeadlineCancelled), findsOneWidget); + expect(_cancelButton(), findsNothing); + }); + + testWidgets('no nudge either while the row is still loading', ( + tester, + ) async { + final nudges = []; + tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + (call) async { + if (call.method == 'HapticFeedback.vibrate') nudges.add('$call'); + return null; + }, + ); + addTearDown( + () => tester.binding.defaultBinaryMessenger.setMockMethodCallHandler( + SystemChannels.platform, + null, + ), + ); + final book = StreamController(); + addTearDown(() => unawaited(book.close())); + book.add(OrderStatus.pending); + await _pumpRoutedTradeDetail( + tester, + orderId: orderId, + status: OrderStatus.pending, + statusUpdates: book.stream, + loadTrades: () => Completer>().future, + ); + await _finishPageTransition(tester); + + book.add(OrderStatus.inProgress); + await tester.pump(); + await tester.pump(); + + expect(nudges, isEmpty); + }); + testWidgets('an ended take stays ended when someone else completes it', ( tester, ) async { From f88ecdb6e8a91b613961e5715c859e9b42f68a24 Mon Sep 17 00:00:00 2001 From: Catrya <140891948+Catrya@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:16:37 -0600 Subject: [PATCH 4/4] fix(order): reading the trades cannot strand the take screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The role lookup moved from get_trade_role, which logs a database error and answers None, to listTrades, which propagates it. The take screen assumes it never fails: initState calls it unawaited and without a catch, and the Take button awaits it before leaving its idle state, so a thrown future meant an unhandled async error and a button that did nothing. It now answers null when the rows cannot be read, keeping the contract the bridge call had: an unreadable store is no proof of participation, and a take the user does hold is still refused by the daemon, which the screen reports. The read goes through tradeListReaderProvider, the injectable reader main added. The take screen's tests override that instead of the role lookup, so they exercise the real composition — the rule and its error handling — rather than a copy of it. --- .../order/providers/trade_state_provider.dart | 31 ++++++++++---- .../order/screens/take_order_screen_test.dart | 41 ++++++++++++++++--- 2 files changed, 59 insertions(+), 13 deletions(-) diff --git a/lib/features/order/providers/trade_state_provider.dart b/lib/features/order/providers/trade_state_provider.dart index 906039908..99134f263 100644 --- a/lib/features/order/providers/trade_state_provider.dart +++ b/lib/features/order/providers/trade_state_provider.dart @@ -132,14 +132,29 @@ final tradeStatusLookupProvider = ); /// The local user's role in an order they still take part in -/// ([participatingRole]), read through the bridge; injectable so screens that -/// must know whether they already participate can be tested without the Rust -/// side. -final tradeRoleLookupProvider = Provider Function(String)>( - (ref) => - (orderId) async => - participatingRole(await orders_api.listTrades(), orderId), -); +/// ([participatingRole]), over the rows [tradeListReaderProvider] reads. +/// Screens that must know whether they already participate override that +/// reader, so this composition is what their tests exercise. +/// +/// Null when the rows cannot be read at all, as the `get_trade_role` bridge +/// call this replaced did with a database error: an unreadable store is no +/// proof of participation, and the take screen calls this where a thrown +/// future would strand it — unawaited in `initState`, and before the Take +/// button leaves its idle state. A take the user does hold is still refused +/// by the daemon, which the screen reports. +final tradeRoleLookupProvider = Provider Function(String)>(( + ref, +) { + final readTrades = ref.watch(tradeListReaderProvider); + return (orderId) async { + try { + return participatingRole(await readTrades(), orderId); + } catch (e, st) { + debugPrint('[tradeRoleLookup] reading the trades failed: $e\n$st'); + return null; + } + }; +}); /// The role of the user's trade on [orderId] among [trades], or null when /// they no longer take part in it: no row at all, or only a take that has diff --git a/test/features/order/screens/take_order_screen_test.dart b/test/features/order/screens/take_order_screen_test.dart index 46f77550e..b48bc72dd 100644 --- a/test/features/order/screens/take_order_screen_test.dart +++ b/test/features/order/screens/take_order_screen_test.dart @@ -31,9 +31,10 @@ const _dark = OrderDetailPalette.dark; const _book = OrderBookPalette.dark; /// Pumps the take-order screen over a book fed by [books], with the node's -/// rate stubbed and the user's trade rows being [trades] (none by default). -/// 1 000 ARS is 1 000 sats at the stubbed rate, so the estimates are easy to -/// read. +/// rate stubbed and the user's trade rows being [trades] (none by default), +/// or whatever [readTrades] answers — the screen reads them through the real +/// `tradeRoleLookupProvider`. 1 000 ARS is 1 000 sats at the stubbed rate, so +/// the estimates are easy to read. typedef _Take = Future Function({ required String orderId, @@ -48,6 +49,7 @@ Future>> _pump( double? rate = 100000000, _Take? take, List trades = const [], + Future> Function()? readTrades, instance.MostroInstance? node, int? bondEstimate, }) async { @@ -62,8 +64,8 @@ Future>> _pump( yield [order]; yield* books.stream; }), - tradeRoleLookupProvider.overrideWithValue( - (orderId) async => participatingRole(trades, orderId), + tradeListReaderProvider.overrideWithValue( + readTrades ?? () async => trades, ), if (take != null) takeOrderActionProvider.overrideWithValue(take), exchangeRateProvider.overrideWith((ref, code) async => rate), @@ -482,5 +484,34 @@ void main() { expect(find.text('trade'), findsNothing); }); }); + + testWidgets('an unreadable trade store still lets the take go out', ( + tester, + ) async { + // Reading the rows can fail where the bridge call it replaced could + // not: unawaited in initState, and before the button leaves idle. An + // unreadable store is no proof of participation, and a take the user + // does hold is refused by the daemon and reported. + await withClock(Clock.fixed(kFakeNow), () async { + final taken = []; + await _pump( + tester, + order: _order(), + readTrades: () async => throw Exception('storage is unreadable'), + take: ({required orderId, required role, fiatAmount}) { + taken.add(orderId); + return Completer().future; + }, + ); + expect(find.byType(TakeOrderScreen), findsOneWidget); + + await tester.tap(find.text('Take order')); + await tester.pump(); + await tester.pump(); + + expect(taken, [_id]); + expect(find.text('Taking…'), findsOneWidget); + }); + }); }); }