Fix/child range orders add #311
Conversation
- Added parentOrderId tracking to Session so range-order children can inherit maker context and persist to storage (lib/data/models/session.dart:15-157). - Expanded SessionNotifier with a pre-release child session cache and helper APIs to link the child order once mostrod confirms it; state emission now keeps subscriptions in sync and is heavily commented for clarity (lib/shared/notifiers/session_notifier.dart:15-255). - Extended MostroService to prepare child sessions during releaseOrder and to link them immediately when the encrypted Action.newOrder arrives, spinning up the downstream OrderNotifier on the spot (lib/services/mostro_service.dart:44-200). Documentation - Rewrote the child-order sections in docs/architecture/SESSION_AND_KEY_MANAGEMENT.md so they describe the simplified service-driven flow instead of the outdated notifier-based approach. Add agents file for AI agents
WalkthroughAdds contributor docs and detailed session/key management for child orders: pre-creates and links child-order sessions, extends Session with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI
participant MostroService
participant SessionNotifier
participant SessionStore as Session/State
participant Mostro as Mostro Network
User->>UI: Release range order
UI->>MostroService: releaseOrder(session)
MostroService->>Mostro: Send release (may include NextTrade payload)
alt Session has role and next trade key
MostroService->>SessionNotifier: createChildOrderSession(tradeKey, keyIndex, parentOrderId, role)
SessionNotifier->>SessionStore: Persist pending child session (indexed by tradeKeyPublic)
else Missing role/key
MostroService->>MostroService: Log and skip pre-create
end
Mostro-->>MostroService: DM: new-order(childOrderId, tradeKeyPublic)
MostroService->>MostroService: _maybeLinkChildOrder(message, session)
alt Pending child matches tradeKeyPublic
MostroService->>SessionNotifier: linkChildSessionToOrderId(childOrderId, tradeKeyPublic)
SessionNotifier->>SessionStore: Move pending -> active, attach orderId
MostroService->>Mostro: Subscribe/notify UI for child order
else No match / already linked
MostroService->>MostroService: No-op/drop message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/data/models/session.dart (1)
43-52: Critical: toJson/fromJson mismatch in Session
- lib/data/models/session.dart — toJson (≈lines 43–52) writes 'trade_key' as a public String and omits 'master_key'; Session.fromJson (≈54–57) requires both 'master_key' and 'trade_key' as NostrKeyPairs. Deserializing JSON produced by toJson will fail.
- Preferred fix: keep private keys out of storage. Modify Session.fromJson to accept trade_key as a public String (nullable), remove master_key from requiredFields, and have SessionStorage (lib/data/repositories/session_storage.dart) use KeyManager to reconstruct NostrKeyPairs by keyIndex when hydrating Session instances.
- Alternative: add explicit public-only fields (master_key_public, trade_key_public) and adjust fromJson to use those.
- I can provide a concrete diff for the preferred approach.
🧹 Nitpick comments (5)
AGENTS.md (1)
5-7: Nice, concise contributor workflow.Docs cover the essentials (analyze, format, unit/integration tests). Consider making “flutter test” a hard pre-push requirement and documenting a simple pre-push hook to enforce it for consistency with team practice.
I can draft a pre-push hook snippet if you want it added to the repo.
lib/shared/notifiers/session_notifier.dart (1)
179-188: Defensive removals are good; minor enhancement.After removing the in-memory mappings, you emit state—good. Optional: return a boolean to indicate if anything was removed to aid callers.
lib/data/models/session.dart (1)
136-148: parent_order_id parsing LGTM; guard remains consistent.Optional: trim whitespace on parent_order_id before accepting.
Also applies to: 149-159
lib/services/mostro_service.dart (1)
86-109: Helper is correct but idempotency depends on upstream dedup.You rely on eventStore.hasItem() for dedup—fine. Consider logging at debug for the happy path to reduce noise.
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (1)
410-410: Fix markdown lint: add languages to fenced code blocks (MD040).Specify a language for each fence (dart, text, json, sh) to satisfy markdownlint and improve readability.
Example:
-``` +```text ... -``` +```Also applies to: 474-474, 615-615, 641-641, 878-878, 893-893, 906-906, 943-943
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AGENTS.md(1 hunks)docs/architecture/SESSION_AND_KEY_MANAGEMENT.md(1 hunks)lib/data/models/session.dart(4 hunks)lib/services/mostro_service.dart(2 hunks)lib/shared/notifiers/session_notifier.dart(9 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Run flutter format and keep code formatted
Files:
lib/shared/notifiers/session_notifier.dartlib/services/mostro_service.dartlib/data/models/session.dart
🧠 Learnings (5)
📚 Learning: 2025-07-20T23:45:23.811Z
Learnt from: chebizarro
PR: MostroP2P/mobile#151
File: lib/services/trade_history_restoration_service.dart:67-67
Timestamp: 2025-07-20T23:45:23.811Z
Learning: In the Mostro protocol, recipient keys (trade keys) are used only once, ensuring no collisions when merging session maps in trade history restoration.
Applied to files:
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md
📚 Learning: 2025-09-18T17:17:38.944Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T17:17:38.944Z
Learning: Use Riverpod for all state management and encapsulate business logic in Notifiers
Applied to files:
lib/shared/notifiers/session_notifier.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/shared/notifiers/session_notifier.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:
lib/services/mostro_service.dart
📚 Learning: 2025-09-18T17:17:38.944Z
Learnt from: CR
PR: MostroP2P/mobile#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-18T17:17:38.944Z
Learning: Always run `flutter test` after any code change (mandatory before commits)
Applied to files:
AGENTS.md
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md
410-410: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
474-474: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
615-615: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
641-641: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
878-878: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
893-893: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
906-906: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
943-943: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (11)
lib/shared/notifiers/session_notifier.dart (8)
21-25: Good addition: pending child session cache.Clear intent and keying by tradeKey.public is appropriate for quick lookup and subscription.
54-60: State emission includes all three maps—watch for duplicates.If the same Session instance is present in multiple maps, UI consumers may see duplicates. You already clean up in saveSession/cleanup; just confirm no path can re-add the same instance to multiple maps.
81-86: Cleanup covers pending child sessions.LGTM. Cutoff-based eviction is consistent with persisted session cleanup.
123-127: Correct removal from transient maps on save.Removing by identical() prevents transient duplication after persistence.
156-161: Trade key lookup order makes sense.Prioritizing active, then pending, then request sessions is reasonable.
195-201: Temporary request session cleanup is robust.Cleans all maps and emits state with a useful log.
203-231: Pending child sessions are in-memory only—confirm restart behavior.If the app is background-killed between release and new-order arrival, the child DM cannot be decrypted (no session → no subscription). Is that acceptable for now?
I can propose a lightweight persistence strategy (separate store keyed by tradeKey.public) if restarts must be tolerated.
233-255: Linking flow is tight and side-effecting correctly.Moves session to active map, persists, emits, and boots the OrderNotifier downstream. Solid.
lib/data/models/session.dart (1)
16-19: Field addition is appropriate and documented.parentOrderId captures the parent-child relationship cleanly.
lib/services/mostro_service.dart (2)
80-81: Good: link child order immediately after decrypt.This closes the timing gap and ensures OrderNotifier starts promptly.
199-212: Pre-creating child session on release is the right place.Role inheritance and use of getNextKeyIndex/deriveTradeKeyFromIndex are sensible.
| _logger.i( | ||
| 'Received DM, Event ID: ${decryptedEvent.id} with payload: ${decryptedEvent.content}', | ||
| ); | ||
|
|
There was a problem hiding this comment.
Avoid logging decrypted payloads at info level (privacy).
decryptedEvent.content can include sensitive data (e.g., invoices). Log only action/id or move to debug with redaction.
Apply this diff:
- _logger.i(
- 'Received DM, Event ID: ${decryptedEvent.id} with payload: ${decryptedEvent.content}',
- );
+ _logger.d(
+ 'Received DM, id=${decryptedEvent.id}, action=${msg.action}, orderId=${msg.id}',
+ );📝 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.
| _logger.i( | |
| 'Received DM, Event ID: ${decryptedEvent.id} with payload: ${decryptedEvent.content}', | |
| ); | |
| _logger.d( | |
| 'Received DM, id=${decryptedEvent.id}, action=${msg.action}, orderId=${msg.id}', | |
| ); |
🤖 Prompt for AI Agents
In lib/services/mostro_service.dart around lines 76 to 79, the current
info-level log prints decryptedEvent.content which may contain sensitive data;
change the log to only include non-sensitive identifiers (e.g.,
decryptedEvent.id and action/type) at info level and either remove content
entirely or move it to a debug-level log after redacting sensitive fields.
Implement by replacing the logger.i call to log a minimal message like "Received
DM, Event ID: <id>, action: <action>" and, if you need the payload for
troubleshooting, add a logger.d call that uses a
redactPayload(decryptedEvent.content) helper to strip/obfuscate sensitive values
before logging.
Refactored the duplicated range-order child preparation: both sendFiatSent and releaseOrder now call _prepareChildOrderIfNeeded, which centralises the key-derivation and session pre-creation
Remove redundant data
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (2)
628-657: Call out key-index reservation explicitly to avoid reuse.Add a note that, after pre-creating a child session, the KeyManager index must be advanced (e.g., setCurrentKeyIndex(nextIndex + 1)) to prevent future sessions from reusing the same index.
410-410: Specify fenced code block languages (MD040).Add language identifiers to the shown code fences (e.g., text, dart, bash) to satisfy markdownlint and improve readability.
Example fixes:
- Triple-backtick “text” for ASCII flow diagrams
- “dart” for Dart code samples
- “bash” for shell snippets
Also applies to: 474-474, 607-607, 633-633, 888-888, 901-901
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md(1 hunks)lib/services/mostro_service.dart(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.dart
📄 CodeRabbit inference engine (CLAUDE.md)
Run flutter format and keep code formatted
Files:
lib/services/mostro_service.dart
🧠 Learnings (1)
📚 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.dart
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md
410-410: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
474-474: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
607-607: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
633-633: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
888-888: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
901-901: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (4)
lib/services/mostro_service.dart (3)
86-109: LGTM: Child-order linking on decrypt is correct and contained.The guard on action/id and parent context is right; spinning up OrderNotifier immediately keeps subscriptions in sync.
187-199: LGTM: releaseOrder now reuses shared preparation helper.Reduces duplication and centralizes range-order child prep.
76-81: Do not log decrypted payloads at info level; log only non-sensitive identifiers.This still prints decryptedEvent.content at info. Move payload to debug (or remove) and log only action/id minimally. Also keep linkage call.
Apply:
- _logger.i( - 'Received DM, Event ID: ${decryptedEvent.id} with payload: ${decryptedEvent.content}', - ); + _logger.d( + 'Received DM, id=${decryptedEvent.id}, action=${msg.action}, orderId=${msg.id}', + );docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (1)
402-601: Docs content is clear and aligned with code.The problem statement, flow, and responsibilities are well explained; the pre-emptive child-session design is coherent.
| Future<void> sendFiatSent(String orderId) async { | ||
| final payload = await _prepareChildOrderIfNeeded( | ||
| orderId, | ||
| callerLabel: 'fiatSent', | ||
| ); | ||
|
|
||
| await publishOrder( | ||
| MostroMessage( | ||
| action: Action.fiatSent, | ||
| id: orderId, | ||
| payload: payload, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
Verify protocol support before attaching NextTrade to fiatSent.
Confirm mostrod accepts NextTrade payload on Action.fiatSent across deployed versions; if not, gate it or restrict to release only.
🌐 Web query:
Does the Mostro protocol accept a NextTrade payload on the fiatSent action, or is it only supported on release?
💡 Result:
The Mostro protocol supports a next_trade (next_trade) payload on the release action only — the release docs show payload.next_trade usage. The fiat-sent action is described separately (marks buyer as having sent fiat) and does not document a next_trade payload. [1][2]
Sources:
- Release (shows payload.next_trade on release). [1]
- How it works / fiat-sent description (fiat-sent is a distinct action; no next_trade shown). [2]
Remove NextTrade from Action.fiatSent — next_trade is only supported on release.
Mostro docs show payload.next_trade only for Action.release; fiat-sent does not accept it. Location: lib/services/mostro_service.dart (lines 171–182). Remove or gate attaching NextTrade here; emit next_trade only from the release flow or add a version/feature check so only compatible mostrod instances receive it.
🤖 Prompt for AI Agents
In lib/services/mostro_service.dart around lines 171 to 182, the fiatSent flow
is currently including a NextTrade/next_trade field in the message payload but
Mostro only accepts next_trade for Action.release; remove next_trade from
fiatSent messages or guard it behind a release-only/feature-version check.
Update the sendFiatSent path so the payload returned by
_prepareChildOrderIfNeeded does not contain next_trade (or explicitly
delete/omit that key) before creating MostroMessage(Action.fiatSent), or add a
runtime check (mostrod version/feature flag) and only attach next_trade when
sending Action.release.
There was a problem hiding this comment.
Actually NextTrade is supported by fiatSent and release actions, you can read the protocol docs docs/protocol/fiatsent.md
There was a problem hiding this comment.
🧩 Analysis chain
@grunch Thank you for the correction! Let me verify this by checking the protocol documentation you referenced.
🏁 Script executed:
#!/bin/bash
# Check if the protocol documentation exists and examine fiatSent support for NextTrade
find . -name "fiatsent.md" -type f
cat docs/protocol/fiatsent.md 2>/dev/null || echo "File not found"
Length of output: 1654
@grunch You're absolutely right! I apologize for the incorrect analysis. The protocol documentation clearly shows that NextTrade payload is supported for fiatSent action, specifically for range orders when the maker is the buyer.
The documentation shows the exact format:
{
"payload": {
"next_trade": ["<trade pubkey>", <trade index>]
}
}Thank you for the correction and for pointing me to the specific documentation. The implementation in the code is correct as designed.
✏️ Learnings added
Learnt from: grunch
PR: MostroP2P/mobile#311
File: lib/services/mostro_service.dart:171-182
Timestamp: 2025-09-22T19:42:55.306Z
Learning: NextTrade payload is supported by both fiatSent and release actions in the Mostro protocol, as documented in docs/protocol/fiatsent.md
Learnt from: grunch
PR: MostroP2P/mobile#311
File: lib/services/mostro_service.dart:171-182
Timestamp: 2025-09-22T19:42:55.306Z
Learning: NextTrade payload is supported by both fiatSent and release actions in the Mostro protocol. For fiatSent action, it's specifically used when the maker is the buyer on a range order, as documented in docs/protocol/fiatsent.md with the payload format: { "next_trade": ["<trade pubkey>", <trade index>] }
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| Future<Payload?> _prepareChildOrderIfNeeded( | ||
| String orderId, { | ||
| required String callerLabel, | ||
| }) async { | ||
| final order = ref.read(orderNotifierProvider(orderId)).order; | ||
| if (order?.minAmount == null || | ||
| order?.maxAmount == null || | ||
| order!.minAmount! >= order.maxAmount!) { | ||
| return null; | ||
| } | ||
|
|
||
| final minAmount = order.minAmount!; | ||
| final maxAmount = order.maxAmount!; | ||
| final selectedAmount = order.fiatAmount; | ||
| final remaining = maxAmount - selectedAmount; | ||
|
|
||
| if (remaining < minAmount) { | ||
| _logger.i( | ||
| '[$callerLabel] Range order $orderId exhausted (remaining $remaining < min $minAmount); skipping child preparation.', | ||
| ); | ||
| return null; | ||
| } | ||
|
|
||
| final keyManager = ref.read(keyManagerProvider); | ||
| final nextKeyIndex = await keyManager.getNextKeyIndex(); | ||
| final nextTradeKey = await keyManager.deriveTradeKeyFromIndex(nextKeyIndex); | ||
|
|
||
| final sessionNotifier = ref.read(sessionNotifierProvider.notifier); | ||
| final currentSession = sessionNotifier.getSessionByOrderId(orderId); | ||
| if (currentSession != null && currentSession.role != null) { | ||
| await sessionNotifier.createChildOrderSession( | ||
| tradeKey: nextTradeKey, | ||
| keyIndex: nextKeyIndex, | ||
| parentOrderId: orderId, | ||
| role: currentSession.role!, | ||
| ); | ||
| _logger.i( | ||
| '[$callerLabel] Prepared child session for $orderId using key index $nextKeyIndex', | ||
| ); | ||
| } else { | ||
| _logger.w( | ||
| '[$callerLabel] Unable to prepare child session for $orderId; session or role missing.', | ||
| ); | ||
| } | ||
|
|
||
| return NextTrade( | ||
| key: nextTradeKey.public, | ||
| index: nextKeyIndex, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Reserve/advance key index and avoid duplicate child-session prep (prevents key reuse and duplicate children).
Two problems:
- Key index pointer isn’t advanced after pre-allocating the child; subsequent newSession() may reuse the same index, colliding keys.
- Calling from both fiatSent and release can create multiple pending children for the same parent.
Fix by (a) short-circuiting if a pending child exists for this parent and reusing that info, and (b) advancing the key index after creating the child session.
final order = ref.read(orderNotifierProvider(orderId)).order;
if (order?.minAmount == null ||
order?.maxAmount == null ||
order!.minAmount! >= order.maxAmount!) {
return null;
}
final minAmount = order.minAmount!;
final maxAmount = order.maxAmount!;
- final selectedAmount = order.fiatAmount;
- final remaining = maxAmount - selectedAmount;
+ final selectedAmount = order.fiatAmount;
+ if (selectedAmount == null) {
+ _logger.w('[$callerLabel] Order $orderId has null fiatAmount; skipping child preparation.');
+ return null;
+ }
+ final remaining = maxAmount - selectedAmount;
if (remaining < minAmount) {
_logger.i(
'[$callerLabel] Range order $orderId exhausted (remaining $remaining < min $minAmount); skipping child preparation.',
);
return null;
}
+ // Avoid duplicate prep when called from both fiatSent and release
+ final sessions = ref.read(sessionNotifierProvider);
+ final existingPending = sessions.firstWhereOrNull(
+ (s) => s.parentOrderId == orderId && s.orderId == null,
+ );
+ if (existingPending != null) {
+ _logger.d(
+ '[$callerLabel] Pending child already exists for $orderId (keyIndex=${existingPending.keyIndex}); skipping new preparation.',
+ );
+ return NextTrade(
+ key: existingPending.tradeKey.public,
+ index: existingPending.keyIndex,
+ );
+ }
+
final keyManager = ref.read(keyManagerProvider);
final nextKeyIndex = await keyManager.getNextKeyIndex();
final nextTradeKey = await keyManager.deriveTradeKeyFromIndex(nextKeyIndex);
final sessionNotifier = ref.read(sessionNotifierProvider.notifier);
final currentSession = sessionNotifier.getSessionByOrderId(orderId);
if (currentSession != null && currentSession.role != null) {
await sessionNotifier.createChildOrderSession(
tradeKey: nextTradeKey,
keyIndex: nextKeyIndex,
parentOrderId: orderId,
role: currentSession.role!,
);
+ // Reserve/advance pointer to prevent future reuse of the just-allocated index
+ await keyManager.setCurrentKeyIndex(nextKeyIndex + 1);
_logger.i(
'[$callerLabel] Prepared child session for $orderId using key index $nextKeyIndex',
);
} else {
_logger.w(
'[$callerLabel] Unable to prepare child session for $orderId; session or role missing.',
);
}📝 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.
| Future<Payload?> _prepareChildOrderIfNeeded( | |
| String orderId, { | |
| required String callerLabel, | |
| }) async { | |
| final order = ref.read(orderNotifierProvider(orderId)).order; | |
| if (order?.minAmount == null || | |
| order?.maxAmount == null || | |
| order!.minAmount! >= order.maxAmount!) { | |
| return null; | |
| } | |
| final minAmount = order.minAmount!; | |
| final maxAmount = order.maxAmount!; | |
| final selectedAmount = order.fiatAmount; | |
| final remaining = maxAmount - selectedAmount; | |
| if (remaining < minAmount) { | |
| _logger.i( | |
| '[$callerLabel] Range order $orderId exhausted (remaining $remaining < min $minAmount); skipping child preparation.', | |
| ); | |
| return null; | |
| } | |
| final keyManager = ref.read(keyManagerProvider); | |
| final nextKeyIndex = await keyManager.getNextKeyIndex(); | |
| final nextTradeKey = await keyManager.deriveTradeKeyFromIndex(nextKeyIndex); | |
| final sessionNotifier = ref.read(sessionNotifierProvider.notifier); | |
| final currentSession = sessionNotifier.getSessionByOrderId(orderId); | |
| if (currentSession != null && currentSession.role != null) { | |
| await sessionNotifier.createChildOrderSession( | |
| tradeKey: nextTradeKey, | |
| keyIndex: nextKeyIndex, | |
| parentOrderId: orderId, | |
| role: currentSession.role!, | |
| ); | |
| _logger.i( | |
| '[$callerLabel] Prepared child session for $orderId using key index $nextKeyIndex', | |
| ); | |
| } else { | |
| _logger.w( | |
| '[$callerLabel] Unable to prepare child session for $orderId; session or role missing.', | |
| ); | |
| } | |
| return NextTrade( | |
| key: nextTradeKey.public, | |
| index: nextKeyIndex, | |
| ); | |
| } | |
| Future<Payload?> _prepareChildOrderIfNeeded( | |
| String orderId, { | |
| required String callerLabel, | |
| }) async { | |
| final order = ref.read(orderNotifierProvider(orderId)).order; | |
| if (order?.minAmount == null || | |
| order?.maxAmount == null || | |
| order!.minAmount! >= order.maxAmount!) { | |
| return null; | |
| } | |
| final minAmount = order.minAmount!; | |
| final maxAmount = order.maxAmount!; | |
| final selectedAmount = order.fiatAmount; | |
| if (selectedAmount == null) { | |
| _logger.w('[$callerLabel] Order $orderId has null fiatAmount; skipping child preparation.'); | |
| return null; | |
| } | |
| final remaining = maxAmount - selectedAmount; | |
| if (remaining < minAmount) { | |
| _logger.i( | |
| '[$callerLabel] Range order $orderId exhausted (remaining $remaining < min $minAmount); skipping child preparation.', | |
| ); | |
| return null; | |
| } | |
| // Avoid duplicate prep when called from both fiatSent and release | |
| final sessions = ref.read(sessionNotifierProvider); | |
| final existingPending = sessions.firstWhereOrNull( | |
| (s) => s.parentOrderId == orderId && s.orderId == null, | |
| ); | |
| if (existingPending != null) { | |
| _logger.d( | |
| '[$callerLabel] Pending child already exists for $orderId (keyIndex=${existingPending.keyIndex}); skipping new preparation.', | |
| ); | |
| return NextTrade( | |
| key: existingPending.tradeKey.public, | |
| index: existingPending.keyIndex, | |
| ); | |
| } | |
| final keyManager = ref.read(keyManagerProvider); | |
| final nextKeyIndex = await keyManager.getNextKeyIndex(); | |
| final nextTradeKey = await keyManager.deriveTradeKeyFromIndex(nextKeyIndex); | |
| final sessionNotifier = ref.read(sessionNotifierProvider.notifier); | |
| final currentSession = sessionNotifier.getSessionByOrderId(orderId); | |
| if (currentSession != null && currentSession.role != null) { | |
| await sessionNotifier.createChildOrderSession( | |
| tradeKey: nextTradeKey, | |
| keyIndex: nextKeyIndex, | |
| parentOrderId: orderId, | |
| role: currentSession.role!, | |
| ); | |
| // Reserve/advance pointer to prevent future reuse of the just-allocated index | |
| await keyManager.setCurrentKeyIndex(nextKeyIndex + 1); | |
| _logger.i( | |
| '[$callerLabel] Prepared child session for $orderId using key index $nextKeyIndex', | |
| ); | |
| } else { | |
| _logger.w( | |
| '[$callerLabel] Unable to prepare child session for $orderId; session or role missing.', | |
| ); | |
| } | |
| return NextTrade( | |
| key: nextTradeKey.public, | |
| index: nextKeyIndex, | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (13)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (13)
441-466: Good problem statement; fix missing fenced-code language (markdownlint MD040).The step-by-step flow block at Lines 449–466 lacks a language. Use text to satisfy linters.
-``` +```text 1. Parent Order Release ... -``` +```
513-535: Add a language to the flow block (MD040).Same as above—use text.
-``` +```text 1. Parent Order Release ... -``` +```
628-640: Ensure this is actually invoked from the decryption path.You later show _onData(), but the example doesn’t call _maybeLinkChildOrder. Add the call in that snippet to avoid confusion.
// after parsing MostroMessage msg - final msg = MostroMessage.fromJson(result[0]); + final msg = MostroMessage.fromJson(result[0]); + // Link pending child session if applicable + await _maybeLinkChildOrder(msg, matchingSession);
646-662: Add language to flow block (MD040).-``` +```text 1. releaseOrder() ... -``` +```
672-683: Add language to flow block (MD040).-``` +```text Parent Order: ... -``` +```
927-947: Add language to both flow blocks (MD040 at 927 and 940).-``` +```text User completes range order ... -``` +```-``` +```text Mostrod creates child order with ID "child-456" ... -``` +```
980-1009: Fix path typo: “notfiers” → “notifiers”.Broken path may confuse readers.
-// File: lib/features/order/notfiers/add_order_notifier.dart:71-85 +// File: lib/features/order/notifiers/add_order_notifier.dart:71-85
1000-1004: Avoid logging full public keys in production logs.Public identity/trade keys can deanonymize users. Recommend masking (e.g., first/last 6 chars) or gating behind a verbose/dev flag.
-logger.d(' - Identity key (index 0): ${session.masterKey.public}'); -logger.d(' - Trade key (index ${session.keyIndex}): ${session.tradeKey.public}'); +logger.v(' - Identity key (index 0): ${maskKey(session.masterKey.public)}'); // dev only +logger.v(' - Trade key (index ${session.keyIndex}): ${maskKey(session.tradeKey.public)}'); // dev only
1057-1106: Decryption example: explicitly show child-link step.To keep the narrative consistent, embed the call to _maybeLinkChildOrder here as well (see earlier suggestion).
- final msg = MostroMessage.fromJson(result[0]); + final msg = MostroMessage.fromJson(result[0]); + await _maybeLinkChildOrder(msg, matchingSession);
5-16: Add TOC entry for the new child-order section.Make it discoverable from the top.
- [Specific Scenarios](#specific-scenarios) +- [Child Order Session Management After Release](#child-order-session-management-after-release) - [Code Flow Examples](#code-flow-examples)
1119-1123: Update “Last updated” date or remove hard-coded date.This PR is from Sep 20, 2025; bump the date or make it part of the release process.
-*Last updated: September 15, 2025* +*Last updated: September 20, 2025*
449-449: Lint hygiene: address all MD040 occurrences in one pass.You can also switch some blocks to dart/json as appropriate for better highlighting; otherwise use text.
Also applies to: 513-513, 646-646, 672-672, 927-927, 940-940
688-696: Fallback idea (optional): soft-recovery if no pending child session exists.Consider documenting an optional recovery path: if a new-order arrives for an unknown trade key, scan recent parent releases that emitted a matching NextTrade, then synthesize the session. Can be behind a feature flag.
Also applies to: 850-900
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-20T23:45:23.811Z
Learnt from: chebizarro
PR: MostroP2P/mobile#151
File: lib/services/trade_history_restoration_service.dart:67-67
Timestamp: 2025-07-20T23:45:23.811Z
Learning: In the Mostro protocol, recipient keys (trade keys) are used only once, ensuring no collisions when merging session maps in trade history restoration.
Applied to files:
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md
🪛 markdownlint-cli2 (0.18.1)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md
449-449: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
513-513: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
646-646: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
672-672: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
927-927: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
940-940: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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)
docs/architecture/SESSION_AND_KEY_MANAGEMENT.md (6)
470-488: Root-cause snippet is clear.Reads well and is correctly labeled as the broken path. No changes needed.
606-621: Release/fiat-sent payload wiring is consistent.The helper gating NextTrade is clear.
688-696: Call out that this placeholder is resolved later.Nice contrast; reads well.
905-920: Linking flow is correct and idempotent enough.Second delivery logs a warn and exits—acceptable tradeoff.
951-973: Role inheritance code is correct and matches the described flow.
540-555: Model extension looks good — Session persists only public keys.toJson serializes public fields (e.g. 'trade_key': tradeKey.public); ripgrep found no storage writes of private/secret/priv in lib (only i18n strings mention "secret words"). computeSharedKey does produce private NostrKeyPairs but there’s no evidence it’s persisted.
| Two new methods were added to `SessionNotifier`: | ||
|
|
||
| ```dart | ||
| // lib/shared/notifiers/session_notifier.dart | ||
| /// Create a session for a child order using pre-generated trade key | ||
| Future<Session> createChildOrderSession({ | ||
| required NostrKeyPairs tradeKey, | ||
| required int keyIndex, | ||
| required String parentOrderId, | ||
| required Role role, | ||
| }) async { | ||
| final masterKey = ref.read(keyManagerProvider).masterKeyPair!; | ||
| final session = Session( | ||
| startTime: DateTime.now(), | ||
| masterKey: masterKey, | ||
| keyIndex: keyIndex, | ||
| tradeKey: tradeKey, | ||
| fullPrivacy: _settings.fullPrivacyMode, | ||
| parentOrderId: parentOrderId, | ||
| role: role, // Inherit role from parent order | ||
| ); | ||
| // Add to state but don't assign orderId yet | ||
| state = [...sessions, session]; | ||
| return session; | ||
| } | ||
| /// Link a child order session to its assigned order ID | ||
| Future<void> linkChildSessionToOrderId(String childOrderId, String tradeKeyPublic) async { | ||
| final sessionIndex = state.indexWhere((s) => | ||
| s.tradeKey.public == tradeKeyPublic && s.orderId == null && s.parentOrderId != null | ||
| ); | ||
| if (sessionIndex != -1) { | ||
| final session = state[sessionIndex]; | ||
| session.orderId = childOrderId; | ||
| _sessions[childOrderId] = session; | ||
| await _storage.putSession(session); | ||
| state = sessions; | ||
| } | ||
| } | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate to a single, authoritative SessionNotifier API section.
Keep only the _pendingChildSessions-based implementation and move rationale (why a pre-cache) into a note to prevent future reversion.
Also applies to: 762-813
🤖 Prompt for AI Agents
In docs/architecture/SESSION_AND_KEY_MANAGEMENT.md around lines 559-603 (and
also apply the same change to lines 762-813), remove the newly added
createChildOrderSession and linkChildSessionToOrderId method snippets and
instead keep the single authoritative SessionNotifier API section that uses the
pendingChildSessions-based implementation; consolidate any duplicated content
into that one section, move the rationale for pre-caching child sessions into a
short "Note" immediately below the API (no implementation code there), and
update any cross-references to point to the retained pendingChildSessions API so
the doc has one clear source of truth.
Duplicate and conflicting API docs for SessionNotifier methods. Keep one canonical version.
This block shows an older implementation (list search + state append). Later (Lines 762–813) you document the current _pendingChildSessions-based version. Drop this earlier block to avoid drift.
-/// Create a session for a child order using pre-generated trade key
-Future<Session> createChildOrderSession({ ... }) async { ... }
-
-/// Link a child order session to its assigned order ID
-Future<void> linkChildSessionToOrderId(String childOrderId, String tradeKeyPublic) async { ... }
+// (Removed: superseded by the _pendingChildSessions-based APIs documented later.)📝 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.
| Two new methods were added to `SessionNotifier`: | |
| ```dart | |
| // lib/shared/notifiers/session_notifier.dart | |
| /// Create a session for a child order using pre-generated trade key | |
| Future<Session> createChildOrderSession({ | |
| required NostrKeyPairs tradeKey, | |
| required int keyIndex, | |
| required String parentOrderId, | |
| required Role role, | |
| }) async { | |
| final masterKey = ref.read(keyManagerProvider).masterKeyPair!; | |
| final session = Session( | |
| startTime: DateTime.now(), | |
| masterKey: masterKey, | |
| keyIndex: keyIndex, | |
| tradeKey: tradeKey, | |
| fullPrivacy: _settings.fullPrivacyMode, | |
| parentOrderId: parentOrderId, | |
| role: role, // Inherit role from parent order | |
| ); | |
| // Add to state but don't assign orderId yet | |
| state = [...sessions, session]; | |
| return session; | |
| } | |
| /// Link a child order session to its assigned order ID | |
| Future<void> linkChildSessionToOrderId(String childOrderId, String tradeKeyPublic) async { | |
| final sessionIndex = state.indexWhere((s) => | |
| s.tradeKey.public == tradeKeyPublic && s.orderId == null && s.parentOrderId != null | |
| ); | |
| if (sessionIndex != -1) { | |
| final session = state[sessionIndex]; | |
| session.orderId = childOrderId; | |
| _sessions[childOrderId] = session; | |
| await _storage.putSession(session); | |
| state = sessions; | |
| } | |
| } | |
| ``` | |
| // lib/shared/notifiers/session_notifier.dart | |
| // (Removed: superseded by the _pendingChildSessions-based APIs documented later.) |
🤖 Prompt for AI Agents
In docs/architecture/SESSION_AND_KEY_MANAGEMENT.md around lines 559 to 603,
there is a duplicate older implementation of SessionNotifier methods
(createChildOrderSession + linkChildSessionToOrderId) that conflicts with the
canonical _pendingChildSessions-based version documented later; remove this
earlier block so only the current _pendingChildSessions implementation remains,
ensuring the documentation references and examples point to the single canonical
API and update any cross-references or indices if they mention the removed
methods.
| Future<void> releaseOrder(String orderId) async { | ||
| final payload = await _prepareChildOrderIfNeeded( | ||
| orderId, | ||
| callerLabel: 'release', | ||
| ); | ||
| await publishOrder( | ||
| MostroMessage( | ||
| action: Action.release, | ||
| id: orderId, | ||
| payload: payload, // null for regular orders, NextTrade for range orders | ||
| payload: payload, | ||
| ), | ||
| ); | ||
| } | ||
| ``` | ||
| ### Scenario 4: Full Privacy Mode Operation | ||
|
|
||
| #### Flow Description | ||
| User operates in full privacy mode where identity keys are never shared with mostrod. | ||
| Future<void> sendFiatSent(String orderId) async { | ||
| final payload = await _prepareChildOrderIfNeeded( | ||
| orderId, | ||
| callerLabel: 'fiatSent', | ||
| ); | ||
| #### Key Usage Log | ||
| await publishOrder( | ||
| MostroMessage( | ||
| action: Action.fiatSent, | ||
| id: orderId, | ||
| payload: payload, | ||
| ), | ||
| ); | ||
| } | ||
| ``` | ||
| 1. User enables full privacy mode | ||
| → Settings.fullPrivacyMode = true | ||
| → No reputation tracking possible | ||
|
|
||
| 2. Order creation in full privacy mode | ||
| → Session creation: fullPrivacy = true | ||
| → masterKey still generated (for local use) | ||
| → tradeKey: 06u5t4s3r2... (index 3) | ||
| ```dart | ||
| Future<Payload?> _prepareChildOrderIfNeeded( | ||
| String orderId, { | ||
| required String callerLabel, | ||
| }) async { | ||
| final order = ref.read(orderNotifierProvider(orderId)).order; | ||
| if (order?.minAmount == null || | ||
| order?.maxAmount == null || | ||
| order!.minAmount! >= order.maxAmount!) { | ||
| return null; | ||
| } | ||
| final minAmount = order.minAmount!; | ||
| final maxAmount = order.maxAmount!; | ||
| final selectedAmount = order.fiatAmount; | ||
| final remaining = maxAmount - selectedAmount; | ||
| 3. Gift wrap process (full privacy) | ||
| → Rumor: NO signature included | ||
| → Seal: signed by TRADE key (06u5t4s3r2...) instead of identity key | ||
| → Wrapper: signed by ephemeral key (07q1p0o9n8...) | ||
| if (remaining < minAmount) { | ||
| _logger.i( | ||
| '[$callerLabel] Range order $orderId exhausted (remaining $remaining < min $minAmount); child order skipped.', | ||
| ); | ||
| return null; | ||
| } | ||
| 4. Message structure difference | ||
| Standard mode: [message, trade_key_signature] | ||
| Full privacy: [message, null] | ||
| final keyManager = ref.read(keyManagerProvider); | ||
| final nextKeyIndex = await keyManager.getNextKeyIndex(); | ||
| final nextTradeKey = await keyManager.deriveTradeKeyFromIndex(nextKeyIndex); | ||
| 5. Mostrod processing | ||
| → Cannot link orders to identity | ||
| → No reputation tracking | ||
| → Order processed anonymously | ||
| final sessionNotifier = ref.read(sessionNotifierProvider.notifier); | ||
| final currentSession = sessionNotifier.getSessionByOrderId(orderId); | ||
| if (currentSession != null && currentSession.role != null) { | ||
| await sessionNotifier.createChildOrderSession( | ||
| tradeKey: nextTradeKey, | ||
| keyIndex: nextKeyIndex, | ||
| parentOrderId: orderId, | ||
| role: currentSession.role!, | ||
| ); | ||
| _logger.i( | ||
| '[$callerLabel] Prepared child session for $orderId using key index $nextKeyIndex', | ||
| ); | ||
| } else { | ||
| _logger.w( | ||
| '[$callerLabel] Unable to prepare child session for $orderId; session or role missing.', | ||
| ); | ||
| } | ||
| return NextTrade( | ||
| key: nextTradeKey.public, | ||
| index: nextKeyIndex, | ||
| ); | ||
| } | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
Key-index concurrency: document single-flight/atomicity around “next key” usage.
If multiple release/fiatSent calls race, they could select the same next index. Note in docs that getNextKeyIndex/deriveTradeKeyFromIndex is guarded (mutex/single-flight) or uses CAS on the stored index.
🏁 Script executed:
#!/bin/bash
# Inspect for locking/atomicity around key index reads/writes
rg -nP -C3 '(getNextKeyIndex|readTradeKeyIndex|storeTradeKeyIndex|setCurrentKeyIndex)' lib/features/key_manager
# Look for mutex/synchronization around _prepareChildOrderIfNeeded callers
rg -nP -C3 '_prepareChildOrderIfNeeded\(' libLength of output: 5326
Make trade-key index allocation atomic (prevent concurrent getNextKeyIndex races).
getNextKeyIndex performs a read → increment → write without synchronization; concurrent callers can receive the same index and cause trade-key reuse.
-
Evidence/locations:
- lib/features/key_manager/key_manager.dart — getNextKeyIndex (lines ~110–115) and setCurrentKeyIndex (117–125).
- lib/features/key_manager/key_storage.dart — readTradeKeyIndex / storeTradeKeyIndex (lines ~40–50).
- Call sites: lib/services/mostro_service.dart — _prepareChildOrderIfNeeded and sendFiatSent/releaseOrder (≈170–205).
-
Fix (choose one):
- Wrap the read-modify-write in a mutex/Lock (e.g., async Lock / package:synchronized) so getNextKeyIndex is single‑flight, OR
- Provide an atomic increment/CAS API in key_storage and use it from key_manager, OR
- Ensure a single-flight/dedupe at the caller so only one in-flight allocation runs.
Duplicate indices are a critical correctness/security issue and must be fixed.
🤖 Prompt for AI Agents
In docs/architecture/SESSION_AND_KEY_MANAGEMENT.md around lines 818–900 the
_prepareChildOrderIfNeeded call path uses keyManager.getNextKeyIndex which
currently does a read→increment→write without synchronization allowing
concurrent callers to receive duplicate indices; fix by making index allocation
atomic: modify key_manager.dart/getNextKeyIndex to perform the read-modify-write
while holding a process-wide async mutex (e.g., package:synchronized Lock) or by
adding an atomic increment API in key_storage and calling that from key_manager;
ensure all callers (including _prepareChildOrderIfNeeded, sendFiatSent,
releaseOrder) use the updated atomic allocation so only one allocation can run
at a time and remove any duplicate race-prone read/write code paths.
Documentation
Add agents file for AI agents
Summary by CodeRabbit
New Features
Bug Fixes
Documentation