Reviewing files that changed from the base of the PR and between 7aea2fb and da5b982.

• pubspec.lock is excluded by !**/*.lock

• lib/app/app.dart (1 hunks)
• lib/app/app_routes.dart (0 hunks)
• lib/app/app_settings.dart (1 hunks)
• lib/app/config.dart (1 hunks)
• lib/data/models/cant_do.dart (1 hunks)
• lib/data/models/enums/storage_keys.dart (1 hunks)
• lib/data/models/mostro_message.dart (1 hunks)
• lib/data/models/order.dart (2 hunks)
• lib/data/models/payload.dart (2 hunks)
• lib/data/models/session.dart (1 hunks)
• lib/data/repositories/mostro_repository.dart (3 hunks)
• lib/data/repositories/secure_storage_manager.dart (0 hunks)
• lib/data/repositories/session_manager.dart (1 hunks)
• lib/features/add_order/notifiers/add_order_notifier.dart (2 hunks)
• lib/features/key_manager/key_derivator.dart (1 hunks)
• lib/features/key_manager/key_manager.dart (1 hunks)
• lib/features/key_manager/key_manager_errors.dart (1 hunks)
• lib/features/key_manager/key_manager_provider.dart (1 hunks)
• lib/features/key_manager/key_storage.dart (1 hunks)
• lib/features/order/notfiers/order_notifier.dart (1 hunks)
• lib/features/order/providers/order_notifier_provider.dart (1 hunks)
• lib/features/take_order/notifiers/take_buy_order_notifier.dart (1 hunks)
• lib/features/take_order/notifiers/take_sell_order_notifier.dart (1 hunks)
• lib/features/take_order/screens/take_sell_order_screen.dart (1 hunks)
• lib/main.dart (1 hunks)
• lib/notifiers/mostro_orders_notifier.dart (0 hunks)
• lib/services/mostro_service.dart (2 hunks)
• lib/services/nostr_service.dart (3 hunks)
• lib/shared/notifiers/app_settings_controller.dart (2 hunks)
• lib/shared/providers/app_init_provider.dart (1 hunks)
• lib/shared/providers/mostro_service_provider.dart (2 hunks)
• lib/shared/providers/session_manager_provider.dart (1 hunks)
• lib/shared/providers/shared_preferences_provider.dart (0 hunks)
• lib/shared/providers/storage_providers.dart (1 hunks)
• lib/shared/utils/nostr_utils.dart (1 hunks)
• pubspec.yaml (2 hunks)
• test/models/order_test.dart (1 hunks)
• test/services/key_derivator_test.dart (1 hunks)

• lib/notifiers/mostro_orders_notifier.dart
• lib/app/app_routes.dart
• lib/shared/providers/shared_preferences_provider.dart
• lib/data/repositories/secure_storage_manager.dart

• lib/app/config.dart

lib/services/nostr_service.dart (1)

23-23: Favor consistent callback structure for maintainability.
The inline callback for onRelayListening is succinct, but consider inline documentation or at least a single comment if additional logic is introduced later (e.g., tracking all active relays, handling partial connections, etc.).

lib/data/models/cant_do.dart (1)

6-8: Validate presence and type of cant_do.
Consider verifying that json['cant_do'] is valid (e.g., non-null and a string). Throwing a more descriptive error can help with debugging when the field is missing or malformed.

lib/data/models/payload.dart (2)

1-1: Import statement check.
The import for cant_do.dart looks good, but ensure it does not introduce dependency cycles.

---

14-15: Confirm 'cant_do' key usage.
This logic is correct for handling the new cant_do payload. Verify that server responses consistently include this key and that it matches the renamed type if you decide to rename it.

lib/data/models/session.dart (4)

7-10: Consider verifying if keys should be serialized internally or handled only by SessionManager.
Storing complete key pairs directly in this class could lead to unintentional exposures if the object is serialized. Currently, toJson() avoids outputting these keys, which is good; however, explicitly clarifying whether these fields must remain in memory or if they need extra protection would improve clarity.

---

14-20: Constructor readability is good.
The constructor organizes required parameters effectively. Ensure that orderId defaults remain meaningful if not provided.

---

22-27: Confirm the intent to exclude cryptographic keys from toJson().
This approach is appropriate from a security standpoint, but if you intend to reconstruct the session object from JSON alone, the keys won't be restored unless you re-inject them, which you do later via SessionManager. Validate that this design aligns with app requirements.

---

32-37: Potential mismatch with toJson() for master/trade keys.
fromJson reads master_key and trade_key, yet these fields are absent in the toJson() output. This design offloads key restoration to separate storage, which is acceptable if carefully handled. Otherwise, storing partial data may lead to confusion.

lib/features/key_manager/key_manager.dart (7)

12-15: Method hasMasterKey() is straightforward.
This simple check is clear and efficient. Be sure to handle any potential storage exceptions upstream if needed.

---

17-26: Validate master key generation coverage and error handling.
While generating a new mnemonic and master key is correct, consider explicit error handling if _derivator.generateMnemonic() or _derivator.extendedKeyFromMnemonic(...) fails unexpectedly.

---

28-37: Good practice to throw MasterKeyNotFoundException.
Ensuring a consistent exception for missing keys is helpful. Consider clarifying if this condition indicates a first-time user or if it implies potential data corruption.

---

39-42: Retrieval of mnemonic is correct.
No concerns: returning null if no mnemonic is found is intuitive.

---

44-58: Incrementing the trading key index after derivation is appropriate.
Ensure concurrency control if multiple trade keys might be derived in parallel, e.g., by different parts of the code. Also double check that skipping await _storage.storeTradeKeyIndex(currentIndex + 1); on error won’t cause index misalignment.

---

60-69: deriveTradeKeyFromIndex(int index) method is efficient.
This approach is straightforward. Potential concurrency or re-entrancy issues with storing or skipping index updates do not apply here, so it’s simpler.

---

71-73: Method getCurrentKeyIndex() is neat.
This is a clean wrapper around _storage.readTradeKeyIndex().

lib/features/order/notfiers/order_notifier.dart (4)

9-14: Constructor clarifies initial state retrieval.
Instantiating state from getOrderById or defaulting to Action.notFound is sensible. Properly handles the scenario where the order does not exist.

---

39-41: Error handling with notifications is succinct.
This helps ensure the user is aware of streaming or connectivity issues. Possibly add reconnection logic if needed.

---

43-80: Switch-case usage for different actions is well-organized.
The separate handling for each Action is readable. Consider adding comments for unimplemented Action cases or merging them into a catch-all category only if you’re sure they never occur in normal operation.

---

83-87: Dispose logic is correct.
Canceling the subscription prevents memory leaks and ensures an orderly teardown.

lib/data/repositories/mostro_repository.dart (11)

2-3: Import usage recognized.
Reading and writing with json plus secure storage is sensible for sensitive data handling.

---

13-13: Storing the FlutterSecureStorage instance is fine.
Secure storage is a good choice for sensitive data. Just be careful when reading large volumes of data if performance is critical.

---

16-16: Changed _subscriptions key type
Now using int (aligned with session.keyIndex), which clarifies the subscription mapping.

---

18-18: Constructor injection
The repository now depends on both MostroService and FlutterSecureStorage. This is a positive shift for testability and single-responsibility.

---

20-21: Getter methods for messages
getOrderById and allMessages are straightforward. No immediate concerns.

---

23-41: Subscription logic persists messages to secure storage
Good approach for real-time updates. Make sure _messages remains consistent if storage writes fail. Potentially consider concurrency or partial failures.

---

43-44: Resubscribe by orderId
This is simplistic. As soon as orderId yields a session, re-establish the stream. Validate session != null upstream if the service unexpectedly returns null.

---

70-70: Asynchronous cancellation
Changing cancelOrder to a Future<void> is good for ensuring the call has completed.

---

82-86: saveMessage(MostroMessage)
Storing single messages is more granular than saveMessages(). Good approach for incremental updates.

---

88-91: deleteMessage
Cleanly deletes from in-memory cache and secure storage. This keeps data consistent and prevents orphans.

---

93-105: loadMessages()
Attempts to deserialize all known message entries. The try/catch is crucial to avoid halting on corrupted entries. Possibly add logging for partial failures.

lib/data/repositories/session_manager.dart (11)

8-12: Use of in-memory map _sessions
Reflecting sessions by keyIndex is logical. Keep in mind concurrency if multiple parts of the code might update sessions simultaneously.

---

18-20: Constructor auto-runs _initializeCleanup()
This is a useful pattern for housekeeping. Ensure that in test scenarios, you manage or mock out the timer.

---

22-36: Initialization from storage
Decoding all session entries in init() is good. Consider chunking or pagination if storage grows large.

---

38-53: newSession
Retrieves master key, obtains the next index, derives a trade key, and creates a Session. This is well-structured, but ensure concurrency is safe if multiple sessions or key derivations happen at once.

---

55-60: saveSession
Serializes and writes the session. The secure approach is consistent with the rest of the code.

---

62-68: getSessionByOrderId
Uses a simple .firstWhere approach. This is concise and left out concurrency complexities.

---

70-86: loadSession
Loads a session from storage if not cached. The error handling is correct. Might consider logging or removing the failing entry if decode fails.

---

98-102: deleteSession
Removes session from _sessions and secure storage. Straightforward.

---

104-134: clearExpiredSessions
Properly checks for expired sessions, removing them in manageable batches. This ensures that corrupted entries are also removed, which is beneficial for data integrity.

---

136-143: Cleanup timer
Periodic disposal of expired sessions is a nice strategy. Evaluate if user activity might require a more dynamic or event-based approach.

---

145-150: Dispose pattern
Cancels the timer to avoid memory leaks. Good practice.

lib/services/mostro_service.dart (13)

2-3: Imports for hashing and hex encoding
No issues found. Usage of crypto and convert is consistent and correct for cryptographic hashing and hex operations.

---

9-9: New import for session manager
Integration with the new SessionManager aligns with the key management refactoring in the PR.

---

14-14: Addition of SessionManager field
Storing _sessionManager as a final property centralizes session handling within MostroService.

---

16-16: Constructor updated to accept SessionManager
This change injects session capabilities into the service and keeps responsibilities clear.

---

19-28: Consider adding error handling in the subscription flow
Decryption errors in asyncMap will propagate as stream errors. This might be acceptable, but ensure the UI or calling code can gracefully handle and display such errors.

---

32-34: Simple pass-through
Fetching the session by order ID is straightforward. No issues noted.

---

39-39: Enhanced code structure for order-taking
Creating a new session for each order and leveraging newMessage clarifies the workflow for takeSellOrder. It improves readability and maintainability.

Also applies to: 48-49

---

65-68: Symmetry with takeSellOrder
Implementation parallels takeSellOrder with consistent handling for takeBuyOrder.

---

73-88: Robust handling of partial or full privacy
This method cleanly adjusts content based on the fullPrivacy flag. Signing logic is correct for the hashed JSON. Consider adding checks if message['order'] is null or malformed.

---

94-95: cancelOrder procedure
Straightforward usage of newMessage with Action.cancel. No immediate issues.

---

99-101: sendFiatSent utility
Consistent approach to building and sending the message. No concerns found.

---

103-106: releaseOrder procedure
Same pattern of message creation and sending, consistent with other actions.

---

146-161: Potential mismatch in key usage
createRumor is always passed session.tradeKey as the sender key pair, even when fullPrivacy is false and keySet = session.masterKey. Confirm this aligns with your intended cryptographic design and usage of keys.

lib/shared/utils/nostr_utils.dart (1)

198-227: Orchestrated encryption for NIP-59 events
This multi-step flow (rumor, seal, wrap) looks well-structured. Validate that isValidPrivateKey supports all key types (e.g., BIP32, ephemeral) to avoid cryptographic edge cases.

lib/app/app_settings.dart (2)

2-2: Renamed property for privacy
Switching from isFirstLaunch to fullPrivacyMode better reflects the feature’s function.

---

4-4: Mandatory constructor parameter
Enforcing initialization of fullPrivacyMode ensures its state is never ambiguous.

lib/features/key_manager/key_manager_errors.dart (2)

1-7: MasterKeyNotFoundException
Clear exception for missing master key scenarios. The custom message aids debugging.

---

9-15: TradeKeyDerivationException
Targeted exception class for derivation failures. Separating it from other errors improves maintainability.

lib/shared/providers/storage_providers.dart (2)

5-7: secureStorageProvider introduction
Providing a single FlutterSecureStorage instance across the app ensures consistency and easier testing.

---

9-11: sharedPreferencesProvider stub
Throwing UnimplementedError is an effective placeholder. Just ensure it’s overridden properly in main so no runtime error occurs.

lib/shared/providers/session_manager_provider.dart (2)

1-4: Imports look good.
All necessary imports are present for Riverpod, session management, key management, and storage.

---

6-10: Provider definition is coherent.
The sessionManagerProvider correctly composes the SessionManager by injecting required dependencies (keyManager, secureStorage) via ref.read. This approach aligns well with Riverpod best practices.

lib/features/order/providers/order_notifier_provider.dart (2)

1-4: Imports are appropriate.
Essential imports for the notifier, models, and repository have been included. Satisfies the requirements for Riverpod-based state management.

---

6-15: Use of StateNotifierProvider.family is correct.
The code properly constructs an OrderNotifier based on the orderId, ensuring each order is tracked separately. The usage of .watch() to retrieve the repository is clean and aligns with Riverpod.

lib/shared/notifiers/app_settings_controller.dart (3)

3-4: Imports updated to use shared providers and typed storage keys.
This is an improvement in maintainability. Explicit references to storage_keys.dart and storage_providers.dart reduce the chance of typos and mismatches.

---

14-16: Default privacy mode set to true.
By defaulting to true, users start with full privacy mode if no preference is set. Ensure this matches the desired UX.

---

18-18: State copy is straightforward.
Updating AppSettings with fullPrivacyMode is consistent with the new setting logic.

lib/features/key_manager/key_manager_provider.dart (1)

7-15: Key manager composition is sound.
Dependencies are retrieved from secureStorageProvider and sharedPreferencesProvider, encapsulated in KeyStorage, and combined with a specific derivation path in KeyDerivator. The pattern is consistent and modular.

lib/shared/providers/mostro_service_provider.dart (1)

16-17: Confirm that the secureStorageProvider meets all storage requirements.
While passing secureStorage to MostroRepository is logical, verify that MostroRepository is fully aware of encryption and data lifecycles. For example, confirm that this storage is suitable for all repository interactions where sensitive data is handled, and that error handling is in place to manage read/write failures.

lib/data/models/enums/storage_keys.dart (2)

1-25: Validate string-to-enum conversion logic.
Enums with a _valueMap significantly reduce the risk of typos when referencing storage keys. Double-check that each key's string correctly matches the usage throughout the codebase, especially around key_index vs. full_privacy. Additionally, ensure that the thrown ArgumentError is handled gracefully upstream.

---

27-53: Ensure consistency across secure storage keys.
The structure mirrors that of SharedPreferencesKeys, which is great for consistency. Review possible future expansions (e.g., ephemeral keys, refresh tokens) to confirm the naming scheme remains straightforward and predictable. This uniform approach helps maintain clarity and prevents collisions.

lib/features/key_manager/key_storage.dart (3)

5-9: Confirm that both FlutterSecureStorage and SharedPreferencesAsync align with security requirements.
Depending on the threat model, verify that storing the trade key index in shared preferences doesn't leak sensitive usage patterns. Consider disclaimers or documentation if partial data exposure is acceptable.

---

14-16: Balance convenience with security on read operations.
Reading the master key can be sensitive. Confirm that there are guardrails in place if this is called from multiple points, and that your code never logs or exposes the returned value.

---

30-32: Default index is 1; verify this matches the upstream logic.
Ensure the logic that increments or checks this index correlates with your app’s derivation strategy. If the app expects an initial index of 0 in certain scenarios, this could lead to subtle off-by-one issues.

lib/main.dart (2)

3-3: Ensure proper usage of these new imports.

The imports for flutter_secure_storage, storage_providers, and shared_preferences introduce secure storage and preference management. Confirm that platform differences (e.g., iOS Keychain vs. Android Keystore) are properly handled and that the new providers are used consistently throughout the codebase.

Also applies to: 8-8, 10-10

---

26-27: Double-check error handling on provider overrides.

While overriding these providers works for injecting shared preferences and secure storage, consider adding error trapping in case initialization fails (e.g., device-specific storage issues). This ensures you can handle such incidents gracefully during runtime.

test/models/order_test.dart (1)

38-38: Validate changes in JSON structure.

Switching from content to payload for accessing order reflects a new JSON structure. Ensure all usage across the codebase is consistently updated to avoid missing references to content. Also confirm that older references to content or a hybrid structure won't cause runtime errors or test failures.

lib/features/key_manager/key_derivator.dart (5)

1-4: Confirm library compatibility and updates.

These imports (bip39, bip32, convert, dart_nostr) rely on active maintenance and correct versioning. Ensure you're using up-to-date, stable versions of these cryptographic libraries and confirm they don’t conflict with each other or with the rest of the application.

---

11-12: Non-validated derivation path.

While you accept derivationPath in the constructor, confirm that you handle incorrect or unexpected formats. You might wish to add a format check or at least some default in case the derivation path is invalid.

---

13-16: Leverage cryptographic randomness best practices.

Your generateMnemonic() function delegates to bip39.generateMnemonic(). Ensure that the underlying random source meets cryptographic-grade randomness. In general, bip39 is secure, but be sure it remains updated.

---

21-27: Validate child index usage.

masterPrivateKeyFromMnemonic derives a child path by appending /0. Ensure this aligns with your specs for identity mode vs. ephemeral trade keys. Some standards use m/44'/... while others might differ. Confirm the correct path for your use case.

---

44-48: Check for off-by-one risk in key pair derivation.

privateToPublicKey uses dart_nostr to generate a public key. Confirm you’re deriving the correct index for your ephemeral or identity keys before calling this method to avoid accidental mismatch or reuse.

lib/data/models/mostro_message.dart (2)

19-20: Ensure backward compatibility.

You’re still writing 'request_id': id in the JSON. If older code expects 'request_id', confirm that it also handles the new field name 'id' properly or consider renaming the JSON key if consistent with your domain model.

---

30-35: Verify robust error handling and fallback logic.

Your factory method transitions from 'content' to 'payload', checks for 'cant-do', extracts 'tradeIndex', and sets 'id'. Ensure all edge cases are covered—especially if order, payload, or cant-do is missing or incomplete. This also applies to the new fields 'trade_index' and 'id', which might be absent in older or partial messages.

Also applies to: 41-42, 45-51

lib/features/take_order/notifiers/take_buy_order_notifier.dart (1)

39-39: Use caution with null assertion on state.id!.
While it's likely handled by upstream logic, forcing a null check with ! can throw an unhandled exception if id becomes unexpectedly null. Consider adding validation or a fallback to ensure safe navigation.

lib/app/app.dart (3)

10-10: Good addition of app_init_provider.dart.
Importing app_init_provider is essential for asynchronous initialization. Ensure that it remains a stable dependency to prevent circular imports.

---

17-17: Initialization state handling is well-structured.
Storing the initAsyncValue in a local variable clarifies the read operation from the provider. This improves readability compared to chaining method calls.

---

19-44: Handle final route changes in data state carefully.
Inside the data branch, you listen to the auth state and navigate the user accordingly. Confirm that the app won't re-trigger identical navigations when the AuthState transitions remain unchanged. If re-navigations are problematic, consider gating repeated calls based on the prior state.

lib/features/take_order/notifiers/take_sell_order_notifier.dart (1)

36-36: Method signature update to allow int? amount is consistent.
Ensuring amount can be null aligns with optional user inputs; confirm your repository and upstream calls can handle this scenario gracefully.

test/services/key_derivator_test.dart (6)

1-3: Test file organization looks good.
Import statements are minimal and purposeful. Great job keeping test dependencies clean.

---

4-6: Documentation references are clear and helpful.
Linking to NIP-06 ensures future readers can understand these test vectors.

---

7-9: Logical test grouping.
Creating a dedicated group for NIP-06 tests clarifies the purpose. Keep grouping consistent for future expansions.

---

10-26: Test Vector 1 covers essential checks.
The mnemonic validity, private key derivation, and public key checks comprehensively validate the first example. No improvements needed here.

---

28-44: Test Vector 2 demonstrates consistency.
Another thorough coverage of mnemonic validity, private, and public key derivations. Good job verifying a second vector for robust coverage.

---

52-63: Child key derivation test is thorough.
Verifying the derived key length alongside the exact expected hex fosters confidence in correctness.

lib/features/add_order/notifiers/add_order_notifier.dart (1)

30-31: Consider double-checking ID initialization to avoid null usage.
Currently, the ID is set to null, presumably awaiting assignment from an external source (e.g., a back-end response). Ensure that subsequent logic accounts for a potentially null ID, or supply a placeholder value if needed to prevent any runtime crashes before the id is set.

lib/data/models/order.dart (2)

36-36: Confirm that a default premium of 0 meets expected business logic.
Previously, the default was 1, so changing it to 0 could affect pricing or calculations downstream. Ensure no features implicitly rely on premium being 1.

---

54-55: Always including min_amount and max_amount might introduce null fields.
If the server or other clients expect these fields to be absent rather than null, it could cause data format issues. Verify that sending them as null is safe.

pubspec.yaml (2)

75-75: Check for compatibility of bip32 ^2.0.0 with existing cryptographic flows.
Switching from bip32_bip44 to bip32 might impact key derivation. Verify that all modules expecting BIP44 functionalities are updated accordingly.

---

92-92: Make use of build_runner for efficient code generation.
Having added build_runner can streamline code generation tasks (e.g., JSON serializable). Verify your build scripts are updated to leverage this dependency.

Reviewing files that changed from the base of the PR and between 3d17946 and f295e4a.

• lib/features/key_manager/key_manager_provider.dart (1 hunks)
• lib/features/take_order/notifiers/take_sell_order_notifier.dart (1 hunks)
• lib/features/take_order/screens/add_lightning_invoice_screen.dart (2 hunks)

• lib/features/take_order/notifiers/take_sell_order_notifier.dart
• lib/features/key_manager/key_manager_provider.dart

lib/features/take_order/screens/add_lightning_invoice_screen.dart (1)

6-6: LGTM! Clean import additions.

The new imports are well-organized and properly support the enhanced order management functionality.

Also applies to: 10-10

Reviewing files that changed from the base of the PR and between f295e4a and d81f1cb.

• lib/features/take_order/screens/add_lightning_invoice_screen.dart (2 hunks)

lib/features/take_order/screens/add_lightning_invoice_screen.dart (2)

6-6: LGTM! Import statements are properly organized.

The new imports are necessary for the enhanced cancel order flow and follow proper conventions.

Also applies to: 10-10

---

63-92: ⚠️ Potential issue

Fix critical issues in the cancel order flow.

There are several issues in the current implementation:

1. mostroRepo.cancelOrder(orderId) is called twice (before and inside try-catch)
2. The navigation call after try-catch is unreachable
3. Navigation occurs even if cancellation fails

Apply this diff to fix the issues:

-                        mostroRepo.cancelOrder(orderId);
                         try {
                           if (message == null) {
                             context.go('/');
                             return;
                           }
                           
                           // Dispose notifier first
                           switch (message.action) {
                             case action.Action.takeBuy:
                               ref.read(takeBuyOrderNotifierProvider(orderId).notifier).dispose();
                               break;
                             case action.Action.takeSell:
                               ref.read(takeSellOrderNotifierProvider(orderId).notifier).dispose();
                               break;
                             default:
                               // Log unexpected action type
                               break;
                           }
                           
                           // Then cancel order
                           mostroRepo.cancelOrder(orderId);
                           context.go('/');
                         } catch (e) {
                           // Show error to user
                           ScaffoldMessenger.of(context).showSnackBar(
                             SnackBar(content: Text('Failed to cancel order: ${e.toString()}')),
                           );
-                        }
-                        context.go('/');
+                        }

Run this script to check for similar patterns in other files:

✅ Verification successful

Confirmed: Double cancellation issue is isolated to this file

The review comment is correct. The double cancellation and navigation issues are unique to add_lightning_invoice_screen.dart. Other files in the codebase implement the cancel order flow correctly with a single call to cancelOrder.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for potential double cancellation patterns in other files
# Look for files that might have similar cancel order implementations

rg -l "cancelOrder.*context\.go" 

Length of output: 34

---

Script:

#!/bin/bash
# Search for cancelOrder calls and their context
rg "cancelOrder" -A 5 -B 5

# Search for files containing both mostroRepo and context.go
rg -l "mostroRepo.*context\.go|context\.go.*mostroRepo"

Length of output: 6261