Development fund #554
Development fund #554
Conversation
Add comprehensive unit tests for get_dev_fee() function covering: - Standard 30% fee calculation - Rounding behavior with decimal results - Zero fee edge case - Small amount handling (< 10 sats) Tests include proper settings initialization using Once guard to ensure thread-safe test execution. All tests passing: 4 passed, 0 failed
Add detailed documentation for development fee mechanism: Technical Specification (docs/DEV_FEE_TECHNICAL_SPEC.md): - Architecture overview with fee flow diagrams - Complete configuration guide with validation rules - Implementation details with code references and line numbers - Database schema and migration documentation - Monitoring queries for tracking fees and success rates - Log filtering commands and troubleshooting procedures - Security considerations and audit trail explanation - Performance impact analysis - Migration guide for existing installations - Manual retry procedures for failed payments README.md Updates: - New Development Fee section after Mostro's reputation - Key features summary (configurable, transparent, non-blocking) - Practical example: 100k sats order with fee breakdown - Link to technical specification for details CHANGELOG.md Updates: - New Unreleased section with comprehensive feature list - Database migration details - Configuration changes - Documentation additions - Dependency updates (mostro-core v0.6.57) - Confirmed backward compatibility Documentation provides operators with complete guidance for: - Initial setup and configuration - Monitoring payment success rates - Troubleshooting failed payments - Understanding security model - Reconciling development fee payments
Implement comprehensive development fee system providing sustainable funding for Mostro platform development through automatic Lightning payments on successful orders. Core Implementation: - Automatic fee collection: configurable percentage (default 30%, min 10%) - Lightning Address payment to development@mostro.network - Non-blocking architecture: failed payments logged, orders complete normally - Seller-pays model: dev fee included in hold invoice - Full database audit trail with payment tracking Configuration & Validation: - New constants module with hardcoded limits and destination - Settings validation on daemon startup - Fee range enforcement (10-100% of Mostro fee) - Template updates with dev_fee_percentage setting Fee Calculation & Integration: - get_dev_fee() function with proper rounding - Integration in order preparation workflow - Hold invoice amount calculation includes dev fee - Example: 100k sats + 1k fee + 300 dev fee = 101,300 sats seller pays Payment Execution: - LNURL resolution for Lightning Address - LND connector integration - 30-second timeout with comprehensive error handling - Detailed logging with 'dev_fee' target for monitoring Database Schema: - Migration 20251126120000: three new tracking columns - dev_fee: amount in satoshis - dev_fee_paid: boolean payment status - dev_fee_payment_hash: Lightning payment hash for reconciliation - Backward compatible with existing orders Testing & Quality: - Unit tests for fee calculation covering edge cases - Settings initialization for test environment - All tests passing with proper thread safety Documentation: - Technical specification with architecture and operations guide - README section explaining fee mechanism - CHANGELOG entries for all changes - Monitoring queries and troubleshooting procedures Files Modified: 10 code files, 3 documentation files Dependencies: Updated to mostro-core v0.6.57 Breaking Changes: None - fully backward compatible
- Updated settings validation to use proper Lightning Address validation and align error types.
- Replaced ad-hoc string check with a real reachability check via ln_exists(...).
- Adjusted error variants: range checks now return MostroInternalErr(ServiceError::IOError(...)) (ConfigError variant isn’t available in
ServiceError), and Lightning Address reachability returns MostroInternalErr(ServiceError::InvoiceInvalidError).
- Made validate_mostro_settings async and invoked it from init_configuration_file using tokio::runtime::Handle::current().block_on(...).
Fix test_lnurl_validation_with_test_server failing due to port mismatch between test server and Lightning address resolver. Issue: - Test server was binding to random port (port 0) - Lightning address resolver in test mode hardcodes port 8080 - MostroP2P@127.0.0.1 validation failed (wrong port) Solution: - Try binding to port 8080 first (required for Lightning addresses) - Fallback to random port if 8080 unavailable - Return port number from start_test_server() - Only run Lightning address tests if port 8080 was bound Result: All 107 tests now passing
WalkthroughAdds a configurable development-fee feature: new settings/constants and startup validation, DB migration to track dev fees, fee calculation and propagation through order/invoice flows, non-blocking scheduler/LND payment processing, LNURL/test adjustments, and documentation/changelog updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Release as release.rs
participant DB as Database
participant Scheduler as scheduler.rs
participant Resolver as LN Address Resolver
participant LND as LND/Wallet
Release->>DB: mark order Success, reset failed_payment/payment_attempts, enqueue rating
Release->>Scheduler: note dev-fee pending (non-blocking)
par Immediate order completion
Release-->>DB: return order completed
and Async dev-fee processing
Scheduler->>DB: find_orders_with_unpaid_dev_fee()
DB-->>Scheduler: orders[]
loop for each order
Scheduler->>Resolver: resolve DEV_FEE_LIGHTNING_ADDRESS -> invoice
alt resolved
Scheduler->>LND: send payment (with timeout)
alt success
LND-->>Scheduler: payment_hash
Scheduler->>DB: set dev_fee_paid=1, store payment_hash
else fail/timeout
Scheduler-->>DB: log failure for retry
end
else resolve fail
Scheduler-->>DB: log resolution failure for retry
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)src/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
src/app/**/*.rs📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (17)📓 Common learnings📚 Learning: 2025-06-25T22:48:44.741ZApplied to files:
📚 Learning: 2025-12-17T13:04:13.036ZApplied to files:
📚 Learning: 2025-07-17T13:51:17.338ZApplied to files:
📚 Learning: 2025-06-25T22:48:44.741ZApplied to files:
📚 Learning: 2025-10-09T14:06:50.067ZApplied to files:
📚 Learning: 2025-04-27T20:07:24.558ZApplied to files:
📚 Learning: 2025-02-21T13:26:17.026ZApplied to files:
📚 Learning: 2025-11-06T22:54:07.055ZApplied to files:
📚 Learning: 2025-12-12T20:35:23.220ZApplied to files:
📚 Learning: 2025-12-12T20:35:23.220ZApplied to files:
📚 Learning: 2025-12-12T20:35:23.220ZApplied to files:
📚 Learning: 2025-12-12T20:35:23.220ZApplied to files:
📚 Learning: 2025-12-12T20:35:23.220ZApplied to files:
📚 Learning: 2025-12-12T20:35:23.220ZApplied to files:
📚 Learning: 2025-06-13T12:50:53.424ZApplied to files:
📚 Learning: 2025-03-11T23:26:33.763ZApplied to files:
🧬 Code graph analysis (1)src/app/release.rs (4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
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}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
src/lightning/invoice.rs (1)
322-329: Consider more defensive error handling in the probe check.The probe logic on line 325 could panic if
probe.is_ok()but calling.status()on the response fails. While unlikely, a safer approach would be:- if let Ok(decoded) = LnUrl::decode(lnurl.clone()) { - let probe = crate::lnurl::HTTP_CLIENT.get(&decoded.url).send().await; - if probe.is_err() || !probe.unwrap().status().is_success() { - server_handle.abort(); - return; - } - } + if let Ok(decoded) = LnUrl::decode(lnurl.clone()) { + let probe = crate::lnurl::HTTP_CLIENT.get(&decoded.url).send().await; + let is_reachable = probe.is_ok() && probe.as_ref().unwrap().status().is_success(); + if !is_reachable { + server_handle.abort(); + return; + } + }Or more concisely with pattern matching:
- if let Ok(decoded) = LnUrl::decode(lnurl.clone()) { - let probe = crate::lnurl::HTTP_CLIENT.get(&decoded.url).send().await; - if probe.is_err() || !probe.unwrap().status().is_success() { - server_handle.abort(); - return; - } - } + if let Ok(decoded) = LnUrl::decode(lnurl.clone()) { + let is_reachable = crate::lnurl::HTTP_CLIENT + .get(&decoded.url) + .send() + .await + .map(|resp| resp.status().is_success()) + .unwrap_or(false); + if !is_reachable { + server_handle.abort(); + return; + } + }src/config/util.rs (2)
85-88: Duplicated comment on lines 85-86.Remove the duplicate comment line.
// Validate settings before initializing - // Validate settings before initializing // Network check for Lightning Address reachability requires async tokio::runtime::Handle::current().block_on(validate_mostro_settings(&settings))?;
36-39: Consider skipping Lightning address validation when dev fee is disabled.When
dev_fee_percentageis 0, no dev fee will be charged, so validating the Lightning address is unnecessary and could cause startup failures due to network issues.// Validate Lightning Address format and reachability - if ln_exists(DEV_FEE_LIGHTNING_ADDRESS).await.is_err() { - return Err(MostroInternalErr(ServiceError::InvoiceInvalidError)); + if dev_fee > MIN_DEV_FEE_PERCENTAGE { + if ln_exists(DEV_FEE_LIGHTNING_ADDRESS).await.is_err() { + return Err(MostroInternalErr(ServiceError::LnAddressParseError)); + } }src/util.rs (1)
1222-1232: Test migration approach is acceptable but could be more robust.Using
.ok()to swallow errors works for test setup, but consider using the actual migration file to ensure tests match production schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)Cargo.toml(1 hunks)README.md(1 hunks)docs/DEV_FEE_TECHNICAL_SPEC.md(1 hunks)migrations/20251126120000_dev_fee.sql(1 hunks)settings.tpl.toml(1 hunks)src/app/release.rs(3 hunks)src/config/constants.rs(1 hunks)src/config/mod.rs(3 hunks)src/config/types.rs(2 hunks)src/config/util.rs(2 hunks)src/lightning/invoice.rs(5 hunks)src/util.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/config/constants.rssrc/config/mod.rssrc/config/types.rssrc/util.rssrc/config/util.rssrc/lightning/invoice.rssrc/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/config/constants.rssrc/config/mod.rssrc/config/types.rssrc/util.rssrc/config/util.rssrc/lightning/invoice.rssrc/app/release.rs
migrations/**
📄 CodeRabbit inference engine (AGENTS.md)
DB migrations should be stored in
migrations/directory
Files:
migrations/20251126120000_dev_fee.sql
migrations/**/*.sql
📄 CodeRabbit inference engine (AGENTS.md)
After SQL/schema changes, run
cargo sqlx prepare -- --bin mostrodto refreshsqlx-data.json
Files:
migrations/20251126120000_dev_fee.sql
settings.tpl.toml
📄 CodeRabbit inference engine (AGENTS.md)
Keep configuration templates in
settings.tpl.toml
Files:
settings.tpl.toml
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (14)
📓 Common learnings
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: Cross-repository dependencies in Mostro project: when PRs introduce new types like PaymentFailedInfo from mostro-core, compilation errors are expected until the dependency PR is merged and the version is updated in the consuming repository.
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use SCREAMING_SNAKE_CASE for constants in Rust
Applied to files:
src/config/constants.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/config/constants.rssrc/util.rsREADME.mdsrc/app/release.rs
📚 Learning: 2025-08-21T10:00:43.096Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 513
File: src/config/types.rs:96-97
Timestamp: 2025-08-21T10:00:43.096Z
Learning: In the Mostro project, all configuration parameters present in settings.tpl.toml are required for the mostro daemon to start. The daemon will not start if any parameter is missing from the settings.toml file. This means serde defaults are not relevant for backward compatibility since users must explicitly configure all values during upgrades.
Applied to files:
src/config/mod.rssrc/config/types.rssettings.tpl.tomlREADME.mdsrc/config/util.rs
📚 Learning: 2025-05-02T14:21:39.391Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 481
File: src/config/util.rs:48-76
Timestamp: 2025-05-02T14:21:39.391Z
Learning: The `init_default_dir` function in Mostro intentionally exits the process (std::process::exit(0)) after creating a new configuration file so users can edit the configuration before restarting the application.
Applied to files:
src/config/mod.rssrc/config/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: Cross-repository dependencies in Mostro project: when PRs introduce new types like PaymentFailedInfo from mostro-core, compilation errors are expected until the dependency PR is merged and the version is updated in the consuming repository.
Applied to files:
CHANGELOG.mdREADME.md
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to migrations/**/*.sql : After SQL/schema changes, run `cargo sqlx prepare -- --bin mostrod` to refresh `sqlx-data.json`
Applied to files:
Cargo.tomlsrc/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util.rssrc/config/util.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/util.rssrc/config/util.rssrc/lightning/invoice.rssrc/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to settings.toml : Do not commit populated `settings.toml`; copy from `settings.tpl.toml` to `~/.mostro/settings.toml` for local runs
Applied to files:
settings.tpl.tomlsrc/config/util.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/lightning/invoice.rs
🧬 Code graph analysis (4)
src/util.rs (2)
src/config/settings.rs (2)
get_mostro(45-50)init_mostro_settings(24-28)src/config/types.rs (2)
default(67-73)default(114-137)
src/config/util.rs (2)
src/config/settings.rs (1)
init_mostro_settings(24-28)src/lnurl.rs (1)
ln_exists(45-70)
src/lightning/invoice.rs (1)
src/util.rs (1)
HTTP_CLIENT(48-53)
src/app/release.rs (2)
src/lightning/mod.rs (1)
new(38-52)src/lnurl.rs (1)
resolv_ln_address(72-124)
🪛 LanguageTool
docs/DEV_FEE_TECHNICAL_SPEC.md
[grammar] ~229-~229: Use a hyphen to join words.
Context: ..._LOG="dev_fee=debug" mostrod **View only errors:**bash RUST_LOG="dev_fee...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/DEV_FEE_TECHNICAL_SPEC.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
237-237: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
243-243: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
252-252: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
253-253: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
258-258: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
264-264: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
437-437: Bare URL used
(MD034, no-bare-urls)
438-438: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (20)
Cargo.toml (2)
72-72: Ensure mostro-core version 0.6.57 is published and includes the development-fee functionality required by this PR.Verify the updated version exists on crates.io and confirm it includes all necessary changes for the development fee mechanism without introducing breaking changes. The sqlx feature is correctly preserved.
72-72: Schema changes present but verify new queries were added with dev_fee columns.The migration
20251126120000_dev_fee.sqlwas added with columns tracking development fees. However, no Rust code in this PR references the newdev_fee,dev_fee_paid, ordev_fee_payment_hashcolumns. If this PR adds SQL queries that use these columns, runcargo sqlx prepare -- --bin mostrodto save metadata for all invocations of query macros tosqlx-data.json. If the schema change is incomplete or queries will be added separately (perhaps in mostro-core), clarify in the PR description.src/lightning/invoice.rs (3)
232-267: LGTM! Port-aware test server setup improves test reliability.The graceful fallback from port 8080 to a random port, combined with returning the actual bound port, allows tests to adapt to different environments. The switch from
localhostto127.0.0.1ensures consistency with the Lightning address test cases.
314-318: LGTM! Environment guard prevents false test failures.The early return when socket binding is restricted ensures tests don't fail in constrained environments (e.g., sandboxed CI). While this silently skips the test, it's a reasonable trade-off for test portability.
342-361: Note the coupling between test configuration and lnurl.rs implementation.The conditional test execution based on
port == 8080is tightly coupled to the hardcoded port inlnurl.rs'scfg!(test)implementation. While your comment documents this, be aware that changes to the test configuration inlnurl.rscould silently skip these Lightning address tests without failing.settings.tpl.toml (1)
54-56: LGTM! Development fee configuration added correctly.The configuration follows the template pattern and clearly documents the minimum 10% requirement. The 30% default is reasonable for sustainable development funding.
src/config/mod.rs (3)
2-2: LGTM! Module declaration follows Rust conventions.The new constants module is properly declared.
18-18: LGTM! Constants re-exported correctly.The re-exports provide convenient access to the development fee configuration constants.
89-90: LGTM! Test data updated appropriately.The test TOML now includes the
dev_fee_percentagefield with the default value, ensuring test coverage of the new configuration parameter.src/config/types.rs (3)
78-78: LGTM! Correctly switched from derived to manual Default.Removing
Defaultfrom the derive macro and implementing it manually is the correct approach when custom initialization logic is needed.
108-111: LGTM! Field addition is well-documented.The
dev_fee_percentagefield is properly typed asf64and includes clear documentation explaining it as a percentage of the Mostro fee.
113-138: Code is correct—validation is properly implemented.The
dev_fee_percentagebounds are enforced at daemon startup. Thevalidate_mostro_settings()function insrc/config/util.rschecks thatdev_fee_percentagestays within the bounds defined byMIN_DEV_FEE_PERCENTAGE(0.10) andMAX_DEV_FEE_PERCENTAGE(1.0), and the default value of 0.30 is correctly within range.migrations/20251126120000_dev_fee.sql (1)
5-7:andsrc/app/release.rs (2)
532-563: Dev fee integration looks good - non-blocking design is appropriate.The dev fee payment failure is correctly handled as non-fatal, allowing the order to complete even if the fee payment fails. The success/failure state is properly tracked on the order for later reconciliation.
584-601: Good defensive programming for dev fee validation.The early return for zero/negative dev_fee values is clean and prevents unnecessary network calls. The structured logging with target "dev_fee" enables easy log filtering.
src/config/util.rs (1)
17-42: Validation logic is correct and comprehensive.The bounds checking for
dev_fee_percentageagainstMIN_DEV_FEE_PERCENTAGEandMAX_DEV_FEE_PERCENTAGEis appropriate. The function correctly fails startup if configuration is invalid.src/util.rs (4)
156-162: Dev fee calculation is straightforward and well-documented.The function correctly calculates the dev fee as a percentage of the Mostro fee (not the total amount), using rounding for satoshi precision.
652-695: Seller sees different amount than what they pay in hold invoice.The hold invoice amount includes
dev_fee(line 654), but thenew_ordersent to the seller at line 695 only showsorder.amount + order.feewithout the dev fee. This means the seller's displayed amount is less than the actual invoice amount.Please verify this is intentional. If the dev fee should be transparent to sellers, consider including it in the displayed amount:
new_order.status = Some(Status::WaitingPayment); - new_order.amount = order.amount + order.fee; + new_order.amount = order.amount + order.fee + order.dev_fee;
376-397: Order initialization with dev fee fields is correct.The new fields are properly initialized:
dev_feeis calculated when amount is known,dev_fee_paiddefaults tofalse, anddev_fee_payment_hashisNoneuntil payment succeeds.
1461-1489: Good test coverage for dev fee calculation edge cases.Tests cover the key scenarios: standard percentage calculation, rounding behavior (99.9 → 100), zero input, and small amounts where rounding matters (0.9 → 1).
This commit implements two related improvements to the dev fee system: 1. 50/50 Development Fee Split: - Update dev fee calculation to use total Mostro fee (both parties combined) - Seller pays half of dev fee in hold invoice (order.amount + fee + dev_fee/2) - Buyer pays half of dev fee from received amount (order.amount - fee - dev_fee/2) - Total dev fee now stored in order.dev_fee field - Enhanced function documentation and test comments for clarity 2. Lazy Validation for Lightning Address: - Keep settings initialization synchronous for faster startup - Defer Lightning Address validation until first actual use - Remove network calls from daemon startup sequence - Validation happens in send_dev_fee_payment() via resolv_ln_address() Example: 100k sat order with 1% Mostro fee (1,000 sats) and 30% dev fee: - Total dev fee: 300 sats (150 buyer + 150 seller) - Seller pays: 100,650 sats (100k + 500 Mostro fee + 150 dev fee) - Buyer receives: 99,350 sats (100k - 500 Mostro fee - 150 dev fee) Changes: - src/util.rs: Update get_dev_fee() to calculate from total Mostro fee, modify order creation and hold invoice generation for 50/50 split, enhance test comments - src/app/release.rs: Deduct buyer's half of dev fee from received amount - src/config/util.rs: Remove Lightning Address validation from startup, keep validate_mostro_settings() synchronous - src/cli.rs: Keep settings_init() synchronous - src/main.rs: Call settings_init() synchronously without await - docs/DEV_FEE_TECHNICAL_SPEC.md: Update all sections for 50/50 split model - README.md: Update dev fee example to reflect 50/50 split All 107 tests passing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/release.rs (1)
635-691: Timeout logic may fail on non-terminal payment statuses.The 30-second timeout wraps a single
rx.recv()call. If the first message received has statusInFlightorPending, the catch-all branch at line 652 treats it as a failure, even though the payment may succeed later.Consider looping until a terminal status (
SucceededorFailed) is received, treatingInFlight/Pendingas non-terminal and continuing to wait within the 30-second timeout window.Note: This issue was flagged in previous review comments (lines 635-690). Since dev fee payments are non-blocking and failures don't prevent order completion, the impact is limited to missed dev fee collections rather than broken core functionality.
🧹 Nitpick comments (1)
docs/DEV_FEE_TECHNICAL_SPEC.md (1)
17-17: Optional: Address markdown linting issues for documentation polish.Static analysis flagged several formatting improvements:
- Missing language specifiers on code blocks (lines 17, 92, 271, 277, 287)
- Bare URLs at lines 473-474 (prefer markdown links)
- Minor grammar suggestion at line 263
These don't affect functionality but would improve documentation quality.
Also applies to: 92-92, 271-271, 277-277, 287-287, 473-474
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)docs/DEV_FEE_TECHNICAL_SPEC.md(1 hunks)src/app/release.rs(4 hunks)src/config/util.rs(3 hunks)src/util.rs(7 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/util.rssrc/config/util.rssrc/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/util.rssrc/config/util.rssrc/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (16)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:07.466Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-12-17T13:04:07.466Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:07.466Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
README.mdsrc/util.rssrc/config/util.rssrc/app/release.rs
📚 Learning: 2025-08-21T10:00:43.096Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 513
File: src/config/types.rs:96-97
Timestamp: 2025-08-21T10:00:43.096Z
Learning: In the Mostro project, all configuration parameters present in settings.tpl.toml are required for the mostro daemon to start. The daemon will not start if any parameter is missing from the settings.toml file. This means serde defaults are not relevant for backward compatibility since users must explicitly configure all values during upgrades.
Applied to files:
README.mdsrc/config/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: Cross-repository dependencies in Mostro project: when PRs introduce new types like PaymentFailedInfo from mostro-core, compilation errors are expected until the dependency PR is merged and the version is updated in the consuming repository.
Applied to files:
README.md
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
README.mdsrc/util.rssrc/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/util.rssrc/config/util.rssrc/app/release.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/util.rssrc/app/release.rsdocs/DEV_FEE_TECHNICAL_SPEC.md
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util.rssrc/config/util.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/util.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to migrations/**/*.sql : After SQL/schema changes, run `cargo sqlx prepare -- --bin mostrod` to refresh `sqlx-data.json`
Applied to files:
src/util.rs
📚 Learning: 2025-05-02T14:21:39.391Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 481
File: src/config/util.rs:48-76
Timestamp: 2025-05-02T14:21:39.391Z
Learning: The `init_default_dir` function in Mostro intentionally exits the process (std::process::exit(0)) after creating a new configuration file so users can edit the configuration before restarting the application.
Applied to files:
src/config/util.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to settings.toml : Do not commit populated `settings.toml`; copy from `settings.tpl.toml` to `~/.mostro/settings.toml` for local runs
Applied to files:
src/config/util.rs
📚 Learning: 2025-12-17T13:04:07.466Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:07.466Z
Learning: In docs/DEV_FEE_TECHNICAL_SPEC.md, note that the Lightning address used in the Mostro development fee implementation is temporary for testing: mostro_p2psats.mobi. The production address should be developmentmostro.network as documented. This mismatch is intentional for testing and must be resolved before production deployment. Ensure the doc reflects the correct production address and verify that any references to test vs. production addresses are clearly distinguished.
Applied to files:
docs/DEV_FEE_TECHNICAL_SPEC.md
🧬 Code graph analysis (1)
src/util.rs (2)
src/config/settings.rs (2)
get_mostro(45-50)init_mostro_settings(24-28)src/config/types.rs (2)
default(67-73)default(114-137)
🪛 LanguageTool
docs/DEV_FEE_TECHNICAL_SPEC.md
[grammar] ~263-~263: Use a hyphen to join words.
Context: ..._LOG="dev_fee=debug" mostrod **View only errors:**bash RUST_LOG="dev_fee...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/DEV_FEE_TECHNICAL_SPEC.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
271-271: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
277-277: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
286-286: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
287-287: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
292-292: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
298-298: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
473-473: Bare URL used
(MD034, no-bare-urls)
474-474: Bare URL used
(MD034, no-bare-urls)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (8)
src/config/util.rs (1)
14-34: LGTM! Clear startup validation for dev fee configuration.The validation logic correctly enforces dev_fee_percentage bounds using the imported constants, with clear error messages that include the actual value and limits. The placement before database initialization ensures that misconfigured instances fail fast at startup.
Also applies to: 77-78
README.md (1)
53-69: LGTM! Clear and concise documentation of the development fee mechanism.The Development Fee section effectively communicates the key features (configurable percentage, transparent destination, non-blocking operation, audit trail) with a practical example. The link to the technical specification provides operators with detailed guidance.
src/app/release.rs (2)
440-442: Buyer dev fee calculation is correct.The code correctly subtracts the buyer's share of the development fee (half of the total dev_fee) from the payment amount. Integer division for odd amounts will truncate (e.g., 301/2 = 150), which is acceptable for satoshi-level precision.
534-565: LGTM! Non-blocking dev fee payment flow correctly implemented.The payment flow properly handles success, zero-fee, and error cases. Failures are logged with detailed context but don't prevent order completion, aligning with the non-blocking design principle. Database fields (dev_fee_paid, dev_fee_payment_hash) are updated appropriately.
src/util.rs (4)
156-164: LGTM! Dev fee calculation is clear and correct.The function properly calculates the total development fee as a percentage of the total Mostro fee, with appropriate rounding. The comment clearly states that both input and output are totals (to be split 50/50 between parties).
378-383: LGTM! Dev fee calculation correctly derives from total Mostro fee.The flow is correct:
get_fee()returns each party's share of the Mostro fee (split)- Line 382 calculates the total Mostro fee by doubling the split
- Line 383 applies the dev fee percentage to the total
This ensures the dev fee is calculated from the combined Mostro fee as documented.
1225-1235: LGTM! Test migration properly adds dev fee columns.The test setup correctly applies the same schema changes as the production migration, ensuring test database structure matches production. Using
.ok()to ignore duplicate column errors is appropriate for test scenarios.
1439-1499: LGTM! Dev fee tests cover key scenarios effectively.The test suite validates:
- Standard percentage calculation (30% of 1000 sats)
- Rounding behavior (99.9 → 100)
- Zero input handling
- Small amount edge cases (0.9 → 1)
The settings initialization uses
Onceto avoid conflicts with other tests, and the test cases are focused and clear.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
creo que no se está cobrando el dev_fee, o no sé si estoy haciendo algo mal, porque me sale en 0 en la db, tengo puesto de fee 0.6%
(en la imagen la 1era orden que sale con dev fee 1 era de antes del ultimo commit)
There was a problem hiding this comment.
Eliminé y volví a crear la base de datos y ya cobra fee, asi que el comentario anterior no hay que hacerle caso.
Pero le puse de fee al mostro 1% y creé una orden de 100000 sats pero al comprador no le cobró el dev_fee, solo al vendedor que sí le cobró bien lo que debía.
El comprador recibió order amount - order fee, sin descontar dev_fee
- orden 100000 sats
- vendedor pagó 100650 sats
- comprador recibió 99500 sats (debió recibir 99350 sats)
En este mensaja de pay-invoice se ve la invoice del comprador y la invoice que paga el vendedor
2025-12-17T22:31:09.478704Z INFO mostrod::util: Sending DM, Event ID: 758a29748bb92ed9c18570e3ef54ba1d88253cb3d56bf6e27a57ca90203fd2da to e6f606d1571723a350bb691a7b5b17e357cb74caadc49ba8be3abba7995741e7 with payload: "{\"order\":{\"version\":1,\"request_id\":null,\"trade_index\":6,\"id\":\"ac2211e2-79f3-47bd-9f02-7cffa9a6d4a5\",\"action\":\"pay-invoice\",\"payload\":{\"payment_request\":[{\"id\":\"ac2211e2-79f3-47bd-9f02-7cffa9a6d4a5\",\"kind\":\"sell\",\"status\":\"waiting-payment\",\"amount\":100500,\"fiat_code\":\"CUP\",\"min_amount\":null,\"max_amount\":null,\"fiat_amount\":5000,\"payment_method\":\"test\",\"premium\":0,\"buyer_invoice\":\"lnbcrt995u1p55xtc4pp5h0m6280xw69sjnaqfsqf9tg4yha3ww6h5r037c3nunsxtdvl8hpsdqqcqzzsxqyz5vqsp5czg9ycnunhg4xdw6flwjn3gfvl53wg667jdsaspaj2quqgzekwnq9qyyssq4xl4vx9e02j8kcdgc82lzv43e0w5j9ypmzg5cfccnjtcmfpfqw9kpdua3kxuqs9n0ucqtedn69545tmj3jx6ek5ee9jvq333cfsnfngpm9hzxt\",\"created_at\":1766010377,\"expires_at\":1766046377},\"lnbcrt1006500n1p55xtevpp5l7arfjrcwv7kmxjl0wxkd0ks0mmvc2ajjam2g6jlhm7fjmaqucmqdfcg4ekxun0wusxzmt0w4h8ggz0wfjx2u3qydskxv3jxyck2v3dxuukvvedxsmkyepd89nrqv3dxa3kvenp89snvep5vy6n5gzng4xycgzz23pjqen0wgsyx42syq6nqvpsyqkjqjt5ypt5jnzvypr9y329tfzjqj2wypt5znzvg42zugzfwss8w6tvdss8yetvv4shxefqdahxxefq09hh2grjv4kx2ctnv5hzqjt5ypmkjmrvypex2ar4wfhzq6txyp3827t9wgsxgmm9wvsxumm5yp3k7mnxd9ex6gr5dpjjqurp09kk2mn5cqzf2xqyz5vqsp58v094w8sjr52w8wkr89affypfvfn8y3ynfmwqkfet7a8kx62xgpq9qyyssqydnyzjdr29kejkducwfedfy9ju8m5ty3epg6t6xf4ytnz7a5sfpx37xx6vsp70dcjlw6cx8zk07l4fjsdn8xytf7aq89n205y8zxh3cpdk0hd4\",null]}}}"
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
en el mensaje de pay-invoice al vendedor le dice la cantidad que debe pagar incorrecta, porque solo muestra order amount + order fee, pero le falta + dev fee
Por ejemplo, le dice que la cantidad a pagar es de 100500 pero la invoice es de 100650
Corrected amount calculations in messages sent to buyers and sellers to properly include their respective share of the development fee. Changes: - Buyer amounts now deduct buyer_dev_fee (dev_fee / 2) from final amount - Seller amounts now include seller_dev_fee (dev_fee / 2) in total amount - Ensures displayed amounts match actual payment requirements Modified locations: - src/util.rs: Fixed buyer amount in AddInvoice message (line 803-807) - src/util.rs: Fixed seller amount in PayInvoice message (line 698) - src/flow.rs: Fixed buyer amount in HoldInvoicePaymentAccepted (line 71-75) - src/flow.rs: Fixed buyer amount in AddInvoice fallback (line 83-84) - src/app/release.rs: Fixed buyer amount in payment retry (line 71-75) Example with 100,000 sats order, 1% Mostro fee, 30% dev fee: - Buyer receives: 99,350 sats (100,000 - 500 fee - 150 dev_fee) - Seller pays: 100,650 sats (100,000 + 500 fee + 150 dev_fee) All 107 tests passing.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/app/release.rs (1)
639-694: Timeout handling concern remains unaddressed.The timeout logic still only waits for the first status update from
rx.recv(). If the payment is in-flight when the first message arrives, it will hit the catch-all branch at line 656 and report failure even though the payment may succeed later.This was flagged in a previous review. The suggested fix was to loop until a terminal status (
SucceededorFailed) is received, rather than treating the first non-Succeededstatus as a failure.src/util.rs (1)
655-657: Integer division truncation for odd dev fee amounts.This was flagged in a previous review. When
order.dev_feeis odd, bothseller_dev_feeandbuyer_dev_fee(computed asorder.dev_fee / 2elsewhere) will truncate, losing 1 satoshi total.Example:
dev_fee = 301→ seller pays 150, buyer pays 150, total = 300 (1 sat lost).The documentation states "one party pays 1 sat more" for odd amounts, but the implementation truncates instead.
🧹 Nitpick comments (4)
src/flow.rs (3)
71-75: Dev fee calculation is correct and consistent.The buyer's share of the development fee is correctly computed as half of the total dev fee. The use of
saturating_subprevents underflow.Note: The
as i64cast on line 71 is redundant sinceorder.dev_feeis alreadyi64and integer division preserves the type. Consider removing it for clarity:- let buyer_dev_fee = (order.dev_fee / 2) as i64; + let buyer_dev_fee = order.dev_fee / 2;
86-88: Inconsistent subtraction approach between branches.The
ifbranch (line 74-75) usessaturating_subfor safe subtraction, but theelsebranch uses regular subtraction which could theoretically underflow iforder.fee + buyer_dev_fee > order_data.amount.Consider using saturating_sub for consistency:
let buyer_dev_fee = (order.dev_fee / 2) as i64; - let new_amount = order_data.amount - order.fee - buyer_dev_fee; + let new_amount = order_data.amount + .saturating_sub(order.fee) + .saturating_sub(buyer_dev_fee);
310-329: Test coverage gap: dev fee not included in fee calculation tests.The
test_fee_calculation_logictest only validatesoriginal_amount - feebut doesn't account for the newbuyer_dev_feededuction that was added to thehold_invoice_paidfunction. Consider adding test cases that verify the combined fee + dev_fee deduction logic.src/app/release.rs (1)
444-446: Consider using saturating subtraction for consistency.This calculation uses regular subtraction on
u64values. While the amounts should be valid at this point, usingsaturating_subwould align with the defensive approach used elsewhere in the codebase (e.g.,check_failure_retries).Suggested improvement:
let buyer_dev_fee = (order.dev_fee / 2) as u64; - let amount = order.amount as u64 - order.fee as u64 - buyer_dev_fee; + let amount = (order.amount as u64) + .saturating_sub(order.fee as u64) + .saturating_sub(buyer_dev_fee);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/release.rs(5 hunks)src/flow.rs(2 hunks)src/util.rs(9 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/flow.rssrc/util.rssrc/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/flow.rssrc/util.rssrc/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (11)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/flow.rssrc/util.rssrc/app/release.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/util.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to migrations/**/*.sql : After SQL/schema changes, run `cargo sqlx prepare -- --bin mostrod` to refresh `sqlx-data.json`
Applied to files:
src/util.rs
🧬 Code graph analysis (2)
src/util.rs (2)
src/config/settings.rs (2)
get_mostro(45-50)init_mostro_settings(24-28)src/config/types.rs (2)
default(67-73)default(114-137)
src/app/release.rs (2)
src/lightning/mod.rs (1)
new(38-52)src/lnurl.rs (1)
resolv_ln_address(72-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (12)
src/app/release.rs (4)
71-78: Dev fee deduction in failure retry path is correctly implemented.The buyer's dev fee share is properly calculated and deducted using
saturating_sub, with appropriate validation ensuring the resulting amount is positive.
538-570: Dev fee integration is well-designed with proper non-blocking behavior.The implementation correctly:
- Attempts dev fee payment after buyer payment succeeds
- Records success/failure status on the order
- Logs appropriately at different levels
- Does not block order completion on dev fee failure
The order with
dev_fee_paidanddev_fee_payment_hashupdates is correctly persisted viaupdate_order_eventandupdate(&pool)at lines 571-575.
631-637: LGTM: LND connection and payment initiation.The implementation correctly creates a new LND connection for the dev fee payment and uses an appropriately sized channel buffer for receiving payment status updates.
600-629: Lightning Address resolution with proper error handling.The implementation correctly handles the case where
resolv_ln_addressreturns an empty string (which can occur for invalid addresses, amount bounds issues, or callback failures per thesrc/lnurl.rssnippet).Note: Based on learnings, the
DEV_FEE_LIGHTNING_ADDRESSconstant (mostro_p2psats.mobi) is temporary for testing, with production addressdevelopmentmostro.networkplanned before deployment.src/util.rs (8)
156-164: Well-documented dev fee calculation utility.The function clearly documents that it takes the total Mostro fee (both parties combined) and returns the total dev fee to be split 50/50. The use of
round()ensures consistent behavior for fractional results.
378-384: Dev fee calculation during order creation is correct.The logic properly:
- Gets the per-party fee via
get_fee()- Reconstructs the total Mostro fee (
fee * 2)- Calculates the total dev fee from the total Mostro fee
This ensures the dev fee is proportional to the total platform fee as designed.
398-400: Order initialization with dev fee fields is correct.The new fields are properly initialized:
dev_feewith the calculated value,dev_fee_paidasfalse, anddev_fee_payment_hashasNone.
682-682: Helpful clarifying comment.The comment correctly explains that
dev_feeis set during order creation and doesn't need updating here, which helps maintainers understand the data flow.
696-698: Seller's payment amount correctly includes dev fee share.The SmallOrder sent to the seller for the hold invoice correctly includes their portion of the dev fee (
order.amount + order.fee + seller_dev_fee), ensuring the platform collects the appropriate fees.
803-807: Buyer's final amount correctly calculated with defensive subtraction.The implementation properly uses
saturating_subto prevent underflow when calculating the buyer's final amount after fee deductions.
1229-1240: Test database setup for dev fee columns.The test setup correctly adds the dev fee columns to the in-memory test database. Using
.ok()appropriately handles the case where the columns may already exist in some test scenarios.
1468-1503: Good test coverage for dev fee calculations.The tests cover:
- Standard 30% calculation (1000 × 0.30 = 300 ✓)
- Rounding behavior (333 × 0.30 = 99.9 → 100 ✓)
- Zero input handling
- Small amount rounding (3 × 0.30 = 0.9 → 1 ✓)
The test initialization pattern using
SETTINGS_INITwithcatch_unwindproperly handles the case where settings may already be initialized by other tests.
Fixed critical bug where dev_fee payment results were not being
persisted to the database. The issue occurred because order fields
were modified before update_order_event(), which creates a clone,
causing the changes to be lost.
Changes:
1. Payment status persistence (src/app/release.rs):
- Store dev_fee payment results in temporary variables
- Apply dev_fee_paid and dev_fee_payment_hash to order_updated
AFTER update_order_event() but BEFORE saving to database
- Ensures payment status is correctly recorded in orders table
2. Error handling improvements (src/lnurl.rs):
- Changed resolv_ln_address() to return proper errors instead of
empty strings on failure
- Makes error diagnosis easier with clear error propagation
- Affected cases: invalid tag, amount out of range, callback failure
Impact:
- dev_fee_paid field now correctly reflects payment status (0 or 1)
- dev_fee_payment_hash now stores payment hash when successful
- Error logs will show specific failure reasons for debugging
- Lightning address resolution errors are now explicit
All 107 tests passing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/release.rs (1)
643-699: Timeout only waits for first status update, not final payment result.The 30-second timeout wraps a single
rx.recv()call, which returns the first payment status message. If the payment status isInFlight,Pending, orUnknown, it will match the catch-all_ =>branch at line 660 and report failure—even though the payment may succeed later. The implementation should loop until a terminal status (SucceededorFailed) is received, or the overall timeout expires.The past review comment for this section provides a detailed fix with a loop-based approach that continues waiting for terminal status while tracking elapsed time.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/release.rs(5 hunks)src/lnurl.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/lnurl.rssrc/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/lnurl.rssrc/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (9)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/lnurl.rssrc/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/lnurl.rssrc/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/lnurl.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/lnurl.rssrc/app/release.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/lnurl.rssrc/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/lnurl.rssrc/app/release.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/lnurl.rssrc/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/app/release.rs
🧬 Code graph analysis (1)
src/app/release.rs (2)
src/util.rs (1)
update_order_event(580-617)src/lnurl.rs (1)
resolv_ln_address(72-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
src/lnurl.rs (1)
93-125: Improved error handling - LGTM!Converting silent
Ok("")returns to explicit error propagation is a solid improvement. The error types are appropriate for each failure case, and line 120-122 correctly usesLnPaymentErrorfor callback failures rather than the more genericLnAddressParseError.src/app/release.rs (2)
71-78: LGTM - Good defensive programming.The use of
saturating_subfollowed by validation prevents underflow issues when calculating the buyer's net amount after deducting fees and dev fee share.
538-590: LGTM - Non-blocking dev fee integration.The development fee payment is correctly implemented as non-blocking: failures are logged but don't prevent order completion. The dev fee fields are applied after
update_order_eventand then persisted withupdate(&pool), ensuring the order status is updated before tracking dev fee payment status.
Fixed critical bug where providing a new Lightning address after payment failure caused mostrod to pay infinitely in a loop instead of paying once. Root cause: - When payment failed, failed_payment flag was set to true - pay_new_invoice() reset payment_attempts but NOT failed_payment - Background scheduler kept finding the order and calling do_payment repeatedly - Each do_payment spawned async tasks in parallel, creating multiple payments Solution implemented with two layers of protection: 1. pay_new_invoice() now: - Resets both payment_attempts and failed_payment to false - Updates order in database before starting payment - Calls do_payment() to initiate payment with new invoice 2. do_payment() now: - Resets failed_payment to false at function start - Updates database immediately to prevent scheduler retries - Provides protection against race conditions This ensures only one payment is made when buyer provides new invoice and prevents scheduler from retrying orders that are already being processed. Files modified: - src/app/add_invoice.rs: Added failed_payment reset and do_payment call - src/app/release.rs: Added failed_payment reset at start of do_payment
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/app/release.rs (1)
657-713: Timeout logic issue: treats non-terminal status as failure.As noted in a previous review, the current 30-second timeout only waits for the first message from
rx.recv(). If that message has an intermediate status likePaymentStatus::InFlightorPaymentStatus::Pending, the catch-all_branch at line 674 incorrectly treats it as a failure, even though the payment may succeed later.The fix requires looping until a terminal status (
SucceededorFailed) is observed or the overall timeout expires. Please refer to the detailed solution in the previous review comment.
🧹 Nitpick comments (1)
src/app/release.rs (1)
457-460: Good fix for arithmetic safety; clippy suggests minor simplification.The checked arithmetic correctly prevents underflow, addressing the earlier review concern. The pipeline reports that line 460's closure doesn't capture any variables, so
ok_or(...)can replaceok_or_else(|| ...)for slightly cleaner code.🔎 Optional simplification:
let amount = (order.amount as u64) .checked_sub(order.fee as u64) .and_then(|a| a.checked_sub(buyer_dev_fee)) - .ok_or_else(|| MostroCantDo(CantDoReason::InvalidAmount))?; + .ok_or(MostroCantDo(CantDoReason::InvalidAmount))?;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/app/add_invoice.rs(3 hunks)src/app/release.rs(5 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/release.rssrc/app/add_invoice.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/app/release.rssrc/app/add_invoice.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rssrc/app/add_invoice.rs
🧠 Learnings (10)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/app/release.rssrc/app/add_invoice.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/app/release.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/app/release.rssrc/app/add_invoice.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/release.rssrc/app/add_invoice.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/app/release.rs
📚 Learning: 2025-11-06T22:54:07.055Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app.rs:182-186
Timestamp: 2025-11-06T22:54:07.055Z
Learning: In the Mostro codebase, as of PR #545, the code in check_trade_index() prevents new regular users from being created with last_trade_index = 0. Users must provide a trade_index >= 1 when creating or taking their first order. The guard at lines 184-186 in src/app.rs rejects trade_index == 0 before user creation.
Applied to files:
src/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/app/release.rssrc/app/add_invoice.rs
🧬 Code graph analysis (1)
src/app/add_invoice.rs (1)
src/app/release.rs (1)
do_payment(437-528)
🪛 GitHub Actions: CI
src/app/release.rs
[error] 457-457: Clippy error: unnecessary closure used to substitute value for Option::None. Consider using ok_or(...) instead. (unnecessary_lazy_evaluations)
🔇 Additional comments (5)
src/app/add_invoice.rs (1)
17-41: LGTM! Well-structured payment initiation flow.The payment state reset, database pre-update, buyer notification, and payment initiation sequence is well-designed. This defensive approach ensures the order state is persisted before the async payment begins and prevents race conditions with the payment scheduler.
src/app/release.rs (4)
71-75: LGTM! Safe arithmetic for retry notifications.The buyer dev fee calculation correctly splits the fee (buyer pays half), and the use of
saturating_subwith subsequent validation prevents underflow issues.
443-451: LGTM! Prevents payment retry race condition.Resetting
failed_paymentbefore processing and persisting it to the database prevents the scheduler from attempting concurrent retries. This matches the pattern established inadd_invoice.rs.
552-583: LGTM! Proper non-blocking dev fee handling.The three-outcome pattern (success with hash, no fee, or failure) is well-structured. Correctly making dev fee failures non-blocking ensures order completion isn't dependent on the dev fee payment, which is the right design decision. Logging is comprehensive and uses appropriate levels.
585-589: LGTM! Correct field application ordering.Setting
dev_fee_paidanddev_fee_payment_hashafterupdate_order_eventensures these fields are included in the subsequent database update, asupdate_order_eventonly updates in-memory state and sends the Nostr event. Based on learnings, this is the correct pattern.
|
Tengo algunas órdenes donde no se cobró el dev_fee, pensaba que era solo que no se ponía bien en la base de datos, pero sacando cuentas no se cobró tampoco, pero debería, no son cantidades < 0 
|
|
Cuando falla el pago a un comprador, no le está pidiendo una nueva invoice porque no está aumentando el contador de pagos fallidos, y siempre dice: 2025-12-18T23:54:55.363548Z INFO mostrod::app::release: Error during ln payment : Error caused by Lightning payment failure cause: Error caused by Lightning payment failure cause: status: Unknown, message: "invoice not for current active network 'regtest'", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
2025-12-18T23:54:55.367735Z INFO mostrod::app::release: Order id 4587a9f4-df15-4199-a7f0-250f414aee15 has 1 failed payments retries
2025-12-18T23:55:56.005397Z INFO mostrod::app::release: Error during ln payment : Error caused by Lightning payment failure cause: Error caused by Lightning payment failure cause: status: Unknown, message: "invoice not for current active network 'regtest'", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
2025-12-18T23:55:56.338152Z INFO mostrod::app::release: Order id 4587a9f4-df15-4199-a7f0-250f414aee15 has 1 failed payments retries
2025-12-18T23:56:56.968782Z INFO mostrod::app::release: Error during ln payment : Error caused by Lightning payment failure cause: Error caused by Lightning payment failure cause: status: Unknown, message: "invoice not for current active network 'regtest'", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
2025-12-18T23:56:56.969461Z INFO mostrod::app::release: Order id 4587a9f4-df15-4199-a7f0-250f414aee15 has 1 failed payments retries
2025-12-18T23:57:57.704356Z INFO mostrod::app::release: Error during ln payment : Error caused by Lightning payment failure cause: Error caused by Lightning payment failure cause: status: Unknown, message: "invoice not for current active network 'regtest'", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
2025-12-18T23:57:57.706718Z INFO mostrod::app::release: Order id 4587a9f4-df15-4199-a7f0-250f414aee15 has 1 failed payments retries
2025-12-18T23:58:58.359287Z INFO mostrod::app::release: Error during ln payment : Error caused by Lightning payment failure cause: Error caused by Lightning payment failure cause: status: Unknown, message: "invoice not for current active network 'regtest'", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc"} }
2025-12-18T23:58:58.361442Z INFO mostrod::app::release: Order id 4587a9f4-df15-4199-a7f0-250f414aee15 has 1 failed payments retries |
Fixed two critical bugs affecting order completion and payment retries: Bug 1 - Invoice validation rejecting correct amounts: - validate_invoice was only checking order.fee, missing buyer_dev_fee - For 100k order: expected 99,500 but buyer correctly sent 99,350 - Now includes both Mostro fee and dev fee in validation - Formula: invoice_amount == order.amount - order.fee - buyer_dev_fee Bug 2 - Payment retry counter stuck at 1: - Removing failed_payment reset before payment attempt was causing count_failed_payment to always treat failures as first failure - Counter never incremented beyond 1, preventing new invoice requests - Solution: Only reset failed_payment after successful payment - Allows proper retry counting: 1, 2, 3... up to max retries Changes: - src/util.rs: Include buyer_dev_fee in validate_invoice calculation - src/app/release.rs: Remove premature failed_payment reset from do_payment - src/app/release.rs: Add failed_payment reset in payment_success after completion This ensures invoice validation matches buyer expectations and payment retries work correctly with proper counter incrementation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/util.rs (1)
655-657: Minor: Odd dev_fee amounts lose 1 satoshi due to truncation.Integer division
order.dev_fee / 2truncates for odd amounts. For example, iforder.dev_fee = 301, thenseller_dev_fee = 150andbuyer_dev_fee = 150(calculated elsewhere), resulting in a total of 300 sats collected instead of 301 (1 sat lost).Consider having one party pay the extra sat for odd amounts:
let seller_dev_fee = order.dev_fee / 2; let buyer_dev_fee = order.dev_fee - seller_dev_fee;Then use
buyer_dev_feewhen calculating the buyer's amount to ensure both parts sum toorder.dev_fee.src/app/release.rs (1)
651-707: Major: Timeout only waits for first status update, not terminal payment result.The 30-second timeout waits only for
rx.recv()to return the first message. If the payment status isInFlightorPendingwhen this message arrives, the catch-all_branch at line 668 will treat it as a failure and return an error, even though the payment may succeed later.Consider looping until a terminal status (
SucceededorFailed) is received:🔎 Proposed fix
- // Wait for payment result with 30-second timeout - let payment_result = tokio::time::timeout(std::time::Duration::from_secs(30), rx.recv()).await; - - match payment_result { - Ok(Some(msg)) => { - if let Ok(status) = PaymentStatus::try_from(msg.payment.status) { - match status { - PaymentStatus::Succeeded => { + // Wait for terminal payment status with 30-second timeout + let timeout_duration = std::time::Duration::from_secs(30); + let start = std::time::Instant::now(); + + loop { + let remaining = timeout_duration.saturating_sub(start.elapsed()); + if remaining.is_zero() { + tracing::error!( + target: "dev_fee", + order_id = %order.id, + "Payment timeout after 30 seconds" + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Timeout".to_string(), + ))); + } + + match tokio::time::timeout(remaining, rx.recv()).await { + Ok(Some(msg)) => { + if let Ok(status) = PaymentStatus::try_from(msg.payment.status) { + match status { + PaymentStatus::Succeeded => { let hash = msg.payment.payment_hash; tracing::info!( target: "dev_fee", order_id = %order.id, payment_hash = %hash, "Development fee payment succeeded" ); - Ok(hash) - } - _ => { - tracing::error!( - target: "dev_fee", - order_id = %order.id, - status = ?status, - "Development fee payment failed" - ); - Err(MostroInternalErr(ServiceError::LnPaymentError(format!( - "Payment failed: {:?}", - status - )))) + return Ok(hash); + } + PaymentStatus::Failed => { + tracing::error!( + target: "dev_fee", + order_id = %order.id, + status = ?status, + "Development fee payment failed" + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError(format!( + "Payment failed: {:?}", + status + )))); + } + // InFlight, Pending, Unknown - continue waiting + _ => continue, } + } else { + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Invalid payment status".to_string(), + ))); } - } else { - Err(MostroInternalErr(ServiceError::LnPaymentError( - "Invalid payment status".to_string(), - ))) } + Ok(None) => { + tracing::error!( + target: "dev_fee", + order_id = %order.id, + "Payment channel closed unexpectedly" + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Channel closed".to_string(), + ))); + } + Err(_) => { + tracing::error!( + target: "dev_fee", + order_id = %order.id, + "Payment timeout after 30 seconds" + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Timeout".to_string(), + ))); + } } - Ok(None) => { - tracing::error!( - target: "dev_fee", - order_id = %order.id, - "Payment channel closed unexpectedly" - ); - Err(MostroInternalErr(ServiceError::LnPaymentError( - "Channel closed".to_string(), - ))) - } - Err(_) => { - tracing::error!( - target: "dev_fee", - order_id = %order.id, - "Payment timeout after 30 seconds" - ); - Err(MostroInternalErr(ServiceError::LnPaymentError( - "Timeout".to_string(), - ))) - } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/release.rs(5 hunks)src/config/constants.rs(1 hunks)src/util.rs(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/config/constants.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/util.rssrc/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/util.rssrc/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/util.rssrc/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to migrations/**/*.sql : After SQL/schema changes, run `cargo sqlx prepare -- --bin mostrod` to refresh `sqlx-data.json`
Applied to files:
src/util.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/app/release.rs
📚 Learning: 2025-11-06T22:54:07.055Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app.rs:182-186
Timestamp: 2025-11-06T22:54:07.055Z
Learning: In the Mostro codebase, as of PR #545, the code in check_trade_index() prevents new regular users from being created with last_trade_index = 0. Users must provide a trade_index >= 1 when creating or taking their first order. The guard at lines 184-186 in src/app.rs rejects trade_index == 0 before user creation.
Applied to files:
src/app/release.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (9)
src/util.rs (5)
156-164: LGTM!The
get_dev_feefunction correctly calculates the total development fee from the total Mostro fee using the configured percentage, with proper rounding. The documentation clearly states the returned value should be split 50/50 between buyer and seller.
803-807: Good: Safe arithmetic with saturating_sub.The calculation correctly uses
saturating_subto prevent underflow when computing the buyer's final amount. Note thatbuyer_dev_feeuses the same truncating division as elsewhere, which is consistent but loses 1 sat for odd dev_fee values (see related comment on lines 655-657).
1098-1106: LGTM with minor note.The validation correctly includes both Mostro fee and dev fee when validating the buyer's invoice. The truncation issue for odd dev_fee values applies here as well (consistent with other calculations).
1233-1243: LGTM!The test migration correctly adds the dev_fee columns (
dev_fee,dev_fee_paid,dev_fee_payment_hash) matching the production migration schema. The.ok()at the end properly ignores errors if columns already exist.
1447-1507: LGTM!The test infrastructure and test cases provide comprehensive coverage of the dev_fee calculation logic, including edge cases like rounding, zero amounts, and small amounts. The tests verify that the 30% calculation and rounding behavior work as expected.
src/app/release.rs (4)
71-75: LGTM with note.The calculation safely uses
saturating_subto adjust the order amount sent to the buyer. Thebuyer_dev_feecalculation uses the same truncating division as elsewhere, which is consistent throughout the codebase.
444-450: LGTM!The checked arithmetic correctly prevents underflow when calculating the buyer's amount. The use of
checked_subchain with proper error handling addresses the previous concern about potential underflow.
542-599: LGTM!The development fee payment workflow is well-structured and properly handles all cases:
- Successful payment with hash
- No dev fee to send (zero amount)
- Failed payment (logged, non-blocking)
The order of operations is correct:
update_order_eventfirst, then applydev_fee_paidanddev_fee_payment_hash, then save to database. The non-fatal error handling ensures dev fee failures don't block order completion.
437-518: Incorrect diagnosis: Stale order data indo_paymentis not the root cause of payment retry counter not incrementing.The scheduler (
scheduler.rs:188) already loads fresh orders from the database viafind_failed_payment()before eachdo_payment()call, so passing stale data from the scheduler is not the issue.The actual problem is in the spawned async closure within
do_payment(lines 479–515): theordervariable is captured bymove, and if multiple payment status messages arrive on the channel, each call tocheck_failure_retries(&order, ...)operates on the same stale closure-capturedorderobject. The returned updatedorderfromcheck_failure_retriesis logged but never assigned back to the closure'sordervariable, so subsequent failures see the original value.The proposed fix (reloading at the start of
do_payment) would not resolve this issue. Instead, reload the order inside the closure before each failure retry, or refactor to avoid the stale data problem entirely.
Market-price orders (created with amount=0 and using fiat exchange rates) were not calculating the development fee, resulting in no dev_fee payment when orders completed. Changes: - Modified get_market_amount_and_fee() to calculate and return dev_fee alongside amount and fee based on the total Mostro fee - Updated take_buy.rs and take_sell.rs to assign the calculated dev_fee to orders when market price is determined - Removed custom log targets from dev_fee payment logs to ensure they are visible with standard RUST_LOG configuration - Added error logging for payment_success failures to improve debugging This ensures all orders (both fixed-price and market-price) properly calculate and pay the 30% development fee to the configured Lightning Address.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/app/release.rs (1)
650-702: Timeout only waits for first status update, not final payment result.The 30-second timeout wraps a single
rx.recv()call. If the first status update isInFlightorPending, it will match the catch-all_ =>branch at line 666 and return a failure error, even though the payment may succeed later.A past review comment on lines 650-701 already identified this issue and provided a detailed fix with a loop that waits for terminal status (
SucceededorFailed).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/app/release.rs(6 hunks)src/app/take_buy.rs(1 hunks)src/app/take_sell.rs(1 hunks)src/util.rs(11 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/take_sell.rssrc/app/take_buy.rssrc/app/release.rssrc/util.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/app/take_sell.rssrc/app/take_buy.rssrc/app/release.rssrc/util.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/take_sell.rssrc/app/take_buy.rssrc/app/release.rs
🧠 Learnings (13)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/take_sell.rssrc/app/take_buy.rssrc/app/release.rssrc/util.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/app/take_sell.rssrc/app/take_buy.rssrc/app/release.rssrc/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/app/release.rssrc/util.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/app/release.rssrc/util.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/app/release.rssrc/util.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/app/release.rssrc/util.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/app/release.rs
📚 Learning: 2025-11-06T22:54:07.055Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app.rs:182-186
Timestamp: 2025-11-06T22:54:07.055Z
Learning: In the Mostro codebase, as of PR #545, the code in check_trade_index() prevents new regular users from being created with last_trade_index = 0. Users must provide a trade_index >= 1 when creating or taking their first order. The guard at lines 184-186 in src/app.rs rejects trade_index == 0 before user creation.
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/app/release.rssrc/util.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/app/release.rssrc/util.rs
📚 Learning: 2025-09-04T18:55:53.663Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 516
File: src/app/dispute.rs:171-172
Timestamp: 2025-09-04T18:55:53.663Z
Learning: In the Mostro codebase, Dispute::new(order_id, order_status) internally initializes the dispute with "Initiated" status while storing the provided order_status (like "Active" or "FiatSent") as historical data in the dispute table to track what status the order had before the dispute was created.
Applied to files:
src/util.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to migrations/**/*.sql : After SQL/schema changes, run `cargo sqlx prepare -- --bin mostrod` to refresh `sqlx-data.json`
Applied to files:
src/util.rs
🧬 Code graph analysis (2)
src/app/release.rs (2)
src/util.rs (1)
update_order_event(580-617)src/lnurl.rs (1)
resolv_ln_address(72-126)
src/util.rs (2)
src/config/settings.rs (2)
get_mostro(45-50)init_mostro_settings(24-28)src/config/types.rs (2)
default(67-73)default(114-137)
🪛 GitHub Actions: CI
src/app/release.rs
[error] 488-492: cargo fmt --all -- --check detected formatting changes needed in Rust code. Please run 'cargo fmt' to format the code according to rustfmt.
🔇 Additional comments (10)
src/app/release.rs (2)
491-497: LGTM: Non-fatal dev fee error handling.The non-blocking error handling for
payment_successensures that dev fee payment failures don't block order completion, which aligns with the documented design indocs/DEV_FEE_TECHNICAL_SPEC.md.
547-603: LGTM: Dev fee payment workflow is correctly implemented.The dev fee payment logic properly handles all outcomes (success, no fee, failure) and updates the order fields accordingly. The non-fatal error handling ensures order completion is not blocked by dev fee payment failures.
src/app/take_buy.rs (1)
56-57: LGTM: Dev fee properly assigned for market-price buy orders.The assignment of
amount_fees.2toorder.dev_feecorrectly populates the development fee for market-price orders where the amount is calculated from the market quote.src/app/take_sell.rs (1)
108-109: LGTM: Dev fee properly assigned for market-price sell orders.The assignment of
amount_fees.2toorder.dev_feecorrectly populates the development fee for market-price orders where the amount is calculated from the market quote.src/util.rs (6)
156-164: LGTM: Dev fee calculation is correct.The
get_dev_feefunction correctly calculates the total development fee from the total Mostro fee using the configured percentage, with proper rounding.
785-794: LGTM: Market price calculation includes dev fee.The
get_market_amount_and_feefunction correctly calculates and returns the development fee alongside the amount and Mostro fee for market-price orders.
1102-1110: LGTM: Invoice validation includes dev fee.The invoice validation correctly accounts for both the Mostro fee and the buyer's share of the development fee when validating the buyer's invoice amount.
1476-1511: LGTM: Comprehensive test coverage for dev fee calculation.The test cases properly validate the dev fee calculation logic including percentage calculation, rounding behavior, zero inputs, and small amounts.
378-384: The dev_fee persistence for market-price orders is working correctly. Dev_fee is assigned in take_buy.rs and take_sell.rs, then passed to show_hold_invoice where it's persisted via order_updated.update(&pool) after update_order_event.Likely an incorrect or invalid review comment.
1-34: The review comment discusses the wrong file and lacks sufficient information for verification.The comment references src/util.rs lines 1-34 but the actual
check_failure_retriesfunction being reviewed is in src/app/release.rs. Additionally, thecount_failed_paymentmethod is part of the Order struct from the external mostro-core dependency (v0.6.57), which cannot be examined in this repository. Verification of the retry counter increment issue requires access to the mostro-core source code to examine thecount_failed_paymentimplementation.
| let buyer_dev_fee = order.dev_fee / 2; | ||
| order_payment_failed.amount = order_payment_failed | ||
| .amount | ||
| .saturating_sub(order.fee) | ||
| .saturating_sub(buyer_dev_fee); |
There was a problem hiding this comment.
Dev fee truncation loses 1 satoshi for odd amounts.
Integer division order.dev_fee / 2 truncates for odd dev_fee values, causing 1 satoshi to be lost. The same truncation occurs in multiple locations (lines 445, 656 in this file; lines 807, 1103 in src/util.rs).
For consistency and to ensure the full dev_fee is collected, consider having one party (e.g., buyer) pay the extra satoshi:
let seller_dev_fee = order.dev_fee / 2;
let buyer_dev_fee = order.dev_fee - seller_dev_fee;This matches the approach documented in docs/DEV_FEE_TECHNICAL_SPEC.md line 150 which states "one party pays 1 sat more" for odd amounts.
🤖 Prompt for AI Agents
In src/app/release.rs around lines 71 to 75, the current calculation uses
integer division that truncates odd order.dev_fee values (losing 1 satoshi);
change the split so one party (buyer) pays the extra satoshi by computing
seller_dev_fee = order.dev_fee / 2 and buyer_dev_fee = order.dev_fee -
seller_dev_fee, then use buyer_dev_fee in the saturating_sub call; apply the
same pattern to the other occurrences cited (lines ~445, ~656 in this file and
lines ~807, ~1103 in src/util.rs) to ensure the full dev_fee is accounted for
consistently.
…ment CRITICAL FIX: Wrap send_dev_fee_payment with 45-second timeout to ensure orders complete successfully even if dev_fee payment hangs. Root Cause: - ln_client.send_payment() could hang indefinitely with no timeout - This caused payment_success() to never return - Orders stayed stuck in "settled-hold-invoice" status forever - Buyers already received payment, but order never completed Changes: 1. Added tokio::time::timeout wrapper around send_dev_fee_payment() - 45-second timeout (10s Lightning Address + 5s LND + 30s payment) - Handles all cases: success, failure, timeout - Order ALWAYS completes regardless of dev_fee payment result 2. Fixed error handling in payment_success() - Changed from "if let Ok(...)" to proper error propagation with "?" - Previously returned Ok even when update_order_event failed - Now properly propagates all errors 3. Comprehensive logging throughout payment_success flow - Added info-level logs at each critical step - Removed custom log targets for better visibility - Helps diagnose payment flow issues Impact: - Orders will now ALWAYS update to "success" status after buyer payment - Dev fee payment failures/timeouts logged but don't block completion - Fixes critical issue where buyers got paid but orders never completed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ARCHITECTURAL CHANGE: Decouple dev_fee payment from order completion critical path by processing payments asynchronously via scheduler. Problem: - Dev_fee payment logic in payment_success() blocked order completion - Even with 45-second timeout, added unnecessary latency to critical path - Mixed concerns: release handler managed settle, buyer payment, success status AND dev_fee payment in same flow Solution: Move dev_fee payment to dedicated scheduler job that: - Runs every payment_retries_interval seconds - Queries orders with: status='success' AND dev_fee > 0 AND dev_fee_paid = 0 - Attempts payment with 45-second timeout - Retries indefinitely until payment succeeds - Non-blocking: failures logged but don't crash job Changes: 1. src/db.rs: - Added find_orders_with_unpaid_dev_fee() query function - Finds orders needing dev_fee payment 2. src/scheduler.rs: - Added job_pay_dev_fees() scheduler job - Added process_dev_fee_payment() helper function - Registered job in start_scheduler() - Imports: Added SqlitePool 3. src/app/release.rs: - Made send_dev_fee_payment() public (for scheduler access) - Removed dev_fee payment logic from payment_success() - Removed dev_fee_paid and dev_fee_payment_hash field assignments - Added simple log: "Development fee payment will be processed by scheduler" Benefits: ✅ Non-blocking critical path: Order updates to "success" immediately ✅ Separation of concerns: release.rs focuses on order completion only ✅ Reliable retries: Indefinite retries until payment succeeds ✅ Idempotent: Database query prevents duplicate payments ✅ Observable: Easy to query unpaid dev_fees and monitor health ✅ Follows established pattern: Similar to job_retry_failed_payments() Behavior: - Order completion: Fast, no waiting for dev_fee payment - Scheduler: Processes unpaid dev_fees every interval - Retries: Continues indefinitely until payment succeeds - No data loss: Existing orders with dev_fee_paid=false picked up by scheduler 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/app/release.rs (1)
680-732: CRITICAL: Timeout only waits for first status update, not final result.The 30-second timeout at line 681 wraps a single
rx.recv()call. If the Lightning payment returns an intermediate status (PaymentStatus::InFlightorPaymentStatus::Pending), the catch-all branch at line 696 treats it as a failure, even though the payment may still succeed.Impact: Dev fee payments that take longer than instantaneous processing will be incorrectly marked as failed, causing:
- False failure logs
- Unnecessary payment retries by the scheduler
- Potential duplicate payment attempts
- Inaccurate
dev_fee_paidtrackingRoot cause: The code receives only the first status message instead of looping until a terminal status is reached.
🔧 Recommended fix
Loop receiving status updates until a terminal status (
SucceededorFailed) is observed, or the 30-second overall timeout elapses:tracing::info!( "Order Id {}: Waiting for dev fee payment result (30s timeout)", order.id ); - // Wait for payment result with 30-second timeout - let payment_result = tokio::time::timeout(std::time::Duration::from_secs(30), rx.recv()).await; - - match payment_result { - Ok(Some(msg)) => { - if let Ok(status) = PaymentStatus::try_from(msg.payment.status) { - match status { - PaymentStatus::Succeeded => { + // Wait for terminal payment status with 30-second overall timeout + let timeout_duration = std::time::Duration::from_secs(30); + let start = std::time::Instant::now(); + + loop { + let remaining = timeout_duration.saturating_sub(start.elapsed()); + if remaining.is_zero() { + tracing::error!( + "Order Id {}: Dev fee payment timeout after 30 seconds", + order.id + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Timeout".to_string(), + ))); + } + + match tokio::time::timeout(remaining, rx.recv()).await { + Ok(Some(msg)) => { + if let Ok(status) = PaymentStatus::try_from(msg.payment.status) { + match status { + PaymentStatus::Succeeded => { let hash = msg.payment.payment_hash; tracing::info!( "Order Id {}: Development fee payment succeeded - hash: {}", order.id, hash ); - Ok(hash) - } - _ => { - tracing::error!( - "Order Id {}: Development fee payment failed - status: {:?}", - order.id, - status - ); - Err(MostroInternalErr(ServiceError::LnPaymentError(format!( - "Payment failed: {:?}", - status - )))) + return Ok(hash); + } + PaymentStatus::Failed => { + tracing::error!( + "Order Id {}: Development fee payment failed - status: {:?}", + order.id, + status + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError(format!( + "Payment failed: {:?}", + status + )))); + } + // InFlight, Pending, Unknown - continue waiting + _ => { + tracing::debug!( + "Order Id {}: Payment status {:?}, continuing to wait", + order.id, + status + ); + continue; + } } + } else { + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Invalid payment status".to_string(), + ))); } - } else { - Err(MostroInternalErr(ServiceError::LnPaymentError( - "Invalid payment status".to_string(), - ))) } - } - Ok(None) => { - tracing::error!( - "Order Id {}: Development fee payment channel closed unexpectedly", - order.id - ); - Err(MostroInternalErr(ServiceError::LnPaymentError( - "Channel closed".to_string(), - ))) - } - Err(_) => { - tracing::error!( - "Order Id {}: Development fee payment timeout after 30 seconds", - order.id - ); - Err(MostroInternalErr(ServiceError::LnPaymentError( - "Timeout".to_string(), - ))) + Ok(None) => { + tracing::error!( + "Order Id {}: Dev fee payment channel closed unexpectedly", + order.id + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Channel closed".to_string(), + ))); + } + Err(_) => { + tracing::error!( + "Order Id {}: Dev fee payment timeout after 30 seconds", + order.id + ); + return Err(MostroInternalErr(ServiceError::LnPaymentError( + "Timeout".to_string(), + ))); + } } }
🧹 Nitpick comments (1)
src/app/release.rs (1)
71-75: Consider consistent dev fee split for odd amounts.The integer division
order.dev_fee / 2truncates for odd dev_fee values, losing 1 satoshi. While past reviews noted this issue, the current implementation doesn't ensure one party consistently pays the extra satoshi.For transparency and to match documentation (docs/DEV_FEE_TECHNICAL_SPEC.md line 150), consider:
let seller_dev_fee = order.dev_fee / 2; let buyer_dev_fee = order.dev_fee - seller_dev_fee;This ensures the buyer pays the extra satoshi for odd amounts, making the fee split explicit and verifiable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/release.rs(6 hunks)src/db.rs(1 hunks)src/scheduler.rs(3 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/release.rssrc/db.rssrc/scheduler.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/app/release.rssrc/db.rssrc/scheduler.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (18)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/app/release.rssrc/db.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/release.rssrc/db.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/app/release.rs
📚 Learning: 2025-11-06T22:54:07.055Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app.rs:182-186
Timestamp: 2025-11-06T22:54:07.055Z
Learning: In the Mostro codebase, as of PR #545, the code in check_trade_index() prevents new regular users from being created with last_trade_index = 0. Users must provide a trade_index >= 1 when creating or taking their first order. The guard at lines 184-186 in src/app.rs rejects trade_index == 0 before user creation.
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Apply Rustfmt formatting with `cargo fmt` command
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Pull requests must link the motivating issue, include `cargo test` output, and call out schema or config changes
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Ensure clippy lints are clean by running `cargo clippy --all-targets --all-features`
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Ensure `cargo test` passes locally before pushing code
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use 4-space indent in Rust 2021 code
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use SCREAMING_SNAKE_CASE for constants in Rust
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/app/release.rssrc/db.rs
📚 Learning: 2025-06-16T23:17:54.795Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 501
File: src/util.rs:0-0
Timestamp: 2025-06-16T23:17:54.795Z
Learning: The `get_db_pool()` function in the codebase returns `Arc<SqlitePool>`, not `&SqlitePool`. When calling functions that expect `&SqlitePool`, you need to use `&pool` to convert `Arc<SqlitePool>` to `&SqlitePool` through automatic dereferencing.
Applied to files:
src/scheduler.rs
🧬 Code graph analysis (3)
src/app/release.rs (4)
src/db.rs (3)
connect(480-611)None(650-650)new(1437-1440)src/util.rs (2)
update_order_event(580-617)enqueue_order_msg(951-966)src/lightning/mod.rs (1)
new(38-52)src/lnurl.rs (1)
resolv_ln_address(72-126)
src/db.rs (2)
src/app/orders.rs (1)
orders(36-39)src/app.rs (1)
e(391-391)
src/scheduler.rs (3)
src/db.rs (17)
sqlx(353-353)sqlx(365-365)sqlx(386-386)sqlx(690-690)sqlx(706-706)sqlx(723-723)sqlx(742-742)sqlx(761-761)sqlx(866-866)sqlx(881-881)sqlx(898-898)sqlx(913-913)sqlx(938-938)sqlx(955-955)sqlx(1080-1080)sqlx(1224-1224)find_orders_with_unpaid_dev_fee(897-910)src/config/settings.rs (2)
get_ln(37-42)get_db_pool(31-33)src/app/release.rs (1)
send_dev_fee_payment(618-733)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
src/db.rs (1)
895-910: LGTM! Clean database query for unpaid dev fees.The query logic correctly identifies orders needing dev fee processing by filtering on status, non-zero dev_fee, and unpaid flag. Error handling appropriately maps database errors to
MostroInternalErr.src/scheduler.rs (1)
260-332: LGTM with timeout consideration.The function correctly wraps
send_dev_fee_paymentwith a 45-second timeout and updates the database on success. Error handling appropriately logs failures for retry on the next scheduler cycle.The 45-second timeout is longer than the 30-second timeout inside
send_dev_fee_payment, providing a safety margin for the overall operation.src/app/release.rs (3)
444-450: LGTM! Proper checked arithmetic prevents underflow.The calculation now uses
checked_subchained appropriately to prevent underflow panics when subtracting fee and buyer_dev_fee from order amount. ReturnsCantDoReason::InvalidAmounton underflow as expected.
491-500: Appropriate non-blocking error handling.Changing from error propagation to logging ensures that order completion isn't blocked by dev fee payment issues. The scheduler will retry dev fee payments asynchronously, making this approach consistent with the non-blocking design.
528-613: Well-structured non-blocking payment flow.The refactored
payment_successcorrectly:
- Marks the order as Success immediately without waiting for dev fee payment
- Defers dev fee processing to the scheduler (lines 560-566)
- Resets payment retry flags after successful buyer payment
- Includes comprehensive logging for monitoring
- Handles database errors appropriately
This design ensures order completion isn't blocked by dev fee payment issues.
| /// Job to process unpaid development fees | ||
| /// Runs every payment_retries_interval seconds | ||
| /// Retries indefinitely until payment succeeds | ||
| async fn job_pay_dev_fees() { | ||
| let ln_settings = Settings::get_ln(); | ||
| let interval = ln_settings.payment_retries_interval as u64; | ||
|
|
||
| // Arc clone db pool for thread safety | ||
| let pool = get_db_pool(); | ||
|
|
||
| tokio::spawn(async move { | ||
| loop { | ||
| info!( | ||
| "Development fee payment job running (every {} seconds) - checking for unpaid dev fees", | ||
| interval | ||
| ); | ||
|
|
||
| // Query orders with unpaid dev_fee | ||
| match crate::db::find_orders_with_unpaid_dev_fee(&pool).await { | ||
| Ok(orders) => { | ||
| if orders.is_empty() { | ||
| tracing::debug!("No unpaid development fees found"); | ||
| } else { | ||
| tracing::info!("Found {} orders with unpaid dev fees", orders.len()); | ||
|
|
||
| for order in orders.into_iter() { | ||
| // Attempt payment with timeout | ||
| match process_dev_fee_payment(&pool, order.clone()).await { | ||
| Ok(payment_hash) => { | ||
| tracing::info!( | ||
| "Order Id {}: Dev fee payment succeeded - hash: {}", | ||
| order.id, | ||
| payment_hash | ||
| ); | ||
| } | ||
| Err(e) => { | ||
| tracing::error!( | ||
| "Order Id {}: Dev fee payment failed: {:?} - will retry on next cycle", | ||
| order.id, | ||
| e | ||
| ); | ||
| // Continue to next order, will retry this one on next cycle | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Err(e) => { | ||
| tracing::error!("Failed to query unpaid dev fees: {:?}", e); | ||
| } | ||
| } | ||
|
|
||
| tokio::time::sleep(tokio::time::Duration::from_secs(interval)).await; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add max retry limit for dev fee payments.
Unlike job_retry_failed_payments (lines 174-201) which respects payment_attempts limit, this scheduler will retry failed dev fee payments indefinitely. If a dev fee payment consistently fails (e.g., invalid Lightning address, network issues), it will retry forever on each cycle.
Consider tracking retry attempts per order (similar to payment_attempts for buyer payments) and either giving up after a threshold or alerting administrators to investigate persistent failures.
💡 Suggested approach
Add a dev_fee_payment_attempts field to the Order struct and increment it in process_dev_fee_payment on failure. Check this count in job_pay_dev_fees and skip orders that have exceeded the max attempts, logging them for admin review.
🤖 Prompt for AI Agents
In src/scheduler.rs around lines 203 to 258, job_pay_dev_fees currently retries
dev-fee payments forever; add a max-attempts guard and persistence so orders
stop being retried indefinitely: add a dev_fee_payment_attempts integer to the
Order model (or use a new config setting like ln.dev_fee_payment_attempts),
increment that counter and persist it in process_dev_fee_payment on each failed
attempt, have job_pay_dev_fees skip (and log/warn) orders whose
dev_fee_payment_attempts >= max attempts (or flag them for admin review), and
ensure the DB update and any logging/error flow mirror the existing
payment_attempts behavior used by job_retry_failed_payments.
…handling Added multi-layer timeout protection to prevent dev fee payments from blocking indefinitely when encountering issues like self-payment attempts or network problems. Changes to payment execution (src/app/release.rs): - Added 15s timeout to LNURL resolution (prevents DNS/network hanging) - Added 5s timeout to send_payment() call (prevents LND hanging on self-payments) - Maintained 25s timeout on payment result waiting (for routing completion) - Created match_payment_result() helper function for cleaner result handling - Added comprehensive [Step X/3] logging for better observability - Total time budget: 45s max (LNURL 15s + send_payment 5s + result 25s) Changes to scheduler (src/scheduler.rs): - Increased scheduler timeout from 45s to 50s to accommodate new time budget - Updated error messages to reflect new timeout duration Documentation updates (docs/DEV_FEE_TECHNICAL_SPEC.md): - Documented scheduler-based payment execution model - Explained that dev fee payment happens asynchronously after buyer receives sats - Added critical section on market price orders and dev_fee reset behavior - Documented that dev_fee field must be reset to 0 when order returns to pending - Updated all timeout values throughout documentation - Added common error scenarios (self-payment, LNURL failures, routing issues) - Updated performance impact and troubleshooting sections This fix resolves hanging issues where send_payment() would block for 50+ seconds before the scheduler timeout fired. Now fails fast (5-10s) with clear error logging, allowing scheduler to retry on next cycle without blocking other dev fee payments.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/release.rs (1)
464-469: Critical: Update order variable with check_failure_retries result.The returned order from
check_failure_retriescontains the updatedpayment_attemptscounter, but the localordervariable is never updated. This is likely the root cause of the issue reported in PR objectives where "the retry counter remains at 1 across multiple attempts."Without updating the local order, subsequent payment attempts use the stale counter value, preventing proper retry logic.
🔎 Proposed fix
let payment_task = ln_client_payment.send_payment(&payment_request, amount as i64, tx); if let Err(paymement_result) = payment_task.await { info!("Error during ln payment : {}", paymement_result); - if let Ok(failed_payment) = check_failure_retries(&order, request_id).await { + if let Ok(updated_order) = check_failure_retries(&order, request_id).await { + order = updated_order; info!( "Order id {} has {} failed payments retries", - failed_payment.id, failed_payment.payment_attempts + order.id, order.payment_attempts ); } }
♻️ Duplicate comments (1)
src/app/release.rs (1)
71-71: Dev fee truncation loses 1 satoshi for odd amounts.Integer division
order.dev_fee / 2truncates for odd dev_fee values, causing 1 satoshi to be lost. This occurs at line 71 and line 445.For consistency with the specification (docs/DEV_FEE_TECHNICAL_SPEC.md line 226: "one party pays 1 sat more"), consider:
let seller_dev_fee = order.dev_fee / 2; let buyer_dev_fee = order.dev_fee - seller_dev_fee;This ensures the full dev_fee is collected rather than losing the truncated satoshi.
Also applies to: 445-445
🧹 Nitpick comments (1)
docs/DEV_FEE_TECHNICAL_SPEC.md (1)
17-17: Optional: Address markdown linting for improved documentation quality.Static analysis detected several markdown formatting issues that could be improved:
- Missing language specifiers on code blocks (lines 17, 92, 136, 407, 413, 423)
- Bare URLs at lines 615-616
- Emphasis used instead of headings (lines 422, 428, 434)
While these don't affect functionality, adding language identifiers to code fences and converting bare URLs to proper markdown links would improve documentation quality.
Example fixes
Add language identifiers to code blocks:
-``` +```text Order Creation → Fee Calculation → Hold Invoice → Order Success → Dev PaymentConvert bare URLs to links:
-- GitHub: https://github.com/MostroP2P/mostro/issues +- GitHub: [https://github.com/MostroP2P/mostro/issues](https://github.com/MostroP2P/mostro/issues)Also applies to: 92-92, 136-136, 407-407, 413-413, 615-616
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/DEV_FEE_TECHNICAL_SPEC.md(1 hunks)src/app/release.rs(6 hunks)src/scheduler.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/scheduler.rs
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (19)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/app/release.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/app/release.rsdocs/DEV_FEE_TECHNICAL_SPEC.md
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: Cross-repository dependencies in Mostro project: when PRs introduce new types like PaymentFailedInfo from mostro-core, compilation errors are expected until the dependency PR is merged and the version is updated in the consuming repository.
Applied to files:
src/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/release.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/app/release.rs
📚 Learning: 2025-11-06T22:54:07.055Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app.rs:182-186
Timestamp: 2025-11-06T22:54:07.055Z
Learning: In the Mostro codebase, as of PR #545, the code in check_trade_index() prevents new regular users from being created with last_trade_index = 0. Users must provide a trade_index >= 1 when creating or taking their first order. The guard at lines 184-186 in src/app.rs rejects trade_index == 0 before user creation.
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Apply Rustfmt formatting with `cargo fmt` command
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Pull requests must link the motivating issue, include `cargo test` output, and call out schema or config changes
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Ensure clippy lints are clean by running `cargo clippy --all-targets --all-features`
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Ensure `cargo test` passes locally before pushing code
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use 4-space indent in Rust 2021 code
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use SCREAMING_SNAKE_CASE for constants in Rust
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-17T13:04:07.466Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:07.466Z
Learning: In docs/DEV_FEE_TECHNICAL_SPEC.md, note that the Lightning address used in the Mostro development fee implementation is temporary for testing: mostro_p2psats.mobi. The production address should be developmentmostro.network as documented. This mismatch is intentional for testing and must be resolved before production deployment. Ensure the doc reflects the correct production address and verify that any references to test vs. production addresses are clearly distinguished.
Applied to files:
docs/DEV_FEE_TECHNICAL_SPEC.md
🧬 Code graph analysis (1)
src/app/release.rs (4)
src/db.rs (2)
connect(480-611)new(1437-1440)src/util.rs (2)
update_order_event(580-617)enqueue_order_msg(951-966)src/lightning/mod.rs (1)
new(38-52)src/lnurl.rs (1)
resolv_ln_address(72-126)
🪛 GitHub Actions: CI
src/app/release.rs
[error] 707-707: Clippy error: this expression creates a reference which is immediately dereferenced by the compiler. Change this to: order. Command: cargo clippy --all-targets --all-features -- -D warnings
🪛 markdownlint-cli2 (0.18.1)
docs/DEV_FEE_TECHNICAL_SPEC.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
92-92: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
136-136: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
407-407: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
413-413: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
422-422: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
423-423: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
428-428: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
434-434: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
615-615: Bare URL used
(MD034, no-bare-urls)
616-616: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (3)
src/app/release.rs (3)
615-727: LGTM: Well-structured dev fee payment implementation.The implementation correctly handles:
- Zero dev fee early return
- Appropriate timeout configurations (15s LNURL, 5s send_payment, 25s result)
- Comprehensive logging with step markers for troubleshooting
- Non-fatal error handling that doesn't block order completion
The timeout strategy prevents hanging on self-payments and network issues, which aligns with the documented behavior.
729-784: Consider handling in-flight payment statuses.The current implementation treats all non-
Succeededstatuses (includingInFlightandPending) as immediate failures. While this is acceptable for the non-blocking dev fee payment flow (since the scheduler retries every 60 seconds), it may generate premature error logs for payments that are still in progress.However, given that:
- Dev fee payments are non-fatal
- Automatic retries occur via scheduler
- The 25-second timeout allows time for completion
This approach may be acceptable. Just be aware that
InFlightstatuses will be logged as errors even though the payment might succeed shortly after.If you want to reduce false-positive error logs, consider waiting for terminal statuses like the buyer invoice payment flow does, or add a note in the logs distinguishing "in-flight timeout" from actual payment failures.
528-613: LGTM: payment_success properly implements non-blocking dev fee flow.The refactored function correctly:
- Completes buyer payment immediately without blocking on dev fee
- Delegates dev fee processing to scheduler for async handling
- Resets failed payment counters after successful buyer payment
- Provides detailed logging for troubleshooting
- Handles errors gracefully with appropriate fallbacks
This aligns with the architecture described in the technical specification where dev fee payments are processed asynchronously to avoid blocking order completion.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/release.rs (1)
479-521: Critical: Stale order variable prevents retry counter from incrementing.The
ordervariable is moved into the async block at line 480 and never refreshed from the database. Whencheck_failure_retriesis called at line 509, it receives the stale order, clones it, increments the counter, saves to DB, and returns the updated order. However, the returned updated order is discarded, so the next payment failure still uses the original stale order with the oldpayment_attemptsvalue.This explains the reported issue where logs show "Order id ... has 1 failed payments retries" repeatedly - the counter increments in the DB but the in-memory order used for subsequent calls never sees the update.
🔎 Proposed fix to use the returned updated order
// We redeclare vars to use inside this block +let mut order = order; // Make order mutable in this scope // Receiving msgs from send_payment() while let Some(msg) = rx.recv().await { if let Ok(status) = PaymentStatus::try_from(msg.payment.status) { match status { PaymentStatus::Succeeded => { info!( "Order Id {}: Invoice with hash: {} paid!", order.id, msg.payment.payment_hash ); if let Err(e) = payment_success(&mut order, buyer_pubkey, &my_keys, request_id) .await { tracing::error!( "Order Id {}: payment_success failed: {:?}", order.id, e ); } } PaymentStatus::Failed => { info!( "Order Id {}: Invoice with hash: {} has failed!", order.id, msg.payment.payment_hash ); // Mark payment as failed - if let Ok(failed_payment) = + if let Ok(updated_order) = check_failure_retries(&order, request_id).await { + order = updated_order; // Use the updated order for next iteration info!( "Order id {} has {} failed payments retries", - failed_payment.id, failed_payment.payment_attempts + order.id, order.payment_attempts ); } } _ => {} } } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/app/release.rs(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Runtime code lives in
src/directory, organized into:src/app/for order flows and business logic,src/lightning/for LND bindings and Lightning helpers,src/rpc/for gRPC service and types, andsrc/config/for settings and loaders
Files:
src/app/release.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Use 4-space indent in Rust 2021 code
Usesnake_casefor function names in Rust
UsePascalCasefor type names in Rust
Use SCREAMING_SNAKE_CASE for constants in Rust
Document non-obvious public APIs with///doc comments
Prefertracingspans over ad-hoc logging in Rust
Co-locate tests in their modules undermod testsblocks
Name test functions descriptively, e.g.,handles_expired_hold_invoice
Scrub logs that might leak invoices or Nostr keys; rotate secrets promptly if exposed
Apply Rustfmt formatting withcargo fmtcommand
Ensure clippy lints are clean by runningcargo clippy --all-targets --all-features
Files:
src/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/release.rs
🧠 Learnings (17)
📓 Common learnings
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: The PaymentFailedInfo struct used in src/app/release.rs is defined in mostro-core PR #111, creating a cross-repository dependency that causes compilation errors until the dependency is resolved.
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-17T13:04:13.036Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 554
File: docs/DEV_FEE_TECHNICAL_SPEC.md:1-434
Timestamp: 2025-12-17T13:04:13.036Z
Learning: In the Mostro development fee implementation, the Lightning address `mostro_p2psats.mobi` in src/config/constants.rs is temporary for testing purposes. The production address will be `developmentmostro.network` as documented in docs/DEV_FEE_TECHNICAL_SPEC.md. This intentional mismatch is planned to be resolved before production deployment.
Applied to files:
src/app/release.rs
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:108-114
Timestamp: 2025-07-17T13:51:17.338Z
Learning: In Lightning invoice validation for Mostro buy orders (src/lightning/invoice.rs), zero-amount invoices (amount_sat == 0) should always pass validation even when an expected amount is provided. This is because users can submit invoices at order creation time before knowing the exact satoshi amount they'll receive. The condition `if amount_sat != res && amount_sat != 0` correctly implements this by allowing zero-amount invoices to pass while enforcing exact amount matching for non-zero invoices.
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-25T22:48:44.741Z
Learnt from: Catrya
Repo: MostroP2P/mostro PR: 502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: PaymentFailedInfo struct and Payload::PaymentFailed variant are defined in mostro-core PR #111, creating a cross-repository dependency with the Mostro repository that requires careful coordination of merging and version updates.
Applied to files:
src/app/release.rs
📚 Learning: 2025-10-09T14:06:50.067Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 530
File: src/app/order.rs:278-279
Timestamp: 2025-10-09T14:06:50.067Z
Learning: In the Mostro project, tests in files like `src/app/order.rs` may use structural checks to ensure functions like `order_action` do not panic during message construction, rather than validating specific error variants.
Applied to files:
src/app/release.rs
📚 Learning: 2025-04-27T20:07:24.558Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 475
File: src/db.rs:792-802
Timestamp: 2025-04-27T20:07:24.558Z
Learning: The decrypt_data function from mostro_core::order returns a type that needs to be wrapped with MostroInternalErr to convert it to MostroError, so calls should keep the .map_err(MostroInternalErr) conversion.
Applied to files:
src/app/release.rs
📚 Learning: 2025-02-21T13:26:17.026Z
Learnt from: grunch
Repo: MostroP2P/mostro PR: 444
File: src/app/take_buy.rs:28-31
Timestamp: 2025-02-21T13:26:17.026Z
Learning: In the Mostro codebase, error messages should be descriptive and specific rather than generic (e.g., prefer `PendingOrderExists` over `InvalidAction`) to help users better understand why their actions were rejected.
Applied to files:
src/app/release.rs
📚 Learning: 2025-11-06T22:54:07.055Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 545
File: src/app.rs:182-186
Timestamp: 2025-11-06T22:54:07.055Z
Learning: In the Mostro codebase, as of PR #545, the code in check_trade_index() prevents new regular users from being created with last_trade_index = 0. Users must provide a trade_index >= 1 when creating or taking their first order. The guard at lines 184-186 in src/app.rs rejects trade_index == 0 before user creation.
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Apply Rustfmt formatting with `cargo fmt` command
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Pull requests must link the motivating issue, include `cargo test` output, and call out schema or config changes
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Ensure clippy lints are clean by running `cargo clippy --all-targets --all-features`
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Ensure `cargo test` passes locally before pushing code
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use 4-space indent in Rust 2021 code
Applied to files:
src/app/release.rs
📚 Learning: 2025-12-12T20:35:23.220Z
Learnt from: CR
Repo: MostroP2P/mostro PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-12T20:35:23.220Z
Learning: Applies to **/*.rs : Use SCREAMING_SNAKE_CASE for constants in Rust
Applied to files:
src/app/release.rs
📚 Learning: 2025-06-13T12:50:53.424Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 499
File: src/lightning/invoice.rs:21-27
Timestamp: 2025-06-13T12:50:53.424Z
Learning: `ln_exists(address: &str)` returns `Result<(), MostroError>`: `Ok(())` means the Lightning address exists (`tag == "payRequest"`), and `Err(_)` represents any invalid or unreachable address scenario.
Applied to files:
src/app/release.rs
📚 Learning: 2025-03-11T23:26:33.763Z
Learnt from: bilthon
Repo: MostroP2P/mostro PR: 464
File: src/app/add_invoice.rs:73-82
Timestamp: 2025-03-11T23:26:33.763Z
Learning: In the Mostro codebase, the `update_order_event` function does not perform database operations - it only updates an order in memory and sends a Nostr event. The actual database update happens separately when calling `.update(pool)` afterward.
Applied to files:
src/app/release.rs
🧬 Code graph analysis (1)
src/app/release.rs (4)
src/db.rs (2)
connect(480-611)new(1437-1440)src/util.rs (1)
update_order_event(580-617)src/lightning/mod.rs (1)
new(38-52)src/lnurl.rs (1)
resolv_ln_address(72-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (2)
src/app/release.rs (2)
728-782: Verify fast-fail behavior for non-terminal payment statuses.The
match_payment_resultfunction treats all non-Succeededstatuses (includingInFlight,Pending,Unknown) as immediate failures (lines 745-755). This means if LND returns an in-flight status whenrx.recv()completes, the payment will be marked as failed even though it may succeed later.Given that dev fee payments are non-blocking and can be retried by the scheduler, this fast-fail approach may be acceptable. However, it could lead to unnecessary retries for payments that are legitimately in-flight.
Consider either:
- Confirming this fast-fail behavior is intentional for dev fee payments (non-blocking retry design), or
- Implementing a loop that continues receiving messages until a terminal status (
SucceededorFailed) is observed, with the overall 25-second timeout applied to the loopIf the fast-fail approach is intentional, please add a comment explaining the design decision:
// Fast-fail on non-terminal statuses for dev fee payments. // Since dev fees are non-blocking and retried by scheduler, // we don't wait for in-flight payments to complete. _ => { tracing::error!( "Order Id {}: [Step 3/3 FAILED] Development fee payment failed - status: {:?}", order.id, status ); ... }
615-725: Scheduler integration and DB persistence for dev fee payments are already implemented.The
send_dev_fee_paymentfunction is called byprocess_dev_fee_paymentin scheduler.rs (line 278), which explicitly updatesdev_fee_paidanddev_fee_payment_hash(lines 285-286) and persists the changes viaorder.update(pool)(line 289).If Catrya reported missing dev fee charges, investigate the payment failure logs from
match_payment_result(lines 728-770) to identify why payments are failing—likely LNURL resolution timeouts (15s), LND send timeouts (5s), or payment result receive timeouts (25s).Likely an incorrect or invalid review comment.
Technical Specification (docs/DEV_FEE_TECHNICAL_SPEC.md):
README.md Updates:
CHANGELOG.md Updates:
Documentation provides operators with complete guidance for:
Summary by CodeRabbit
New Features
Documentation
Database
Configuration
Scheduler
Tests
Dependencies
✏️ Tip: You can customize this high-level summary in your review settings.