Conversation
Adds the 'initiator' field to the `RestoredDispute` model to indicate which party initiated the dispute, simplifying the dispute initiation detection logic and removing the need for trade index comparison.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdded an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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.
🧹 Nitpick comments (2)
lib/data/models/restore_response.dart (1)
63-94: Normalize/validateinitiatorduring deserialization.Using raw strings makes downstream checks fragile if the protocol ever changes casing or sends unexpected values; consider lowercasing and allow‑listing buyer/seller (or mapping to an enum) so unknown values become
nullconsistently.♻️ Suggested adjustment
factory RestoredDispute.fromJson(Map<String, dynamic> json) { + final rawInitiator = (json['initiator'] as String?)?.toLowerCase(); + final initiator = (rawInitiator == 'buyer' || rawInitiator == 'seller') + ? rawInitiator + : null; return RestoredDispute( disputeId: json['dispute_id'] as String, orderId: json['order_id'] as String, tradeIndex: json['trade_index'] as int, status: json['status'] as String, - initiator: json['initiator'] as String?, + initiator: initiator, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/data/models/restore_response.dart` around lines 63 - 94, RestoredDispute.fromJson currently assigns initiator directly from JSON which may vary in casing or unexpected values; update RestoredDispute.fromJson to normalize and allow‑list the initiator field (e.g., lowercase the input and accept only "buyer" or "seller", otherwise set initiator to null or map into an enum) so downstream checks are stable; modify the factory RestoredDispute.fromJson to perform this validation/normalization before assigning the initiator property and ensure any mapping uses the RestoredDispute.initiator field (or a new enum) consistently.lib/features/restore/restore_manager.dart (1)
589-596: Normalizeinitiatorbefore role comparison.The comparison is currently case-sensitive; normalizing avoids mislabeling disputes if upstream sends “Buyer/Seller” or other casing variants.
♻️ Suggested adjustment
- final userInitiated = restoredDispute.initiator != null && session?.role != null - ? (session!.role == Role.buyer && restoredDispute.initiator == 'buyer') || - (session.role == Role.seller && restoredDispute.initiator == 'seller') - : false; + final initiator = restoredDispute.initiator?.toLowerCase(); + final userInitiated = initiator != null && session?.role != null + ? (session!.role == Role.buyer && initiator == 'buyer') || + (session.role == Role.seller && initiator == 'seller') + : false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/restore/restore_manager.dart` around lines 589 - 596, The initiator string comparison is case-sensitive; normalize restoredDispute.initiator before comparing to session.role to avoid mismatches. Trim and convert restoredDispute.initiator to a consistent case (e.g., lowerCase) and then compare that normalized value to 'buyer'/'seller' (or Role enum names normalized) when computing userInitiated within the block that references restoredDispute.initiator, session.role, Role.buyer, Role.seller and setting action to Action.disputeInitiatedByYou/Action.disputeInitiatedByPeer.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/data/models/restore_response.dart`:
- Around line 63-94: RestoredDispute.fromJson currently assigns initiator
directly from JSON which may vary in casing or unexpected values; update
RestoredDispute.fromJson to normalize and allow‑list the initiator field (e.g.,
lowercase the input and accept only "buyer" or "seller", otherwise set initiator
to null or map into an enum) so downstream checks are stable; modify the factory
RestoredDispute.fromJson to perform this validation/normalization before
assigning the initiator property and ensure any mapping uses the
RestoredDispute.initiator field (or a new enum) consistently.
In `@lib/features/restore/restore_manager.dart`:
- Around line 589-596: The initiator string comparison is case-sensitive;
normalize restoredDispute.initiator before comparing to session.role to avoid
mismatches. Trim and convert restoredDispute.initiator to a consistent case
(e.g., lowerCase) and then compare that normalized value to 'buyer'/'seller' (or
Role enum names normalized) when computing userInitiated within the block that
references restoredDispute.initiator, session.role, Role.buyer, Role.seller and
setting action to Action.disputeInitiatedByYou/Action.disputeInitiatedByPeer.
Normalizes the `initiator` field in the `RestoredDispute` model to ensure consistency. It trims whitespace and converts the value to lowercase before validation. If the normalized value is 'buyer' or 'seller', it's returned; otherwise, it returns null. This prevents issues caused by inconsistent initiator naming when restoring disputes.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good simplification overall — using the initiator field from the protocol is much cleaner than the old trade-index heuristic. However, there are a few issues that need attention before merging:
Issues
1. String comparison for role matching is fragile
The logic at line ~591 compares session.role == Role.buyer with restoredDispute.initiator == 'buyer'. This relies on the string "buyer"/"seller" matching the enum semantics forever. If the Role enum or the protocol ever changes the string representation, this silently breaks.
Suggestion: Add a helper method or extension on Role to make this explicit and type-safe:
bool get isBuyer => this == Role.buyer;
String get initiatorValue => this == Role.buyer ? 'buyer' : 'seller';Then the comparison becomes:
final userInitiated = restoredDispute.initiator != null && session?.role != null
? session!.role.initiatorValue == restoredDispute.initiator
: false;This eliminates the duplicated buyer/seller string checks and makes it a single comparison.
2. Missing test coverage
The old method had complex logic that was presumably untested (since it was wrong). The new logic is simpler but still has branching paths:
initiatoris null → defaults to peersessionis null → defaults to peerinitiatoris"buyer"and role is buyer → user initiatedinitiatoris"seller"and role is buyer → peer initiated- Edge:
initiatoris some unexpected string →_normalizeInitiatorreturns null → peer
Per the repo's AGENTS.md: "Add targeted scenarios when expanding complex logic or async workflows." At minimum, unit tests for _normalizeInitiator and the initiator-matching logic should be included.
3. _normalizeInitiator is private static but could be useful elsewhere
If dispute initiator normalization is needed in other contexts (e.g., future dispute list views), this will need to be extracted. Consider making it a package-private utility or a method on RestoredDispute that's accessible.
4. tradeIndex is now unused for dispute detection
The tradeIndex field on RestoredDispute was previously the core of dispute detection. Now it's only used for... what? If it's still needed for session matching elsewhere, fine. If not, document why it's kept (or remove it if truly unused).
What looks good
- ✅
_normalizeInitiatorwith whitelist validation is solid defensive coding - ✅ CI passes, no conflicts with main
- ✅ The simplification removes ~60 lines of complex, error-prone logic
- ✅ Conditional serialization in
toJson(if (initiator != null)) is clean
1 participant
Summary
Fixed dispute initiator detection during order restoration using the
initiatorfield from Mostro protocol.Changes:
initiator: String?field toRestoredDisputemodelRestoreManagerto directly compareinitiator("buyer"/"seller") with user's role_determineIfUserInitiatedDispute()methodTest plan
initiatorfield parsingCloses #401
Summary by CodeRabbit