Actionable comments posted: 2

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-26: The phase introduction still describes the migration as
future work, which conflicts with the updated completed-state wording. Update
the introductory text in TRANSPORT_V2_MIGRATION.md near the phases section so it
no longer says the phases are for “subsequent branches” or that the document
“does not execute them,” and make it consistent with the completed status
already stated; use the surrounding phase overview text and headings as the
anchor for the edit.

In `@lib/features/settings/about_screen.dart`:
- Around line 338-343: The protocol version row in about_screen’s info card is
rendering nullable data directly via AboutScreen/_buildInfoRowWithDialog, so
legacy nodes can show the literal string "null". Update the value passed for
instance.protocolVersion to use a null fallback consistent with the other
optional fields in this card, such as S.of(context)!.notAvailableShort, so
nullable protocolVersion is displayed as a friendly unavailable label instead of
null.

---

Nitpick comments:
In `@lib/data/models/mostro_message.dart`:
- Around line 30-34: The default version value in MostroMessage is being
described too broadly as transport-derived, but non-wire call sites like
MostroService still use order.toJson() without overriding it and will log
storage payloads with version 1. Narrow the comment in MostroMessage.toJson() so
it only describes the actual send paths, or thread the resolved version through
the logging/storage serialization path in MostroMessage and MostroService so the
logged payload matches the wire version.

Looks good to me. I verified the transport split, the protocol_version-driven subscription filter, and the UI/docs updates.

Code Review Summary

Verdict: Approve

I rechecked the current head of PR #637 and the transport-v2 wiring looks internally consistent.

What I verified

• MostroMessage.wrapForTransport() is now the single entry point for outbound transport selection.
• wrapNip44() stamps version: 2, while the legacy path still defaults to version: 1.
• SubscriptionManager.buildOrdersFilter() correctly splits receive-side filters:
  • v1 → kind 1059 with p = tradeKeys
  • v2 → kind 14 with authors = [mostroPubkey] and p = tradeKeys
• The node's protocol_version is surfaced through MostroInstance.protocolVersion and is used consistently on both send and receive paths.
• The About screen change is safe because it reads MostroInstance.protocolVersion from the model, not the nullable Nostr extension getter.
• The docs now match the implementation and the migration is clearly marked complete.

Test coverage

• The new buildOrdersFilter() tests cover both transport branches.
• The existing service tests still exercise message signing / verification behavior on the send path.

Notes

• I did not find a blocking correctness issue in the current head.
• The only thing I'd keep watching is that the transport switch relies on the node info event arriving and the subscription manager re-subscribing when it does; the current implementation already accounts for that, so this is a watchpoint rather than a blocker.

Overall: clean implementation, sensible decomposition, and the migration is wired coherently end-to-end.

I reviewed the current head and I do not see a blocking issue.

What I checked:

• Removing the global Config.mostroVersion is the right cleanup. The message version is now derived from the wire transport at the serialization/wrap boundary, which avoids drift between v1 gift-wrap and v2 NIP-44 paths.
• MostroMessage.toJson() defaulting to version: 1 is consistent with the remaining v1/storage/logging callers, while wrapNip44 explicitly forces version: 2 for protocol-v2 traffic.
• Extracting buildOrdersFilter() is a good move and the v2 filter keeps the important authors: [mostroPubkey] pin, so kind-14 Mostro replies stay disambiguated from peer chat.
• The About screen surfacing protocol_version is aligned with the migration model and helps explain transport selection without changing behavior.
• The docs update matches the code state for this branch.

I could not run Flutter tests in this sandbox because flutter is not installed here, but the PR check run on the current head is green and I do not see a correctness blocker in the changes themselves.

Approved on the current revision.

Strict review: full diff inspected against the surrounding code, all call sites of toJson/serialize/sign/wrap* grepped across lib/ and test/, PR-relevant test suites executed locally (all green), plus multi-pass analysis (correctness, silent failures, test coverage, docs/comment accuracy, Flutter idioms).

Correctness — no findings

• Config.mostroVersion removal is clean: zero dangling references; every outbound send (mostro_service.dart, restore_manager.dart ×3, dispute_repository.dart) routes through wrapForTransport, so there is no v1/v2 version-drift risk.
• sign/serialize (v1) and wrapNip44 (v2) build tuple element 0, the trade signature and the identity-proof payload from the same serialized JSON — they cannot drift apart.
• The version field written to Sembast (mostro_storage.dart) is write-only metadata (fromJson/deserialized never read it), so the ?? 1 default is behavior-preserving for storage/logging.
• buildOrdersFilter extraction is a pure refactor; calling a @visibleForTesting top-level function from the same library is valid and lint-clean. _appliedOrdersTransport tracking preserved.
• instance.protocolVersion in the About screen is the non-nullable MostroInstance.protocolVersion (defaults to 1), so no "null" can render.
• ARB: both new keys present and consistent in all 5 locales (en/es/it/de/fr), flat structure, metadata convention matches sibling mostroDaemonVersion. Docs (§4.2, §4.3, Phase C/D, "Migration complete") verified accurate against the code and the five referenced test files.