New features from v1-reference - #16
Merged
Merged
Conversation
State machine corrections (spec.md, data-model.md, contracts/types.md, research.md, tasks.md, plan.md): - PaymentFailed → documented as Action notification, not a Status (order stays in SettledHoldInvoice) - CooperativelyCanceled → documented as client-side UI state only (protocol doesn't change order status) - InProgress → added as new state (admin took dispute) - Order-type-dependent transitions documented (sell→WaitingBuyerInvoice; buy→WaitingPayment directly) New features from v1-reference (spec.md, contracts/types.md, tasks.md): - FR-052/053: Nym identity (deterministic pseudonyms/avatars from pubkey) - FR-054: Default fiat currency setting - FR-055: Lightning Address setting - FR-056: Mostro node selector - FR-057/058: About screen with app info and node details - US5 scenario 8 (nym in chat), US7 scenarios 10-13 (new settings + about) - 6 new tasks (T059a/b for nym, T082a-d for settings/about) - NymIdentity struct added to types.md Checklist updated: 58 FRs (up from 51), new v1 parity items checked.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis pull request updates the Mostro P2P client specifications across multiple documentation files. The changes reclassify Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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: 5
🧹 Nitpick comments (2)
specs/001-mostro-p2p-client/tasks.md (1)
249-250: T082c overlaps with T090; define a single source of truth for node switching behavior.Both tasks include node switching plus relay reset warning semantics, which risks duplicate implementation and inconsistent completion criteria.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/tasks.md` around lines 249 - 250, T082c and T090 both implement node switching and relay reset warning, causing duplication; consolidate into a single source of truth by extracting the node switch logic and relay-reset warning into a shared component/service (e.g., a MostroNodeSelector widget and a NodeSwitchService) and update lib/screens/settings/settings_screen.dart and lib/screens/settings/about_screen.dart to consume that shared logic; ensure the shared implementation encapsulates: list of known nodes + custom entry, switch action that triggers relay reset warning per FR-056 and the US7 scenario 12 flow, and a single canonical place for validation, state mutation, and UI strings so behavior and completion criteria remain consistent across both screens (reference T082c, T090, FR-056, US7 scenario 12).specs/001-mostro-p2p-client/spec.md (1)
410-411: FR-052/053 overlap existing FR-011a/011b/011c and should be consolidated.This duplicates deterministic nym/avatar requirements already defined earlier, increasing maintenance and traceability drift risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-mostro-p2p-client/spec.md` around lines 410 - 411, FR-052 and FR-053 duplicate deterministic pseudonym/avatar requirements already expressed in FR-011a/011b/011c; remove FR-052 and FR-053 and consolidate their wording into the existing FR-011 series so a single requirement covers deterministic adjective-noun pseudonyms and visual avatars (icon + color) derived from public keys without exposing key material; update any cross-references and the traceability matrix to point to FR-011a/011b/011c and ensure the deterministic guarantees (same key → same name/icon/color across sessions/devices) are preserved verbatim in the FR-011 text.
🤖 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/checklists/requirements.md`:
- Line 87: The wording on line 87 is misleading: clarify that PaymentFailed is
being reclassified as an Action (not a Status) and that InProgress is being
added to the list of Status values, rather than saying InProgress "replaces"
PaymentFailed; update the text to explicitly state "PaymentFailed — moved to
Actions (no longer a Status)" and "InProgress — newly added Status", and also
note that CooperativelyCanceled remains a client-side UI-only state to avoid
confusion between action vs status categories.
In `@specs/001-mostro-p2p-client/contracts/types.md`:
- Around line 224-229: The NymIdentity contract fields diverge from the
canonical v1 model; update the NymIdentity definition to use the canonical field
names and types: replace name with pseudonym: String, replace icon_name with
icon_index: u8 (index into the static icon list 0-36), and change color_hue from
f64 to color_hue: u16 (0-359) so Dart/Rust implementations can convert to double
for HSV; ensure the comments reflect the new semantics (pseudonym format, icon
index bounds, and hue integer range).
In `@specs/001-mostro-p2p-client/data-model.md`:
- Around line 79-84: The spec is inconsistent about CooperativelyCanceled being
UI-only vs part of the "15 mostro-core statuses"; decide on a single canonical
model and make it consistent: either remove CooperativelyCanceled from the
enumerated mostro-core statuses (and add a clear "UI-only states" section
listing CooperativelyCanceled) or mark it as a protocol status and remove the
UI-only note; update the sentence that lists "15 mostro-core statuses" and the
earlier line that annotated CooperativelyCanceled to match the chosen model, and
ensure all references to CooperativelyCanceled elsewhere in the document
(state-machine descriptions, tests, and protocol notes) use the same
classification.
In `@specs/001-mostro-p2p-client/spec.md`:
- Line 401: FR-043 currently mixes protocol statuses with a UI-only state;
update the requirement to separate protocol-defined order statuses from
client-side UI-only states: list the protocol statuses (exclude
cooperativelyCanceled) and adjust the stated count accordingly, then add a
distinct sentence that `cooperativelyCanceled` is a client-side UI state and
that `paymentFailed` is an action notification rather than a status; reference
the FR-043 text and the tokens `cooperativelyCanceled` and `paymentFailed` when
making the change so tests and implementations can unambiguously validate
protocol vs UI semantics.
In `@specs/001-mostro-p2p-client/tasks.md`:
- Around line 144-145: Update the task T059a wording to request deterministic
pseudonym generation that outputs an integer icon_index and integer hue (not an
icon name or a string color), and update T059b to consume/display icon_index and
integer hue; specifically change references to rust/src/crypto/nym.rs to produce
(adjective-noun, icon_index, hue) per FR-052/FR-053 and change
lib/widgets/nym_avatar.dart to accept icon_index and integer hue (not icon name)
for rendering per US5 scenario 8 so the Rust-first deterministic nym model stays
consistent across layers.
---
Nitpick comments:
In `@specs/001-mostro-p2p-client/spec.md`:
- Around line 410-411: FR-052 and FR-053 duplicate deterministic
pseudonym/avatar requirements already expressed in FR-011a/011b/011c; remove
FR-052 and FR-053 and consolidate their wording into the existing FR-011 series
so a single requirement covers deterministic adjective-noun pseudonyms and
visual avatars (icon + color) derived from public keys without exposing key
material; update any cross-references and the traceability matrix to point to
FR-011a/011b/011c and ensure the deterministic guarantees (same key → same
name/icon/color across sessions/devices) are preserved verbatim in the FR-011
text.
In `@specs/001-mostro-p2p-client/tasks.md`:
- Around line 249-250: T082c and T090 both implement node switching and relay
reset warning, causing duplication; consolidate into a single source of truth by
extracting the node switch logic and relay-reset warning into a shared
component/service (e.g., a MostroNodeSelector widget and a NodeSwitchService)
and update lib/screens/settings/settings_screen.dart and
lib/screens/settings/about_screen.dart to consume that shared logic; ensure the
shared implementation encapsulates: list of known nodes + custom entry, switch
action that triggers relay reset warning per FR-056 and the US7 scenario 12
flow, and a single canonical place for validation, state mutation, and UI
strings so behavior and completion criteria remain consistent across both
screens (reference T082c, T090, FR-056, US7 scenario 12).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f6e683a-a33e-4aa2-99ba-e37f3f570d82
📒 Files selected for processing (6)
specs/001-mostro-p2p-client/checklists/requirements.mdspecs/001-mostro-p2p-client/contracts/types.mdspecs/001-mostro-p2p-client/data-model.mdspecs/001-mostro-p2p-client/research.mdspecs/001-mostro-p2p-client/spec.mdspecs/001-mostro-p2p-client/tasks.md
1. checklists/requirements.md:87 — Replaced misleading "InProgress replaces PaymentFailed" with accurate phrasing: PaymentFailed moved out of the status enum; InProgress newly added; CooperativelyCanceled is in mostro-core but set client-side. 2. contracts/types.md NymIdentity — Corrected fields: name → pseudonym: String, icon_name: String → icon_index: u8, color_hue: f64 → color_hue: u16 (0–359). 3. data-model.md:79 — Removed contradictory "client-side UI state only" wording; now accurately states it's in the mostro-core enum but set client-side via action notifications. 4. spec.md FR-043 — Changed "protocol-defined" to "mostro-core" and updated the cooperativelyCanceled note to match the corrected canonical description. 5. tasks.md T059a/T059b — Updated to use icon_index: u8 (0–36) and color_hue: u16 (0–359) instead of icon_name: String and float hue.
This was referenced Mar 23, 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.
State machine corrections (spec.md, data-model.md, contracts/types.md, research.md, tasks.md, plan.md):
New features from v1-reference (spec.md, contracts/types.md, tasks.md):
Checklist updated: 58 FRs (up from 51), new v1 parity items checked.
Summary by CodeRabbit
New Features
Documentation