plan: update artifacts for 15 states, range orders, and regenerate tasks - #5
Merged
Conversation
- Add PaymentFailed and CompletedByAdmin to OrderStatus enum (13→15 states) - Add range order fields (fiat_amount_min/max) to OrderInfo, NewOrderParams, and Order data model with validation rules - Add RelaySource enum and is_blacklisted field to Relay entity - Add ThemeMode, LogLevel enums and LogEntry struct to shared types - Add privacy_mode and logging_enabled to Settings known keys - Expand AppState with theme, privacy_mode, logging_enabled fields - Update state machine diagrams in research.md and data-model.md - Regenerate tasks.md: 93 tasks across 15 phases organized by user story - Update agent context via update-agent-context.sh
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 (1)
WalkthroughSpecification updates add optional fixed-or-range fiat fields for orders, introduce new order states (PaymentFailed, CompletedByAdmin), add UI/logging enums and logging state, extend relay metadata, and reorganize the development task plan and priorities. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 4
🧹 Nitpick comments (1)
specs/001-mostro-p2p-client/tasks.md (1)
6-6: Add explicit test tasks for protocol-critical paths.Given the new fiat mode validation and 15-state lifecycle changes, omitting test tasks entirely increases regression risk. Add targeted tasks for state-machine transitions and fixed-vs-range validation.
🤖 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` at line 6, Add explicit test tasks covering protocol-critical paths: create checklist items in the specs/001-mostro-p2p-client/tasks.md to test the new fiat mode validation (validate fixed-vs-range pricing behavior and edge cases), exercise the 15-state lifecycle state-machine transitions (each transition, invalid transitions, and recovery paths), and include integration scenarios that combine fiat validation with lifecycle changes; reference the specific features by name (fiat mode validation, fixed-vs-range validation, 15-state lifecycle, state-machine transitions) and add them to the `/speckit.checklist` so CI/tests surface regressions.
🤖 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 28-29: The "Recent Changes" section contains a duplicated entry
for "001-mostro-p2p-client"; remove the second identical line so only one
"001-mostro-p2p-client: Added Rust stable 1.75+ ..." entry remains, ensuring the
file CLAUDE.md no longer has the repeated verbatim line.
In `@specs/001-mostro-p2p-client/contracts/types.md`:
- Around line 118-120: TradeHistoryEntry currently only defines fiat_amount
(fiat_amount: f64) which loses range info from OrderInfo; update the
TradeHistoryEntry type to include nullable fiat_amount and add fiat_amount_min
and fiat_amount_max (fiat_amount: f64?; fiat_amount_min: f64?; fiat_amount_max:
f64?) so entries created from range orders preserve min/max values, and adjust
any serializers/deserializers and mapping logic that populate TradeHistoryEntry
from OrderInfo to set these three fields accordingly.
In `@specs/001-mostro-p2p-client/data-model.md`:
- Around line 57-58: Update the spec text to explicitly forbid providing both a
fixed amount and a range by adding a mutual-exclusivity clause: state that
Either `fiat_amount` OR both `fiat_amount_min` and `fiat_amount_max` MUST be
provided, but NOT both, and ensure the validation rules require that if
`fiat_amount` is present then neither `fiat_amount_min` nor `fiat_amount_max`
may be present, and conversely if `fiat_amount_min`/`fiat_amount_max` are
present then `fiat_amount` must be absent; reference the `fiat_amount`,
`fiat_amount_min`, and `fiat_amount_max` fields when updating the wording and
validation guidance.
In `@specs/001-mostro-p2p-client/tasks.md`:
- Line 49: Update task T017 wording to avoid the incorrect "Pending through
Expired" range: state the requirement as "implement all order states defined in
rust/src/protocol/state_machine.rs" and list or reference the actual state names
(e.g., Pending, Expired, Dispute, CompletedByAdmin, etc.) so implementers know
to include the extra states beyond Expired; keep the file reference to
rust/src/protocol/state_machine.rs and the task ID T017 so reviewers can locate
the implementation target.
---
Nitpick comments:
In `@specs/001-mostro-p2p-client/tasks.md`:
- Line 6: Add explicit test tasks covering protocol-critical paths: create
checklist items in the specs/001-mostro-p2p-client/tasks.md to test the new fiat
mode validation (validate fixed-vs-range pricing behavior and edge cases),
exercise the 15-state lifecycle state-machine transitions (each transition,
invalid transitions, and recovery paths), and include integration scenarios that
combine fiat validation with lifecycle changes; reference the specific features
by name (fiat mode validation, fixed-vs-range validation, 15-state lifecycle,
state-machine transitions) and add them to the `/speckit.checklist` so CI/tests
surface regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5b89cde-bb22-4559-8735-01767ed82d89
📒 Files selected for processing (7)
CLAUDE.mdspecs/001-mostro-p2p-client/contracts/orders.mdspecs/001-mostro-p2p-client/contracts/types.mdspecs/001-mostro-p2p-client/data-model.mdspecs/001-mostro-p2p-client/plan.mdspecs/001-mostro-p2p-client/research.mdspecs/001-mostro-p2p-client/tasks.md
…dation - Remove duplicate Recent Changes entry in CLAUDE.md - Add fiat_amount_min/max to TradeHistoryEntry so range orders preserve min/max values in trade history - Add mutual-exclusivity clause to data-model validation: fixed amount and range amount cannot both be present - List all 15 state names explicitly in T017 task description instead of misleading "Pending through Expired" range
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/data-model.md`:
- Line 21: Specify a single source of truth for privacy_mode and a clear sync
contract: declare whether Identity.privacy_mode or Settings.privacy_mode is
authoritative (e.g., "Identity.privacy_mode is authoritative"), define the write
path (which API/method updates it — e.g., calls to setPrivacyMode on Identity
should be used; Settings writes must forward to Identity), and document
conflict-resolution and sync timing (e.g., Settings updates are eventual and
must read Identity first, Identity wins on conflict, propagate
Identity->Settings immediately and enqueue retries on failure, include
timestamps or vector clock to resolve concurrent updates). Reference
Identity.privacy_mode and Settings.privacy_mode in the spec and add a short
paragraph describing the required developer-side enforcement (update methods to
always update authoritative source and publish a sync event).
- Line 201: Clarify the lifecycle of the Settings key `logging_enabled`: update
the spec text so it explicitly states whether `logging_enabled` is persisted to
storage or treated as ephemeral and whether startup code overwrites any
persisted value; for example, state either “persisted in Settings storage and
honored on restart” or “not persisted — runtime-only and reset to false on
process start (startup code sets `logging_enabled = false`)”; include the
corresponding implementation note so readers know whether to change persistence
logic or add startup override in code that initializes Settings.
- Line 75: Clarify the timeout semantics by updating the definitions of the
terminal states so they are unambiguous: state that Expired represents automatic
termination due to protocol-level inactivity timeouts (e.g., offer/accept
deadlines or global TTLs enforced by the system), while Canceled represents
explicit termination initiated by the creator or actor (user-triggered cancel)
and only falls back to Canceled-by-timeout when a creator-intended auto-cancel
timer (a creator-configured cancel window) elapses; update the prose for the
symbols Expired and Canceled and any associated state-transition bullets or
examples (including the “Canceled (by creator or timeout)” line) to reflect this
mapping, and add a short example showing an Expired path (system-enforced
deadline) versus a Canceled path (creator action or creator-configured
auto-cancel).
In `@specs/001-mostro-p2p-client/tasks.md`:
- Line 22: Update the T001 task text to use the capitalized platform name
"macOS" for prose consistency: locate the checklist item string containing "T001
Initialize Flutter project ... (ios/, android/, web/, macos/, windows/, linux/)"
and change the prose reference "macos/" to "macOS" while keeping the literal
folder path (macos/) unchanged where necessary so folder names remain accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cd6d286-32cb-4ed8-b5a0-1814d2dadca9
📒 Files selected for processing (4)
CLAUDE.mdspecs/001-mostro-p2p-client/contracts/types.mdspecs/001-mostro-p2p-client/data-model.mdspecs/001-mostro-p2p-client/tasks.md
✅ Files skipped from review due to trivial changes (1)
- CLAUDE.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/001-mostro-p2p-client/contracts/types.md
…antics - Declare Identity.privacy_mode as authoritative source; writes must go through Identity API and propagate to Settings, not the reverse - Clarify logging_enabled is runtime-only and not persisted to storage; startup unconditionally resets to false - Distinguish Expired (protocol-enforced inactivity timeout) from Canceled (explicit user action) in state machine documentation - Use proper platform names in T001 task description (iOS, macOS, etc.) while keeping directory paths lowercase
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/tasks.md`:
- Around line 97-105: The tasks T039 and T047 (and related entries lines
111-121) currently tagged [US1] implement core browse/filter behavior tied to
US4 scenarios—rename and retag these tasks to [US4] and update their
descriptions (e.g., T039: lib/screens/home/home_screen.dart - "Implement order
list with filters (buy/sell, currency, payment method) per US4 phase X"; T047:
lib/providers/orders_provider.dart - "Orders provider: fetch/cache/apply filters
per US4") so they represent the full US4 scope; split out any phase-specific or
incremental items (offline/filter persistence, web fallbacks) into separate
tasks with explicit phase tags (e.g., US4-PHASE5) and clear acceptance criteria
so each task is independently testable; ensure references to files like
lib/screens/home/home_screen.dart, lib/providers/orders_provider.dart, and task
IDs (T039, T047) are used in the updated descriptions to preserve traceability.
- Around line 91-92: Update the order creation validation in create_order() (and
the NewOrderParams type) to enforce explicit rules: require either a fixed
fiat_amount OR a range (fiat_amount_min and fiat_amount_max), but not both or
neither; validate that fiat_amount, fiat_amount_min, and fiat_amount_max are
positive numbers; ensure fiat_amount_min < fiat_amount_max when both are
provided; and return clear validation errors for each violation so callers and
tests can rely on these acceptance criteria.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22fa309f-38dd-4d8e-9090-74806ba8ea83
📒 Files selected for processing (2)
specs/001-mostro-p2p-client/data-model.mdspecs/001-mostro-p2p-client/tasks.md
🚧 Files skipped from review as they are similar to previous changes (1)
- specs/001-mostro-p2p-client/data-model.md
Retagged to [US4] with note "(required by US1/US2 for order selection)"; Phase 5 updated to clarify it adds incremental enhancements T033 missing validation rules: Added explicit validation: fixed-vs-range mutual exclusivity, positive amounts, min < max, distinct errors per violation
This was referenced Mar 23, 2026
Merged
Merged
This was referenced Mar 31, 2026
grunch
added a commit
that referenced
this pull request
Apr 2, 2026
orders.rs — Fix #7: store_trade_key_index before publish Moved store_trade_key_index(&order_id, trade_index) from before the action dispatch to inside the publish-success else branch. A publish failure no longer leaves a stale key mapping. orders.rs — Fix #5: ln_address logged in plain text Replaced ln_address={:?} (which printed the actual address) with ln_address=present/none using if ln_address_ref.is_some(). orders.rs — Fix #4: fixed 30-min deadline regardless of activity Replaced the fixed deadline with a last_activity instant that resets on each matching event. The idle timeout is now 30 minutes of inactivity, not 30 minutes of total uptime. add_lightning_invoice_screen.dart — Fix #2: Lightning Address sent with 1-sat amount _isValid now returns false for @ inputs when _resolvedSats is null. _submit also guards against it and surfaces an error instead of falling through to BigInt.one. trade_detail_screen.dart — Fix #3: OrderStatus.pending mapped to TradeStatus.active Added TradeStatus.loading to the enum and made it the default branch in _mapOrderStatus. Unresolved/initial statuses no longer match the active-trade action button sections.
4 tasks
6 tasks
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.
Summary by CodeRabbit
New Features
Documentation