feat: re-apply dual send for transport v2 (Phase B) without Riverpod 3.x#632
feat: re-apply dual send for transport v2 (Phase B) without Riverpod 3.x#632AndreaDiazCorreia wants to merge 8 commits into
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 <fjcalderon@gmail.com>
…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
|
Warning Review limit reached
More reviews will be available in 32 minutes and 22 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
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
Sequence Diagram(s)sequenceDiagram
participant AddOrderNotifier
participant RestoreService
participant MostroMessage
participant NostrUtils
participant MostroNode
AddOrderNotifier->>RestoreService: acquireSessionLock()
RestoreService-->>AddOrderNotifier: release callback
AddOrderNotifier->>MostroMessage: wrapForTransport(protocolVersion, tradeKey, masterKey, ...)
MostroMessage->>MostroMessage: resolveTransport(protocolVersion)
alt transport == nip44
MostroMessage->>NostrUtils: encryptNIP44([messageMap, tradeSig, identityProof])
NostrUtils-->>MostroMessage: ciphertext
MostroMessage-->>AddOrderNotifier: kind 14 event
else transport == giftWrap
MostroMessage-->>AddOrderNotifier: kind 1059 event
end
AddOrderNotifier->>MostroNode: publish(event)
MostroNode-->>RestoreService: restore response (kind 14 or 1059)
RestoreService->>NostrUtils: decrypt or unwrap response
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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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"]]}]
2 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