Send right payload on range order release #283
Conversation
|
Warning Rate limit exceeded@grunch has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughNextTrade JSON moved from a map to a two-element list and its fromJson now accepts dynamic and validates list input. KeyManager gained getNextKeyIndex(). MostroService attaches NextTrade for range orders. OrderNotifier subscribes to public events and expands timeout/cancellation cleanup. Tests, mocks, and Android minSdk handling updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant MostroService
participant OrderStore as OrderNotifier(provider)
participant KeyManager
participant Crypto as KeyDerivation
User->>MostroService: releaseOrder(orderId)
MostroService->>OrderStore: read order state (orderId)
alt range order (min & max, min < max)
MostroService->>KeyManager: getNextKeyIndex()
KeyManager-->>MostroService: nextIndex
MostroService->>Crypto: derive next trade pubkey (nextIndex)
Crypto-->>MostroService: nextPubkey
MostroService-->>User: publish order with payload NextTrade([key,index])
else non-range
MostroService-->>User: publish order without NextTrade
end
sequenceDiagram
autonumber
participant OrderNotifier
participant PublicEvents as PublicEventsStream
participant Storage
participant UI as UI/Router
Note over OrderNotifier: init -> subscribe to public events
OrderNotifier->>PublicEvents: subscribe()
PublicEvents-->>OrderNotifier: event
OrderNotifier->>Storage: fetch & sort messages (with timeout)
OrderNotifier->>OrderNotifier: _checkTimeoutAndCleanup(latestEvent)
alt should cleanup (public cancel / timeout)
OrderNotifier->>UI: invalidate provider / navigate / delete session
else maker timeoutReversal
OrderNotifier->>Storage: persist reversal message
OrderNotifier->>UI: show reversal notification / update state
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/features/order/notfiers/order_notifier.dart (1)
41-53: Guard microtask timeout checks with the same reentrancy flag._handleEvent() can race with _subscribeToPublicEvents() as only the latter uses _isProcessingTimeout. Reuse the same guard to avoid duplicated cleanup and inconsistent state transitions.
- Future.microtask(() async { - final shouldCleanup = await _checkTimeoutAndCleanup(state, event); - if (shouldCleanup && mounted) { - // Session was cleaned up, invalidate this provider - ref.invalidateSelf(); - } - }); + Future.microtask(() async { + if (_isProcessingTimeout) { + logger.d('Timeout processing already in progress for order $orderId'); + return; + } + _isProcessingTimeout = true; + try { + final shouldCleanup = await _checkTimeoutAndCleanup(state, event); + if (shouldCleanup && mounted) { + ref.invalidateSelf(); + } + } finally { + _isProcessingTimeout = false; + } + });
🧹 Nitpick comments (5)
lib/features/order/notfiers/order_notifier.dart (1)
161-166: Remove unused latestGiftWrap parameter from _checkTimeoutAndCleanup.The param is only null-checked and otherwise unused. Simplify the API and call sites.
- Future<bool> _checkTimeoutAndCleanup( - OrderState currentState, MostroMessage? latestGiftWrap) async { - if (latestGiftWrap == null) { - return false; - } + Future<bool> _checkTimeoutAndCleanup(OrderState currentState) async {Update callers:
- final shouldCleanup = await _checkTimeoutAndCleanup(state, event); + final shouldCleanup = await _checkTimeoutAndCleanup(state);- final shouldCleanup = - await _checkTimeoutAndCleanup(state, latestGiftWrap) + final shouldCleanup = + await _checkTimeoutAndCleanup(state)Also applies to: 374-381, 46-53
test/services/mostro_service_test.dart (3)
237-242: Remove duplicate stubs for publishEvent.The double when(...).thenAnswer(...) is redundant and can hide test intent.
- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future<void>.value()); - - when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value()); + when(mockNostrService.publishEvent(any)) + .thenAnswer((_) async => Future<void>.value());- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future<void>.value()); - - when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value()); + when(mockNostrService.publishEvent(any)) + .thenAnswer((_) async => Future<void>.value());Also applies to: 300-305
145-149: Fix misleading variable name: it’s hex, not base64.Minor clarity improvement for signature verification helper.
- final messageBase64 = hex.encode(jsonString.codeUnits); - - return NostrKeyPairs.verify(userPubKey, messageBase64, signatureHex); + final messageHex = hex.encode(jsonString.codeUnits); + return NostrKeyPairs.verify(userPubKey, messageHex, signatureHex);
394-449: Range detection test is good; add a releaseOrder NextTrade assertion.Validate that releaseOrder attaches NextTrade only for valid ranges and serializes as [key, index].
I can add a TestableMostroService that captures the MostroMessage passed to publishOrder and assert:
- for min < max: payload is NextTrade and NextTrade.toJson() == {'next_trade': [pubkey, index]}
- otherwise: payload == null.
Also consider a NextTrade round-trip test (toJson -> fromJson).
lib/services/mostro_service.dart (1)
155-177: Correctly attach NextTrade only for range orders.Logic matches min/max semantics; index allocation via KeyManager and deriving pubkey from that index is sound.
Extract detection into a helper for reuse and readability:
bool _isRangeOrder(Order? o) => o?.minAmount != null && o?.maxAmount != null && o!.minAmount! < o.maxAmount!;Then call it from releaseOrder.
Also applies to: 183-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
lib/data/models/next_trade.dart(1 hunks)lib/features/key_manager/key_manager.dart(1 hunks)lib/features/order/notfiers/order_notifier.dart(9 hunks)lib/services/mostro_service.dart(4 hunks)test/services/mostro_service_test.dart(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
lib/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
lib/**/*.dart: Use S.of(context) for all user-facing strings (no hardcoded UI text)
Target zero flutter analyze issues; remove unused imports and use latest non-deprecated APIs (e.g., withValues() over withOpacity())
Check mounted before using BuildContext after async gaps
Prefer const constructors and const widgets where possible
Files:
lib/features/key_manager/key_manager.dartlib/services/mostro_service.dartlib/data/models/next_trade.dartlib/features/order/notfiers/order_notifier.dart
test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under test/ with Mockito-generated mocks as needed
Files:
test/services/mostro_service_test.dart
🧠 Learnings (7)
📚 Learning: 2025-06-04T19:35:20.209Z
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/services/mostro_service.darttest/services/mostro_service_test.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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/services/mostro_service.darttest/services/mostro_service_test.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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.
Applied to files:
lib/services/mostro_service.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/mocks.mocks.dart : Do not modify test/mocks.mocks.dart; it already contains file-level ignores
Applied to files:
test/services/mostro_service_test.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/**/*.dart : Place unit tests under test/ with Mockito-generated mocks as needed
Applied to files:
test/services/mostro_service_test.dart
📚 Learning: 2025-06-26T15:03:23.529Z
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/order_notifier.dart
📚 Learning: 2025-07-20T23:33:17.689Z
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/order_notifier.dart
⏰ 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 (10)
lib/features/key_manager/key_manager.dart (1)
110-115: Make getNextKeyIndex atomic and clarify return valueFuture<int> getNextKeyIndex() async { - final currentIndex = await getCurrentKeyIndex(); - await setCurrentKeyIndex(currentIndex + 1); - - return currentIndex + 1; + final currentIndex = await getCurrentKeyIndex(); + final nextIndex = currentIndex + 1; + await setCurrentKeyIndex(nextIndex); + return nextIndex; }• To prevent lost increments under concurrency, consider exposing an atomic
incrementAndGetKeyIndex()in your storage layer and calling it here.
• Run a parallel stress test againstgetNextKeyIndex()to confirm monotonic, gapless results.lib/features/order/notfiers/order_notifier.dart (5)
15-16: Good state flags for reentrancy.Defensive flags for sync and timeout processing look right.
26-27: Helpful structured logging.Event/action log improves traceability.
246-264: Resilient persistence with timeout: good.Persisting a timeout-reversal message and tolerating timeouts prevents state loss across restarts.
333-341: Nice concurrency guard in subscription path._preventing overlapping cleanup runs avoids reentry bugs.
401-409: Notification keys are fine (not user-facing strings).Keys are routed to a notifier; no violation of UI i18n guidelines.
lib/services/mostro_service.dart (2)
14-17: Required imports look correct.
216-218: Structured send log: LGTM.lib/data/models/next_trade.dart (2)
15-16: List-based payload output aligns with spec intent.
19-27: Support legacy map shape in NextTrade.fromJson
Add a branch to handleMap<String, dynamic>withkeyandindexfields alongside the existingListparsing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/services/mostro_service_test.dart (3)
141-145: Fix mismatch: comment says SHA-256, code hex-encodes JSON; variable name is misleading.Keep behavior but make it self-consistent to avoid future verification bugs.
- // Compute SHA-256 hash of the message JSON + // Hex-encode the JSON bytes for signing/verification (matches client test signing) final jsonString = jsonEncode(messageContent); - final messageBase64 = hex.encode(jsonString.codeUnits); - - return NostrKeyPairs.verify(userPubKey, messageBase64, signatureHex); + final messageHex = hex.encode(jsonString.codeUnits); + return NostrKeyPairs.verify(userPubKey, messageHex, signatureHex);
321-322: Use a well-formed hex signature to avoid brittle failures.Passing non-hex can throw inside crypto libs. Provide a valid-but-incorrect signature instead.
- signatureHex: 'validSignatureReused', + signatureHex: identityKeyPair.sign('deadbeef'), // valid hex, won't match the message
180-203: Assert published event payload
Afterawait mostroService.takeSellOrder(...), capture the argument passed tomockNostrService.publishEventand verify its decoded JSON matchesmessageContent:final captured = verify(mockNostrService.publishEvent(captureAny)).captured.single; final sent = jsonDecode((captured as Event).content) as Map<String, dynamic>; expect(sent, equals(messageContent));
🧹 Nitpick comments (5)
android/app/build.gradle (1)
92-101: Avoid shipping “release” with debug signing.Fallback to debug signing for release builds is risky; prefer failing unless explicitly allowed via a Gradle property.
- } else { - // Fallback to debug signing for development - signingConfig signingConfigs.debug - logger.warn("Release build using debug signing - keystore not available") - } + } else { + if (project.hasProperty('allowDebugSigningFallback') && + project.property('allowDebugSigningFallback') == 'true') { + signingConfig signingConfigs.debug + logger.warn("Release build using debug signing - keystore not available") + } else { + throw new GradleException("Keystore not available. Aborting release build. Set -PallowDebugSigningFallback=true to override.") + } + }test/services/mostro_service_test.dart (4)
234-239: Remove duplicate stubbing of publishEvent.Redundant stubs add noise and can mask intent.
- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value());
297-302: Remove duplicate stubbing of publishEvent.- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value());
357-362: Remove duplicate stubbing of publishEvent.- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value());
448-470: Make child-order recognition test exercise real code paths.Instead of raw key equality, drive the notifier/service logic that marks an order as child-of (e.g., via a helper that inspects trade keys from sessions).
Example:
- seed a session with
tradeKey- feed a new order/event to the notifier
- assert it’s recognized as child using the public API/state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
android/app/build.gradle(1 hunks)lib/services/mostro_service.dart(4 hunks)test/services/mostro_service_test.dart(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/services/mostro_service.dart
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under test/ with Mockito-generated mocks as needed
Files:
test/services/mostro_service_test.dart
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
PR: MostroP2P/mobile#270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
android/app/build.gradle
📚 Learning: 2025-06-04T19:35:20.209Z
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:
test/services/mostro_service_test.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/**/*.dart : Place unit tests under test/ with Mockito-generated mocks as needed
Applied to files:
test/services/mostro_service_test.dart
⏰ 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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
android/app/build.gradle (1)
86-87: Good guard for minSdk floor; aligns with flutter_secure_storage API 23+.This prevents accidental builds targeting <23. Consider logging when an override happens to aid CI diagnostics.
You can emit a build-time note:
- // Required by flutter_secure_storage (needs API 23+) - minSdkVersion Math.max(flutter.minSdkVersion as int, 23) + // Required by flutter_secure_storage (needs API 23+) + def _requested = flutter.minSdkVersion as int + def _effective = Math.max(_requested, 23) + if (_effective > _requested) { + logger.lifecycle("minSdkVersion raised from ${_requested} to ${_effective} to satisfy flutter_secure_storage (API 23+).") + } + minSdkVersion _effective
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
android/app/build.gradle(1 hunks)test/services/mostro_service_test.dart(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/services/mostro_service_test.dart
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-08-15T01:37:12.243Z
Learnt from: Catrya
PR: MostroP2P/mobile#270
File: lib/shared/widgets/order_filter.dart:133-135
Timestamp: 2025-08-15T01:37:12.243Z
Learning: The MostroP2P/mobile project requires Flutter >=3.27.0 as specified in pubspec.yaml, which supports the Color.withValues() method, so usage of withValues() throughout the codebase is valid and should not be flagged as a compatibility issue.
Applied to files:
android/app/build.gradle
⏰ 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
aligns with flutter_secure_storage API 23+. This prevents accidental builds targeting <23. Consider logging when an override happens to aid CI diagnostics.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/services/mostro_service_test.dart (3)
135-157: Don’t reconstruct messages; assert what MostroService actually emitsserverVerifyMessage validates a locally re-built JSON, not the service’s output. Capture the published MostroMessage and assert its JSON/action/payload.
Consider using TestableReleaseOrderService (below) for all message assertions instead of recomputing messageContent.
217-272: Invalid-signature test builds its own payloadSame concern: verify against the emitted message/signature rather than a locally forged one.
274-331: Trade-index reuse scenario should derive content from emitted messageYou pre-seed the index store, but still validate a locally built JSON. Capture the service’s message and then call serverVerifyMessage on that JSON/signature.
♻️ Duplicate comments (2)
test/services/mostro_service_test.dart (2)
160-215: These “take-sell” tests still don’t validate the service outputThey assert cryptographic acceptance of a handcrafted JSON. Capture and assert the actual emitted message instead.
400-411: You defined a capture-friendly MostroService but never use itUse TestableReleaseOrderService to verify the release payload (core PR goal).
Add tests for range vs non-range release and assert next_trade presence/shape:
@@ group('MostroService Integration Tests', () { + test('release attaches next_trade only for range orders', () async { + // Arrange + final captured = <MostroMessage>[]; + when(mockRef.read(keyManagerProvider)).thenReturn(mockKeyManager); + when(mockKeyManager.getNextKeyIndex()).thenAnswer((_) async => 5); + + // Session with trade key + final mnemonic = keyDerivator.generateMnemonic(); + final xprv = keyDerivator.extendedKeyFromMnemonic(mnemonic); + final userPriv = keyDerivator.derivePrivateKey(xprv, 0); + final tradePriv = keyDerivator.derivePrivateKey(xprv, 1); + final identity = NostrKeyPairs(private: userPriv); + final tradeKey = NostrKeyPairs(private: tradePriv); + const orderId = 'range-release-order'; + + mockSessionNotifier.setMockSession(Session( + startTime: DateTime.now(), + masterKey: identity, + keyIndex: 1, + tradeKey: tradeKey, + orderId: orderId, + fullPrivacy: false, + )); + + // Use service that captures publishOrder + final service = TestableReleaseOrderService(mockRef, captured); + + // Act: for a range order (min < max), next_trade must be included + // Ensure Order provider/state yields a range order as needed by service. + // If MostroService reads order details elsewhere, stub that accordingly. + await service.releaseOrder(orderId); + + // Assert + expect(captured.length, 1); + final json = captured.single.toJson(); + expect(json['order']['action'], 'release'); + final nextTrade = json['order']['payload']['next_trade']; + expect(nextTrade, isA<List>()); + expect(nextTrade.length, 2); + expect(nextTrade[0], identity.public); // or tradeKey.public depending on spec + expect(nextTrade[1], 5); + + // Reset capture and emulate non-range order (min == max or nulls) + captured.clear(); + // Stub order as non-range in the same way the service inspects it. + await service.releaseOrder(orderId); + final json2 = captured.single.toJson(); + expect(json2['order']['action'], 'release'); + expect(json2['order']['payload'].containsKey('next_trade'), isFalse); + });If the service infers "range" from Order state via order_notifier_provider, stub mockRef.read(orderNotifierProvider(orderId).notifier) to return a MockOrderNotifier whose state/order has minAmount/maxAmount for the first call and equal/null for the second.
I can adjust the stubs to match your concrete Order state lookup if you share that path.
🧹 Nitpick comments (3)
test/mocks.dart (1)
143-151: Unsubscribing doesn’t cancel underlying stream subscriptionsunsubscribeByType/remove only drop the entry but never cancel the StreamSubscription, which may leak listeners in longer-lived tests.
Apply:
@override void unsubscribeByType(SubscriptionType type) { - _subscriptions.remove(type); + final sub = _subscriptions.remove(type); + if (sub != null) { + await sub.streamSubscription.cancel(); + sub.onCancel(); + } } @override void unsubscribeAll() { - _subscriptions.clear(); + for (final sub in _subscriptions.values) { + sub.streamSubscription.cancel(); + sub.onCancel(); + } + _subscriptions.clear(); }test/services/mostro_service_helper_functions.dart (1)
20-34: Strengthen structural validation to include trade_indexSince serverVerifyMessage requires order.trade_index, validateMessageStructure should check for it too.
- // Check for required fields in 'order' - final requiredFields = ['version', 'id', 'action', 'payload']; + // Check for required fields in 'order' + final requiredFields = ['version', 'id', 'action', 'payload', 'trade_index']; for (var field in requiredFields) { if (!order.containsKey(field)) return false; }Optionally also validate that if payload.next_trade exists, it’s a 2-element list [String pubkey, int index].
test/services/mostro_service_test.dart (1)
362-367: Remove duplicate stubpublishEvent is stubbed twice consecutively.
- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
android/app/build.gradle(1 hunks)test/mocks.dart(2 hunks)test/mocks.mocks.dart(29 hunks)test/services/mostro_service_helper_functions.dart(2 hunks)test/services/mostro_service_test.dart(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/build.gradle
🧰 Additional context used
📓 Path-based instructions (3)
test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under test/ with Mockito-generated mocks as needed
Files:
test/services/mostro_service_helper_functions.darttest/mocks.darttest/services/mostro_service_test.darttest/mocks.mocks.dart
test/**/*.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Never manually edit Mockito-generated *.mocks.dart files (especially test/mocks.mocks.dart)
Files:
test/mocks.mocks.dart
test/mocks.mocks.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Do not modify test/mocks.mocks.dart; it already contains file-level ignores
Files:
test/mocks.mocks.dart
🧠 Learnings (10)
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/mocks.mocks.dart : Do not modify test/mocks.mocks.dart; it already contains file-level ignores
Applied to files:
test/services/mostro_service_helper_functions.darttest/mocks.darttest/services/mostro_service_test.darttest/mocks.mocks.dart
📚 Learning: 2025-06-04T19:35:20.209Z
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:
test/services/mostro_service_helper_functions.darttest/services/mostro_service_test.darttest/mocks.mocks.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/**/*.dart : Place unit tests under test/ with Mockito-generated mocks as needed
Applied to files:
test/services/mostro_service_helper_functions.darttest/mocks.darttest/services/mostro_service_test.darttest/mocks.mocks.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/**/*.mocks.dart : Never manually edit Mockito-generated *.mocks.dart files (especially test/mocks.mocks.dart)
Applied to files:
test/mocks.darttest/services/mostro_service_test.darttest/mocks.mocks.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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:
test/mocks.darttest/services/mostro_service_test.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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.
Applied to files:
test/services/mostro_service_test.darttest/mocks.mocks.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to lib/features/relays/relays_notifier.dart : Use removeRelayWithBlacklist for Mostro/default relays and removeRelay for user relays; maintain dual storage sync via _saveRelays()
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-08-21T14:45:43.974Z
Learnt from: Catrya
PR: MostroP2P/mobile#272
File: lib/features/relays/widgets/relay_selector.dart:13-15
Timestamp: 2025-08-21T14:45:43.974Z
Learning: In the Mostro mobile app's RelaySelector widget (lib/features/relays/widgets/relay_selector.dart), watching relaysProvider.notifier correctly triggers rebuilds because the relaysProvider itself depends on settingsProvider (line 8 in relays_provider.dart). When blacklist changes via toggleMostroRelayBlacklist(), the settingsProvider updates, causing relaysProvider to rebuild, which then notifies widgets watching the notifier. The UI correctly reflects active/inactive states in real-time through this dependency chain.
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to lib/services/nostr_service.dart : Route all Nostr protocol interactions through NostrService (services/nostr_service.dart)
Applied to files:
test/mocks.mocks.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to lib/core/models/relay_list_event.dart : Parse and validate NIP-65 kind 10002 events with null-safe handling and WebSocket-only URLs
Applied to files:
test/mocks.mocks.dart
⏰ 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)
test/mocks.dart (2)
20-21: OK to mock Order state/notifier for testsImporting OrderState and OrderNotifier here is appropriate for Mockito generation supporting the new scenarios.
40-43: Good additions to @GenerateMocksAdding OrderState, OrderNotifier, and NostrKeyPairs aligns mocks with the test needs around release/range and signing flows.
test/services/mostro_service_helper_functions.dart (1)
36-49: Factory helper looks finecreateTestOrder provides a convenient way to generate range and fixed-amount orders for tests.
test/services/mostro_service_test.dart (1)
49-51: Registering a KeyManager dummy is good; also stub keyManagerProvider where usedrelease flow will call KeyManager.getNextKeyIndex(); ensure mockRef.read(keyManagerProvider) returns mockKeyManager and stub getNextKeyIndex().
You already create mockKeyManager; add:
when(mockRef.read(nostrServiceProvider)).thenReturn(mockNostrService); +when(mockRef.read(keyManagerProvider)).thenReturn(mockKeyManager); +when(mockKeyManager.getNextKeyIndex()).thenAnswer((_) async => 5);test/mocks.mocks.dart (2)
295-303: Type alignment with RelayInformations/Order/OrderInfo updatesThe updated return types align with public API shifts; compilation should pass.
1-1: Verify generated mocks file remains unchanged
Regenerate mocks withflutter pub run build_runner build --delete-conflicting-outputsand confirmtest/mocks.mocks.darthas no diffs to ensure it wasn’t hand-edited.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/services/mostro_service_test.dart (3)
61-63: Avoid double “dummy vs mock” SessionNotifier wiring.You provide a global dummy SessionNotifier here and later return a different mock from the provider in setUp. Drop the top-level provideDummy to prevent confusion.
- final dummySessionNotifier = MockSessionNotifier( - dummyRef, dummyKeyManager, dummySessionStorage, dummySettings); - provideDummy<SessionNotifier>(dummySessionNotifier); + // Defer SessionNotifier wiring to setUp() via mockRef.read(...).
148-151: Comment and signing input don’t match.The code hex-encodes JSON; the comment says “Compute SHA-256 hash.” Either compute the hash or fix the comment. Given this PR isn’t about crypto, prefer fixing the comment to avoid misleading readers.
- // Compute SHA-256 hash of the message JSON + // Encode the message JSON as hex (NOTE: production Nostr signs the event id per NIP-01) final jsonString = jsonEncode(messageContent); final messageHex = hex.encode(jsonString.codeUnits); - return NostrKeyPairs.verify(userPubKey, messageHex, signatureHex); + return NostrKeyPairs.verify(userPubKey, messageHex, signatureHex);
359-361: Duplicate stub of publishEvent.Second stub is redundant; remove for clarity.
- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future.value());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/services/mostro_service_test.dart(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under test/ with Mockito-generated mocks as needed
Files:
test/services/mostro_service_test.dart
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-06-04T19:35:20.209Z
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:
test/services/mostro_service_test.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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:
test/services/mostro_service_test.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/mocks.mocks.dart : Do not modify test/mocks.mocks.dart; it already contains file-level ignores
Applied to files:
test/services/mostro_service_test.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/**/*.dart : Place unit tests under test/ with Mockito-generated mocks as needed
Applied to files:
test/services/mostro_service_test.dart
⏰ 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)
test/services/mostro_service_test.dart (7)
8-8: Import OK.Needed for KeyManager stubbing in tests.
21-21: Import OK.Required for TestableReleaseOrderService.publishOrder signature.
87-87: Derivation path: confirm it matches production for tests that assert pubkeys.If production uses a different path, pubkeys/indices may differ and break next_trade assertions. Align or document.
95-95: Provider stub OK.NostrService is read via provider and properly mocked.
103-103: Listener stub OK.Prevents provider listen from throwing in tests.
111-113: SessionNotifier provider wiring OK.Ensures MostroService reads the mocked notifier.
116-118: SubscriptionManager provider wiring OK.
and validate next_trade payload.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
test/services/mostro_service_test.dart (1)
405-433: Nice! Dedicated group to capture publishOrder for release flow.
This directly exercises MostroService.releaseOrder without duplicating logic—addressing earlier feedback.
🧹 Nitpick comments (1)
test/services/mostro_service_test.dart (1)
451-453: Nit: publishEvent stubs are unnecessary in this group.
publishOrder is overridden; these stubs add noise. Safe to remove.- when(mockNostrService.publishEvent(any)) - .thenAnswer((_) async => Future<void>.value()); + // No-op: publishOrder is overridden in TestableReleaseOrderService.Also applies to: 513-515
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/services/mostro_service_test.dart(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
test/**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Place unit tests under test/ with Mockito-generated mocks as needed
Files:
test/services/mostro_service_test.dart
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-06-04T19:35:20.209Z
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:
test/services/mostro_service_test.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/**/*.dart : Place unit tests under test/ with Mockito-generated mocks as needed
Applied to files:
test/services/mostro_service_test.dart
📚 Learning: 2025-05-06T15:49:26.443Z
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:
test/services/mostro_service_test.dart
📚 Learning: 2025-08-24T16:01:53.137Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T16:01:53.137Z
Learning: Applies to test/mocks.mocks.dart : Do not modify test/mocks.mocks.dart; it already contains file-level ignores
Applied to files:
test/services/mostro_service_test.dart
⏰ 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 (5)
test/services/mostro_service_test.dart (5)
8-8: Imports look correct and align tests with new interfaces.
KeyManager, MostroMessage, Order, OrderState, providers, and enums are appropriately imported to exercise release flow.Also applies to: 21-28
50-58: Good Mockito dummies for KeyManager and OrderState.
Prevents generic type issues and enables stubbing in later tests.
125-132: Provider wiring for SessionNotifier and SubscriptionManager is sound.
Ensures the service under test resolves dependencies via ref.read().
162-165: Signature verification helper matches wire format.
Hex-encoding the JSON string and verifying with NostrKeyPairs is consistent with the protocol’s “JSON-serialized string + signature” model.
559-570: TestableReleaseOrderService approach is clean and focused.
Capturing MostroMessage isolates payload validation and avoids over-mocking.
| test('releaseOrder with range amounts includes next_trade payload', | ||
| () async { | ||
| // Arrange | ||
| const orderId = 'range-order-id'; | ||
|
|
||
| // Stub keyManagerProvider to return our mock | ||
| when(mockRef.read(keyManagerProvider)).thenReturn(mockKeyManager); | ||
|
|
||
| // Stub mockKeyManager.getNextKeyIndex() to return 5 | ||
| when(mockKeyManager.getNextKeyIndex()).thenAnswer((_) async => 5); | ||
|
|
||
| // Stub mockKeyManager.deriveTradeKeyFromIndex to return a mock key pair | ||
| when(mockKeyManager.deriveTradeKeyFromIndex(5)) | ||
| .thenAnswer((_) async => masterKey); | ||
|
|
||
| // Stub mockNostrService.publishEvent | ||
| when(mockNostrService.publishEvent(any)) | ||
| .thenAnswer((_) async => Future<void>.value()); | ||
|
|
||
| // Mock order state with range amounts | ||
| final mockOrder = Order( | ||
| id: orderId, | ||
| status: Status.active, | ||
| kind: OrderType.sell, | ||
| fiatCode: 'USD', | ||
| fiatAmount: 100, | ||
| paymentMethod: 'Lightning', | ||
| amount: 100, | ||
| minAmount: 50, | ||
| maxAmount: 150, | ||
| premium: 0, | ||
| createdAt: DateTime.now().millisecondsSinceEpoch ~/ 1000, | ||
| expiresAt: | ||
| DateTime.now().add(Duration(hours: 1)).millisecondsSinceEpoch ~/ | ||
| 1000, | ||
| ); | ||
|
|
||
| final mockOrderState = OrderState( | ||
| status: Status.active, | ||
| action: Action.release, | ||
| order: mockOrder, | ||
| ); | ||
|
|
||
| // Stub the order notifier provider to return our mock | ||
| when(mockRef.read(orderNotifierProvider(orderId))) | ||
| .thenReturn(mockOrderState); | ||
|
|
||
| // Act | ||
| await svc.releaseOrder(orderId); | ||
|
|
||
| // Assert | ||
| expect(capturedMessages, hasLength(1)); | ||
| final message = capturedMessages.first; | ||
| expect(message.action, equals(Action.release)); | ||
| expect(message.id, equals(orderId)); | ||
|
|
||
| final payload = message.toJson()['payload']; | ||
| expect(payload, contains('next_trade')); | ||
| expect(payload['next_trade'], equals([masterKey.public, 5])); | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Strengthen assertions and interaction checks.
Validate list shape and that KeyManager methods were actually invoked.
Apply this diff:
@@
- final payload = message.toJson()['payload'];
- expect(payload, contains('next_trade'));
- expect(payload['next_trade'], equals([nextTradeKey.public, 5]));
+ final payload = message.toJson()['payload'] as Map<String, dynamic>;
+ expect(payload.containsKey('next_trade'), isTrue);
+ final nt = payload['next_trade'] as List;
+ expect(nt.length, 2);
+ expect(nt[0], nextTradeKey.public);
+ expect(nt[1], 5);
+ verify(mockKeyManager.getNextKeyIndex()).called(1);
+ verify(mockKeyManager.deriveTradeKeyFromIndex(5)).called(1);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/services/mostro_service_test.dart around lines 435 to 495, strengthen
the assertions and interaction checks for the releaseOrder test by validating
the exact shape and contents of the next_trade payload and confirming KeyManager
interactions: assert payload['next_trade'] is a List with length 2, that
payload['next_trade'][0] equals masterKey.public and payload['next_trade'][1]
equals 5 (and optionally check types), and add verifies that
mockKeyManager.getNextKeyIndex() was called once and
mockKeyManager.deriveTradeKeyFromIndex(5) was called once to ensure those
methods were invoked.
| test( | ||
| 'releaseOrder with equal min and max amounts does not include next_trade payload', | ||
| () async { | ||
| // Arrange | ||
| const orderId = 'fixed-order-id'; | ||
|
|
||
| // Stub keyManagerProvider to return our mock | ||
| when(mockRef.read(keyManagerProvider)).thenReturn(mockKeyManager); | ||
|
|
||
| // Stub mockKeyManager.getNextKeyIndex() to return 5 | ||
| when(mockKeyManager.getNextKeyIndex()).thenAnswer((_) async => 5); | ||
|
|
||
| // Stub mockKeyManager.deriveTradeKeyFromIndex to return a mock key pair | ||
| when(mockKeyManager.deriveTradeKeyFromIndex(5)) | ||
| .thenAnswer((_) async => masterKey); | ||
|
|
||
| // Stub mockNostrService.publishEvent | ||
| when(mockNostrService.publishEvent(any)) | ||
| .thenAnswer((_) async => Future<void>.value()); | ||
|
|
||
| // Mock order state with fixed amount | ||
| final mockOrder = Order( | ||
| id: orderId, | ||
| status: Status.active, | ||
| kind: OrderType.sell, | ||
| fiatCode: 'USD', | ||
| fiatAmount: 100, | ||
| paymentMethod: 'Lightning', | ||
| amount: 100, | ||
| minAmount: 100, | ||
| maxAmount: 100, | ||
| premium: 0, | ||
| createdAt: DateTime.now().millisecondsSinceEpoch ~/ 1000, | ||
| expiresAt: | ||
| DateTime.now().add(Duration(hours: 1)).millisecondsSinceEpoch ~/ | ||
| 1000, | ||
| ); | ||
|
|
||
| final mockOrderState = OrderState( | ||
| status: Status.active, | ||
| action: Action.release, | ||
| order: mockOrder, | ||
| ); | ||
|
|
||
| // Stub the order notifier provider to return our mock | ||
| when(mockRef.read(orderNotifierProvider(orderId))) | ||
| .thenReturn(mockOrderState); | ||
|
|
||
| // Act | ||
| await svc.releaseOrder(orderId); | ||
|
|
||
| // Assert | ||
| expect(capturedMessages, hasLength(1)); | ||
| final message = capturedMessages.first; | ||
| expect(message.action, equals(Action.release)); | ||
| expect(message.id, equals(orderId)); | ||
|
|
||
| final payload = message.toJson()['payload']; | ||
| expect(payload, isNull); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also assert that next_trade logic is skipped for fixed amounts.
Verify no key-rotation calls occur and make the payload assertion resilient.
Apply this diff:
@@
- final payload = message.toJson()['payload'];
- expect(payload, isNull);
+ final payload = message.toJson()['payload'];
+ // Payload may be null or an empty map depending on implementation.
+ if (payload == null) {
+ expect(payload, isNull);
+ } else {
+ expect((payload as Map<String, dynamic>).containsKey('next_trade'), isFalse);
+ }
+ verifyNever(mockKeyManager.getNextKeyIndex());
+ verifyNever(mockKeyManager.deriveTradeKeyFromIndex(any));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test( | |
| 'releaseOrder with equal min and max amounts does not include next_trade payload', | |
| () async { | |
| // Arrange | |
| const orderId = 'fixed-order-id'; | |
| // Stub keyManagerProvider to return our mock | |
| when(mockRef.read(keyManagerProvider)).thenReturn(mockKeyManager); | |
| // Stub mockKeyManager.getNextKeyIndex() to return 5 | |
| when(mockKeyManager.getNextKeyIndex()).thenAnswer((_) async => 5); | |
| // Stub mockKeyManager.deriveTradeKeyFromIndex to return a mock key pair | |
| when(mockKeyManager.deriveTradeKeyFromIndex(5)) | |
| .thenAnswer((_) async => masterKey); | |
| // Stub mockNostrService.publishEvent | |
| when(mockNostrService.publishEvent(any)) | |
| .thenAnswer((_) async => Future<void>.value()); | |
| // Mock order state with fixed amount | |
| final mockOrder = Order( | |
| id: orderId, | |
| status: Status.active, | |
| kind: OrderType.sell, | |
| fiatCode: 'USD', | |
| fiatAmount: 100, | |
| paymentMethod: 'Lightning', | |
| amount: 100, | |
| minAmount: 100, | |
| maxAmount: 100, | |
| premium: 0, | |
| createdAt: DateTime.now().millisecondsSinceEpoch ~/ 1000, | |
| expiresAt: | |
| DateTime.now().add(Duration(hours: 1)).millisecondsSinceEpoch ~/ | |
| 1000, | |
| ); | |
| final mockOrderState = OrderState( | |
| status: Status.active, | |
| action: Action.release, | |
| order: mockOrder, | |
| ); | |
| // Stub the order notifier provider to return our mock | |
| when(mockRef.read(orderNotifierProvider(orderId))) | |
| .thenReturn(mockOrderState); | |
| // Act | |
| await svc.releaseOrder(orderId); | |
| // Assert | |
| expect(capturedMessages, hasLength(1)); | |
| final message = capturedMessages.first; | |
| expect(message.action, equals(Action.release)); | |
| expect(message.id, equals(orderId)); | |
| final payload = message.toJson()['payload']; | |
| expect(payload, isNull); | |
| }); | |
| }); | |
| test( | |
| 'releaseOrder with equal min and max amounts does not include next_trade payload', | |
| () async { | |
| // Arrange | |
| const orderId = 'fixed-order-id'; | |
| // Stub keyManagerProvider to return our mock | |
| when(mockRef.read(keyManagerProvider)).thenReturn(mockKeyManager); | |
| // Stub mockKeyManager.getNextKeyIndex() to return 5 | |
| when(mockKeyManager.getNextKeyIndex()).thenAnswer((_) async => 5); | |
| // Stub mockKeyManager.deriveTradeKeyFromIndex to return a mock key pair | |
| when(mockKeyManager.deriveTradeKeyFromIndex(5)) | |
| .thenAnswer((_) async => masterKey); | |
| // Stub mockNostrService.publishEvent | |
| when(mockNostrService.publishEvent(any)) | |
| .thenAnswer((_) async => Future<void>.value()); | |
| // Mock order state with fixed amount | |
| final mockOrder = Order( | |
| id: orderId, | |
| status: Status.active, | |
| kind: OrderType.sell, | |
| fiatCode: 'USD', | |
| fiatAmount: 100, | |
| paymentMethod: 'Lightning', | |
| amount: 100, | |
| minAmount: 100, | |
| maxAmount: 100, | |
| premium: 0, | |
| createdAt: DateTime.now().millisecondsSinceEpoch ~/ 1000, | |
| expiresAt: | |
| DateTime.now().add(Duration(hours: 1)).millisecondsSinceEpoch ~/ | |
| 1000, | |
| ); | |
| final mockOrderState = OrderState( | |
| status: Status.active, | |
| action: Action.release, | |
| order: mockOrder, | |
| ); | |
| // Stub the order notifier provider to return our mock | |
| when(mockRef.read(orderNotifierProvider(orderId))) | |
| .thenReturn(mockOrderState); | |
| // Act | |
| await svc.releaseOrder(orderId); | |
| // Assert | |
| expect(capturedMessages, hasLength(1)); | |
| final message = capturedMessages.first; | |
| expect(message.action, equals(Action.release)); | |
| expect(message.id, equals(orderId)); | |
| final payload = message.toJson()['payload']; | |
| // Payload may be null or an empty map depending on implementation. | |
| if (payload == null) { | |
| expect(payload, isNull); | |
| } else { | |
| expect((payload as Map<String, dynamic>).containsKey('next_trade'), isFalse); | |
| } | |
| verifyNever(mockKeyManager.getNextKeyIndex()); | |
| verifyNever(mockKeyManager.deriveTradeKeyFromIndex(any)); | |
| }); |
Reference: https://mostro.network/protocol/release.html
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores