Improve MOSTRO_PUBKEY resolution and remove startup panic #152
Conversation
WalkthroughA resolver function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/fiat.rs (1)
62-1476: Fix critical syntax errors preventing compilation.The function has two blocking issues:
- Type mismatch: Line 62 declares return type
FiatList, but line 1467 uses the?operator which requiresResult<FiatList>.- Missing closing brace: The
forloop starting at line 1469 is never closed. Lines 1473-1475 (sort and return) appear outside the loop scope.🔎 Proposed fix
-pub fn load_fiat_values() -> FiatList { +pub fn load_fiat_values() -> Result<FiatList> { let fiat_names = r#" { ... }"#; // Parse fiat names let fiat_json = serde_json::from_str(fiat_names).map_err(|e| anyhow::anyhow!("Failed to parse fiat names: {}", e))?; let mut fiatlist = FiatList::new(); for elem in fiat_json.iter() { fiatlist.push((elem.0.to_string(), elem.1.name.clone())); + } //Return list fiatlist.sort_by(|a, b| a.0.cmp(&b.0)); - fiatlist + Ok(fiatlist) }Note: Callers of this function will need updating to handle the Result.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/.fiat.rs.un~src/cli.rssrc/fiat.rssrc/fiat.rs~
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli.rs:301-306
Timestamp: 2025-09-12T20:02:14.291Z
Learning: The mostro-cli is designed as a test client that allows users to test both regular user operations and admin operations from the terminal. Therefore, MOSTRO_PUBKEY should always be required regardless of whether NSEC_PRIVKEY is present, as both user and admin operations need to know which Mostro instance to interact with.
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/add_invoice.rs:80-82
Timestamp: 2025-09-09T19:58:58.506Z
Learning: In the mostro-cli codebase, when the trade_index parameter isn't needed inside a function like wait_for_dm, it should be made Optional<i64> instead of requiring callers to pass hardcoded values like 0. This makes the API clearer and avoids unnecessary computation for resolving indices when they're not used.
📚 Learning: 2025-09-12T20:02:14.291Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli.rs:301-306
Timestamp: 2025-09-12T20:02:14.291Z
Learning: The mostro-cli is designed as a test client that allows users to test both regular user operations and admin operations from the terminal. Therefore, MOSTRO_PUBKEY should always be required regardless of whether NSEC_PRIVKEY is present, as both user and admin operations need to know which Mostro instance to interact with.
Applied to files:
src/cli.rs
📚 Learning: 2025-09-09T19:58:58.506Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/add_invoice.rs:80-82
Timestamp: 2025-09-09T19:58:58.506Z
Learning: In the mostro-cli codebase, when the trade_index parameter isn't needed inside a function like wait_for_dm, it should be made Optional<i64> instead of requiring callers to pass hardcoded values like 0. This makes the API clearer and avoids unnecessary computation for resolving indices when they're not used.
Applied to files:
src/cli.rs
📚 Learning: 2025-09-14T14:01:01.728Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/util.rs:614-619
Timestamp: 2025-09-14T14:01:01.728Z
Learning: In the mostro-cli codebase, the user arkanoider indicated that accessing `public_key` without parentheses (as a field) is valid, even though most of the codebase uses `public_key()` method calls. This suggests there may be `Deref` implementation or other Rust features that allow both syntaxes to work.
Applied to files:
src/cli.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, arkanoider confirmed that tokio::spawn with async move is correct and compiles without errors when using PublicKey (which implements Copy). The spawning approach is acceptable for DM sending in this context.
Applied to files:
src/cli.rs
📚 Learning: 2025-09-13T20:48:31.581Z
Learnt from: arkanoider
Repo: MostroP2P/mostro-cli PR: 135
File: src/cli/send_msg.rs:89-110
Timestamp: 2025-09-13T20:48:31.581Z
Learning: In mostro-cli's send_msg.rs, tokio::spawn with async move is valid for sending DMs when PublicKey implements Copy, and arkanoider confirmed this approach works without memory issues or compilation errors.
Applied to files:
src/cli.rs
🧬 Code graph analysis (1)
src/cli.rs (1)
src/util/messaging.rs (1)
var(214-216)
🔇 Additional comments (4)
src/fiat.rs (1)
906-915: LGTM! MWK currency entry is well-formed.The Malawian Kwacha entry follows the correct structure and is properly positioned alphabetically after MUR.
src/cli.rs (3)
43-43: LGTM! Import simplification is appropriate.The import correctly reflects that
set_varis used via this path whilevaris now fully qualified at its call site (line 376).
373-385: Excellent error handling improvement!The resolver properly prioritizes CLI flag over environment variable and replaces panic-prone
expect()with idiomatic Result-based error handling. The error message clearly guides users on how to provide the required value.Based on learnings, MOSTRO_PUBKEY is correctly enforced as required for all operations.
419-421: LGTM! Proper integration of the resolver.The resolver is correctly invoked with error propagation, and PublicKey parsing errors are also properly handled.
|
Thank you for bringing this to my attention.
You are correct this commit is not related to the current PR. I apologize
for the oversight. I will remove the unrelated commit and update the PR so
that it contains only the relevant changes.
Thank you for the review and your guidance.
Kind regards,
Yankho Ngolleka
(codaMW)
…On Mon, Jan 5, 2026 at 7:56 PM Catrya ***@***.***> wrote:
***@***.**** commented on this pull request.
Hi @codaMW <https://github.com/codaMW>, this commit is not related to
this PR, please fix it. Commits should not be mixed between PRs
—
Reply to this email directly, view it on GitHub
<#152 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BBCHOJPT3A3JYYK6PPIYKCL4FKQWTAVCNFSM6AAAAACQUVTIGOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTMMRXHA4DMNBWGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
codaMW
force-pushed
the
fix-graceful-mostro-pubkey
branch
from
d520e38 to
abeaef7
Compare
|
Thanks @Catrya for the quick feedback! I've force-pushed a cleaned-up version:
|
2 participants
Summary
This PR refactors how
MOSTRO_PUBKEYis resolved in the CLI to avoidpanic-based startup failures and follow idiomatic Rust error handling.
Changes
MOSTRO_PUBKEYfrom:--mostropubkeyCLI flag, orMOSTRO_PUBKEYenvironment variableexpect()with properOption -> ResultconversionMotivation
The previous implementation panicked if
MOSTRO_PUBKEYwas not set,causing abrupt CLI termination. This change improves user experience,
maintainability, and aligns with Rust best practices.
Testing
cargo fmtcargo checkcargo testNotes
This change is backward compatible and does not alter runtime behavior
when
MOSTRO_PUBKEYis correctly provided.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.