fix: handle out_of_range_sats_amount cantDo with session cleanup and retry mechanism #262
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…etry mechanism - Add specific handling for out_of_range_sats_amount cant_do messages - Implement session cleanup to remove temporary sessions on retry - Add reset functionality in AddOrderNotifier for failed order attempts - Generate new requestId and trade key for each retry attempt
Contributor
WalkthroughThis change introduces specific handling for the "out_of_range_sats_amount" error case in order-related notifiers. When this error occurs, temporary session cleanup is triggered, and the order notifier is reset to allow a retry. A new session cleanup method is added to the session notifier. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AddOrderNotifier
participant AbstractMostroNotifier
participant SessionNotifier
User->>AddOrderNotifier: Submit Order
AddOrderNotifier->>AbstractMostroNotifier: Handle Event
AbstractMostroNotifier->>AbstractMostroNotifier: Detects cantDo with "out_of_range_sats_amount"
AbstractMostroNotifier->>SessionNotifier: cleanupRequestSession(requestId)
SessionNotifier-->>AbstractMostroNotifier: Session cleaned
AbstractMostroNotifier->>AddOrderNotifier: Notifies cantDo
AddOrderNotifier->>AddOrderNotifier: _resetForRetry()
AddOrderNotifier->>AddOrderNotifier: Reset state, new requestId, resubscribe
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Possibly related PRs
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
224-224: Consider using enum comparison instead of string comparison.Using
toString()on the enum for comparison makes the code fragile to enum implementation changes. Consider comparing directly with the enum value if possible.-if (cantDo?.cantDoReason.toString() == 'out_of_range_sats_amount') { +if (cantDo?.cantDoReason == CantDoReason.outOfRangeSatsAmount) {lib/features/order/notfiers/add_order_notifier.dart (1)
47-47: Consider using enum comparison instead of string comparison.Similar to the abstract notifier, using
toString()on the enum makes the code fragile. Consider direct enum comparison if possible.-if (cantDo?.cantDoReason.toString() == 'out_of_range_sats_amount') { +if (cantDo?.cantDoReason == CantDoReason.outOfRangeSatsAmount) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/features/order/notfiers/abstract_mostro_notifier.dart(1 hunks)lib/features/order/notfiers/add_order_notifier.dart(3 hunks)lib/shared/notifiers/session_notifier.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
lib/**/*.dart: UseS.of(context).yourKeyfor all user-facing strings
Always checkmountedbefore using context after async operations
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/shared/notifiers/session_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.dart: Remove unused imports and dependencies
Useconstconstructors where possible
Files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/shared/notifiers/session_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
lib/shared/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Shared utilities and widgets must be placed in
shared/
Files:
lib/shared/notifiers/session_notifier.dart
🧠 Learnings (14)
📓 Common learnings
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Encapsulate business logic in Notifiers
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
📚 Learning: in abstractmostronotifier, the handleevent method is synchronous and returns void, not future....
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.689Z
Learning: In AbstractMostroNotifier, the handleEvent method is synchronous and returns void, not Future<void>. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
📚 Learning: in abstractmostronotifier, state updates occur for all messages regardless of timestamp to hydrate t...
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/features/order/notfiers/abstract_mostro_notifier.dart:45-54
Timestamp: 2025-06-26T15:03:23.529Z
Learning: In AbstractMostroNotifier, state updates occur for all messages regardless of timestamp to hydrate the OrderNotifier from MostroStorage during sync, while handleEvent is only called for recent messages (within 60 seconds) to prevent re-triggering side effects like notifications and navigation for previously handled messages. This design prevents displaying stale notifications when the app is reopened or brought to the foreground.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
📚 Learning: in the cantdo model parsing in lib/data/models/cant_do.dart, the inconsistent key naming between 'ca...
Learnt from: chebizarro
PR: MostroP2P/mobile#127
File: lib/data/models/cant_do.dart:10-27
Timestamp: 2025-07-08T05:40:47.527Z
Learning: In the CantDo model parsing in lib/data/models/cant_do.dart, the inconsistent key naming between 'cant_do' (top-level) and 'cant-do' (nested) is required by the protocol specification and should not be changed as it would break message parsing compatibility.
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
📚 Learning: applies to lib/core/mostro_fsm.dart : order state transitions must be managed by `core/mostro_fsm.da...
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Applies to lib/core/mostro_fsm.dart : Order state transitions must be managed by `core/mostro_fsm.dart`
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
📚 Learning: encapsulate business logic in notifiers...
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Encapsulate business logic in Notifiers
Applied to files:
lib/features/order/notfiers/abstract_mostro_notifier.dartlib/features/order/notfiers/add_order_notifier.dart
📚 Learning: in the orderstate refactor, public keys should be accessed via `tradestate.peer?.publickey` from the...
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: lib/features/trades/widgets/mostro_message_detail_widget.dart:134-141
Timestamp: 2025-06-08T23:54:09.987Z
Learning: In the OrderState refactor, public keys should be accessed via `tradeState.peer?.publicKey` from the OrderState instance rather than through `session?.peer?.publicKey`, as the OrderState encapsulates peer information directly.
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: in the mostro protocol, recipient keys (trade keys) are used only once, ensuring no collisions when ...
Learnt from: chebizarro
PR: MostroP2P/mobile#151
File: lib/services/trade_history_restoration_service.dart:67-67
Timestamp: 2025-07-20T23:45:23.811Z
Learning: In the Mostro protocol, recipient keys (trade keys) are used only once, ensuring no collisions when merging session maps in trade history restoration.
Applied to files:
lib/shared/notifiers/session_notifier.dart
📚 Learning: applies to lib/services/nostr_service.dart : all nostr protocol interactions must go through `servic...
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Applies to lib/services/nostr_service.dart : All Nostr protocol interactions must go through `services/nostr_service.dart`
Applied to files:
lib/features/order/notfiers/add_order_notifier.dart
📚 Learning: mostroservice methods like takebuyorder() and takesellorder() return future and trigger side e...
Learnt from: chebizarro
PR: MostroP2P/mobile#110
File: test/notifiers/take_order_notifier_test.dart:72-74
Timestamp: 2025-06-04T19:35:20.209Z
Learning: MostroService methods like takeBuyOrder() and takeSellOrder() return Future<void> and trigger side effects through other mechanisms rather than direct return values. When testing these methods, focus on verifying method calls and testing state changes through the provider system rather than mocking return values.
Applied to files:
lib/features/order/notfiers/add_order_notifier.dart
📚 Learning: applies to lib/data/models.dart : models must be exported through `data/models.dart`...
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Applies to lib/data/models.dart : Models must be exported through `data/models.dart`
Applied to files:
lib/features/order/notfiers/add_order_notifier.dart
📚 Learning: applies to lib/shared/providers/mostro_database_provider.dart : database initialization must occur i...
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-03T09:43:38.727Z
Learning: Applies to lib/shared/providers/mostro_database_provider.dart : Database initialization must occur in `shared/providers/mostro_database_provider.dart`
Applied to files:
lib/features/order/notfiers/add_order_notifier.dart
📚 Learning: in the mostro mobile codebase, `eventstorageprovider` is exported from `package:mostro_mobile/shared...
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:70-76
Timestamp: 2025-05-06T15:49:26.443Z
Learning: In the Mostro Mobile codebase, `eventStorageProvider` is exported from `package:mostro_mobile/shared/providers/mostro_service_provider.dart` and not from a separate `event_storage_provider.dart` file.
Applied to files:
lib/features/order/notfiers/add_order_notifier.dart
📚 Learning: in the mostro protocol, an order cannot be canceled unless it has a valid orderid, so null checks on...
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/services/mostro_service.dart:97-103
Timestamp: 2025-05-06T15:49:55.079Z
Learning: In the Mostro protocol, an order cannot be canceled unless it has a valid orderId, so null checks on orderId during cancel operations are unnecessary.
Applied to files:
lib/features/order/notfiers/add_order_notifier.dart
🔇 Additional comments (4)
lib/shared/notifiers/session_notifier.dart (1)
163-173: LGTM! Clean implementation of session cleanup.The
cleanupRequestSessionmethod properly removes temporary sessions from both the internal map and state list, with appropriate debug logging. The implementation follows the established patterns in the class.lib/features/order/notfiers/abstract_mostro_notifier.dart (1)
225-230: LGTM! Proper session cleanup on error.The cleanup logic correctly handles the out_of_range_sats_amount error by removing temporary sessions and includes appropriate null checking for requestId.
lib/features/order/notfiers/add_order_notifier.dart (2)
8-8: LGTM! Necessary import for OrderState.The import is required for the new OrderState instantiation in the reset method.
87-104: LGTM! Well-implemented retry mechanism.The
_resetForRetry()method properly:
- Generates a new requestId for the retry attempt
- Resets state to a clean initial state
- Properly closes the existing subscription before re-subscribing
- Includes appropriate logging for debugging
The implementation follows good practices for state management and subscription lifecycle.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit