Implements #78 Trade History Restoration on Mnemonic Import #151
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a modal dialog for mnemonic import, enhance mnemonic display with show/hide and tooltip features, and reorganize key management UI. A new service and provider for trade history restoration are added, enabling batch scanning of trade keys and updating the trade key index after import. Localization files are updated for these features. Batch storage methods were added to support bulk session writes. A new configuration parameter for trade history scan duration was added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KeyManagementScreen
participant KeyManager
participant TradeHistoryRestorationService
participant NostrService
User->>KeyManagementScreen: Click "Import User"
KeyManagementScreen->>User: Show import mnemonic dialog
User->>KeyManagementScreen: Enter mnemonic, click "Import"
KeyManagementScreen->>KeyManager: Import mnemonic, reset session/storage
KeyManagementScreen->>TradeHistoryRestorationService: restoreTradeHistory()
TradeHistoryRestorationService->>KeyManager: scanForUsedTradeKeys()
loop For each batch
TradeHistoryRestorationService->>NostrService: Query relays for trade key events
NostrService-->>TradeHistoryRestorationService: Return events for keys
end
TradeHistoryRestorationService->>KeyManager: Update trade key index
TradeHistoryRestorationService-->>KeyManagementScreen: Return restoration result
KeyManagementScreen->>User: Show snackbar with restoration status
Estimated code review effort3 (~45 minutes) Suggested reviewers
Poem
✨ 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/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
lib/shared/providers.dart (1)
8-8: Keep export list sorted for easier scanning.Consider inserting the new
trade_history_restoration_provider.dartexport in alphabetical order to keep the list tidy and avoid merge-conflict churn.lib/shared/providers/trade_history_restoration_provider.dart (1)
6-14: Make the providerautoDispose& usewatchto refresh on deps change.
TradeHistoryRestorationServiceis cheap to rebuild and depends onkeyManagerProvider&nostrServiceProvider.
UsingProvider.autoDisposeavoids a lingering instance after the screen is gone, andref.watchensures the service picks up fresh dependencies (e.g. when a new mnemonic is imported).-final tradeHistoryRestorationProvider = Provider<TradeHistoryRestorationService>((ref) { - final keyManager = ref.read(keyManagerProvider); - final nostrService = ref.read(nostrServiceProvider); +final tradeHistoryRestorationProvider = + Provider.autoDispose<TradeHistoryRestorationService>((ref) { + final keyManager = ref.watch(keyManagerProvider); + final nostrService = ref.watch(nostrServiceProvider); return TradeHistoryRestorationService( keyManager, nostrService, ); });lib/services/trade_history_restoration_service.dart (1)
70-79: Missing subscription creation for restored keysAccording to the PR objectives, the feature should create relay subscriptions for restored keys, but the current implementation only updates the key index.
The comment on line 70 mentions subscribing to found trade keys, but this functionality is not implemented. Would you like me to help implement the subscription logic or open an issue to track this missing feature?
lib/features/key_manager/key_management_screen.dart (1)
174-189: Add loading indicator for trade history restorationThe trade history restoration runs in the background without any visual feedback to the user that it's in progress.
Consider adding a loading indicator or progress notification:
+ bool _restoringHistory = false; // Start trade history restoration in the background final restorationService = ref.read(tradeHistoryRestorationProvider); + setState(() { + _restoringHistory = true; + }); restorationService.restoreTradeHistory().then((highestIndex) { setState(() { _tradeKeyIndex = highestIndex; + _restoringHistory = false; });Then show a progress indicator in the UI when
_restoringHistoryis true.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
lib/features/key_manager/key_management_screen.dart(5 hunks)lib/features/key_manager/key_manager.dart(1 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)lib/services/trade_history_restoration_service.dart(1 hunks)lib/shared/providers.dart(1 hunks)lib/shared/providers/trade_history_restoration_provider.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
lib/**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
lib/l10n/intl_{en,es,it}.arb
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
lib/l10n/*.arb
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (6)
lib/shared/providers.dart (3)
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.
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, Riverpod code generation is used with `@Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to **/*.dart : Remove unused imports and dependencies
lib/l10n/intl_en.arb (3)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/l10n/*.arb : Use proper ARB metadata for strings with parameters
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Test localization changes in all supported languages
lib/shared/providers/trade_history_restoration_provider.dart (5)
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, Riverpod code generation is used with `@Riverpod` annotations. Providers like `eventStorageProvider` are generated in `.g.dart` files from annotated functions in the main provider files. These providers are accessible by importing the main provider file (e.g., `mostro_service_provider.dart`), not by importing a separate provider file.
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.
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Use Riverpod for all state management
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/features/trades/models/trade_state.dart:1-15
Timestamp: 2025-05-08T16:06:33.665Z
Learning: In the context of the Mostro Mobile app, the `TradeState` class is specifically constructed using the `tradeStateProvider`. While some fields are nullable (`lastAction` and `orderPayload`), they are still marked as required parameters to ensure they are explicitly considered during state construction.
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.
lib/l10n/intl_it.arb (2)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/l10n/*.arb : Use proper ARB metadata for strings with parameters
lib/l10n/intl_es.arb (2)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/l10n/intl_{en,es,it}.arb : Add new localization keys to all three ARB files (en, es, it)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/l10n/*.arb : Use proper ARB metadata for strings with parameters
lib/features/key_manager/key_management_screen.dart (1)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-08T17:55:04.549Z
Learning: Applies to lib/**/*.dart : Use `S.of(context).yourKey` for all user-facing strings
🔇 Additional comments (2)
lib/features/key_manager/key_management_screen.dart (2)
89-151: Import dialog implementation looks goodThe modal dialog for mnemonic import is well-implemented with proper styling, localization, and user feedback.
258-286: Well-implemented mnemonic visibility toggleThe show/hide functionality for the mnemonic with tooltip is a great UX improvement, properly using localized strings and maintaining security.
…eriod and i18n updates
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/services/trade_history_restoration_service.dart (1)
109-111: Good implementation with proper null safetyThe null safety check for
event.recipientproperly addresses the previous review comment and prevents potential runtime crashes.
🧹 Nitpick comments (2)
lib/services/trade_history_restoration_service.dart (2)
20-20: Update documentation to match implementationThe comment mentions "subscribing to them" but the implementation only updates the key index without creating any subscriptions.
- /// Restore trade history by scanning for used trade keys and subscribing to them + /// Restore trade history by scanning for used trade keys
59-60: Simplify max comparison usingdart:mathThe ternary operator can be replaced with the more readable
max()function.Add the import at the top of the file:
+import 'dart:math';Then simplify the comparison:
- highestUsedIndex = - entry.key > highestUsedIndex ? entry.key : highestUsedIndex; + highestUsedIndex = max(entry.key, highestUsedIndex);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/core/config.dart(1 hunks)lib/features/key_manager/key_manager.dart(1 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(2 hunks)lib/l10n/intl_it.arb(2 hunks)lib/services/trade_history_restoration_service.dart(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- lib/core/config.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/l10n/intl_en.arb
- lib/features/key_manager/key_manager.dart
- lib/l10n/intl_es.arb
- lib/l10n/intl_it.arb
🧰 Additional context used
📓 Path-based instructions (2)
lib/**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (1)
lib/services/trade_history_restoration_service.dart (2)
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
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.
⏰ 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)
- GitHub Check: build
|
Hi @chebizarro , let me tell you what I did, I created an pending order, backup the seed, uninstalled and installed tha app again, imported the seed but the pending order is not being displayed |
|
ping @chebizarro |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/services/trade_history_restoration_service.dart (1)
26-29: Consider adding parameter validation.The method signature looks good, but consider adding parameter validation to ensure
batchSizeis positive andmaxKeysToScanis reasonable.Future<(int, Map<String, Session>)> restoreTradeHistory({ int maxKeysToScan = 100, int batchSize = 20, }) async { + if (batchSize <= 0) throw ArgumentError('batchSize must be positive'); + if (maxKeysToScan <= 0) throw ArgumentError('maxKeysToScan must be positive'); _logger.i('Starting trade history restoration...');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/data/repositories/base_storage.dart(1 hunks)lib/data/repositories/session_storage.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(6 hunks)lib/services/trade_history_restoration_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/key_manager/key_management_screen.dart
🧰 Additional context used
📓 Path-based instructions (3)
lib/**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
lib/data/repositories/**/*.dart
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- CLAUDE.md
🧠 Learnings (3)
lib/data/repositories/session_storage.dart (1)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T01:48:43.306Z
Learning: Applies to lib/data/repositories/**/*.dart : All data access should go through repository classes in `data/repositories/`
lib/data/repositories/base_storage.dart (1)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T01:48:43.306Z
Learning: Applies to lib/data/repositories/**/*.dart : All data access should go through repository classes in `data/repositories/`
lib/services/trade_history_restoration_service.dart (6)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T01:48:43.306Z
Learning: Applies to lib/services/nostr_service.dart : Nostr protocol interactions go through `services/nostr_service.dart`
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.
Learnt from: chebizarro
PR: MostroP2P/mobile#74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.659Z
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.
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.659Z
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.
Learnt from: Catrya
PR: MostroP2P/mobile#215
File: lib/features/order/notfiers/order_notifier.dart:0-0
Timestamp: 2025-07-20T23:33:17.659Z
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.
🔇 Additional comments (8)
lib/data/repositories/base_storage.dart (1)
83-89: Well-implemented batch storage method.The
putAllmethod correctly implements batch insertion using a single transaction, which ensures atomicity and better performance compared to individual puts. The implementation follows the established patterns in the class and properly utilizestoDbMapfor data conversion.lib/data/repositories/session_storage.dart (1)
55-57: Clean batch storage implementation.The
putSessionsmethod provides a domain-specific interface for batch session storage by delegating to the inheritedputAllmethod. The implementation is clean and follows the established patterns.Note that unlike
putSession, this method doesn't validate that sessions have non-nullorderIdvalues, which appears appropriate for the trade history restoration use case where sessions are already validated.lib/services/trade_history_restoration_service.dart (6)
1-22: Well-structured service class setup.The imports are appropriate and the class structure follows good practices with dependency injection through the constructor. The logger initialization is clean and the service encapsulates trade history restoration logic appropriately.
38-47: Efficient batch processing implementation.The batch processing logic correctly handles index ranges and uses the
generateTradeKeyBatchmethod efficiently. Theclampoperation ensures we don't exceedmaxKeysToScan.
72-76: Smart optimization with early termination.The early termination logic when no messages are found after index 20 is a good optimization to avoid unnecessary network requests while still allowing for sparse key usage patterns.
116-121: Proper null safety and error handling.The null checks for
event.recipientand proper error handling for decryption failures are well implemented. This addresses potential issues with malformed events.
126-145: Robust session creation with validation.The session creation logic properly validates the message structure and skips invalid messages. The logging provides good visibility into the restoration process. The session construction uses appropriate values for trade history restoration.
83-91: No changes needed: restoring the next index beyond the highest-used key is correctI’ve confirmed that:
- KeyManager.setCurrentKeyIndex is the sole mechanism for advancing the stored trade-key index (it enforces index >= 1 and writes it to storage).
- generateTradeKeyBatch and deriveTradeKeyPair(index) are pure derivations (they don’t modify the stored index).
- All downstream consumers (e.g.
deriveTradeKey()and session creation) read exactly that stored index.By setting the index to
highestUsedIndex + 1, we safely avoid re-deriving any keys at or below the highest one seen and resume on a fresh key. Skipping unused “gaps” is intentional to prevent accidental reuse of any derived key.
No changes required here.
…eriod and i18n updates
…andling and UI updates
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lib/services/trade_history_restoration_service.dart (1)
104-192: Comprehensive message processing with good validation.The query logic properly handles Nostr events, validates message content, and creates sessions with appropriate role determination. The null safety check for
event.recipientaddresses previous review concerns.Minor improvements for consistency and safety:
- Use already-verified variable: At line 185, use the verified recipient instead of force unwrapping again:
- sessions[event.recipient!] = session; + sessions[pkey.value.public] = session;
- Consider validating masterKeyPair: The force unwrap at line 178 assumes
masterKeyPairis available, but this should be verified earlier in the restoration process.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
lib/core/config.dart(1 hunks)lib/features/key_manager/key_management_screen.dart(4 hunks)lib/features/key_manager/key_manager.dart(1 hunks)lib/l10n/intl_en.arb(2 hunks)lib/l10n/intl_es.arb(3 hunks)lib/l10n/intl_it.arb(3 hunks)lib/services/trade_history_restoration_service.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- lib/core/config.dart
- lib/l10n/intl_en.arb
- lib/features/key_manager/key_manager.dart
- lib/l10n/intl_es.arb
- lib/l10n/intl_it.arb
- lib/features/key_manager/key_management_screen.dart
🧰 Additional context used
📓 Path-based instructions (2)
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/services/trade_history_restoration_service.dart
**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.dart: Remove unused imports and dependencies
Useconstconstructors where possible
Files:
lib/services/trade_history_restoration_service.dart
🧠 Learnings (2)
📓 Common learnings
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.
lib/services/trade_history_restoration_service.dart (7)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T01:48:43.335Z
Learning: Applies to lib/services/nostr_service.dart : Nostr protocol interactions go through services/nostr_service.dart
Learnt from: chebizarro
PR: #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.
Learnt from: chebizarro
PR: #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.
Learnt from: chebizarro
PR: #74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
⏰ 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)
- GitHub Check: build
🔇 Additional comments (3)
lib/services/trade_history_restoration_service.dart (3)
1-14: Import organization looks good.All imports are used and properly organized by type (standard library, external packages, internal modules).
16-25: Clean dependency injection pattern.The constructor properly injects required dependencies and follows good practices.
27-102: Ensure masterKeyPair is non-null before querying relaysBefore invoking
_queryRelaysForKeys(which uses_keyManager.masterKeyPair!on line 178), add an early check or assertion to guarantee the master key is initialized. Otherwise, a missing key will cause an unexpected runtime error. For example, at the top ofrestoreTradeHistory:if (_keyManager.masterKeyPair == null) { throw StateError( 'Master key pair must be initialized before restoring trade history', ); }• File: lib/services/trade_history_restoration_service.dart
• Method: restoreTradeHistory (start of method)
• Concern: force-unwrap on_keyManager.masterKeyPair!in_queryRelaysForKeys(line 178)
…h method and fix null language setting
|
@grunch - this updated PR is ready to be tested/merged and now includes restoring orders |
I did the same I said in my comment above and it failed, here the log: Then I tried again and I got a positive message telling me that the account was imported but I still don't see the pending order, here the logs: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/services/trade_history_restoration_service.dart (1)
80-87: Consider making the early termination logic more robust.The logic
startIndex > 20for early termination seems arbitrary. Consider making this configurable or based on a percentage of consecutive empty batches.- if (batchSessions.isEmpty && startIndex > 20) { + // Stop if we've had multiple consecutive empty batches after scanning some keys + if (batchSessions.isEmpty && startIndex > batchSize * 2) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/features/key_manager/key_management_screen.dart(6 hunks)lib/features/key_manager/key_manager.dart(1 hunks)lib/services/trade_history_restoration_service.dart(1 hunks)lib/shared/providers/trade_history_restoration_provider.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/shared/providers/trade_history_restoration_provider.dart
- lib/features/key_manager/key_manager.dart
- lib/features/key_manager/key_management_screen.dart
🧰 Additional context used
📓 Path-based instructions (2)
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/services/trade_history_restoration_service.dart
**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.dart: Remove unused imports and dependencies
Useconstconstructors where possible
Files:
lib/services/trade_history_restoration_service.dart
🧠 Learnings (2)
📓 Common learnings
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.
lib/services/trade_history_restoration_service.dart (7)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T01:48:43.335Z
Learning: Applies to lib/services/nostr_service.dart : Nostr protocol interactions go through services/nostr_service.dart
Learnt from: chebizarro
PR: #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.
Learnt from: chebizarro
PR: #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.
Learnt from: chebizarro
PR: #74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
⏰ 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)
- GitHub Check: build
🔇 Additional comments (6)
lib/services/trade_history_restoration_service.dart (6)
1-15: Clean imports and dependencies.All imports appear to be used and properly organized. The dependencies align well with the service's functionality.
17-26: Well-structured service class.The constructor and dependencies are properly injected, following good dependency injection patterns.
35-40: Proper null safety guard for master key pair.Good defensive programming by checking for null master key pair and throwing a descriptive exception.
134-139: Potential null safety issue with force unwrapping.The code uses
publicKeyLookup[event.recipient]!which could cause the null check operator error mentioned in testing. While there's a null check forevent.recipient, the lookup result isn't verified before force unwrapping.- final pkey = publicKeyLookup[event.recipient]!; + final pkey = publicKeyLookup[event.recipient]; + if (pkey == null) continue;⛔ Skipped due to learnings
Learnt from: chebizarro PR: MostroP2P/mobile#74 File: lib/notifications/notification_service.dart:54-59 Timestamp: 2025-05-08T16:31:29.582Z Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.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.
95-96: setCurrentKeyIndex validation covers index ≥ 1 – no changes neededI’ve confirmed that
setCurrentKeyIndexis implemented in
lib/features/key_manager/key_manager.dart(around line 110) and explicitly throws anInvalidTradeKeyIndexExceptionif passed a value < 1. Since we only call it whenusedTradeKeys.isNotEmpty,highestUsedIndex + 1will always be ≥ 1. No further edge-case handling is required here.
56-67: No null-safety issue in generateTradeKeyBatchThe
generateTradeKeyBatchmethod (lib/features/key_manager/key_manager.dart lines 121–129) always returns a non-null List of non-nullNostrKeyPairsentries. Since neither the list nor its entries can be null, there’s no need for additional null checks here—callingMap.fromEntries(batchKeys)is safe as written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/services/trade_history_restoration_service.dart(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
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/services/trade_history_restoration_service.dart
**/*.dart
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.dart: Remove unused imports and dependencies
Useconstconstructors where possible
Files:
lib/services/trade_history_restoration_service.dart
🧠 Learnings (2)
📓 Common learnings
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.
lib/services/trade_history_restoration_service.dart (7)
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-07-20T01:48:43.335Z
Learning: Applies to lib/services/nostr_service.dart : Nostr protocol interactions go through services/nostr_service.dart
Learnt from: chebizarro
PR: #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.
Learnt from: chebizarro
PR: #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.
Learnt from: chebizarro
PR: #74
File: lib/notifications/notification_service.dart:54-59
Timestamp: 2025-05-08T16:31:29.582Z
Learning: In the Nostr protocol, event.id will never be null in events returned by relay subscriptions, so null safety checks for this property are unnecessary.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
Learnt from: Catrya
PR: #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. Do not suggest adding await to super.handleEvent() calls as this would cause compilation errors.
⏰ 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)
- GitHub Check: build
🔇 Additional comments (7)
lib/services/trade_history_restoration_service.dart (7)
1-14: Import section looks good.All imports are necessary and properly organized. No unused imports detected.
16-25: Clean class structure with proper dependency injection.The service follows good architectural patterns with clear dependencies and proper logger initialization.
35-40: Good null safety check for master key pair.The early guard clause prevents null pointer exceptions and provides a clear error message.
46-91: Batch processing logic is well implemented.The batch scanning approach with delays between batches is thoughtful and avoids overwhelming relays. The early termination condition after index 20 is a good optimization.
94-102: Key index management looks correct.Setting the current key index to
highestUsedIndex + 1ensures the next generated key won't conflict with existing ones.
134-139: Null safety check is properly implemented.The conditional check ensures
event.recipientis not null before using the force unwrap on line 136. This addresses the previous null safety concern.
186-188: Improved null safety with proper fallbacks.Using null coalescing operators with appropriate fallbacks (exception for critical masterKey, DateTime.now() for timestamp) is much safer than force unwraps.
| for (final event in events) { | ||
| if (event.recipient != null && | ||
| publicKeyLookup.containsKey(event.recipient)) { | ||
| final pkey = publicKeyLookup[event.recipient]!; | ||
| final decryptedEvent = await event.unWrap( | ||
| pkey.value.private, | ||
| ); | ||
| if (decryptedEvent.content == null) continue; | ||
|
|
||
| final result = jsonDecode(decryptedEvent.content!); | ||
| if (result is! List) continue; | ||
|
|
||
| final msg = MostroMessage.fromJson(result[0]); | ||
| if (msg.tradeIndex != null && msg.tradeIndex != pkey.key) { | ||
| _logger.i( | ||
| 'Trade index ${msg.tradeIndex} does not match key index ${pkey.key}'); | ||
| continue; | ||
| } | ||
|
|
||
| if (msg.action != Action.newOrder && | ||
| msg.action != Action.takeBuy && | ||
| msg.action != Action.takeSell) { | ||
| _logger.i( | ||
| 'Message action ${msg.action} is not newOrder, takeBuy or takeSell'); | ||
| continue; | ||
| } | ||
| final order = msg.getPayload<Order>(); | ||
|
|
||
| if (order == null) { | ||
| _logger.i('Message payload is not an Order'); | ||
| continue; | ||
| } | ||
|
|
||
| Role role; | ||
| switch (msg.action) { | ||
| case Action.newOrder: | ||
| role = order.kind == OrderType.buy ? Role.buyer : Role.seller; | ||
| break; | ||
| case Action.takeBuy: | ||
| role = Role.buyer; | ||
| break; | ||
| case Action.takeSell: | ||
| role = Role.seller; | ||
| break; | ||
| default: | ||
| _logger.i( | ||
| 'Message action ${msg.action} is not newOrder, takeBuy or takeSell'); | ||
| continue; | ||
| } | ||
|
|
||
| final session = Session( | ||
| tradeKey: pkey.value, | ||
| fullPrivacy: msg.tradeIndex == null ? true : false, | ||
| masterKey: _keyManager.masterKeyPair ?? (throw Exception('Master key pair is null')), | ||
| keyIndex: msg.tradeIndex ?? pkey.key, | ||
| startTime: decryptedEvent.createdAt ?? DateTime.now(), | ||
| orderId: msg.id, | ||
| role: role, | ||
| ); | ||
|
|
||
| sessions[pkey.value.public] = session; | ||
| _logger.i( | ||
| 'Created session for order ${msg.id} with trade index ${msg.tradeIndex}'); | ||
| } | ||
| } | ||
| return sessions; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Investigate remaining null check operator errors.
While the null safety improvements look good, user feedback indicates "Null check operator used on a null value" errors are still occurring during testing. Let's verify potential sources of these errors.
🏁 Script executed:
#!/bin/bash
# Search for remaining force unwraps in related files that could cause null check operator errors
echo "=== Searching for force unwraps in NostrService ==="
rg "!" lib/services/nostr_service.dart -A 2 -B 1
echo "=== Searching for force unwraps in KeyManager ==="
rg "!" lib/features/key_manager/key_manager.dart -A 2 -B 1
echo "=== Searching for force unwraps in Session model ==="
rg "!" lib/data/models/session.dart -A 2 -B 1
echo "=== Searching for force unwraps in NostrEvent model ==="
rg "!" lib/data/models/nostr_event.dart -A 2 -B 1
echo "=== Searching for force unwraps in MostroMessage model ==="
rg "!" lib/data/models/mostro_message.dart -A 2 -B 1Length of output: 8939
Mitigate remaining force-unwraps to prevent null-check crashes
We still have unchecked ! operators in key derivation and NostrEvent tag getters that will throw if data is missing. Please add proper null guards or fallbacks:
• lib/features/key_manager/key_manager.dart
– deriveTradeKeyPair(int index): _derivator.derivePrivateKey(_masterKeyHex!, index)
• _masterKeyHex can be null if init() didn’t populate it.
→ Check for null (or throw a clear exception) before using !.
• lib/data/models/nostr_event.dart
– orderType getter: OrderType.fromString(_getTagValue('k')!)
– status getter: Status.fromString(_getTagValue('s')!)
– expirationDate: _getTimeStamp(_getTagValue('expiration')!)
– type getter: _getTagValue('z')!
• If any tag is missing, these will NPE at runtime.
→ Either return a nullable type, supply a default/value object, or throw a descriptive error when the tag is absent.
After these guards are in place, rerun your tests to confirm the “Null check operator used on a null value” errors are resolved.
🤖 Prompt for AI Agents
In lib/features/key_manager/key_manager.dart around the deriveTradeKeyPair
method, add a null check for _masterKeyHex before using the ! operator; if it is
null, throw a clear exception indicating initialization is required. In
lib/data/models/nostr_event.dart for the orderType, status, expirationDate, and
type getters, replace the force unwraps (!) on _getTagValue calls with null
checks; handle missing tags by either returning nullable types, providing
default values, or throwing descriptive errors to prevent runtime null pointer
exceptions. After applying these null guards, rerun tests to ensure no null
check operator errors remain.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
This PR can't work right now because we haven't implemented the feature for this on mostrod.
clients ask to mostro for active orders indexes and mostrod should give the information, but this is not implemented yet
|
|
||
| /// Query relays for messages addressed to the given public keys | ||
| /// Returns a set of public keys that have messages | ||
| Future<Map<String, Session>> _queryRelaysForKeys( |
There was a problem hiding this comment.
This never gonna work because relays doesn't have keys of the user, all the public keys of order events published is the Mostro public key
There was a problem hiding this comment.
I don't think you understand what the code is doing. It is querying for the kind 1059 messages on the relay using batches of keys derived from the imported master key. It works perfectly well with my tests, all of the information needed to rehydrate orders is stored on the relays. This will only fail if the user has not used the relay before.
There was a problem hiding this comment.
I know what is doing, this is not an efficient way of doing it, when relays discard old 1059 events users are not going to be able to work with this logic, Mostro knows the real data of the user, we only need to ask to Mostro and move the index
Let's put this on stand by while we finish other issues with higher priority
This PR introduces a robust trade history restoration feature to the Mostro mobile app’s import flow to address issue #78. When a user imports a mnemonic, the app now automatically scans for previously used trade keys by querying relays, and updates the internal key index accordingly. All user feedback is localized, and the implementation follows best practices for code quality and maintainability.
Key Features
Automated Trade History Restoration:
After importing a mnemonic, the app scans relays for messages addressed to batches of derived trade keys. The highest used key index is detected and updated, allowing users to seamlessly continue using their restored account.
Efficient Batch Processing:
Trade keys are scanned in batches (default: 20 at a time, up to 100 total) to optimize network usage and performance.
User Feedback & Localization:
Users receive a localized SnackBar notification indicating the highest restored trade key index.
All new user-facing strings are added to the ARB localization files
Error Handling:
Restoration failures are logged, and error messages are localized for future UI use.
Note:
This implementation only restores the key index and does not create new relay subscriptions for restored keys.
Summary by CodeRabbit
New Features
Improvements