-
Notifications
You must be signed in to change notification settings - Fork 29
fix: order message history by event time and drop late setup messages #723
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
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 |
|---|---|---|
|
|
@@ -232,6 +232,22 @@ class OrderState { | |
| // DEBUG: Log status mapping | ||
| logger.d('Status mapping: $effectiveAction → $newStatus'); | ||
|
|
||
| // A message that would move the order back to a phase it already left is | ||
| // a late delivery, not a transition: relays replay pending events | ||
| // newest-first, decryption is concurrent and the wire `created_at` has | ||
| // one-second resolution, so `waiting-seller-to-pay` can be applied after | ||
| // `hold-invoice-payment-accepted`, or `fiat-sent-ok` after `released`. | ||
| // Taking it would land on a phase whose action table has no fiat-sent, | ||
| // release or chat button, which is how trades looked stuck until a | ||
| // dispute reset the row. | ||
| if (isStaleTransition(effectiveAction, newStatus)) { | ||
| logger.w( | ||
| 'Ignoring late ${message.action} for order ${message.id}: ' | ||
| 'it would move the order from $status back to $newStatus', | ||
| ); | ||
| return this; | ||
| } | ||
|
|
||
| // A pending order has no counterpart: when Mostro republishes it after the | ||
| // taker times out, the previous taker's snapshot must not be carried into | ||
| // the next take and shown as if it belonged to whoever takes it next. | ||
|
|
@@ -402,6 +418,48 @@ class OrderState { | |
| return newState; | ||
| } | ||
|
|
||
| /// Position of a status along the trade lifecycle. Statuses that can | ||
| /// legitimately follow each other in either direction share a rank: a | ||
| /// cooperative cancel can be started from fiat-sent or during a dispute and | ||
| /// the trade can still reach fiat-sent while the cancel is pending; a | ||
| /// payout can fail and be retried while the hold invoice stays settled. | ||
| /// Every terminal status sits at the top so nothing reopens it. | ||
| static int phaseRank(Status status) => switch (status) { | ||
| Status.pending => 0, | ||
| Status.waitingTakerBond || | ||
| Status.waitingPayment || | ||
| Status.waitingBuyerInvoice || | ||
| Status.inProgress => | ||
| 1, | ||
| Status.active => 2, | ||
| Status.fiatSent || Status.cooperativelyCanceled || Status.dispute => 3, | ||
|
Contributor
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 | ⚡ Quick win Separate A delayed Model the permitted equal-rank transitions explicitly, or add a directed guard for 🤖 Prompt for AI Agents
Member
Author
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. Confirmed, but deferred to #724 rather than fixed in this PR. The finding is valid. Reproduced against this branch: Both statuses sit at rank 3 and We disagree on severity — this is Low, not Major. The dispute resolution flow is unaffected. Running the full admin path on top of the corrupted state: Two reasons: the evidence for accepting One correction to the suggested remedy. Modelling equal-rank transitions generically would over-fix: two of the three rank-3 moves are legitimate and must keep working — Deferring because the impact is cosmetic and this app is expected to be superseded before the churn on
Contributor
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.
I agree that the impact is Low. The late I also agree that a generic equal-rank rule would be incorrect. The follow-up should use a directed guard that prevents transitions out of Deferring this change to 🐇 ✏️ Learnings added
You are interacting with an AI system. |
||
| Status.settledHoldInvoice || Status.paymentFailed => 4, | ||
| Status.success => 5, | ||
| Status.canceled || | ||
| Status.canceledByAdmin || | ||
| Status.settledByAdmin || | ||
| Status.completedByAdmin || | ||
| Status.expired => | ||
| 6, | ||
| }; | ||
|
|
||
| /// Whether applying [action] with [newStatus] would move the order back to | ||
| /// a phase it already left. | ||
| /// | ||
| /// The one backwards move the protocol makes is the republish after a taker | ||
| /// timeout: Mostro sends `new-order` with a pending payload to the maker | ||
| /// while the order waits for the taker's invoice or payment. Nothing else | ||
| /// goes backwards, so any other such message is a late copy. | ||
| bool isStaleTransition(Action action, Status newStatus) { | ||
| final current = phaseRank(status); | ||
| final next = phaseRank(newStatus); | ||
| if (next >= current) return false; | ||
| final isRepublish = action == Action.newOrder && | ||
| newStatus == Status.pending && | ||
| current <= phaseRank(Status.waitingPayment); | ||
| return !isRepublish; | ||
| } | ||
|
|
||
| /// Maps actions to their corresponding statuses based on mostrod DM messages | ||
| Status _getStatusFromAction(Action action, Status? payloadStatus) { | ||
| switch (action) { | ||
|
|
||
| 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/enums/action.dart'; | ||
| import 'package:mostro_mobile/data/models/enums/order_type.dart'; | ||
| import 'package:mostro_mobile/data/models/enums/status.dart'; | ||
| import 'package:mostro_mobile/data/models/order.dart'; | ||
| import 'package:mostro_mobile/data/models/mostro_message.dart'; | ||
| import 'package:mostro_mobile/data/repositories/mostro_storage.dart'; | ||
| import 'package:sembast/sembast_memory.dart'; | ||
|
|
||
| /// The history used to be ordered by the local receive time stamped at write. | ||
| /// Relays replay pending events newest-first and the decrypt pipeline is | ||
| /// concurrent, so an earlier message could be written after a later one and | ||
| /// become the "latest" message the order state is derived from. | ||
| void main() { | ||
| late MostroStorage storage; | ||
|
|
||
| setUp(() async { | ||
| final db = | ||
| await newDatabaseFactoryMemory().openDatabase('event_time_test.db'); | ||
| storage = MostroStorage(db: db); | ||
| }); | ||
|
|
||
| MostroMessage message(Action action, {int? eventCreatedAt}) => MostroMessage( | ||
| action: action, | ||
| id: 'order-a', | ||
| eventCreatedAt: eventCreatedAt, | ||
| ); | ||
|
|
||
| test('the latest message follows the event time, not the write order', | ||
| () async { | ||
| // Arrange: the newer event is decrypted and written first. | ||
| await storage.addMessage( | ||
| 'newer', | ||
| message(Action.holdInvoicePaymentAccepted, eventCreatedAt: 2000), | ||
| ); | ||
| await storage.addMessage( | ||
| 'older', | ||
| message(Action.waitingSellerToPay, eventCreatedAt: 1000), | ||
| ); | ||
|
|
||
| // Act | ||
| final history = await storage.getAllMessagesForOrderId('order-a'); | ||
| final latest = await storage.getLatestMessageById('order-a'); | ||
|
|
||
| // Assert | ||
| expect(history.map((m) => m.action), | ||
| [Action.holdInvoicePaymentAccepted, Action.waitingSellerToPay]); | ||
| expect(latest?.action, Action.holdInvoicePaymentAccepted); | ||
| }); | ||
|
|
||
| test('legacy rows without an event time keep their receive-time order', | ||
| () async { | ||
| final first = message(Action.newOrder)..timestamp = 1000; | ||
| final second = message(Action.payInvoice)..timestamp = 2000; | ||
| await storage.addMessage('k1', first); | ||
| await storage.addMessage('k2', second); | ||
|
|
||
| final history = await storage.getAllMessagesForOrderId('order-a'); | ||
|
|
||
| expect(history.map((m) => m.action), [Action.payInvoice, Action.newOrder]); | ||
| }); | ||
|
|
||
| _typedLookup(); | ||
|
|
||
| test('the event time survives a round trip through the database', () async { | ||
| await storage.addMessage( | ||
| 'k1', | ||
| message(Action.waitingSellerToPay, eventCreatedAt: 1234), | ||
| ); | ||
| final reopened = MostroStorage(db: storage.db); | ||
|
|
||
| final history = await reopened.getAllMessagesForOrderId('order-a'); | ||
|
|
||
| expect(history.single.eventCreatedAt, 1234); | ||
| }); | ||
| } | ||
|
|
||
| void _typedLookup() { | ||
| test('the typed latest lookup follows the event time too', () async { | ||
| final db = | ||
| await newDatabaseFactoryMemory().openDatabase('typed_lookup_test.db'); | ||
| final storage = MostroStorage(db: db); | ||
| Order order(Status status) => Order( | ||
| id: 'order-a', | ||
| kind: OrderType.sell, | ||
| status: status, | ||
| fiatCode: 'VES', | ||
| fiatAmount: 100, | ||
| paymentMethod: 'face to face', | ||
| ); | ||
|
|
||
| await storage.addMessage( | ||
| 'newer', | ||
| MostroMessage<Order>( | ||
| action: Action.holdInvoicePaymentAccepted, | ||
| id: 'order-a', | ||
| eventCreatedAt: 2000, | ||
| payload: order(Status.active), | ||
| ), | ||
| ); | ||
| await storage.addMessage( | ||
| 'older', | ||
| MostroMessage<Order>( | ||
| action: Action.waitingSellerToPay, | ||
| id: 'order-a', | ||
| eventCreatedAt: 1000, | ||
| payload: order(Status.waitingPayment), | ||
| ), | ||
| ); | ||
|
|
||
| final latest = await storage.getLatestMessageOfTypeById<Order>('order-a'); | ||
|
|
||
| expect(latest?.getPayload<Order>()?.status, Status.active); | ||
| }); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.