feat(protocol): tell the user when a node speaks a protocol we don't - #252
Conversation
The app is v2-native: it sends NIP-44 signed kind-14 events and nothing else. Protocol v1 (NIP-59 gift wrap) is being removed from this codebase, not implemented — outbound gift wrap already left the P2P chat in #247. Nothing read the node's `protocol_version` tag, though, so pointing the app at a v1 node produced no diagnosis at all: the daemon never decrypts a kind-14 event, never answers and never complains, and every send surfaced as a generic timeout indistinguishable from an unreachable relay. The reference node at mostro 0.18.0 advertises `["protocol_version","1"]` today, so this is not hypothetical. The tag is now parsed from the Kind 38385 event and daemon sends fail fast with an `UnsupportedNodeProtocol:` marker, which Dart maps to a localized message in the create- and take-order screens (new `nodeProtocolUnsupported` string in all five locales) telling the user to pick another node. An absent tag counts as supported: nodes predating it exist, the app has always spoken v2, and refusing them would break setups that work today. Only an explicit version other than 2 is a mismatch — including future versions, which are not assumed compatible. The decision is a pure function over the tag list, tested without touching the process-wide state: v2 supported, v1 not, absent supported, unknown future version not, plus malformed and valueless tags. 205 Rust tests, 190 Dart tests, clippy clean, wasm32 check passes.
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (26)
WalkthroughThe PR adds Mostro protocol-version detection and compatibility checks, prevents wrapping messages for unsupported nodes, and adds localized error messages for add and take order flows. ChangesProtocol compatibility handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant MostroNode
participant RustClient
participant OrderScreen
participant Localizations
MostroNode->>RustClient: Advertise protocol_version in capabilities
RustClient->>RustClient: Store and validate protocol version
RustClient->>RustClient: Reject incompatible wrap_message request
RustClient-->>OrderScreen: UnsupportedNodeProtocol error
OrderScreen->>Localizations: Read nodeProtocolUnsupported
Localizations-->>OrderScreen: Localized message
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![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: 962c712363
ℹ️ 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".
| /// The only protocol this app speaks. | ||
| pub const SUPPORTED_VERSION: u8 = 2; | ||
|
|
||
| static PROTOCOL_VERSION: AtomicU8 = AtomicU8::new(UNKNOWN); |
There was a problem hiding this comment.
After a v1 node stores 1, switching to a node whose Kind 38385 event is absent or whose capability fetch fails leaves this process-wide atomic unchanged: refresh_subscriptions_for_active_node clears only escrow state, while the Ok(None) and Err branches of fetch_and_set_node_capabilities never reset the protocol version. Because an unknown/absent version is intentionally supported, subsequent create and take attempts against the new node will instead keep failing with UnsupportedNodeProtocol for the previous node; clear this state when changing nodes or associate it with the node pubkey.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1bd2518: the verdict is now a watch snapshot tagged with the node (hex pubkey) it was fetched from, set only on a successful tag fetch — the Ok(None)/Err branches leave it alone. The gate (ensure_supported(node)) resolves only against a snapshot for the destination node, so after a switch the previous node's version can neither block nor admit the new node; it waits for the fetch and fails closed with a retryable NodeCapabilitiesUnknown after 10s. Covered by a_v2_advertisement_opens_the_gate_for_that_node_only.
| pub fn set_protocol_version(version: Option<u8>) { | ||
| PROTOCOL_VERSION.store(version.unwrap_or(UNKNOWN), Ordering::Relaxed); |
There was a problem hiding this comment.
When a node advertises protocol_version=0, parsing returns Some(0) and this setter logs it as unsupported, but storing the UNKNOWN sentinel makes get_protocol_version() return None, which node_is_supported() treats as supported. The client therefore sends v2 traffic to an explicitly incompatible version instead of returning the new marker; represent absence separately or encode stored versions so zero cannot collide with the sentinel.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1bd2518: the sentinel is gone — the snapshot stores the parsed Option<u8> directly, so an explicit protocol_version=0 is preserved and rejected as UnsupportedNodeProtocol:0. Regression coverage for explicit 0 and 255 in only_an_explicit_v2_is_supported and explicit_incompatible_versions_fail_with_the_marker.
| "@orderAlreadyTaken": {"description": "Error message when attempting to take an already-taken order"}, | ||
| "bondRequired": "This node requires an anti-abuse bond, which is not supported yet", | ||
| "@bondRequired": {"description": "Error shown when the Mostro node asks for an anti-abuse bond before accepting the order — the app does not support bonds yet"}, | ||
| "nodeProtocolUnsupported": "This Mostro node runs an older version of the protocol that this app no longer speaks. Pick another node in Settings", |
There was a problem hiding this comment.
When a node advertises version 3 or another future version, the new compatibility check deliberately rejects it, but this source string tells users that the node runs an older protocol. In that scenario the actionable remedy may be updating the app rather than choosing an older node, so use version-neutral incompatible/unsupported wording and update the translations accordingly.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1bd2518: all five locales now use version-neutral wording ("a protocol version this app does not support") and mention checking for an app update as well as picking another node.
| crate::mostro::protocol_version::set_protocol_version( | ||
| crate::mostro::protocol_version::parse_protocol_version(&tags), | ||
| ); |
There was a problem hiding this comment.
This adds the protocol_version parsing path while specs/005-transport-v2-migration/spec.md lines 61–65 still define FR-002 as a mandatory requirement that the app must not parse that tag, and the matching plan still says the parse path was removed. Update the active spec and plan with this diagnostic exception so the documented contract no longer declares the implementation noncompliant.
AGENTS.md reference: AGENTS.md:L75-L75
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 1bd2518: FR-002 in specs/005-transport-v2-migration/spec.md now records the diagnostic exception — protocol_version may be parsed to refuse an incompatible node legibly, never to select a transport — including the absent-tag-means-v1 rule and the node-tagged fail-closed readiness invariant; plan.md's constitution row no longer claims the parse path is gone.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
lib/features/order/screens/add_order_screen.dart (1)
207-210: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused coverage for the
UnsupportedNodeProtocolerror mapping.This path is new protocol handling in
add_order_screen.dart; add a targeted test that assertsUnsupportedNodeProtocolshowsnodeProtocolUnsupportedand not the timeout/raw message.🤖 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/features/order/screens/add_order_screen.dart` around lines 207 - 210, Add a focused test for the error-mapping logic in add_order_screen.dart that supplies an error containing UnsupportedNodeProtocol and verifies the displayed message is AppLocalizations.nodeProtocolUnsupported rather than the timeout or raw error text. Keep coverage limited to this new protocol-handling branch.Source: Coding guidelines
🤖 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/l10n/app_en.arb`:
- Around line 159-160: Update the nodeProtocolUnsupported message to describe
the protocol as unsupported or incompatible rather than older, covering future
and past unsupported versions. Apply the equivalent wording in
lib/l10n/app_en.arb lines 159-160, lib/l10n/app_de.arb line 74,
lib/l10n/app_es.arb line 74, and lib/l10n/app_fr.arb line 74; keep the existing
message meaning and translations otherwise unchanged.
In `@lib/l10n/app_it.arb`:
- Line 74: Update the nodeProtocolUnsupported translation in the ARB file,
replacing the awkward protocol phrasing with idiomatic Italian using “che questa
app non supporta più.” Keep the change limited to the localization entry, then
run flutter gen-l10n to regenerate localization outputs.
In `@lib/l10n/app_localizations_fr.dart`:
- Around line 240-242: Update the nodeProtocolUnsupported localization string to
describe the protocol version as unsupported rather than old, ensuring the
French wording accurately covers unknown, newer, and otherwise rejected versions
while preserving the existing guidance to choose another node in Settings.
In `@lib/l10n/app_localizations.dart`:
- Around line 491-495: Update the nodeProtocolUnsupported localization source in
app_en.arb to use neutral wording for any unsupported protocol version,
replacing “older version” with wording that says the app does not support the
protocol version. Then rerun flutter gen-l10n to regenerate
app_localizations.dart; do not edit the generated file directly.
In `@rust/src/api/nostr.rs`:
- Around line 247-253: Update the capability-fetch handling around
set_protocol_version and its Ok(None)/Err branches to represent an explicit
unknown state associated with the currently selected node. Invalidate any prior
node’s protocol result when selection changes, ignore late results from a
no-longer-active node, and keep queued sends blocked until capabilities for the
active node load successfully. Preserve the distinction between a missing
protocol tag and capabilities that have not been fetched; do not map fetch
failures directly to set_protocol_version(None).
In `@rust/src/mostro/actions.rs`:
- Around line 360-364: Update the UnsupportedNodeProtocol error construction in
the surrounding Rust action to return only a stable machine-readable marker with
the advertised protocol value, such as UnsupportedNodeProtocol:{advertised}, and
remove the English explanatory prose and client-version text. Leave user-facing
localization to the Dart layer.
In `@rust/src/mostro/protocol_version.rs`:
- Around line 17-25: The AtomicU8 encoding conflates an explicitly reported
protocol version 0 with the UNKNOWN state, allowing unsupported versions through
node_is_supported(). Update PROTOCOL_VERSION and the
set_protocol_version/get_protocol_version logic to use a distinct encoded
unknown value while preserving all u8 protocol values, including 0 and 255. Add
regression coverage for explicit versions 0 and 255, verifying
get_protocol_version returns them and node_is_supported rejects them.
---
Nitpick comments:
In `@lib/features/order/screens/add_order_screen.dart`:
- Around line 207-210: Add a focused test for the error-mapping logic in
add_order_screen.dart that supplies an error containing UnsupportedNodeProtocol
and verifies the displayed message is AppLocalizations.nodeProtocolUnsupported
rather than the timeout or raw error text. Keep coverage limited to this new
protocol-handling branch.
🪄 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 Plus
Run ID: d992615c-68ac-4d4d-be4e-fc636db080e4
📒 Files selected for processing (17)
lib/features/order/screens/add_order_screen.dartlib/features/order/screens/take_order_screen.dartlib/l10n/app_de.arblib/l10n/app_en.arblib/l10n/app_es.arblib/l10n/app_fr.arblib/l10n/app_it.arblib/l10n/app_localizations.dartlib/l10n/app_localizations_de.dartlib/l10n/app_localizations_en.dartlib/l10n/app_localizations_es.dartlib/l10n/app_localizations_fr.dartlib/l10n/app_localizations_it.dartrust/src/api/nostr.rsrust/src/mostro/actions.rsrust/src/mostro/mod.rsrust/src/mostro/protocol_version.rs
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Blocking changes required
I read the full live conversation (including all 11 current inline threads) before reviewing 962c71236314ca63748dbdf768f7de8e75a2995b. I did not re-post the existing findings; the unresolved node-state/readiness, 0 sentinel, spec contradiction, localization, stable-marker, and focused-test concerns remain valid on this head.
I found two additional correctness blockers:
- The compatibility policy accepts a missing
protocol_version, contrary to Mostro's public migration contract, so pre-tag v1 nodes still receive unreadable kind-14 requests and reproduce the silent timeout this PR is meant to eliminate. - The new gate fails after
create_orderhas durably recorded ownership fingerprints. An unsupported create can therefore make another user's later order look like the local user's order.
There is also a current integration gate: this branch now conflicts with main at 3f280ba0d73f2cf00ea84e4649a1a067c024a527 after PR #251 changed the same capability and wrapping paths. The green checks belong to the pre-rebase head and do not validate the merged behavior. Please rebase, preserve both node-bound PoW readiness and protocol compatibility, and rerun the full Rust/Flutter/Web checks.
| pub fn is_supported(version: Option<u8>) -> bool { | ||
| match version { | ||
| Some(v) => v == SUPPORTED_VERSION, | ||
| None => true, |
There was a problem hiding this comment.
[P1] Treat an absent protocol tag as legacy v1, not as compatible. Mostro's public migration guide states that old daemons predating protocol_version emit no tag and clients must treat that absence as v1 (MostroP2P/protocol@893e008, src/transport_migration.md:36-38). Returning true here makes this v2-only client send kind 14 to exactly those legacy nodes, recreating the silent timeout this PR is intended to diagnose. Malformed and out-of-range explicit values also collapse to None and fail open through this branch. Keep not-fetched/fetch-failed state distinct as the existing threads request, but once capabilities are loaded, only an explicit v2 advertisement can prove compatibility for this v2-native client.
There was a problem hiding this comment.
Addressed in 1bd2518: is_supported(None) is now false — per the migration guide a tagless daemon speaks legacy v1, so only an explicit protocol_version=2 opens the gate, and the tagless case fails with UnsupportedNodeProtocol:1. Not-fetched stays distinct (node-tagged snapshot, set only on successful tag fetch; NodeCapabilitiesUnknown otherwise). Malformed/out-of-range values parse to absent and are now refused through the same v1 branch instead of failing open.
| // localizes. Protocol v1 (gift wrap) is being removed, not implemented — | ||
| // this client is v2-native, and the fix for such a node is to run a v2 | ||
| // daemon (mostrod 0.19.0 runs v2 only). | ||
| if !crate::mostro::protocol_version::node_is_supported() { |
There was a problem hiding this comment.
[P1] Reject before persisting maker ownership, or roll it back on this error. create_order stores both its local UUID and coarse content fingerprint in memory and the DB (api/orders.rs:732-737, via store_trade_key_index at 389-395) before calling actions::new_order. This new bail returns through ? before the publication rollback, leaving both records behind. Later, any public order with the same kind/currency/amount/payment method matches that fingerprint and is marked is_mine, then bound to the unused trade key (api/orders.rs:2696-2712)—including after restart because the fingerprint is durable. Move the compatibility preflight ahead of derivation/persistence (while retaining a defense at wrapping), or make every pre-publish failure remove both mappings; add a regression test that rejects an unsupported create and then ingests a colliding order.
There was a problem hiding this comment.
Addressed in 1bd2518: create_order now runs ensure_supported(active_node) before deriving the trade key and before storing the local-UUID and content-fingerprint mappings, while the wrap-level check remains as defense in depth. Regression test an_unsupported_create_persists_no_maker_ownership rejects an unsupported create and asserts the fingerprint mapping was never stored — the durable record that would otherwise mark a colliding public order as mine.
There was a problem hiding this comment.
Supplemental blocking finding
A delayed parallel audit completed after my formal review; I verified this finding directly on 962c71236314ca63748dbdf768f7de8e75a2995b and re-read the live REST conversation before posting it.
The compatibility gate is centralized in actions::wrap_message, so UnsupportedNodeProtocol is now reachable from every daemon action, not only create/take. However, the new localized classification exists only in add_order_screen.dart and take_order_screen.dart. Existing-trade actions still either expose the raw marker or replace it with unrelated generic failures:
- add invoice:
lib/features/order/screens/add_lightning_invoice_screen.dart:88-100; - cancel / fiat-sent / release / dispute:
lib/features/trades/screens/trade_detail_screen.dart:197-269andlib/features/order/screens/my_order_screen.dart:70-75; - rating:
lib/features/rate/screens/rate_counterpart_screen.dart:47-51after Rust wraps it asRateUserDispatchFailed; - dispute construction wraps it as
ProtocolErrorinrust/src/api/disputes.rs:128-150.
A user with an existing trade who changes to an unsupported node receives no actionable node-selection guidance during time-sensitive invoice, payment, release, cancellation, dispute, or rating operations. Preserve a stable structured classification through Rust wrappers and map it centrally—or handle it in every reachable action screen—then add one regression test outside create/take. This is distinct from the existing marker-stability and create/take widget-test threads, so I have not duplicated those comments.
…1 on #252 Resolves the merge conflict with #251 (first-contact PoW split in actions.rs) and addresses every review finding, rebuilding the protocol gate on the node-tagged capability machinery #251 introduced: - Node-tagged, fail-closed verdict (Codex P1, CodeRabbit Major, ermeme P1): protocol_version now stores one watch snapshot tagged with the node it was fetched from, set only on a successful tag fetch. ensure_supported(node) waits for that node's fetch and fails closed with a retryable NodeCapabilitiesUnknown after 10s, so neither the startup default nor the previous node's verdict ever decides for the active node, in either direction. - No sentinel collision (Codex P2, CodeRabbit Major): the Option<u8> lives in the snapshot; an explicit protocol_version=0 (or 255) is just an unsupported version. Regression-tested. - Absent tag = legacy v1 (ermeme P1): per the protocol migration guide, a tagless daemon predates the tag and speaks v1 — only an explicit 2 opens the gate. is_supported(None) is now false. - Stable marker (CodeRabbit Major): the wrap gate and preflight return UnsupportedNodeProtocol:{advertised} with no English prose; Dart localizes. The gate lives in wrap_message_at, covering every daemon-bound wrap. - No orphaned maker ownership (ermeme P1): create_order runs the compatibility preflight before deriving the trade key and before storing the local-UUID and content-fingerprint mappings, so a rejected create leaves no durable records for a later colliding public order to match as 'is_mine'. Regression test included. - Neutral wording (Codex P2, CodeRabbit): all five ARB locales now say the protocol version is unsupported (not 'older') and suggest an app update as well as picking another node; Italian phrasing fixed; flutter gen-l10n rerun. - Spec/plan updated (Codex P1): FR-002 gains the diagnostic-only protocol_version exception (parse to refuse legibly, never to select a transport) and plan.md's constitution row no longer claims the parse path is gone.
…in Dart ermeme (supplemental): the compatibility gate runs on every daemon-bound wrap, but only the create/take screens localized the UnsupportedNodeProtocol marker — invoice, cancel, fiat-sent, release, dispute, and rating flows showed the raw marker or swallowed it behind unrelated generic failures (the dispute and rating Rust wrappers interpolate the inner error, so the marker survives inside their messages). Add lib/core/daemon_errors.dart: localizedDaemonError() maps the stable markers (UnsupportedNodeProtocol, NodeCapabilitiesUnknown, NoDaemonResponse, StorageUnavailable) by substring — wrapper prefixes included — and returns the screen's own generic fallback otherwise. All six reachable action screens now use it (add/take order refactored onto it too; BondRequired stays take-specific), and the new nodeCapabilitiesUnknown string localizes the fail-closed capability-fetch window in all five locales. Regression test outside create/take: daemon_errors_test.dart covers the dispute ProtocolError and rating RateUserDispatchFailed wrapped forms, the bare markers, and the fallback.
|
Re: supplemental blocking finding — addressed in 45327e0.
|
1 participant
Why
This app is v2-native: it sends NIP-44 signed kind-
14events and nothing else. Protocol v1 (NIP-59 gift wrap) is being removed from this codebase, not implemented — outbound gift wrap already left the P2P chat in #247.What was missing is the diagnosis. Nothing read the node's
protocol_versiontag, so pointing the app at a v1 node produced no signal at all: the daemon never decrypts a kind-14 event, so it never answers and never complains, and every send surfaced as a generic timeout indistinguishable from an unreachable relay.Not hypothetical — the reference node advertises v1 right now:
That is why creating an order against it silently does nothing.
What changes
mostro::protocol_versionparses the tag from the Kind 38385 event, and daemon sends fail fast with anUnsupportedNodeProtocol:marker instead of publishing an event nobody will read. Dart maps the marker to a localized message in the create- and take-order screens (newnodeProtocolUnsupportedstring in en/es/fr/de/it) that points the user at the node selector."2""1""3", malformed, valuelessAn absent tag counts as supported on purpose: nodes predating the tag exist, the app has always spoken v2, and refusing them would break setups that work today. A future version is not assumed compatible.
Test plan
cargo test— 205 passing. The decision is a pure function over the tag list, tested without touching the process-wide state: v2, v1, absent, unknown future version, malformed value, valueless tag (including the exact tag shape the 0.18.0 node publishes).cargo clippy --all-targets— no new warningscargo check --locked --target wasm32-unknown-unknown— passesflutter analyze/flutter test— clean, 190 passingNotes
1059) and is the last NIP-59 user left in the app. Migrating it needs a protocol change first, in the shape of spec(chat): replace gift wrap with shared-key signed kind 14 events protocol#52 — tracked separately.pow_first_contact), which fixes a different silent-drop cause on the same path.Summary by CodeRabbit
New Features
Bug Fixes
Localization