Conversation
WalkthroughBump Changes
Sequence Diagram(s)(Skipped — changes are logging and test data updates; no new control flow to illustrate.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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/app.rs (3)
325-327: Clarify message and add structured context to the warn.Current text mentions “inner message” though we parse (Message, Option). Prefer precise wording and fields for triage.
Apply:
- tracing::warn!("Failed to parse message, inner message, and signature from rumor content: {}", e); + tracing::warn!( + error = %e, + sender = %event.sender, + rumor_pubkey = %event.rumor.pubkey, + "Failed to parse (Message, Option<Signature>) from rumor content" + );
334-336: Include identifiers in serialization warning for better observability.Add sender/rumor pubkey to speed up correlation.
Apply:
- tracing::warn!("Failed to serialize message: {}", e); + tracing::warn!( + error = %e, + sender = %event.sender, + rumor_pubkey = %event.rumor.pubkey, + "Failed to serialize message to JSON" + );
361-362: Differentiate warn vs error by error kind when checking trade index.CantDo should be warn; internal faults should stay error. Keeps signal in dashboards after the downgrade.
Apply:
- tracing::warn!("Error checking trade index: {}", e); - continue; + match &e { + MostroError::MostroCantDo(_) => { + tracing::warn!( + error = %e, + action = ?message.get_inner_message_kind().action, + trade_index = ?message.get_inner_message_kind().trade_index(), + sender = %event.sender, + "CantDo while checking trade index" + ); + } + MostroError::MostroInternalErr(_) => { + tracing::error!( + error = %e, + action = ?message.get_inner_message_kind().action, + trade_index = ?message.get_inner_message_kind().trade_index(), + sender = %event.sender, + "Internal error while checking trade index" + ); + } + } + continue;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
Cargo.toml(2 hunks)src/app.rs(3 hunks)
⏰ 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: tests
🔇 Additional comments (2)
Cargo.toml (2)
91-91: Build dep pin aligns with runtime versions. LGTM.
Matches tonic/prost 0.14.x; no action needed.
67-67: Install C compiler, verify build withmostro-core0.6.50 and update Cargo.lock
Build failed due to missing linker (cc). Install a system C compiler, reruncargo update -p mostro-core cargo check --all-featuresconfirm no API or feature changes, and commit the refreshed Cargo.lock.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lightning/invoice.rs (1)
288-291: Zero-amount test now uses a mainnet (lnbc) invoice — fine; consider clarifying intent.Mixing lnbc/lnbcrt across tests is harmless here since validation is format/expiry based, not network-bound. Adding a short comment avoids future confusion.
- let payment_request = "lnbc1p5tmmdspp5k37c7l45d97axrq0zrazzrhhyfd5mp338urg08txfp03vjx8evpqdq5g9kxy7fqd9h8vmmfvdjscqzzsxqyz5vqsp5tejy26f4wr7d0gcmrc3ndyzg0md6m7kxj3xrnl7rc4e96hhq7nqq9qxpqysgq48ww59a8w5kszspu9jpuv66smgkwjxedj69c6nrry0zwzn46d38x5g022lr95d4np39cfh6pne59vcrpymxwyrnl379cyymer907etqp2497a8".to_string(); + // Zero-amount Bolt11; network prefix (lnbc) is irrelevant to the validation under test. + let payment_request = "lnbc1p5tmmdspp5k37c7l45d97axrq0zrazzrhhyfd5mp338urg08txfp03vjx8evpqdq5g9kxy7fqd9h8vmmfvdjscqzzsxqyz5vqsp5tejy26f4wr7d0gcmrc3ndyzg0md6m7kxj3xrnl7rc4e96hhq7nqq9qxpqysgq48ww59a8w5kszspu9jpuv66smgkwjxedj69c6nrry0zwzn46d38x5g022lr95d4np39cfh6pne59vcrpymxwyrnl379cyymer907etqp2497a8".to_string();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/lightning/invoice.rs(1 hunks)src/main.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Catrya
PR: MostroP2P/mostro#502
File: src/app/release.rs:52-55
Timestamp: 2025-06-25T22:48:44.741Z
Learning: Cross-repository dependencies in Mostro project: when PRs introduce new types like PaymentFailedInfo from mostro-core, compilation errors are expected until the dependency PR is merged and the version is updated in the consuming repository.
📚 Learning: 2025-07-17T13:51:17.338Z
Learnt from: arkanoider
PR: MostroP2P/mostro#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/lightning/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: tests
🔇 Additional comments (1)
src/main.rs (1)
141-144: Confirm buyer_invoice rename and absence of payment_request in tests/fixtures.
Verified no remaining “payment_request” references in any tests or fixtures.
2 participants
Hi @grunch ,
just bumped mostro-core to 6.50 and changed some errors to warn in app.rs
Summary by CodeRabbit
Refactor
Chores
Tests