Aligned premium logic to lnp2p bot #550
Conversation
WalkthroughChanged premium application in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 (3)
src/util.rs (3)
29-29: Remove unnecessary Sub trait; use idiomatic-operator instead.The
Subtrait is imported but used in an unconventional way. Rust idiom favors the-operator over calling.sub()directly on numeric types.Apply this diff to use the standard subtraction operator:
-use std::ops::Sub; +// Remove this import, use - operator insteadThen update the premium calculation:
- if premium != 0 { - sats = sats.sub((premium as f64) / 100_f64 * sats); + if premium != 0 { + sats = sats - (premium as f64) / 100_f64 * sats;Also applies to: 145-146
141-141: Remove temporary debug println! statements.Per the PR objectives, these debug prints are temporary and should be removed after testing. Additionally, consider using the structured logging already available in the codebase (
tracingis imported) instead ofprintln!for production instrumentation.Apply this diff to remove the debug prints:
let mut sats = quote.result * 100_000_000_f64; - println!("Quote result: {}", sats); // Added premium value to have correct sats value if premium != 0 { sats = sats.sub((premium as f64) / 100_f64 * sats); - println!("Quote result + premium of {}%: {}", premium, sats); }If you need to retain instrumentation, use
info!from thetracingcrate:+ info!("Quote result: {}", sats);Also applies to: 146-146
771-774: Remove temporary debug println! statement inget_market_amount_and_fee.Per the PR objectives, this debug output should be removed after testing. Use structured logging if retention is needed.
Apply this diff to remove the debug print:
let new_sats_amount = get_market_quote(&fiat_amount, fiat_code, premium).await?; let fee = get_fee(new_sats_amount); - println!( - "Final sats amount with premium and fee: {} - fees are {} sats", - new_sats_amount, fee - ); Ok((new_sats_amount, fee))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
🔇 Additional comments (1)
src/util.rs (1)
144-147: Premium subtraction logic is correct.The change from addition to subtraction correctly implements the aligned behavior from the lnp2p bot. The percentage calculation and application are accurate.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/util.rs (1)
770-773: Prefer tracing (or remove) instead ofprintln!for production loggingThe debug print:
println!( "Final sats amount with premium and fee: {} - fees are {} sats", new_sats_amount, fee );is fine for temporary debugging, but:
- It bypasses the existing
tracing‑based logging used elsewhere in this module.- The wording implies the first number already includes the fee, which it doesn’t—only the second argument is the fee.
Once you’re done validating this change, consider either:
- Removing this
println!(and the one inget_market_quote), or- Replacing both with
tracing::debug!/info!and clarifying the message text, e.g."Final sats amount (with premium): {}, fee: {} sats".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/util.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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
@grunch, @Catrya,
this fix, aligning with correct implementation in lnp2p bot, the correct invoice amout when order has a premium, in mostro the amount * premium_prc was summed to base amount, normally this is correct, but since the seller here pays the invoice we have to subtract it from base amount for the correctness of logic.
I left to println! to show on terminal the values to help @Catrya in debug, i will remove them when tested.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.