Document development fund payment phases and architecture #556
Conversation
WalkthroughWires a configurable dev_fee into order/invoice lifecycles: new calculation helpers, unit tests, storing dev_fee on orders, splitting dev_fee between seller/buyer, adjustments in hold-invoice, validation, release/add_invoice/cancel math, and expanded DEV_FEE documentation. Phase 3 payment execution is documented but not implemented. Changes
Sequence Diagram(s)(omitted — changes are primarily local calculation/integration and documentation; no implemented multi-component sequential flow added) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/DEV_FEE.md (2)
86-90: Update Lightning address example to match actual hardcoded value.The example shows a generic placeholder
<dev@lightning.address>, but per the existing implementation, the hardcoded address ismostro_p2psats.mobi(temporary for testing). The documentation should explicitly state this is temporary and production will usedevelopmentmostro.network. This matches the learning from PR #554.🔎 Suggested update
pub const MIN_DEV_FEE_PERCENTAGE: f64 = 0.10; // 10% minimum pub const MAX_DEV_FEE_PERCENTAGE: f64 = 1.0; // 100% maximum -pub const DEV_FEE_LIGHTNING_ADDRESS: &str = "<dev@lightning.address>"; +pub const DEV_FEE_LIGHTNING_ADDRESS: &str = "mostro_p2psats.mobi"; // Temporary for testing; production: developmentmostro.network
602-602: Update hardcoded Lightning address reference.This section mentions "Lightning Address:
<dev@lightning.address>" as a hardcoded value, but should reference the actual hardcoded valuemostro_p2psats.mobi(temporary for testing) consistent with PR #554 learnings.🔎 Suggested update
-Lightning Address: `<dev@lightning.address>` (cannot be changed without recompiling) +Lightning Address: `mostro_p2psats.mobi` (temporary for testing; production: `developmentmostro.network`) (cannot be changed without recompiling)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/DEV_FEE.md
🧰 Additional context used
🧠 Learnings (6)
📓 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:
docs/DEV_FEE.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: 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:
docs/DEV_FEE.md
📚 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:
docs/DEV_FEE.md
📚 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:
docs/DEV_FEE.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:
docs/DEV_FEE.md
⏰ 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)
docs/DEV_FEE.md (5)
18-22: Clarify Phase 1 implementation status.The section claims Phase 1 is "COMPLETE" with "Database fields initialized in order creation (currently hardcoded to 0)". This creates ambiguity—if fields are hardcoded to 0, are they truly initialized as part of Phase 1, or is the initialization itself incomplete? Clarify whether Phase 1 covers:
- Schema migration (✅ present)
- Configuration constants (✅ present)
- Field presence in Order struct (needs verification)
- OR if field initialization logic will be completed in Phase 2
152-227: Verify market price order dev fee reset is a required behavior.This section describes detailed reset logic for market price orders (lines 152-200), including a SQL consistency check. This seems critical to correctness, but the documentation doesn't clarify:
- Is this behavior already required/implemented elsewhere in the codebase?
- Is this a new requirement introduced by dev fee feature?
- What triggers the status transition back to
pending(e.g., invoice timeout, user cancellation)?Suggest adding references to the order lifecycle code that will handle these transitions.
247-276: Example calculations are mathematically sound; edge case handling is clear.The fee split calculations (lines 247-261) correctly show 50/50 buyer-seller split. Edge case handling (lines 263-276) properly addresses rounding, zero fees, and tiny amounts. No issues flagged.
306-339: Clarify error handling approach in payment flow specification.The pseudocode (lines 312-333) uses
?operators for error propagation, but the specification (lines 350-357) states failures are logged and retried without blocking. Clarify the actual error handling:
- Should timeouts/failures return
Result<PaymentHash, Error>or log internally?- How are logging calls integrated into the code flow?
- What happens to channel senders (
tx) if errors occur—are they properly cleaned up?This will prevent implementation surprises during Phase 3.
122-124: TheSettings::get_mostro()API is correctly implemented and accessible. The method returns&'static MostroSettings, which contains thedev_fee_percentage: f64field as expected. The code snippet's usage pattern is correct.
- Implement get_dev_fee(total_mostro_fee) and unit tests - Compute/store dev_fee in prepare_new_order() (total fee basis) - Include seller’s dev fee in show_hold_invoice() amount - Subtract buyer’s dev fee when requesting buyer invoice amount - Reset dev_fee for market price orders when returning to pending - Add concise contributor guide in AGENTS.md Notes: - All new fee math follows docs/DEV_FEE.md Phase 2 spec - cargo test passes for new tests; one unrelated test fails due to env permission
grunch
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/util.rs (1)
654-656: Potential 1 sat discrepancy when dev_fee is odd.When
order.dev_feeis an odd number (e.g., 101), integer division by 2 yields 50 for both seller and buyer, totaling 100 sats instead of 101. The remaining 1 sat goes uncollected.This is likely acceptable given the small magnitude, but consider documenting this rounding behavior or assigning the remainder to one party (e.g., seller pays
(dev_fee + 1) / 2).🔎 Optional fix to ensure full dev_fee collection
- // Seller pays their half of the dev fee (order.dev_fee stores total) - let seller_dev_fee = order.dev_fee / 2; + // Seller pays ceiling half of the dev fee (order.dev_fee stores total) + // Seller gets the extra sat on odd amounts + let seller_dev_fee = (order.dev_fee + 1) / 2;And in
set_waiting_invoice_status:- let buyer_dev_fee = order.dev_fee / 2; + // Buyer pays floor half of the dev fee + let buyer_dev_fee = order.dev_fee / 2; // (unchanged, gets the smaller portion)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
AGENTS.mdsrc/app/cancel.rssrc/util.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/cancel.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/cancel.rssrc/util.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/cancel.rs
🧠 Learnings (20)
📓 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-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:
AGENTS.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:
AGENTS.mdsrc/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:
AGENTS.mdsrc/util.rs
📚 Learning: 2025-02-28T15:32:19.572Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 0
File: :0-0
Timestamp: 2025-02-28T15:32:19.572Z
Learning: Rust doctests (code examples in documentation that are automatically executed as tests) are only available for library code, not for binary applications like Mostro. While docstrings are still valuable for documentation in binary applications, the examples within them won't be automatically tested.
Applied to files:
AGENTS.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: Pull requests must link the motivating issue, include `cargo test` output, and call out schema or config changes
Applied to files:
AGENTS.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: Ensure `cargo test` passes locally before pushing code
Applied to files:
AGENTS.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:
AGENTS.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 src/**/*.rs : 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, and `src/config/` for settings and loaders
Applied to files:
AGENTS.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 **/*.rs : Apply Rustfmt formatting with `cargo fmt` command
Applied to files:
AGENTS.md
📚 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:
AGENTS.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 **/*.rs : Co-locate tests in their modules under `mod tests` blocks
Applied to files:
AGENTS.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 **/*.rs : Name test functions descriptively, e.g., `handles_expired_hold_invoice`
Applied to files:
AGENTS.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 src/app/**/*.rs : Mirror test fixtures under `src/app/` where applicable
Applied to files:
AGENTS.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 **/*.rs : Ensure clippy lints are clean by running `cargo clippy --all-targets --all-features`
Applied to files:
AGENTS.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: Base work on `main` branch; keep topics scoped in commits
Applied to files:
AGENTS.md
📚 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.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.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.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.rs
🧬 Code graph analysis (1)
src/util.rs (1)
src/config/settings.rs (1)
get_mostro(45-50)
⏰ 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 (6)
src/app/cancel.rs (1)
22-23: LGTM!The change correctly resets
dev_feealongsideamountandfeewhen republishing an order withprice_from_api. Since the dev fee is derived from the Mostro fee (which itself depends on the amount), resetting all three ensures consistent recalculation from a fresh API quote.src/util.rs (4)
156-164: LGTM!The
get_dev_feefunction follows the same pattern asget_fee, with clear documentation explaining it takes the total combined fee and returns the total dev fee to be split between parties.
377-383: LGTM!The dev fee calculation correctly derives the total Mostro fee by doubling the per-party fee, then computes the total dev fee to be stored on the order. When
amount == 0(price_from_api), both fee and dev_fee remain 0 for later recalculation.
801-806: LGTM!The buyer's final amount correctly deducts their half of the dev fee. Using
saturating_subis good defensive programming to prevent underflow on edge cases with very small amounts.
1434-1460: Initialize Settings for these tests or refactor to avoid dependency.The
get_dev_feetests callSettings::get_mostro()which requiresMOSTRO_CONFIGto be initialized. Theinitialize()function is empty and does not set up any settings, so these tests will panic at runtime with "No Mostro settings found".Consider either:
- Initializing Settings in the test setup (e.g., in
initialize()with a default test configuration)- Refactoring
get_dev_feeto accept the percentage as a parameter instead of reading from Settings- Mocking the Settings dependency for unit tests
AGENTS.md (1)
16-32: LGTM!Minor documentation wording improvements that enhance clarity without changing meaning.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/util.rs (1)
654-656: Integer division may lose 1 sat whendev_feeis odd.When
order.dev_feeis an odd number (e.g., 3), integer division3 / 2 = 1for both seller and buyer, resulting in only 2 sats collected instead of 3. While this is a minor discrepancy (at most 1 sat per trade), consider whether this is acceptable or if you want to assign the remainder to one party consistently.🔎 Optional: assign remainder to seller (who pays first)
// Seller pays their half of the dev fee (order.dev_fee stores total) - let seller_dev_fee = order.dev_fee / 2; + // Seller takes the larger half when dev_fee is odd + let seller_dev_fee = (order.dev_fee + 1) / 2;Then in
set_waiting_invoice_statusandvalidate_invoice, buyer pays the remainder:let buyer_dev_fee = order.dev_fee - (order.dev_fee + 1) / 2;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util.rs
🧰 Additional context used
📓 Path-based instructions (2)
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.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.rs
🧠 Learnings (8)
📓 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.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.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.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.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.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.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
🧬 Code graph analysis (1)
src/util.rs (2)
src/config/settings.rs (1)
get_mostro(45-50)src/lightning/invoice.rs (1)
is_valid_invoice(181-195)
⏰ 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 (6)
src/util.rs (6)
156-164: LGTM!The function correctly calculates the development fee as a percentage of the total Mostro fee with proper rounding. The documentation clearly explains the expected input (total fee from both parties) and output (total dev fee to be split 50/50).
378-382: LGTM!The calculation chain is correct: get split fee → multiply by 2 for total → calculate dev fee from total. The dev fee is properly initialized to 0 for orders with zero amount (API-priced orders).
696-696: LGTM!Correctly updates the order amount sent to the seller to include the hold invoice amount (base + fee + seller's dev fee).
801-806: LGTM!Good use of
saturating_subfor defensive calculation. The buyer's final amount correctly subtracts both the Mostro fee and their portion of the dev fee.
1097-1105: LGTM!Invoice validation now correctly accounts for the total buyer fees (Mostro fee + buyer's dev fee portion), ensuring the submitted invoice matches the expected amount after all deductions.
389-408: Dev fee fields are properly synchronized between database migration and Order struct.The migration file correctly adds
dev_fee(INTEGER),dev_fee_paid(INTEGER), anddev_fee_payment_hash(CHAR) columns. All three fields are properly initialized in the Order struct at order creation time, with dev_fee calculated from the total fee and the fee payments split 50/50 at transaction time.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/util.rs (1)
1453-1478: LGTM! Tests correctly address previous review feedback.The unit tests now call
calculate_dev_feedirectly with explicit percentage values instead ofget_dev_fee(), avoiding the Settings initialization issue flagged in the previous review. This is the recommended approach from the past feedback.The test coverage is appropriate, including edge cases for rounding, zero amounts, and tiny values.
Based on learnings, the previous review suggested refactoring to either initialize Settings in tests or create a pure function. The implementation chose the pure function approach, which is cleaner for testing.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util.rs
🧰 Additional context used
📓 Path-based instructions (2)
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.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.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-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.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.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-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/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.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/util.rs
📚 Learning: 2025-05-27T20:33:25.134Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 487
File: src/config/settings.rs:39-42
Timestamp: 2025-05-27T20:33:25.134Z
Learning: In the Mostro codebase, panicking is the preferred behavior for missing critical infrastructure components like the database pool. The application should fail fast rather than attempt graceful error handling when core components are not properly initialized.
Applied to files:
src/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.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.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.rs
🧬 Code graph analysis (1)
src/util.rs (2)
src/config/settings.rs (1)
get_mostro(45-50)src/lightning/invoice.rs (1)
is_valid_invoice(181-195)
⏰ 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 (5)
src/util.rs (5)
156-170: LGTM! Clean pure function design.The
calculate_dev_feefunction is well-documented and follows good design principles by being a pure function without global state dependencies. This makes it easily testable and reusable.
172-179: LGTM! Appropriate wrapper design.The
get_dev_feefunction correctly serves as a thin wrapper that supplies the configured percentage from settings. The dependency onSettings::get_mostro()is appropriate for production code where configuration must be initialized.
392-398: LGTM! Correct fee calculation logic.The dev fee calculation correctly computes the total Mostro fee (both parties) and applies the percentage to that total. The inline comments clearly explain the split-fee approach.
816-821: LGTM! Good use of saturating_sub for safety.The buyer's final amount calculation correctly deducts their share of the dev fee, and the use of
saturating_subprevents potential underflow issues.
1107-1120: LGTM! Correct fee validation logic.The invoice validation correctly includes both the Mostro fee and the buyer's share of the dev fee in
total_buyer_fees. This ensures buyers cannot submit invoices that don't account for the full fee burden.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/flow.rssrc/util.rs
🧰 Additional context used
📓 Path-based instructions (2)
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.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.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.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.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.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-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/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/util.rs
📚 Learning: 2025-05-27T20:33:25.134Z
Learnt from: arkanoider
Repo: MostroP2P/mostro PR: 487
File: src/config/settings.rs:39-42
Timestamp: 2025-05-27T20:33:25.134Z
Learning: In the Mostro codebase, panicking is the preferred behavior for missing critical infrastructure components like the database pool. The application should fail fast rather than attempt graceful error handling when core components are not properly initialized.
Applied to files:
src/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.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.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.rs
🧬 Code graph analysis (1)
src/util.rs (2)
src/config/settings.rs (1)
get_mostro(45-50)src/lightning/invoice.rs (1)
is_valid_invoice(181-195)
⏰ 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 (6)
src/flow.rs (1)
72-75: Amount calculations correctly apply buyer dev fee.The logic properly subtracts both
order.feeandbuyer_dev_feefrom the amounts, and usessaturating_subto prevent underflow. Once the inconsistency in line 54 is resolved, these calculations will be correct.Also applies to: 86-86
src/util.rs (5)
156-179: Well-designed fee calculation functions.The separation of
calculate_dev_fee(pure function) andget_dev_fee(settings-aware wrapper) is excellent. This allows testing fee calculations without initializing global settings, as demonstrated in the test suite below. The documentation is clear and comprehensive.
393-397: Correct dev fee calculation in order preparation.The logic correctly computes the total Mostro fee (both parties combined) and derives the development fee as a percentage of that total. The dev_fee stored in the order will be split 50/50 between buyer and seller in subsequent flows.
817-824: Excellent: Compensating logic ensures full dev fee recovery.This implementation correctly handles odd amounts by calculating
buyer_dev_fee = order.dev_fee - seller_dev_fee, ensuring the total always equalsorder.dev_fee. The inline comments clearly explain the rationale. This is the pattern that should be used consistently throughout the codebase.
1115-1125: Consistent buyer fee validation.This correctly uses the same compensating logic as
set_waiting_invoice_statusto calculatebuyer_dev_fee, and includes it in the total fees for invoice validation. The comments and implementation are consistent with the broader fee-handling pattern.
1458-1483: Good test coverage for the pure calculation function.The tests correctly exercise
calculate_dev_feedirectly, avoiding the Settings initialization issue mentioned in past review comments. Coverage includes basic calculation, rounding behavior, zero amounts, and tiny amounts.Note that
get_dev_fee(which wrapscalculate_dev_feewith Settings) is not tested here. If integration testing is needed, ensureMOSTRO_CONFIGis properly initialized before callingget_dev_fee.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/flow.rs (1)
90-91: Usesaturating_subfor consistency with other amount calculations.Lines 76-79 use
saturating_subfor the buyer amount calculation, but this path uses regular subtraction. For consistency and to prevent potential negative values, consider using saturating operations.🔎 Proposed fix
- 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);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/add_invoice.rssrc/app/release.rssrc/flow.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/add_invoice.rssrc/flow.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/app/add_invoice.rssrc/flow.rssrc/app/release.rs
src/app/**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Mirror test fixtures under
src/app/where applicable
Files:
src/app/add_invoice.rssrc/app/release.rs
🧠 Learnings (5)
📓 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/app/add_invoice.rssrc/flow.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/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-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
🪛 GitHub Actions: CI
src/app/add_invoice.rs
[error] 87-87: cargo fmt --all -- --check failed. rustfmt detected formatting issues at this location. Run 'cargo fmt' to fix.
[error] 102-102: cargo fmt --all -- --check failed. rustfmt detected formatting issues at this location. Run 'cargo fmt' to fix.
src/flow.rs
[error] 59-59: cargo fmt --all -- --check failed. rustfmt detected formatting issues at this location. Run 'cargo fmt' to fix.
src/app/release.rs
[error] 69-69: cargo fmt --all -- --check failed. rustfmt detected formatting issues at this location. Run 'cargo fmt' to fix.
🔇 Additional comments (5)
src/flow.rs (2)
54-56: LGTM! Proper compensating logic for dev_fee split.The calculation now correctly ensures that
seller_dev_fee + buyer_dev_fee = order.dev_feefor both even and odd amounts, addressing the previous review feedback about potential 1-satoshi loss.
62-64: Amount calculations are correctly structured.Seller sees the total deposited amount (base + fees), and buyer sees the net payout after fee deductions. The use of
saturating_add/saturating_subprevents integer overflow/underflow.Also applies to: 76-79
src/app/add_invoice.rs (2)
88-92: Dev_fee split for seller is correctly implemented.The calculation uses the same compensating pattern as in
flow.rs, ensuring no satoshi loss for odd dev_fee values.
103-107: Buyer dev_fee calculation and amount adjustment are correct.The compensating calculation
buyer_dev_fee = dev_fee - seller_dev_feeensures the full dev_fee is recovered, andsaturating_subprevents underflow.src/app/release.rs (1)
70-74: Dev_fee split in failure retry path is correctly implemented.The compensating logic ensures full dev_fee recovery. The subsequent check at line 75-77 guards against non-positive amounts.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/app/release.rs (1)
444-452: Good fix for the underflow issue.The use of
saturating_subproperly addresses the potential underflow concern from the previous review. The zero-amount check ensures invalid payments aren't attempted.Consider whether
InvoiceInvalidErroris the most semantically accurate error for "amount reduced to zero after fees". A more specific error likeInvalidAmount(similar to line 77) might better communicate the root cause, though this is a minor consistency nit.src/app/add_invoice.rs (1)
104-109: Consider adding a zero-amount validation for consistency.The buyer amount calculation correctly subtracts fees using
saturating_sub. However, unlike the similar calculation indo_payment(release.rs lines 450-452), there's no check to ensure the resulting amount is greater than zero before sending the message to the buyer.While this scenario is unlikely (fees exceeding trade amount), adding a validation would maintain consistency with the payment flow and prevent confusing the buyer with a zero-amount message.
🔎 Proposed validation
// We send a message to buyer saying seller paid let buyer_dev_fee = active_order.dev_fee - seller_dev_fee; let mut buyer_order = SmallOrder::from(active_order.clone()); buyer_order.amount = active_order .amount .saturating_sub(active_order.fee) .saturating_sub(buyer_dev_fee); + if buyer_order.amount <= 0 { + return Err(MostroCantDo(CantDoReason::InvalidAmount)); + } enqueue_order_msg(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/app/add_invoice.rssrc/app/release.rssrc/flow.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/flow.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.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-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.rssrc/app/add_invoice.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.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/app/add_invoice.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.rssrc/app/add_invoice.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.rssrc/app/add_invoice.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-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-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-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 : Name test functions descriptively, e.g., `handles_expired_hold_invoice`
Applied to files:
src/app/add_invoice.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 (2)
src/app/release.rs (1)
70-78: Dev fee splitting and amount calculation look correct.The implementation correctly splits
dev_feebetween seller and buyer, with the buyer portion being subtracted from the payment amount. Usingsaturating_subprevents underflow, and the zero-amount check provides proper validation.One minor observation: the comparison at line 76 uses
<= 0on ani64, but aftersaturating_subthe value can never be negative (it would have saturated at 0). Consider using== 0for clarity, or document why<= 0is preferred for defensive coding.src/app/add_invoice.rs (1)
88-93: Seller amount calculation is correct.The seller's displayed amount correctly includes their fee contribution and their portion of the dev fee. Using
saturating_addprevents overflow.
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 (2)
docs/DEV_FEE.md (2)
277-363: Verify location 4 line range in do_payment().All four dev_fee calculation locations are present and correctly implemented. However, the documented line range for
src/app/release.rs::do_payment()is off by one—the buyer_dev_fee is subtracted at line 449, not within the stated range of 443-448. Update the documentation to reflect lines 443-449 to include the complete calculation:amount = order.amount - order.fee - buyer_dev_fee. The remaining three locations (hold_invoice_paid, add_invoice_action, check_failure_retries) have accurate line numbers and verified implementations matching the documented pattern.
182-256: Dev fee reset on cancel is implemented but recalculation on order re-take is missing.The review comment correctly identifies the problem: dev fees should be recalculated when an order is taken at a different market price. However, the implementation is incomplete:
Method reference is incorrect: The code uses
price_from_apiflag (set whenamount=0) to identify market price orders, not a non-existentis_market_price()method.Reset on cancel is implemented: The
reset_api_quotes()function incancel.rs(lines 18-25) correctly resetsdev_fee=0whenorder.price_from_api=true, with a comment stating "ensure fresh recalculation on re-take."Critical gap—recalculation on re-take is missing: When an order is taken in
take_sell.rs/take_buy.rs(lines 104-112), the code recalculatesorder.amountandorder.feefrom the current market price, butorder.dev_feeis never recalculated. It retains its previous value (0 for new orders, but stale for re-taken orders). Theget_dev_fee()calculation logic exists but is never invoked in the take flow.Premium field as market price indicator: The
premiumfield exists and equals 0 for market price orders, so the SQL query in the docs is valid. However, the actual code uses theprice_from_apiboolean instead.Action: After order amount and fee are recalculated from market price in the take flow, add a corresponding
dev_feerecalculation based on the new fee amount to ensure correct accounting.
🧹 Nitpick comments (2)
docs/DEV_FEE.md (2)
152-157: Add language identifier to formula code block (MD040).The fenced code block at line 153 lacks a language identifier. Add
textor use a triple-backtick withtextspecifier for markdown lint compliance.🔎 Proposed fix
Formula Specification:** -``` +```text total_dev_fee = round(total_mostro_fee × dev_fee_percentage)
90-98: Consider documenting temporary address for testing phase.Per learnings from PR #554, the Lightning address in
src/config/constants.rsis currentlymostro_p2psats.mobi(temporary for testing) and will be updated todevelopmentmostro.networkin production. While the documentation uses a placeholder<dev@lightning.address>, consider adding a note in the Configuration section (around line 96) or Security Considerations to clarify this is a temporary testing address.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/DEV_FEE.md
🧰 Additional context used
🧠 Learnings (5)
📓 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:
docs/DEV_FEE.md
📚 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:
docs/DEV_FEE.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: 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:
docs/DEV_FEE.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:
docs/DEV_FEE.md
🪛 markdownlint-cli2 (0.18.1)
docs/DEV_FEE.md
153-153: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ 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 (7)
docs/DEV_FEE.md (7)
13-52: Phase status clearly communicated with appropriate caveats.The implementation status section accurately delineates completed phases (1-2) from planned work (Phase 3), with helpful checkmarks and specific notes ("No dev fee payments are currently being made"). This is good documentation discipline for tracking multi-phase rollouts.
498-584: Implementation roadmap is well-structured with clear tracking, but completeness depends on earlier verification.The checklist format is excellent for progress tracking. However, the validity of Phase 2 completion claims (all items marked ✅) depends on verification that the referenced line numbers and locations are accurate and that code actually implements what's claimed. Use results from earlier verification scripts to confirm.
1-100: Documentation structure and comprehensiveness are excellent.The file provides thorough coverage of the dev_fee feature across multiple aspects: phases, architecture, configuration, calculations, integration points, monitoring, and troubleshooting. The inclusion of SQL queries, ASCII diagrams, examples with real numbers, and migration guidance demonstrates strong documentation discipline.
The main dependencies for approval are verification that:
- Phase 2 completion claims match actual code implementations
- Line number references in code snippets are still accurate
- All 4 message calculation locations have been updated as claimed
258-276: No action required. The hold invoice implementation is already complete and correct. Lines 669-672 insrc/util.rs::show_hold_invoice()already include the seller's dev_fee calculation (let new_amount = order.amount + order.fee + seller_dev_fee;), which matches the documented requirement. The Phase 2 "✅ COMPLETE" status is accurate; there is no inconsistency.
745-789: Line numbers for unit tests are incorrect and require correction.The four test functions do exist in
src/util.rs, but at different line numbers than documented:
test_get_dev_fee_basic: line 1459 (not 1453-1458)test_get_dev_fee_rounding: line 1466 (not 1460-1465)test_get_dev_fee_zero: line 1473 (not 1467-1471)test_get_dev_fee_tiny_amounts: line 1479 (not 1473-1478)Test implementations match the documented behavior. Additionally, clarify the discrepancy: the coverage checklist lists 5 items but only 4 tests are documented. Tests could not be executed in the verification environment, so confirm
cargo test test_get_dev_feepasses locally per the learnings.Likely an incorrect or invalid review comment.
122-170: Fee calculation approach is sound and well-documented.The two-function pattern (pure
calculate_dev_fee+ settings wrapperget_dev_fee) is a proven approach for testability and separation of concerns. The satoshi-loss prevention formula is correctly implemented and clearly explained with examples. Line numbers match the actual implementation insrc/util.rs(167-170 forcalculate_dev_fee, 176-179 forget_dev_fee), function signatures are correct, and the split logic (seller =dev_fee / 2, buyer =dev_fee - seller_dev_fee) is consistently applied across the codebase. Tests validate the rounding behavior and formula correctness.
375-435: Referenced functions and dependencies exist, but the dev fee payment implementation is incomplete.The functions and modules referenced in the specification (
resolv_ln_address(),LndConnector,send_payment(),tokio::time::timeout) all exist in the codebase. However, the actual dev fee payment logic is not fully implemented:
- Dev fees are deducted from buyer payments in
do_payment()(lines 445-449)- Dev fees are never sent to Mostro—no function exists to send the accumulated fees
- The
dev_fee_paidfield is initialized tofalsebut never set totrue- The scheduler-based processing with timeouts described in the documentation is not implemented—no scheduler job exists for dev fee payment
The current state is that dev fee amounts are subtracted from what buyers receive but are held indefinitely without being paid out.
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.