feat: re-apply dual send for transport v2 (Phase B) without Riverpod 3.x#632
Conversation
* feat(transport): implement protocol v2 (NIP-44 direct) send path with identity proof (Phase B) * feat(transport): route all outbound Mostro sends through wrapForTransport (Phase B) * feat(restore): add dual-receive support for transport v2 (NIP-44 direct) in restore flow * test(restore): add dual-receive transport v2 regression tests for restore flow * fix(transport): bind dispute identity proof and guard restore tuple - DisputeRepository.createDispute now passes masterKey/keyIndex to wrapForTransport so reputation-mode disputes carry the v2 identity proof instead of being silently downgraded to full-privacy, matching publishOrder. - decodeRestoreMessage guards against an empty JSON tuple before indexing [0] to avoid a RangeError. - Refresh stale NIP-59-only comments in the dispute send path. --------- Co-authored-by: grunch <[email protected]>
…ion reset - Add RestoreService.isOperationInProgress and awaitOperationCompletion() to expose restore/sync state - AddOrderNotifier.submitOrder now waits for any in-flight restore to finish before creating an order - Prevents session orphaning when node-switch restore's _clearAll wipes sessions mid-order-creation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds transport-aware NIP-44 sending, updates restore flows to subscribe to and decode both v1 and v2 responses, adds session locking around restore and order submission, and updates tests and architecture docs for the revised transport behavior. ChangesNIP-44 dual-send transport v2
Estimated code review effort: 4 (Complex) | ~55 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (1)
lib/data/models/mostro_message.dart (1)
101-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDe-duplicate the
wrapperKeyselection.The
restore/orderwrapper-key logic is repeated verbatim insign(Lines 103-106),serialize(Lines 124-127), andwrapNip44(Lines 196-199). Since this key is load-bearing for the protocol signature, a future edit to one copy that misses the others would silently produce mismatched signatures. Consider extracting a single getter.♻️ Suggested helper extraction
+ // Use 'restore' key for restore and last-trade-index actions, 'order' otherwise, as per protocol. + String get _wrapperKey => + action == Action.restore || action == Action.lastTradeIndex + ? 'restore' + : 'order'; + String sign(NostrKeyPairs keyPair, {int? version}) { - //IMPORTANT : Use 'restore' key for restore and last-trade-index actions, 'order' for everything else, as per protocol - final wrapperKey = - action == Action.restore || action == Action.lastTradeIndex - ? 'restore' - : 'order'; - final message = {wrapperKey: toJson(version: version)}; + final message = {_wrapperKey: toJson(version: version)}; final serializedEvent = jsonEncode(message); return _mostroSign(serializedEvent, keyPair); }Apply the same substitution in
serializeandwrapNip44.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/data/models/mostro_message.dart` around lines 101 - 134, The wrapperKey selection logic that checks if action is restore or lastTradeIndex is duplicated across the sign method, serialize method, and wrapNip44 method. Extract this logic into a single private getter method (e.g., get wrapperKey) that returns either 'restore' or 'order' based on the action value, then replace all three instances of the duplicated wrapperKey assignment with calls to this new getter. This ensures the protocol-critical logic is maintained in a single location and prevents future signature mismatches from inconsistent edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/restore/restore_manager.dart`:
- Around line 66-80: The `awaitOperationCompletion()` method only waits for the
current operation to finish but does not prevent a new restore operation from
immediately starting afterward, leaving a race window where sessions created
after the await returns can still be wiped by `_clearAll()`. Instead of just
awaiting the current operation, use a proper async locking mechanism (such as a
Mutex) to serialize restore/sync operations and session-creating flows behind
the same critical section. This ensures that while a restore is in progress or
about to start, session creation is blocked, and vice versa, eliminating the
TOCTOU race condition rather than just waiting for completion.
---
Nitpick comments:
In `@lib/data/models/mostro_message.dart`:
- Around line 101-134: The wrapperKey selection logic that checks if action is
restore or lastTradeIndex is duplicated across the sign method, serialize
method, and wrapNip44 method. Extract this logic into a single private getter
method (e.g., get wrapperKey) that returns either 'restore' or 'order' based on
the action value, then replace all three instances of the duplicated wrapperKey
assignment with calls to this new getter. This ensures the protocol-critical
logic is maintained in a single location and prevents future signature
mismatches from inconsistent edits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d8608883-96d3-440c-8c42-04c5e6a6c049
📒 Files selected for processing (9)
docs/architecture/SESSION_RECOVERY_ARCHITECTURE.mddocs/architecture/TRANSPORT_V2_MIGRATION.mdlib/data/models/mostro_message.dartlib/data/repositories/dispute_repository.dartlib/features/order/notifiers/add_order_notifier.dartlib/features/restore/restore_manager.dartlib/services/mostro_service.darttest/data/mostro_message_nip44_test.darttest/features/restore/restore_decode_test.dart
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0226336fd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…rations - Change awaitOperationCompletion from `if` to `while` to wait for any operation that starts while we're already waiting - Prevents race where a second restore begins after the first completes but before the caller proceeds - Update doc comment to clarify looping behavior and note that restore triggers are mutually exclusive with order submission
…event v1/v2 protocol mismatch - Add _waitForNodeConnectivity call before _createTempSubscription to ensure mostroInstance is populated - Prevents wrapForTransport from defaulting to v1 gift wrap when mostroInstance is null at app init - Avoids protocol v2 nodes receiving kind 1059 requests they won't answer, which would leave local key index stale
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Verdict: Request changes
Blocking issue:
awaitOperationCompletion()only waits for the current restore to finish. A restore can start again immediately after the await returns but beforenewSession()runs, and_clearAll()will still wipe the newly created session. This needs real serialization/mutex protection around restore and session creation, not a wait-only check.
…ce in order creation - Replace awaitOperationCompletion with acquireSessionLock in AddOrderNotifier.submitOrder - Wrap session creation in try/finally with lock release to serialize with restore's _clearAll - Add _sessionLockTail future chain to RestoreService to implement critical-section lock - Hold session lock for entire restore process in initRestoreProcess - Prevents race where restore's session reset interleaves with order creation even after wait compl
There was a problem hiding this comment.
Review Summary
Verdict: Request changes
The new lock is only taken by AddOrderNotifier, but the restore path never acquires the same lock. That means _clearAll() can still interleave with newSession(), and you also release the lock before submitOrder(), so the newly created session can still be wiped before the order is published.
Please move the same lock into the restore/reset side (or hold it across the full session-creation + publish critical section) so both flows synchronize on the same primitive.
…rphaning - Move mostroService.submitOrder and state update inside try/finally lock block - Prevents race where restore's _clearAll can wipe session after creation but before publish - Ensures entire create critical section (session + publish) is atomic with respect to restore
There was a problem hiding this comment.
Review Summary
Verdict: Request changes
844fda6 fixes the original AddOrder path: submitOrder() is now inside the lock, and restore holds the same lock around _clearAll(). That closes the specific TOCTOU you asked about.
However, the invariant is still not true globally: there are other session-creating flows (lib/features/order/notifiers/order_notifier.dart → takeSellOrder() / takeBuyOrder()) that still call sessionNotifier.newSession() without acquiring the same session lock. If restore can overlap with those flows, the same session-orphaning race remains.
So this commit resolves the AddOrder race, but not all session-creation paths yet. The shared lock needs to cover every path that creates a session before I can approve.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/order/notifiers/add_order_notifier.dart`:
- Around line 116-133: The create-order critical section in
add_order_notifier.dart is released too early in the flow around submitOrder and
newSession, which lets restore cleanup clear a pending create before the first
terminal response arrives. Update the logic in the notifier path that acquires
acquireSessionLock so the lock stays held until the create request reaches its
first terminal outcome via _confirmOrder or equivalent, or otherwise ensure
pending create sessions survive _clearAll during restore. Keep the
session/sessionNotifier/newSession and mostroService.submitOrder sequence
protected until that terminal response is processed.
In `@lib/features/restore/restore_manager.dart`:
- Around line 1089-1094: `syncTradeIndex()` needs to be serialized with order
session creation so it cannot update the trade index while
`submitOrder()`/`newSession()` is running with stale state. In
`restore_manager.dart`, around `syncTradeIndex()` and `setCurrentKeyIndex(...)`,
acquire the same `acquireSessionLock()` used by `submitOrder()` (or otherwise
block order submission until this sync finishes) and hold it across the
connectivity wait and index update so `newSession()` cannot publish with an
outdated index.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9e129d10-2363-424d-adbe-6708c384a46e
📒 Files selected for processing (2)
lib/features/order/notifiers/add_order_notifier.dartlib/features/restore/restore_manager.dart
…for reuse across order flows
There was a problem hiding this comment.
Review Summary
Verdict: Approve
The shared SessionLifecycleLock now covers all session-creating paths I called out:
AddOrderNotifier.submitOrder()OrderNotifier.takeSellOrder()/takeBuyOrder()RestoreService.initRestoreProcess()around_clearAll()and the restore rebuild
That closes the TOCTOU/session-orphaning race across the current code paths. The lock is centralized, used consistently, and the critical section now includes the dependent publish step.
…e index reads - Acquire sessionLifecycleLock before updating trade index in syncTradeIndex - Prevents order/take flows from creating sessions with stale keyIndex while sync is in progress - Release lock in finally block after index update completes
There was a problem hiding this comment.
Review Summary
Verdict: Approve
The latest commit is correct: syncTradeIndex() now acquires the same shared SessionLifecycleLock, so the stale-index path is serialized with restore and the session-creating flows (add order, take buy/sell). That keeps a session from being created/published while the trade index is mid-update.
I don't see a remaining blocking issue in this head.
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
It doesn't create a new order when I try to create a new order on a mostro node with transport = nip44 and in main head:
======== Exception caught by gesture ===============================================================
The following _Exception was thrown while handling a gesture:
Exception: Nostr is not initialized. Call init() first.
When the exception was thrown, this was the stack:
#1 ProviderContainer.read (package:riverpod/src/framework/container.dart:241:21)
#2 ConsumerStatefulElement.read (package:flutter_riverpod/src/consumer.dart:620:59)
#3 _AddOrderScreenState._submitOrder (package:mostro_mobile/features/order/screens/add_order_screen.dart:504:34)
#4 _MostroReactiveButtonState._startOperation (package:mostro_mobile/shared/widgets/mostro_reactive_button.dart:96:22)
#5 _InkResponseState.handleTap (package:flutter/src/material/ink_well.dart:1224:21)
#6 GestureRecognizer.invokeCallback (package:flutter/src/gestures/recognizer.dart:345:24)
#7 TapGestureRecognizer.handleTapUp (package:flutter/src/gestures/tap.dart:758:11)
#8 BaseTapGestureRecognizer._checkUp (package:flutter/src/gestures/tap.dart:383:5)
#9 BaseTapGestureRecognizer.acceptGesture (package:flutter/src/gestures/tap.dart:353:7)
#10 GestureArenaManager.sweep (package:flutter/src/gestures/arena.dart:173:27)
#11 GestureBinding.handleEvent (package:flutter/src/gestures/binding.dart:532:20)
#12 GestureBinding.dispatchEvent (package:flutter/src/gestures/binding.dart:498:22)
#13 RendererBinding.dispatchEvent (package:flutter/src/rendering/binding.dart:473:11)
#14 GestureBinding._handlePointerEventImmediately (package:flutter/src/gestures/binding.dart:437:7)
#15 GestureBinding.handlePointerEvent (package:flutter/src/gestures/binding.dart:394:5)
#16 GestureBinding._flushPointerEventQueue (package:flutter/src/gestures/binding.dart:341:7)
#17 GestureBinding._handlePointerDataPacket (package:flutter/src/gestures/binding.dart:308:9)
#18 _invoke1 (dart:ui/hooks.dart:372:13)
#19 PlatformDispatcher._dispatchPointerDataPacket (dart:ui/platform_dispatcher.dart:467:7)
#20 _dispatchPointerDataPacket (dart:ui/hooks.dart:307:31)
#21 NostrService.subscribeToEvents (package:mostro_mobile/services/nostr_service.dart:202:7)
#22 OpenOrdersRepository._subscribeToOrders (package:mostro_mobile/data/repositories/open_orders_repository.dart:61:35)
#23 new OpenOrdersRepository (package:mostro_mobile/data/repositories/open_orders_repository.dart:39:5)
#24 orderRepositoryProvider.<anonymous closure> (package:mostro_mobile/shared/providers/order_repository_provider.dart:13:21)
#25 ProviderElementBase.read (package:riverpod/src/framework/element.dart:691:12)
#26 SubscriptionManager._initMostroInstanceListener (package:mostro_mobile/features/subscriptions/subscription_manager.dart:63:15)
#27 new SubscriptionManager (package:mostro_mobile/features/subscriptions/subscription_manager.dart:51:5)
#28 RelaysNotifier._initMostroRelaySync (package:mostro_mobile/features/relays/relays_notifier.dart:416:30)
#29 new RelaysNotifier (package:mostro_mobile/features/relays/relays_notifier.dart:44:5)
#30 relaysProvider.<anonymous closure> (package:mostro_mobile/features/relays/relays_provider.dart:10:10)
#31 ProviderContainer.read (package:riverpod/src/framework/container.dart:241:21)
#32 _initializeRelaySynchronization (package:mostro_mobile/main.dart:89:15)
#33 main (package:mostro_mobile/main.dart:69:3)
Handler: "onTap"
Recognizer: TapGestureRecognizer#f4369
debugOwner: GestureDetector
state: ready
won arena
finalPosition: Offset(295.6, 492.2)
finalLocalPosition: Offset(83.9, 12.2)
button: 1
sent tap down
====================================================================================================
[Nostr] data entity with content: ["EOSE","e9a8731714e672ada2cb747d3c578491f3188ba90b09c35efa499aa96fd4b741"] is already registered, skipping...
[Nostr] Registered data entity: ["EVENT","e9a8731714e672ada2cb747d3c578491f3188ba90b09c35efa499aa96fd4b741",{"content":"","created_at":1782162321,"id":"743e357e8a229cb15f90e0587413fa651b9ad21c1b0d02d936cff64c4fc71b05","kind":10002,"pubkey":"dbe0b1be7aafd3cfba92d7463edbd4e33b2969f61bd554d37ac56f032e13355a","sig":"80f05f44cf6852b490ae8f79525b8d10ee8f5ed883976be45925ecd7aba8a0e3e2a89321f3938b3e0ee469e9add8381f0325fd52a7edf196bc2ba0dea8af9844","tags":[["r","wss://relay.mostro.network"]]}]
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes
Blocking
sendFiatSent()/releaseOrder()create a child session through_prepareChildOrderIfNeeded(), but they are not wrapped in the sharedSessionLifecycleLock. Restore still holds that lock while clearing/rebuilding sessions, so these flows can still interleave with_clearAll()and can also read/update the trade index concurrently. That leaves the same session-orphaning / stale-index window open for range-order follow-up flows. Please serialize these paths behind the same lock (or move the lock into the helper if that covers every caller).
Context
- I re-read the full PR conversation and the latest head before re-reviewing.
- The restore/order race fix is improved, but this child-order path is still a separate session-creating flow that can collide with restore.
- In an edge case with potential loss of funds / orphaned order state, I prefer a manual workaround only if code can't make the invariant hold; here the fix is still feasible in code, so I'm requesting changes rather than accepting a manual-only resolution.
ermeme
Bot
dismissed
their stale review
Posted on the wrong PR; please ignore and treat as withdrawn.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/features/order/notifiers/order_notifier.dart`:
- Around line 92-111: The take-session protection is being released too early in
`OrderNotifier` around `takeSellOrder()`/`takeBuyOrder()`, allowing a queued
restore to clear the newly created taker session before the first terminal
response arrives. Keep the `sessionLifecycleLockProvider.withSessionLock` held
until the first response/cant-do/timeout handling completes, or update the
restore path to preserve pending taker sessions created via
`sessionNotifierProvider.newSession`. Make sure the cleanup helpers
`AbstractMostroNotifier.clearBondCancelDeletion` and
`startSessionTimeoutCleanup` still run at the right time without letting restore
delete the active session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 128732a0-1351-4b62-bcc6-adfe5db3c470
📒 Files selected for processing (5)
lib/features/order/notifiers/add_order_notifier.dartlib/features/order/notifiers/order_notifier.dartlib/features/restore/restore_manager.dartlib/shared/providers.dartlib/shared/providers/session_lifecycle_lock_provider.dart
- Deduplicate the restore/order wrapper-key logic repeated in sign(), serialize() and wrapNip44() into one _wrapperKey getter - The key is load-bearing for the message signature; a single source of truth prevents the three copies from drifting into mismatched signatures
There was a problem hiding this comment.
Code Review Summary
Verdict: Request changes
Blocking
sendFiatSent()andreleaseOrder()still bypassSessionLifecycleLock. They can create a child session via_prepareChildOrderIfNeeded()and then publish it while a restore is free to run_clearAll(), so range-order child sessions can still be wiped before they are linked/persisted. Please serialize these flows with the same lock used bysubmitOrder()/takeSellOrder()/takeBuyOrder().
| } | ||
|
|
||
| final event = await order.wrap( | ||
| // Route through the transport advertised by the connected node (§5 Phase |
There was a problem hiding this comment.
Blocking: this transport switch now covers the locked order/take paths, but sendFiatSent() / releaseOrder() still reach publishOrder() without SessionLifecycleLock. Their child-session creation in _prepareChildOrderIfNeeded() is therefore still exposed to the restore _clearAll() race; please serialize those callers too.
There was a problem hiding this comment.
the residual window is real, but I'm deferring this rather than applying the literal fix, for the following reasons:
This PR is a strict improvement. Before this change there was no session lock at all, so the take/create session was unprotected across the entire flow. The lock now makes session creation + publish atomic, which closes the large window. What remains (publish → first response) is a much smaller residual, not a regression introduced here.
"Hold the lock until the first response" has a worse trade-off. It would hold the lifecycle lock across a network round-trip (up to the 10s timeout), blocking every other session operation (other orders/takes and restore) for that duration, and it would require acquiring in the notifier and releasing in the async response/cant-do/timeout handler — if any release path is missed, the lock is stuck permanently. That's a heavier, more fragile failure mode than the rare orphan it prevents.
The realistic trigger is narrow and partly self-healing. It requires switching nodes (or importing a mnemonic) within the few seconds an in-flight take is awaiting its response:
- On a same-node restore (mnemonic import), restore rebuilds sessions from the daemon's state, so a just-taken order is re-fetched and the session self-heals.
- On a node switch, the take lived on the old node; restore queries the new node, so the loss is inherent to switching mid-trade, not something the lock can fix.
- The existing 10s startSessionTimeoutCleanup is a backstop.
Preferred fix is the other option you mention, as a follow-up: have restore preserve pending sessions (don't let _clearAll wipe sessions awaiting their first ack) instead of extending the lock across the response. That's surgical, doesn't block the UI, and avoids the stuck-lock risk. I've tracked it as a deferred follow-up in our completion plan together with the other session-creating paths (e.g. registerSessionInMemory), to be addressed in a dedicated concurrency PR rather than in this transport-v2 (Phase B) change.
There was a problem hiding this comment.
Review Summary
Verdict: Approve
I re-checked the current head after your clarification. The remaining pending-child-session window is real, but it is a pre-existing follow-up rather than a blocker introduced by this PR. The transport-v2 changes here still materially improve correctness by serializing the main create/take/restore flows behind SessionLifecycleLock, and the child-session linkage gap is best handled in a dedicated follow-up that preserves pending/awaiting-ack sessions during restore.
3 participants
Summary
Re-applies the content of #624 (dual send for transport v2, Phase B) on top of
main(post Riverpod 3.x rollback, #628), without any Riverpod 3.x changes from #613. Also includes a follow-up fix for an order-creation race surfaced while testing the restore flow.The Phase B commit is a clean cherry-pick of the original #624 delta onto the reverted (Riverpod 2.6.1) base: byte-identical to the original and with no Riverpod 3.x API usage (only
ref.read(settingsProvider), identical in 2.x/3.x).Contents
Transport v2 (Phase B) — re-apply of #624
mostro_message.dart,mostro_service.dart)mostro_message_nip44_test.dart,restore_decode_test.dart)Fix: order-creation race with node-switch restore
A node-switch restore resets all sessions (
RestoreService._clearAll→sessionNotifier.reset()). Creating an order while that restore is in flight had its session wiped, leaving the order orphaned on the daemon (it consumed a trade index but never appeared in "My Trades").RestoreService.isOperationInProgress/awaitOperationCompletion()AddOrderNotifier.submitOrdernow waits for any in-flight restore to finish before creating the order sessionVerification
flutter analyze: no issues.flutter test: all tests pass (479).protocol_version.Base
main(revert: roll back Riverpod 3.x migration (#613) and dependent PRs (#624, #625) #628 already merged). No longer depends on the revert branch.Original author: Andrea Diaz (Phase B preserved via cherry-pick).
Summary by CodeRabbit