feat(cashu): F2 — config, escrow mode, and conditional boot#758
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds Cashu ecash escrow support as an alternative to Lightning-based hold invoices. It introduces configuration types, validation rules, setup wizard prompts, a Cashu-only event loop, and startup-time mode selection between Lightning and Cashu execution paths. ChangesCashu Escrow Mode Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0f720c94bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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/main.rs (1)
142-155:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix scheduler startup order: avoid
LN_STATUS.get().unwrap()before LND init (and in Cashu mode).
main.rscallsstart_scheduler(ctx.clone()).awaitbefore eitherLN_STATUS.set(...)(Lightning) or any Cashu-mode initialization/exit. Inscheduler.rs,start_schedulerawaitsjob_info_event_send(...), and that function immediately doeslet ln_status = LN_STATUS.get().unwrap();, so:
- Cashu mode will panic because it returns before
LN_STATUS.setruns.- Lightning mode can also panic/race because the scheduler runs before
LN_STATUS.set(ln_status)atmain.rs:153.Move
start_schedulerafterLN_STATUS.set(...), or makejob_info_event_sendnot unwrap whenLN_STATUSisn’t available in the current mode (e.g., gate it onSettings::is_cashu_enabled()/ conditional LN_STATUS access).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 142 - 155, The scheduler is started before LN_STATUS is ever set, causing job_info_event_send to call LN_STATUS.get().unwrap() and panic in Cashu mode or race in Lightning mode; fix by either (A) moving start_scheduler(ctx.clone()).await in main.rs to after LN_STATUS.set(ln_status) (i.e., after LndConnector::new().await, ln_client.get_node_info().await and LnStatus::from_get_info_response are used and LN_STATUS.set(...) succeeds), or (B) make job_info_event_send/its caller guard LN_STATUS.get() access (check Settings::is_cashu_enabled() or handle None instead of unwrap) so Cashu mode and pre-set scheduler startup do not unwrap a missing LN_STATUS. Ensure references to start_scheduler, job_info_event_send, LN_STATUS.set, Settings::is_cashu_enabled, run_cashu, and LndConnector::new are updated accordingly.
🧹 Nitpick comments (2)
src/app.rs (1)
398-484: 🏗️ Heavy lift
run_cashuduplicates the entirerunevent loop, including security-critical validation.The body of
run_cashu(408-483) is a line-for-line copy ofrun(304-395); the only difference is the dispatch call (handle_message_action_no_lnvshandle_message_action). This duplicates the POW check, NIP-59 unwrap, 10s replay window, inner-signature enforcement, andcheck_trade_indexlogic. Any future fix to these checks must be applied in two places, which is an easy place for the two paths to silently diverge.Consider extracting the shared loop and parameterizing the dispatch step (e.g., an optional
&mut LndConnector, an enum, or a closure), so both modes share one validated path.♻️ Sketch: single loop parameterized by mode
enum EventMode<'a> { Lightning(&'a mut LndConnector), Cashu, } async fn run_loop(ctx: AppContext, mut mode: EventMode<'_>) -> Result<()> { let my_keys = ctx.keys(); let client = ctx.nostr_client(); let pow = ctx.settings().mostro.pow; // ... shared validation ... // dispatch: let res = match &mut mode { EventMode::Lightning(ln) => { handle_message_action(&action, message.clone(), &unwrapped, my_keys, ln, &ctx).await } EventMode::Cashu => { handle_message_action_no_ln(&action, message.clone(), &unwrapped, my_keys, &ctx).await } }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.rs` around lines 398 - 484, run_cashu duplicates the entire run event loop (POW check, unwrap_message, replay window, inner-signature check, check_trade_index) differing only by the dispatch call; extract the shared logic into a single parameterized loop and call it from both modes. Create a new function (e.g., run_loop or similar) that takes AppContext plus a dispatch-mode parameter (an enum like EventMode or a closure) and moves the common validation and event iteration there, keeping the existing symbols unwrap_message, check_trade_index, and inner_message verification unchanged; then have run call the new loop with a mode that invokes handle_message_action and run_cashu call it with a mode that invokes handle_message_action_no_ln. Ensure error handling (manage_errors, warning_msg, MostroError downcast) remains identical inside the shared loop.src/config/wizard.rs (1)
378-388: ⚡ Quick winAdd a unit test for
validate_mint_url.The other validators (
validate_nsec,validate_relays,validate_file_exists) have co-located tests, but this new public validator does not. A small test would lock down the http/https-only contract and reject cases (invalid URL,ws:///wss://, missing scheme).As per coding guidelines: "Co-locate tests in their modules under `mod tests` blocks with descriptive names".💚 Suggested test
#[test] fn test_validate_mint_url() { assert!(validate_mint_url("https://mint.example.com").is_ok()); assert!(validate_mint_url("http://localhost:3338").is_ok()); assert!(validate_mint_url("wss://mint.example.com").is_err()); assert!(validate_mint_url("not a url").is_err()); assert!(validate_mint_url("").is_err()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/config/wizard.rs` around lines 378 - 388, Add a co-located unit test for the public function validate_mint_url in the same module: create a mod tests block with a descriptive test function (e.g., test_validate_mint_url) that asserts ok for "https://mint.example.com" and "http://localhost:3338" and asserts Err for "wss://mint.example.com", "not a url", and the empty string to lock down the http/https-only contract; place the test alongside the other validator tests in this file and run cargo test to verify.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/config/util.rs`:
- Around line 80-98: The current validation uses nostr_sdk::Url::parse and
.scheme() which are not available in nostr-sdk 0.44.1; update the block that
checks settings.cashu (and the cashu.mint_url variable) to parse the mint_url
with url::Url::parse instead and then call url.scheme() to validate
"http"/"https" (preserve the same error messages and
MostroInternalErr/ServiceError::IOError handling), i.e., replace
nostr_sdk::Url::parse(...) and parsed.scheme() usages with
url::Url::parse(&cashu.mint_url) and url.scheme() respectively.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 142-155: The scheduler is started before LN_STATUS is ever set,
causing job_info_event_send to call LN_STATUS.get().unwrap() and panic in Cashu
mode or race in Lightning mode; fix by either (A) moving
start_scheduler(ctx.clone()).await in main.rs to after LN_STATUS.set(ln_status)
(i.e., after LndConnector::new().await, ln_client.get_node_info().await and
LnStatus::from_get_info_response are used and LN_STATUS.set(...) succeeds), or
(B) make job_info_event_send/its caller guard LN_STATUS.get() access (check
Settings::is_cashu_enabled() or handle None instead of unwrap) so Cashu mode and
pre-set scheduler startup do not unwrap a missing LN_STATUS. Ensure references
to start_scheduler, job_info_event_send, LN_STATUS.set,
Settings::is_cashu_enabled, run_cashu, and LndConnector::new are updated
accordingly.
---
Nitpick comments:
In `@src/app.rs`:
- Around line 398-484: run_cashu duplicates the entire run event loop (POW
check, unwrap_message, replay window, inner-signature check, check_trade_index)
differing only by the dispatch call; extract the shared logic into a single
parameterized loop and call it from both modes. Create a new function (e.g.,
run_loop or similar) that takes AppContext plus a dispatch-mode parameter (an
enum like EventMode or a closure) and moves the common validation and event
iteration there, keeping the existing symbols unwrap_message, check_trade_index,
and inner_message verification unchanged; then have run call the new loop with a
mode that invokes handle_message_action and run_cashu call it with a mode that
invokes handle_message_action_no_ln. Ensure error handling (manage_errors,
warning_msg, MostroError downcast) remains identical inside the shared loop.
In `@src/config/wizard.rs`:
- Around line 378-388: Add a co-located unit test for the public function
validate_mint_url in the same module: create a mod tests block with a
descriptive test function (e.g., test_validate_mint_url) that asserts ok for
"https://mint.example.com" and "http://localhost:3338" and asserts Err for
"wss://mint.example.com", "not a url", and the empty string to lock down the
http/https-only contract; place the test alongside the other validator tests in
this file and run cargo test to verify.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d8342ab-614e-4546-9753-0d7254cf999c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
Cargo.tomlsettings.tpl.tomlsrc/app.rssrc/app/context.rssrc/app/dev_fee.rssrc/app/rate_user.rssrc/config/mod.rssrc/config/settings.rssrc/config/types.rssrc/config/util.rssrc/config/wizard.rssrc/lightning/mod.rssrc/main.rs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78120fcc9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
Critical
- Cashu mode is not usable yet: the new no-LN loop still delegates
TakeBuy/TakeSell/AddInvoiceinto the legacy helpers, and those helpers eventually callshow_hold_invoice()/LndConnector::new(). A no-LND Cashu deployment will still fail on the normal trade path instead of using Cashu escrow.
Warnings
- The full scheduler still starts before the mode check, so LND-backed jobs are spawned in Cashu mode as well.
job_process_bond_payouts()will keep retrying a Lightning connection in the background when LND is absent.
Looks Good
- Cashu config validation is doing the right thing: it rejects invalid URLs, enforces
http/https, and blockscashu+anti_abuse_bondtogether.
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
Cashu mode is not ready to merge yet. The new boot path still isn't truly LND-free, and the normal trade flow in Cashu mode still routes into Lightning-only helpers.
Please gate the LND-backed scheduler jobs and replace the remaining Lightning hold-invoice path before approval.
Block TakeSell/TakeBuy/AddInvoice in run_cashu event loop with CantDo(InvalidAction) until escrow wiring lands (F4+). Skip job_process_dev_fee_payment and job_process_bond_payouts when is_cashu_enabled() so they don't attempt LND calls in Cashu-only mode.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be874b9802
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } else { | ||
| println!("\n--- Cashu Mint Configuration ---\n"); | ||
| let cashu = prompt_cashu_settings()?; | ||
| (LightningSettings::default(), Some(cashu)) |
There was a problem hiding this comment.
When the guided wizard is used to create a Cashu config, this stores LightningSettings::default(), whose payment_retries_interval is 0. Cashu startup still calls start_scheduler before entering run_cashu, and job_retry_failed_payments unconditionally uses that lightning interval in tokio::time::sleep(Duration::from_secs(interval)); with the wizard-generated Cashu config this becomes a zero-duration sleep, causing a tight DB polling/logging loop and possible repeated LND attempts if failed payments exist. Populate the normal lightning scheduler defaults even for Cashu, or skip the LN-only scheduler jobs in Cashu mode.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review Summary
Verdict: Changes Requested
You fixed the two big blockers from the previous revision: Cashu trade actions are no longer routed into the Lightning hold-invoice path, and the two LND-heavy scheduler jobs are now skipped in Cashu mode. Good.
There are still two issues I would want changed before approving:
job_cancel_orders()still initializesLndConnectorunconditionally during scheduler startup, so a Cashu-only node still touches LND on boot.- In Cashu mode,
Release,Cancel,AdminCancel, andAdminSettlestill fall through to the no-op branch instead of returning an explicitCantDo. That is a silent success for escrow-dependent actions.
Please gate or remove the remaining LND startup path, and reject unsupported escrow actions explicitly.
…ip retry_failed_payments and cancel_orders scheduler jobs in Cashu mode Extend the F2 CantDo(InvalidAction) block to Release/Cancel/AdminCancel/AdminSettle (all require LN invoice handling until escrow wiring lands in F4+). Skip job_retry_failed_payments and job_cancel_orders when is_cashu_enabled() so they don't attempt LND calls in Cashu-only mode.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dd7c15c4e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Action::TakeSell | ||
| | Action::TakeBuy | ||
| | Action::AddInvoice | ||
| | Action::Release | ||
| | Action::Cancel | ||
| | Action::AdminCancel | ||
| | Action::AdminSettle => { |
There was a problem hiding this comment.
When Cashu mode is enabled, Action::NewOrder is still routed through handle_message_action_no_ln, so makers can publish orders, but this branch rejects every take/add-invoice path and also rejects Cancel. Those orders are visible on the book but no taker can fill them, and the maker cannot remove a pending order except by waiting for expiry. Either reject NewOrder while the Cashu trade flow is still a stub, or allow the non-LN pending cancel path so users don't create stuck orders.
Useful? React with 👍 / 👎.
Extend the F2 CantDo(InvalidAction) block to include Action::NewOrder. Without escrow backend wired, allowing order creation would produce orders that can never be filled or cancelled, giving peers a confusing experience. Rejecting NewOrder up front provides a clear error instead.
2 participants
Adds the CashuSettings config struct and EscrowMode enum, wires them into startup validation (Cashu and anti-abuse bonds are mutually exclusive; mint_url must be a valid HTTP/HTTPS URL), and branches the daemon boot path: when [cashu] enabled = true the node starts without LND and routes all actions through a no-LN event loop stub. Lightning behavior is entirely unchanged when the [cashu] block is absent. Includes wizard support for escrow mode selection and a commented [cashu] block in the settings template.
Summary by CodeRabbit
Release Notes
New Features
Chores