fix: correct amount calculations in seller/buyer messages

[Catrya]

fix #551

• pay-invoice: Show order amount + fee
• buyer-took-order: Show order amount + fee
• hold-invoice-payment-accepted: Show order amount - fee

Summary by CodeRabbit

• Bug Fixes
  • Invoice and order messages now use adjusted amounts so sellers are credited including transaction fees and buyers are charged net of fees; hold invoices and confirmation messages reflect these corrected amounts.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Payload construction for hold/order-related messages now sends fee-adjusted amounts: seller-facing payloads use amount + fee; buyer-facing payloads use amount - fee (saturating subtraction). No public signatures or control flow were changed.

Changes

Cohort / File(s)	Summary
Invoice payload adjustments src/app/add_invoice.rs, src/flow.rs, src/util.rs	Replaced usages of the original order payload with derived order variants whose amount has been adjusted for fee: seller-facing payloads use amount + fee; buyer-facing payloads use amount - fee (saturating). Updated enqueued Payload::Order instances and the amount set when creating hold orders.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Verify saturating subtraction is used where appropriate to avoid underflow.
• Confirm all enqueue points send the intended derived order (seller vs buyer).
• Check downstream consumers of Payload::Order expect and correctly handle the adjusted amounts.

Possibly related PRs

• fix: correct AddInvoice amount calculation on payment failure #524 — Modifies order amount adjustments for AddInvoice/hold-invoice payloads (adds/subtracts fee, uses saturating_sub).
• Sends an updated order status as a reply to the add-invoice action #464 — Related changes to how Payload::Order is constructed and sent from add_invoice_action.
• Pay new added invoice #462 — Changes in src/app/add_invoice.rs touching invoice handling and payload composition.

Suggested reviewers

• grunch
• arkanoider

Poem

🐰 A hop, a tweak, a tiny ledger song,
Seller adds a fee and the numbers belong.
Buyer loses none—safely subtracting with care,
I nibble the math and set the amounts fair.
Hooray for invoices—🥕 hop, hop, declare!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: correcting amount calculations in seller/buyer messages by adjusting fees.
Linked Issues check	✅ Passed	Changes correctly address issue #551 by adjusting amounts in pay-invoice, buyer-took-order, and hold-invoice-payment-accepted messages to include/exclude fees as applicable.
Out of Scope Changes check	✅ Passed	All changes are directly related to correcting amount calculations for seller/buyer messages; no out-of-scope modifications detected.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-seller-message

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/app/add_invoice.rs (1)

88-107: Fee‑adjusted seller/buyer order views look correct; consider saturating_sub for buyer

Using two SmallOrder views with different amounts:

• Seller (BuyerTookOrder): amount + fee – matches the hold‑invoice amount the seller actually paid.
• Buyer (HoldInvoicePaymentAccepted): amount - fee – matches the buyer’s net receive amount.

This aligns the messages with the economic model and with the updated flow in hold_invoice_paid.

If you want extra robustness against misconfigured fees and to mirror set_waiting_invoice_status, you could make the buyer side saturating:

-        let mut buyer_order = SmallOrder::from(active_order.clone());
-        buyer_order.amount = active_order.amount - active_order.fee;
+        let mut buyer_order = SmallOrder::from(active_order.clone());
+        buyer_order.amount = active_order.amount.saturating_sub(active_order.fee);

This is defensive rather than strictly necessary given normal fee settings.

src/flow.rs (1)

58-77: Consistent fee‑adjusted payloads in hold_invoice_paid; optionally harden buyer amount and add a targeted test

The new seller/buyer SmallOrder views here:

• seller_order_data.amount = order.amount + order.fee for BuyerTookOrder.
• buyer_order_data.amount = order.amount - order.fee for HoldInvoicePaymentAccepted.

correctly mirror the economic model (seller pays amount + fee, buyer receives amount - fee) and are consistent with add_invoice_action and set_waiting_invoice_status.

For extra safety and consistency with set_waiting_invoice_status, you might make the buyer side saturating:

-        let mut buyer_order_data = order_data.clone();
-        buyer_order_data.amount = order.amount - order.fee;
+        let mut buyer_order_data = order_data.clone();
+        buyer_order_data.amount = order.amount.saturating_sub(order.fee);

Given this logic is the crux of issue #551, it may also be worth adding a small test that constructs an Order { amount, fee, .. } and asserts that the enqueued Payload::Order for BuyerTookOrder and HoldInvoicePaymentAccepted carries amount + fee and amount - fee respectively (possibly by factoring the construction into a helper so it’s testable without a real DB/LND).

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 47fe2a1 and 482b990.

📒 Files selected for processing (3)

• src/app/add_invoice.rs (1 hunks)
• src/flow.rs (1 hunks)
• src/util.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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
• src/app/add_invoice.rs
• src/flow.rs

🧬 Code graph analysis (2)

src/app/add_invoice.rs (1)

src/util.rs (1)

• enqueue_order_msg (924-939)

src/flow.rs (1)

src/util.rs (1)

• enqueue_order_msg (924-939)

⏰ 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 (1)

src/util.rs (1)

677-680: Pay‑invoice payload now matches seller’s fee‑inclusive hold amount

Setting new_order.amount = order.amount + order.fee makes the PayInvoice PaymentRequest payload consistent with the hold invoice amount the seller actually pays, and with the fee‑adjusted views used elsewhere. This is a clean, localized fix to the display amount.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/app/add_invoice.rs (1)

88-94: Use saturating arithmetic for consistency and safety.

Line 89 uses direct addition which could theoretically overflow. For consistency with the buyer-side calculation (line 101) that uses saturating_sub, consider using saturating_add here.

Apply this diff:

-        seller_order.amount = active_order.amount + active_order.fee;
+        seller_order.amount = active_order.amount.saturating_add(active_order.fee);

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 482b990 and 04fd9a2.

📒 Files selected for processing (2)

• src/app/add_invoice.rs (1 hunks)
• src/flow.rs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/flow.rs

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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.rs

🧬 Code graph analysis (1)

src/app/add_invoice.rs (1)

src/util.rs (1)

• enqueue_order_msg (924-939)

⏰ 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: clippy

🔇 Additional comments (1)

src/app/add_invoice.rs (1)

100-107: saturating_sub is appropriate here; fee constraints are implicitly enforced by configuration.

The saturating_sub on line 101 is correct and necessary. The fee calculation in get_fee() (src/util.rs:149-154) computes (fee_percentage * amount) / 2.0, which mathematically guarantees fee < amount as long as the fee percentage setting is below 200%—a reasonable operational constraint. This implicit validation through configuration is reinforced by the test assertion at src/take_sell.rs:359 that confirms expected_fee < amount. The saturating_sub serves as defensive programming to handle the result safely even if unexpected values occur.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

src/app/add_invoice.rs (1)

88-107: Fee-adjusted SmallOrder payloads for seller/buyer

The new seller/buyer payload amounts look conceptually right: seller’s BuyerTookOrder now uses amount + fee, buyer’s HoldInvoicePaymentAccepted uses amount - fee, and saturating_add / saturating_sub avoid integer overflow in extreme cases.

One thing to verify carefully: with the related show_hold_invoice / new_order change in src/util.rs (where new_order.amount = order.amount + order.fee), is active_order.amount here still the pre-fee trade amount, or has it already been adjusted to include the fee? If it already includes the fee, these lines will effectively:

• show amount + 2 * fee to the seller, and
• show the original amount (not amount - fee) to the buyer.

Might be worth tracing amount through the full flow (order creation → show_hold_invoice → update_order_event → add_invoice_action) to confirm we’re not double-applying the fee in any path. If amount is already seller-side at this stage, the formulas likely need to be based on the original pre-fee amount instead.

Also, if you keep touching this logic, consider a small helper that, given (amount, fee), returns (seller_amount, buyer_amount) so both call sites (here and in hold_invoice_paid) can’t drift apart.

src/flow.rs (1)

58-77: Fee-adjusted SmallOrder amounts in hold_invoice_paid

Using separate clones for seller and buyer and setting:

• seller: amount = order.amount.saturating_add(order.fee) for BuyerTookOrder
• buyer: amount = order.amount.saturating_sub(order.fee) for HoldInvoicePaymentAccepted

matches the intended “seller sees amount+fee, buyer sees amount-fee” behaviour, assuming order.amount is still the pre-fee trade amount at this point.

Given the related change in src/util.rs (where new_order.amount = order.amount + order.fee during the hold-invoice flow), can you double-check what order.amount represents here when hold_invoice_paid runs? If it’s already been updated to amount + fee, this new logic would:

• report amount + 2 * fee to the seller, and
• report just amount (no fee discount) to the buyer,

which would contradict the PR’s goal. Tracing amount/fee through show_hold_invoice → DB update → hold_invoice_paid would clarify whether the base amount is being used consistently.

As a minor follow-up, you might also want to use saturating_sub in the WaitingBuyerInvoice branch (let new_amount = order_data.amount - order.fee;) for consistency with the buyer-facing path here, although realistic values shouldn’t come close to overflowing i64.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 04fd9a2 and 4651a3e.

📒 Files selected for processing (2)

• src/app/add_invoice.rs (1 hunks)
• src/flow.rs (1 hunks)

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

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.

📚 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.rs
• src/flow.rs

🧬 Code graph analysis (2)

src/app/add_invoice.rs (1)

src/util.rs (1)

• enqueue_order_msg (924-939)

src/flow.rs (1)

src/util.rs (1)

• enqueue_order_msg (924-939)

⏰ 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

[grunch]

LGTM

[arkanoider]

Good job @Catrya ! Another detail that was silently there since long time, maybe i will look in the next days if I am able to compact in some way the two buffer variables to send messages to seller and buyer, but it's totally right!

uTACK!!

@grunch can merge this!