-
Notifications
You must be signed in to change notification settings - Fork 7
fix(trades): handle takes left Canceled by the old optimistic cancel #448
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 |
|---|---|---|
|
|
@@ -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<Future<TradeRole?> 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<TradeInfo> trades, String orderId) { | ||
|
Member
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. ⚡ Performance | 🟡 Minor Every take-screen lookup now deserialises the user's whole trade history — twice per take.
Not blocking for the fix, but since the rule lives in Dart only because Rust has no "all rows for an order" query, consider a narrow bridge call instead (e.g. |
||
| 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< | ||
|
|
@@ -88,7 +122,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 +145,9 @@ final tradeUpdatesProvider = StreamProvider.autoDispose<TradeUpdate>(( | |
| } | ||
| }); | ||
|
|
||
| /// 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 +157,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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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<TradeDetailScreen> { | |
| /// 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<void> _cancelOrder(TradeStatus status) async { | ||
| final l10n = AppLocalizations.of(context); | ||
| final confirmed = await showDialog<bool>( | ||
|
|
@@ -207,8 +222,14 @@ class _TradeDetailScreenState extends ConsumerState<TradeDetailScreen> { | |
| 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<TradeDetailScreen> { | |
| 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)); | ||
|
Member
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. 🎯 Functional Correctness | 🟠 Major The leftover take can still flash
Nothing else holds the screen in The same The tests don't catch this because Suggested fix: don't let a live TradeStatus _status() {
final live = ref.watch(tradeStatusProvider(widget.orderId));
// …
if (!live.hasValue) return TradeStatus.loading;
final tradeAsync = ref.watch(tradeInfoProvider(widget.orderId));
// A public `pending` may be a take's leftover (#434): only the row can
// say, so hold `loading` rather than offer a Cancel the daemon refuses.
if (live.value == OrderStatus.pending && tradeAsync.isLoading) {
return TradeStatus.loading;
}
final status = tradeStatusFromOrderStatus(
_shown(live.value!, tradeAsync.valueOrNull),
);
// …
}and add a widget test where 🤖 Prompt for AI Agents |
||
| 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<TradeDetailScreen> { | |
| // 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<AsyncValue<OrderStatus>>(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(); | ||
| } | ||
|
|
||
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.
🎯 Functional Correctness | 🟠 Major
The role lookup now throws on a storage error; before it returned
null. The take screen doesn't handle that.The old implementation,
orders_api.getTradeRole, never failed:get_trade_rolelogs a DB error and returnsOk(None).orders_api.listTrades()propagates it (db.list_trades().await?), so this closure can now complete with an error.Both call sites in
take_order_screen.dartassume it can't:initStatecalls_redirectIfParticipant()unawaited with nocatch, so the error surfaces as an unhandled async exception (reported to the zone / Crashlytics in release)._onTakeOrderawaits it beforesetState(() => _cta = TakeOrderCta.loading)with notry. The future throws,_ctastaysidle, and the Take button silently does nothing — no snackbar, no retry hint.Suggested fix: keep the old contract at the seam (an unreadable store means "not a known participant", as
getTradeRoledid), and log it.Please add a take-screen test with a throwing lookup that asserts the take is still dispatched (or, if you prefer failing closed, that an error snackbar appears — but not a dead button).
🤖 Prompt for AI Agents