💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 808bd1744f

Code Review Summary

Verdict: Changes Requested

Critical

• The foreground v2 branch is wired, but the background receive path is still incomplete. background_notification_service.dart::_decryptAndProcessEvent only accepts kinds 4 and 1059, and _handleTradeKeyEvent still calls event.unWrap(...). Once the resolver flips to v2, backgrounded Mostro replies will be dropped instead of notifying the user or updating the session.

Looks Good

• The transport abstraction is cleanly isolated.
• The foreground decrypt path is structured sensibly for a staged rollout.

Actionable comments posted: 1

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 `@docs/architecture/TRANSPORT_V2_MIGRATION.md`:
- Around line 21-22: The status line in the TRANSPORT_V2_MIGRATION.md document
at line 21 incorrectly states that no `lib/` code has been changed yet, but this
branch has already implemented Phase A receive-path code. Update the status text
to accurately reflect the current implementation state of the branch, replacing
the outdated "No `lib/` code has been changed yet" statement with text that
indicates Phase A receive-path code has been implemented to prevent confusion
during rollout and debugging.

Code Review Summary

Verdict: Changes Requested

Blocking issue

• resolveTransport(null) in subscription_manager.dart is still hardcoded to Transport.giftWrap, so the orders subscription never switches to kind 14 for v2 nodes. That means the new NIP-44 receive path is not actually reachable through the normal subscription flow, which defeats Phase A as described in the migration plan.

Positive

• The background isolate now handles kind 14, which fixes the earlier gap.
• The NIP-44 direct decrypt helper is cleanly isolated.

Approved. The latest commit wires protocol_version auto-detection, switches orders subscriptions to kind 14 for v2 nodes, and fixes the background receive path as well. I don't see any blocking issues on the current head.

Re-reviewed the current head: protocol_version auto-detection is wired, v2 receive is covered in foreground and background, and the added tests line up with the transport resolver. Approving again on the latest commit.

I reviewed the current head and I do see one real blocking issue before merge.

Blocking issue:

• In MostroService._onData, the storage key for persisted history is now transport-dependent: v1 uses the inner rumor id when available, but v2 falls back to the outer kind-14 event.id because there is no inner rumor.
• At the same time, MostroMessage.fromJson(result[0]) still gives you the logical Mostro message whose own id is the durable order/message identity used throughout the app.
• That means the same logical history is no longer keyed consistently across transports. Under v1 you persist under the inner id; under v2 you persist under the transport-level outer event id.

Why this matters:

• This PR is specifically about dual receive across two envelopes while keeping the logical message layer unchanged.
• If persisted history keys diverge by transport, replay/dedup/history queries can silently stop talking about the same thing even when the logical Mostro message is the same.
• In other words, the transport envelope starts leaking into storage identity, which is exactly what this migration should avoid.

What I would want:

• Persist the received message under the same logical identity across both transports, i.e. the Mostro message/order identity parsed from tuple[0], not a transport-specific outer event id.
• The outer Nostr event id can still be logged/inspected if useful, but it should not become the durable storage identity for v2 messages.

I think this is a real blocker for a dual-receive phase because otherwise the history model is no longer transport-invariant.

Review — Phase A dual receive

Reviewed the full diff at head 954a7bd1 plus every review thread. Walked the foreground receive path, the background isolate, the subscription/transport resolution, the NIP‑44 decrypt helper and the new tests. flutter analyze is clean on all 7 changed lib/ files and the two new test files pass (27/27).

Earlier review threads — all addressed

• Background kind‑14 path (codex / ermeme): fixed. _decryptAndProcessEvent now allows kind 14, _handleTradeKeyEvent branches to decryptNIP44DirectEvent, and _loadMostroPubkey reads the node pubkey from persisted AppSettings inside the isolate (no Riverpod there). The orders filter is transferred to the background service via lifecycle_manager, so a v2 node yields a kind‑14 filter there too.
• resolveTransport(null) hardcoded (ermeme): fixed. _resolveOrdersTransport() now reads the real protocol_version from the kind‑38385 info event, and _initMostroInstanceListener re‑subscribes when the info event arrives after the initial subscription. Covered by transport_test.dart and the protocol_version cases in mostro_instance_test.dart.
• Doc status line (CodeRabbit): fixed — now states Phase A is implemented.

On the remaining "storage key is transport‑dependent" blocker

I dug into this one and I don't think it holds up — and the suggested remedy would actually break history.

The persisted record key (decryptedId ?? event.id) must be unique per message, because a single order has many messages. Every history query keys on the logical id field of the stored MostroMessage (Filter.equals('id', orderId) in mostro_storage.dart), which is the order id and is fully transport‑invariant. The record key is only a per‑message uniqueness/dedup handle.

So persisting "under the Mostro message/order identity from tuple[0]" would set the record key to the order id, and addMessage's if (await hasItem(id)) return; would then store only the first message per order and silently drop the rest. The current scheme (inner rumor id for v1, outer kind‑14 event id for v2) is the correct design: distinct per‑message keys, with logical grouping done by the id field at query time.

Cross‑transport divergence also can't occur in practice: a node speaks exactly one transport, and within a transport the identifier is stable across relays, so dedup is sound. History identity is already transport‑invariant where it matters (the id field).

Other things I checked and found fine

• v2 filter pins authors:[mostroPublicKey] + p:[tradeKeys], disambiguating from NIP‑17 peer chat (also kind 14).
• decryptNIP44DirectEvent verifies kind, expected author, and recomputes the event id + Schnorr signature before decrypting — author signature is load‑bearing for v2, good.
• event.recipient reads the p tag, so session matching works identically for kind 14.

LGTM. Approving.

Re-reviewed the current head. Looks good to me — approve.