feat: add settings, nym identity, and node selector contracts - #17
Merged
Conversation
Add settings.md contract covering app preferences (theme, language, default fiat currency, Lightning Address), privacy mode mirroring, and runtime-only logging toggle. Extend identity.md with get_nym_identity() for deterministic pseudonym/icon/color derivation from public key. Extend nostr.md with get_mostro_info() (richer node data for About screen), get_known_mostro_nodes(), and set_active_mostro() for node selection. Fix NymIdentity field types: pseudonym: String, icon_index: u8 (0-36), color_hue: u16 (0-359). Update plan.md contract count (8 → 9) and project structure listing.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR updates the Rust toolchain baseline in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as Settings/Mostro API
participant Storage
participant Relay as Relay/Daemon
participant Stream as on_relay_status_changed()/on_connection_state_changed()
Client->>API: set_active_mostro(pubkey)
API->>Storage: validate pubkey (sync)
alt invalid pubkey
API-->>Client: InvalidPublicKey (sync)
else valid pubkey
API->>Storage: persist active pubkey (sync)
API->>Relay: async unsubscribe/resubscribe for new pubkey (background)
Relay-->>API: subscription success / failure
alt failure after retries
Relay->>Stream: emit relay/connection status changes
end
API-->>Client: return ()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
specs/001-mostro-p2p-client/contracts/settings.md (1)
72-78: Consider relocating privacy_mode setter to Identity contract.This function "delegates to
Identity" and "updatesIdentity.privacy_modeas the authoritative source" (lines 73-75). IfIdentityis the authoritative owner of this field, the setter operation logically belongs in the Identity API contract, not Settings.Current design:
- Settings mirrors Identity.privacy_mode (read dependency)
- Settings updates Identity.privacy_mode (write dependency)
- Creates bidirectional coupling between two contracts
Suggested refactor:
- Move
set_privacy_mode()toidentity.md- Settings only reads/mirrors the value from Identity
- Reduces coupling and clarifies ownership
Alternatively, if Settings truly owns this preference, document why Identity is described as "authoritative."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/contracts/settings.md` around lines 72 - 78, The setter set_privacy_mode(enabled: bool) currently mutates Identity.privacy_mode while living in the Settings contract, creating bidirectional coupling; either move that setter into the Identity contract (add set_privacy_mode in identity.md and have Settings expose a read-only mirror) or, if Settings must own the field, update the spec to state that Settings is authoritative and remove the sentence that says Identity.privacy_mode is authoritative; update references to Identity.privacy_mode and remove direct write/delegate behavior from Settings so only Identity or only Settings performs writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 6-8: Remove the duplicate Rust toolchain entry for
"001-mostro-p2p-client" by deleting the stale line that repeats the Rust
requirement, and normalize the remaining entry to state the minimum and
recommended versions explicitly (e.g., "Rust stable 1.75+ (recommended: 1.94+)")
so the file contains a single, clear requirement for 001-mostro-p2p-client.
In `@specs/001-mostro-p2p-client/contracts/nostr.md`:
- Around line 110-118: Update the set_active_mostro(pubkey: String)
specification to explicitly state its execution model and error semantics:
indicate whether set_active_mostro is synchronous/blocking or returns
immediately and that the re-subscription to kind 10002 happens asynchronously;
specify whether the active node switch is applied atomically before attempting
subscription (i.e., active node is updated even if subscription fails) or rolled
back on subscription failure; and define the timeout and retry behavior that
triggers the NodeUnreachable error (e.g., X seconds, Y retries) as well as
whether NodeUnreachable is returned only after subscription attempt or also on
initial network validation; reference the function name set_active_mostro, the
relay kind 10002, and the error tokens InvalidPublicKey and NodeUnreachable in
the updated text.
- Around line 86-92: Clarify the default semantics by specifying that the client
applies documented defaults when the daemon omits them: update the
MostroNodeInfo spec to state that expiration_hours and expiration_seconds may be
omitted by the daemon and the client will use 24 (hours) and 900 (seconds)
respectively; then implement parsing in the client code path that constructs
MostroNodeInfo (look for the deserialization/constructor handling of
MostroNodeInfo and fields expiration_hours and expiration_seconds) to treat
missing values as default_u32(24) and default_u32(900) and ensure any public API
returns concrete u32 values to callers.
- Around line 104-107: The docs for get_known_mostro_nodes() claim it returns
"hardcoded defaults + any user-added nodes" but the contract exposes no API to
add nodes; update the spec to either remove the "user-added" claim or add a
node-addition API. Either add a new function (e.g.
add_mostro_node(MostroNodeInfo) -> Result/Bool and corresponding
remove_mostro_node/ list mutation behavior) and document how it persists and
interacts with set_active_mostro(), or change the get_known_mostro_nodes()
description to state it returns only hardcoded defaults and reference
set_active_mostro() as the selector without implying node addition.
In `@specs/001-mostro-p2p-client/contracts/settings.md`:
- Around line 11-24: The spec currently lacks guidance for privacy_mode when no
Identity exists; update the get_settings() documentation so
AppSettings.privacy_mode is well-defined in that case—choose one approach and
apply it consistently: either make get_settings() return nullable AppSettings
(AppSettings?), or make privacy_mode optional (privacy_mode: bool?), or
(preferable for minimal change) document that privacy_mode defaults to false
when no Identity exists; reference the get_settings() return type AppSettings
and the Identity.privacy_mode mapping, and update the AppSettings block to
reflect the chosen behavior.
- Around line 45-53: Clarify validation for set_default_fiat_code(code: String?)
by specifying two edge cases: if there is no active Mostro node selected,
perform only a format check (accept any syntactically valid ISO 4217 code) and
do not return UnsupportedCurrency; if an active node exists but
MostroNodeInfo.supported_currencies is null, treat the node’s supported list as
unknown and likewise fall back to format-only validation (accept any valid ISO
4217 code optimistically); only raise UnsupportedCurrency when an active node
provides a non-null supported_currencies Vec and the provided code is not in
that Vec; keep StorageError for persistence failures and mention these rules in
the set_default_fiat_code and reference MostroNodeInfo.supported_currencies and
the UnsupportedCurrency/StorageError errors.
- Around line 35-42: Update the set_language(locale: String) contract in
settings.md to match FR-020d: change the validation from "one of the 10
supported locales" to either reference FR-020d explicitly ("one of the languages
defined in FR-020d") or enumerate the actual BCP-47 codes for the initial
release (en, es, it, fr, de) so that set_language(locale: String) validation and
the Errors section (UnsupportedLocale) align with the five supported languages.
---
Nitpick comments:
In `@specs/001-mostro-p2p-client/contracts/settings.md`:
- Around line 72-78: The setter set_privacy_mode(enabled: bool) currently
mutates Identity.privacy_mode while living in the Settings contract, creating
bidirectional coupling; either move that setter into the Identity contract (add
set_privacy_mode in identity.md and have Settings expose a read-only mirror) or,
if Settings must own the field, update the spec to state that Settings is
authoritative and remove the sentence that says Identity.privacy_mode is
authoritative; update references to Identity.privacy_mode and remove direct
write/delegate behavior from Settings so only Identity or only Settings performs
writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22e6c99e-56d0-413e-941d-f43058686a43
📒 Files selected for processing (5)
CLAUDE.mdspecs/001-mostro-p2p-client/contracts/identity.mdspecs/001-mostro-p2p-client/contracts/nostr.mdspecs/001-mostro-p2p-client/contracts/settings.mdspecs/001-mostro-p2p-client/plan.md
CLAUDE.md - Merged duplicate 001-mostro-p2p-client Rust entries into one: Rust stable 1.94+ - Deduplicated Recent Changes to single entry with normalized version string contracts/nostr.md - MostroNodeInfo: added note that expiration_hours/expiration_seconds may be absent from daemon; client applies defaults of 24 and 900; callers always receive concrete u32 values - get_known_mostro_nodes(): removed false "user-added nodes" claim; now correctly states hardcoded defaults only; references set_active_mostro() as selector - set_active_mostro(): fully specified — returns immediately (async re-subscription), active node persisted before subscription attempt (no rollback), InvalidPublicKey is synchronous format check, NodeUnreachable is emitted asynchronously after 3 retries × 30s timeout contracts/settings.md - Module docstring: privacy_mode is read-only mirror; single write path is set_privacy_mode() in Reputation API - get_settings() AppSettings.privacy_mode: documented as false when no Identity exists - set_language(): "10 supported locales" → explicit FR-020d codes: en, es, it, fr, de - set_default_fiat_code(): added three-tier validation (no node → format-only; node with null currencies → format-only; node with non-null list → UnsupportedCurrency if not in list) - set_privacy_mode(): removed entirely (reputation.md is the single write path; eliminates bidirectional coupling)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/001-mostro-p2p-client/contracts/nostr.md`:
- Around line 137-139: The spec is inconsistent: `ConnectionState` in
contracts/types.md is defined as only Online | Offline | Reconnecting but the
nostr.md text claims `NodeUnreachable` is emitted on
`on_connection_state_changed()`; fix by making the contract consistent — either
add `NodeUnreachable` to the `ConnectionState` union in contracts/types.md
(update the type definition and examples) or change nostr.md to state that
`NodeUnreachable` is emitted via a separate event/stream (e.g.,
`on_connection_error()` or a `Stream<ConnectionEvent>`), and update references
to `on_connection_state_changed()` and any examples to match the chosen approach
so consumers see a single consistent event/type model.
In `@specs/001-mostro-p2p-client/tasks.md`:
- Line 234: The task list incorrectly includes a writable set_privacy_mode in
settings.rs; update the tasks to remove set_privacy_mode and instead document
that privacy is read-only in Settings per the contract and must be changed via
the Reputation API; keep get_settings(), set_theme(ThemeMode),
set_language(locale), set_default_fiat_code(...),
set_default_lightning_address(...), set_logging_enabled(bool), and
on_settings_changed() stream but remove any references to set_privacy_mode and
add a note pointing implementers to use the Reputation API for privacy changes.
- Line 239: The task description for T090 in
specs/001-mostro-p2p-client/tasks.md incorrectly asks for "10 supported
locales"; update the task text and associated acceptance criteria to match the
constrained language contract by replacing "10 supported locales" with the five
supported locales (en, es, it, fr, de) and ensure the referenced screen
lib/screens/settings/preferences_screen.dart (language picker UI) is specified
to implement only those five options and no additional locales.
- Line 44: Update the markdown for the ChaCha20-Poly1305 task to avoid
markdownlint MD052 by escaping or backticking the bracketed token; specifically
edit the line referencing the task (the bullet for T016) so the payload format
shown for encrypt/decrypt (the portion containing
[nonce:12][ciphertext][tag:16]) is either wrapped in backticks or the square
brackets are escaped, ensuring the description of encrypt(plaintext, key) and
decrypt(blob, key) remains unchanged and still references encrypt and decrypt in
rust/src/crypto/file_encrypt.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca4da95a-64d0-4c8a-bc6f-b4e094b396a8
📒 Files selected for processing (4)
CLAUDE.mdspecs/001-mostro-p2p-client/contracts/nostr.mdspecs/001-mostro-p2p-client/contracts/settings.mdspecs/001-mostro-p2p-client/tasks.md
✅ Files skipped from review due to trivial changes (2)
- specs/001-mostro-p2p-client/contracts/settings.md
- CLAUDE.md
Comment on lines
+121
to
+123
| **Execution model**: Returns immediately after validating `pubkey` | ||
| format and persisting the new active node to storage. Re-subscription | ||
| to the new node's kind 10002 relay list happens asynchronously in the |
Contributor
There was a problem hiding this comment.
Persistence guarantee needs a storage-failure error path.
Line 121–Line 123 says the function persists active node before returning, but Line 141 only allows InvalidPublicKey. If persistence fails, behavior is undefined in the contract.
Proposed contract fix
- **Errors**: `InvalidPublicKey` (synchronous, format validation only).
+ **Errors**: `InvalidPublicKey` (synchronous, format validation only),
+ `StorageError` (synchronous, if persisting active node fails).Also applies to: 141-141
1. nostr.md: NodeUnreachable no longer claims to be emitted on on_connection_state_changed() (which only carries Online|Offline|Reconnecting). Unreachability is now reported through the existing RelayStatus.Error via on_relay_status_changed() + ConnectionState.Offline via on_connection_state_changed() — no new types needed. 2. tasks.md T085: Removed set_privacy_mode(bool) from the settings API task; added a note directing implementers to reputation.rs as the single write path. 3. tasks.md T090: "10 supported locales" → "exactly 5 locales: en, es, it, fr, de per FR-020d". 4. tasks.md T016: Payload format [nonce:12][ciphertext][tag:16] wrapped in backticks to satisfy markdownlint MD052.
This was referenced Mar 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add settings.md contract covering app preferences (theme, language, default fiat currency, Lightning Address), privacy mode mirroring, and runtime-only logging toggle.
Extend identity.md with get_nym_identity() for deterministic pseudonym/icon/color derivation from public key.
Extend nostr.md with get_mostro_info() (richer node data for About screen), get_known_mostro_nodes(), and set_active_mostro() for node selection. Fix NymIdentity field types: pseudonym: String, icon_index: u8 (0-36), color_hue: u16 (0-359).
Update plan.md contract count (8 → 9) and project structure listing.
Summary by CodeRabbit
New Features
Documentation