Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change introduces support for splitting Monero swap payouts across multiple addresses with configurable percentages. It adds new abstractions for labeled address pools, updates the database schema and protocol to handle multiple addresses, and implements multi-address sweep functionality in both the Rust and C++ FFI layers. Related application logic, CLI, and GUI code are refactored to use address pools instead of single addresses. Changes
Sequence Diagram(s)sequenceDiagram
participant GUI
participant CLI
participant Protocol
participant DB
participant MoneroWallet
participant FFI
GUI->>CLI: buyXmr({monero_receive_pool: [...]})
CLI->>Protocol: buy_xmr(monero_receive_pool)
Protocol->>DB: insert_monero_address_pool(swap_id, monero_receive_pool)
Protocol->>MoneroWallet: sweep_multi(addresses, ratios)
MoneroWallet->>FFI: createTransactionMultiDest(addresses, amounts)
FFI-->>MoneroWallet: PendingTransaction*
MoneroWallet->>FFI: publish(PendingTransaction*)
FFI-->>MoneroWallet: tx_ids
MoneroWallet->>Protocol: tx_ids
Protocol->>CLI: swap_progress_event(monero_receive_pool)
CLI->>GUI: swapProgressEvent(monero_receive_pool)
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (40)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Einliterflasche
force-pushed
the
feat/voluntary-donate
branch
from
d3315d2 to
5ed30b0
Compare
|
@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: 13
🔭 Outside diff range comments (1)
swap/migrations/20250530215011_multiple_monero_receive_addresses.sql (1)
6-30: Schema allows duplicates & lacks percentage-sum enforcement
- Dropping the primary key without adding a replacement means the same
(swap_id, address)pair can be inserted multiple times.- Nothing enforces that Σ percentage = 1 for a given
swap_id.Consider:
-- composite PK prevents duplicates CREATE TABLE monero_addresses_temp ( swap_id TEXT NOT NULL, address TEXT NOT NULL, percentage REAL NOT NULL CHECK (percentage > 0 AND percentage <= 1), label TEXT NOT NULL DEFAULT 'user address', PRIMARY KEY (swap_id, address) ); -- Optional but recommended: guarantee the pool sums to 1 -- via trigger CREATE TRIGGER trg_monero_percentage_sum AFTER INSERT OR UPDATE ON monero_addresses BEGIN DELETE FROM monero_addresses_error; -- optional helper SELECT CASE WHEN (SELECT ROUND(SUM(percentage), 5) FROM monero_addresses WHERE swap_id = NEW.swap_id) <> 1.0 THEN RAISE(ABORT, 'Percentages must sum to 1') END; END;This keeps data consistent and prevents subtle runtime errors.
♻️ Duplicate comments (2)
swap/src/protocol/bob/swap.rs (2)
720-729: Same payload mismatch in cooperative-redeem pathRepeat of the issue above – ensure the Tauri event schema is consistently updated.
795-796: And again for XmrRedeemed terminal stateUpdate or wrap the pool into a displayable representation before emitting.
🧹 Nitpick comments (7)
.vscode/settings.json (1)
1-71: Out-of-scope editor configuration change.The modifications to VSCode file associations are unrelated to the “voluntary donate” GUI feature. Consider isolating or removing this commit to keep the PR focused on the donation functionality.
swap/src/cli/command.rs (1)
71-75: Consider validating the percentage-sum invariant when constructing pools
monero_receive_poolis blindly created from a single address.
Down-stream layers rely on the invariant “percentages per swap sum to 1”.
Even though this pool currently contains exactly one 100 % entry, an early sanity–check here would prevent accidental creation of ill-formed pools in the future when CLI parameters are extended (e.g. via--donate).let monero_receive_pool: MoneroAddressPool = - monero_address::validate_is_testnet(monero_receive_address, is_testnet)?.into(); + monero_address::validate_is_testnet(monero_receive_address, is_testnet)? + .into() + .ensure_percentage_sum()?;(Assumes a lightweight
ensure_percentage_sum()helper returningResult).
Adding this guard keeps the CLI the first line of defence.swap/src/cli/api/request.rs (1)
650-660: Legacy swaps will panic after DB migration
insert_monero_address_poolis always invoked for new swaps, butresume_swapblindly callsget_monero_address_pool.
Swaps created before the migration only have a single address entry → the DB will returnNotFoundand the resume command aborts.Consider:
let monero_receive_pool = context .db .get_monero_address_pool(swap_id) .await .or_else(|_| context.db.get_legacy_monero_address(swap_id).await.map(|a| a.into_pool()))?;or present a clear error message instructing users to upgrade / re-start the swap.
swap/src/protocol/bob.rs (1)
25-28:monero_receive_poolis stored only in memory
Swap::newreceives the pool but does not persist it – persistence relies on the earlier call site toinsert_monero_address_pool.
If another constructor is added later and forgets that DB write, we create silent inconsistencies.Add an invariant or helper:
debug_assert!( db.has_monero_address_pool(id).await.expect("db") , "monero_receive_pool must be inserted into DB before Swap::new" );or make
Swap::newperform the insertion itself, guaranteeing consistency.swap/src/database/sqlite.rs (1)
112-116: Avoid Decimal → f64 round-tripConverting
Decimaltof64(to_f64) and back (from_f64) sacrifices precision and can introduce rounding errors for small donation percentages.Persist the exact string (
Decimal::to_string()) or the underlying integer representation instead.monero-sys/src/lib.rs (2)
22-24: Duplicate import
use cxx::let_cxx_string;is already imported on the previous line – drop the second import.
419-429: Bring sweep_multi in line with existing retry strategy
transferandsweepare wrapped inretry_notify, butsweep_multiis not.
Add the same retry/back-off pattern for behavioural consistency.
📜 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 (18)
.vscode/settings.json(1 hunks)monero-sys/src/bridge.h(2 hunks)monero-sys/src/bridge.rs(1 hunks)monero-sys/src/lib.rs(3 hunks)src-gui/src/renderer/rpc.ts(2 hunks)swap/.sqlx/query-50a5764546f69c118fa0b64120da50f51073d36257d49768de99ff863e3511e0.json(0 hunks)swap/.sqlx/query-ce270dd4a4b9615695a79864240c5401e2122077365e5e5a19408c068c7f9454.json(0 hunks)swap/migrations/20250530215011_multiple_monero_receive_addresses.sql(1 hunks)swap/src/cli/api/request.rs(7 hunks)swap/src/cli/api/tauri_bindings.rs(2 hunks)swap/src/cli/command.rs(3 hunks)swap/src/database/sqlite.rs(3 hunks)swap/src/monero.rs(1 hunks)swap/src/protocol.rs(2 hunks)swap/src/protocol/bob.rs(6 hunks)swap/src/protocol/bob/state.rs(3 hunks)swap/src/protocol/bob/swap.rs(6 hunks)swap/tests/harness/mod.rs(2 hunks)
💤 Files with no reviewable changes (2)
- swap/.sqlx/query-ce270dd4a4b9615695a79864240c5401e2122077365e5e5a19408c068c7f9454.json
- swap/.sqlx/query-50a5764546f69c118fa0b64120da50f51073d36257d49768de99ff863e3511e0.json
🧰 Additional context used
🧬 Code Graph Analysis (5)
swap/src/cli/command.rs (1)
swap/src/monero.rs (1)
validate_is_testnet(577-587)
src-gui/src/renderer/rpc.ts (1)
src-gui/src/utils/multiAddrUtils.ts (1)
providerToConcatenatedMultiAddr(10-14)
monero-sys/src/bridge.rs (1)
monero-sys/src/bridge.h (4)
createMultiSweepTransaction(132-171)createMultiSweepTransaction(132-135)vector_string_push_back(212-217)vector_string_push_back(212-214)
swap/src/database/sqlite.rs (2)
swap/src/protocol.rs (2)
insert_monero_address_pool(144-148)get_monero_address_pool(149-149)swap/src/monero.rs (7)
address(242-244)addresses(275-277)percentage(246-248)label(250-252)new(234-240)new(262-264)new(411-413)
monero-sys/src/lib.rs (2)
monero-sys/src/bridge.h (6)
vector_string_push_back(212-217)vector_string_push_back(212-214)createMultiSweepTransaction(132-171)createMultiSweepTransaction(132-135)pendingTransactionTxIds(204-207)pendingTransactionTxIds(204-204)monero-sys/src/bridge.rs (3)
vector_string_push_back(242-242)createMultiSweepTransaction(236-240)pendingTransactionTxIds(254-256)
🪛 Biome (1.9.4)
.vscode/settings.json
[error] 71-71: Expected a property but instead found '}'.
Expected a property here.
(parse)
[error] 71-71: expected } but instead the file ends
the file ends here
(parse)
🔇 Additional comments (6)
swap/tests/harness/mod.rs (1)
495-501: LGTM – adapts toMoneroAddressPoolAPIThe additional
.into()calls correctly convert the validatedmonero::Addressinto the new pool type.
No further issues spotted.swap/src/protocol/bob/state.rs (1)
720-752: ```shell
#!/bin/bash
set -eLocate LabeledMoneroAddress definition
rg -n "struct LabeledMoneroAddress" -n swap/src/monero.rs -A5
</details> <details> <summary>swap/src/protocol.rs (1)</summary> `140-150`: **Trait change breaks implementors without a default impl** All DB back-ends must now implement `insert_monero_address_pool` / `get_monero_address_pool`. Make sure `sqlite.rs`, `mock_db.rs` (tests) and any out-of-tree adapters are updated; otherwise compilation fails. If you want partial compatibility, provide default implementations that return `todo!()` so downstream crates fail at runtime instead of compile-time. </details> <details> <summary>swap/src/database/sqlite.rs (1)</summary> `515-530`: **Restore coverage for the new multi-address API** The old single-address test was commented out and not replaced. Add tests exercising `insert_monero_address_pool`/`get_monero_address_pool`, asserting round-trip integrity and percentage accuracy. </details> <details> <summary>swap/src/protocol/bob/swap.rs (1)</summary> `517-526`: **Event payload now mismatches the type expected by the frontend** `TauriSwapProgressEvent::XmrRedeemInMempool { xmr_redeem_address, … }` previously carried a `monero::Address`. You now pass `MoneroAddressPool`, which will not compile unless the enum and UI were updated accordingly. Verify the event definition and adjust the field name/documentation. </details> <details> <summary>swap/src/monero.rs (1)</summary> `266-273`: [web_search] ```web Does rust_decimal PartialEq consider scale when comparing Decimal values?
src-gui/src/renderer/rpc.ts
Outdated
| const DONATE_PERCENTAGE = 0.1; | ||
|
|
||
| export async function buyXmr( |
There was a problem hiding this comment.
Donation percentage off by two orders of magnitude
PR description: “voluntarily donate 0.1 % of the swap amount”.
DONATE_PERCENTAGE = 0.1 represents 10 %, not 0.1 %. Users will unintentionally donate 100× more.
-const DONATE_PERCENTAGE = 0.1;
+// 0.1 % expressed as a fraction
+const DONATE_PERCENTAGE = 0.001;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const DONATE_PERCENTAGE = 0.1; | |
| export async function buyXmr( | |
| // 0.1 % expressed as a fraction | |
| const DONATE_PERCENTAGE = 0.001; | |
| export async function buyXmr( |
🤖 Prompt for AI Agents
In src-gui/src/renderer/rpc.ts around lines 141 to 143, the DONATE_PERCENTAGE
constant is set to 0.1, which represents 10%, but the intended donation
percentage is 0.1%. Fix this by changing the value of DONATE_PERCENTAGE to 0.001
to correctly represent 0.1%.
| fn vector_string_push_back(v: Pin<&mut CxxVector<CxxString>>, s: &CxxString); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
vector_string_push_back mutates a pinned vector – check pin-safety
Pin<&mut CxxVector<_>> promises the vector won’t be re-allocated, yet pushing can re-allocate.
cxx internally allows this but only for an unpinned &mut CxxVector. Either:
- Accept
&mut CxxVector<CxxString>(likeCxxVector::push) or - Document why re-allocation is impossible here (e.g. the vector was pre-reserved).
Otherwise this is unsound.
🤖 Prompt for AI Agents
In monero-sys/src/bridge.rs around lines 242 to 243, the function
vector_string_push_back takes a Pin<&mut CxxVector<CxxString>>, which promises
no re-allocation, but pushing can cause re-allocation making this unsafe. To
fix, change the parameter to accept a plain &mut CxxVector<CxxString> instead of
a pinned reference, matching the signature of CxxVector::push, or add clear
documentation explaining why re-allocation cannot happen here (e.g., the vector
is pre-reserved and cannot grow).
swap/src/monero.rs
Outdated
| // Iterate over all but the last address | ||
| for address in first_addresses.iter() { | ||
| // Calculate the amount for this address | ||
| let split_amount = address.percentage.saturating_mul(amount_decimal).floor(); | ||
|
|
||
| // Convert the Decimal to a u64 | ||
| // We can use expect because we know that the Decimal is floored | ||
| let split_amount = split_amount | ||
| .to_u64() | ||
| .expect("Floored Decimal should be representable as u64"); | ||
|
|
||
| let split_amount = monero::Amount::from_pico(split_amount); | ||
| result.push((address.address, split_amount)); | ||
| } | ||
|
|
||
| // Give the remainder to the last address | ||
| let distributed_amount = | ||
| Amount::from_piconero(result.iter().map(|(_, amount)| amount.as_pico()).sum()); | ||
| let remainder = amount - distributed_amount.into(); | ||
|
|
||
| result.push((last_address.address, remainder)); | ||
|
|
There was a problem hiding this comment.
Borrow-checker violation – moving address out of a &LabeledMoneroAddress
address.address moves a non-Copy field out of a shared reference and will not compile.
- result.push((address.address, split_amount));
+ result.push((address.address(), split_amount));
…
- result.push((last_address.address, remainder));
+ result.push((last_address.address(), remainder));Compile failure example:
cannot move out of `*address` which is behind a shared reference
🤖 Prompt for AI Agents
In swap/src/monero.rs around lines 311 to 332, the code attempts to move the
non-Copy field `address.address` out of a shared reference, causing a
borrow-checker error. To fix this, change the code to clone or otherwise copy
the `address.address` value instead of moving it, for example by calling
`.clone()` on `address.address` when pushing it into the result vector. This
preserves the original reference and satisfies the borrow checker.
monero-sys/src/bridge.h
Outdated
| inline PendingTransaction *createMultiSweepTransaction( | ||
| Wallet &wallet, | ||
| const std::vector<std::string> &dest_addresses, | ||
| const std::vector<double> &sweep_ratios) | ||
| { | ||
| size_t N = dest_addresses.size(); | ||
| if (N == 0 || sweep_ratios.size() != N) | ||
| return nullptr; | ||
|
|
||
| uint64_t bal = wallet.unlockedBalance(); | ||
| std::vector<uint64_t> amounts(N); | ||
| uint64_t sum = 0; | ||
| for (size_t i = 0; i + 1 < N; ++i) | ||
| { | ||
| amounts[i] = static_cast<uint64_t>(std::floor(bal * sweep_ratios[i])); | ||
| sum += amounts[i]; | ||
| } | ||
| // reserve last slot for fee carve‐out | ||
| amounts[N - 1] = 0; | ||
|
|
||
| // build temporary dest list for fee estimation | ||
| std::vector<std::pair<std::string, uint64_t>> fee_dests; | ||
| fee_dests.reserve(N); | ||
| for (size_t i = 0; i < N; ++i) | ||
| fee_dests.emplace_back(dest_addresses[i], amounts[i]); | ||
|
|
||
| // ask wallet to estimate fee assuming N outputs + 1 change | ||
| uint64_t fee = wallet.estimateTransactionFee(fee_dests, PendingTransaction::Priority_Default); | ||
|
|
||
| // carve the fee out of the last output, so sum(amounts)+fee == balance | ||
| amounts[N - 1] = bal - sum - fee; | ||
|
|
||
| // build the actual multi‐dest transaction (no change left ⇒ wallet drops it) | ||
| return wallet.createTransactionMultDest( | ||
| dest_addresses, | ||
| /*payment_id=*/"", | ||
| Monero::optional<std::vector<uint64_t>>(amounts), | ||
| /*mixin_count=*/0, | ||
| PendingTransaction::Priority_Default); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential underflow & unchecked invariants in createMultiSweepTransaction
sweep_ratiosare not validated to be positive and to sum to 1.amounts[N-1] = bal - sum - fee;will underflow (wrap toUINT64_MAX) whenfee + sum > bal, e.g. high fee or rounding error.- Single-destination sweeps (
N == 1) still estimate the fee withamounts[0] = 0, likely producing an incorrect/negative final amount.
Recommendation:
if (N == 0) return nullptr;
double ratio_sum = std::accumulate(sweep_ratios.begin(), sweep_ratios.end(), 0.0);
if (std::fabs(ratio_sum - 1.0) > 1e-12) return nullptr;
/* … */
if (fee + sum > bal) return nullptr; // or clamp / re-estimate
Consider early-return or throwing an error instead of silently producing an invalid transaction.
🤖 Prompt for AI Agents
In monero-sys/src/bridge.h between lines 132 and 171, the
createMultiSweepTransaction function lacks validation for sweep_ratios being
positive and summing to 1, risks underflow when fee plus sum exceeds balance,
and mishandles single-destination cases by estimating fees with zero amounts. To
fix this, add checks to ensure N is not zero, verify that the sum of
sweep_ratios is approximately 1, and confirm that fee plus sum does not exceed
the balance before subtracting; if these conditions fail, return nullptr or
handle the error appropriately. Also, adjust the fee estimation logic for
single-destination sweeps to avoid incorrect amounts.
monero-sys/src/bridge.h
Outdated
| return nullptr; | ||
| } | ||
|
|
||
| // To estimate the correct fee we build a transation that spends 1 piconero |
There was a problem hiding this comment.
this will fail if we have less piconeros than addresses to spend to.
add a sanity check to fail if we have less piconeros than sweep addresses.
monero-sys/src/lib.rs
Outdated
| bail!("Number of addresses and ratios must match") | ||
| } | ||
|
|
||
| const TOLERANCE: f64 = 1e-12; |
There was a problem hiding this comment.
With a swap amount of $100k this allows for a tolerance of: $0.0000001
Really no need to be this precise. Let's allow for more tolerance.
You could also scale the ratios to 1, meaning if they sum to 0.9, you scale the ratios such that they add up to 1
src-gui/src/renderer/rpc.ts
Outdated
| seller: providerToConcatenatedMultiAddr(seller), | ||
| monero_receive_pool: [ | ||
| { | ||
| address: monero_receive_address, |
There was a problem hiding this comment.
Use the donation address here
Use the configured donation percentage from the settings
| label: String, | ||
| } | ||
|
|
||
| impl LabeledMoneroAddress { |
binarybaron
changed the title
|
@Einliterflasche is this ready for review? Have you tested it yet? |
|
Tests still failing, not really ready for review, except maybe the distribute function /tests |
monero-sys/src/bridge.h
Outdated
| uint64_t num_outputs) | ||
| { | ||
| std::vector<std::pair<std::string, uint64_t>> fee_dests; | ||
| for (uint64_t i = 0; i < num_outputs; ++i) |
There was a problem hiding this comment.
Wrong, we need to:
- ensure balance (in piconeros is > num_outputs)
- drop the last address because wallet2 will create a change output. The change output will only be dropped if there is no XMR left after subtracting the fee
| // Check if we have any destinations at all | ||
| if (n == 0) | ||
| { | ||
| // wallet.setStatusError("Number of destinations must be greater than 0"); |
|
thread 'fund_transfer_and_check_tx_key' panicked at monero-harness/tests/wallet.rs:88:10: Caused by: Clearly an issue with the implementation, not just the test |
…reads, use new subtract_fee wallet2 functionality
|
|
||
| let state5 = state.attempt_cooperative_redeem(s_a, lock_transfer_proof); | ||
|
|
||
| match retry( |
swap/src/protocol/bob/swap.rs
Outdated
| use uuid::Uuid; | ||
|
|
||
| const PRE_BTC_LOCK_APPROVAL_TIMEOUT_SECS: u64 = 120; | ||
| const PRE_BTC_LOCK_APPROVAL_TIMEOUT_SECS: u64 = 120 * 10; |
swap/src/database/sqlite.rs
Outdated
| #[tokio::test] | ||
| async fn test_insert_load_monero_address() -> Result<()> { | ||
| let db = setup_test_db().await?; | ||
| // #[tokio::test] |
…stagenet and mainnet donaiton address
swap/src/protocol/bob/swap.rs
Outdated
| ) | ||
| .await | ||
| .context("Failed to redeem Monero")?; | ||
| let xmr_redeem_txids = state |
|
@coderabbitai review this |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Wallet Catch-Up Warning Misfires
The wait_for_wallet_to_catch_up function unconditionally logs a warning "Wallet {} not caught up to expected balance of {}" at its conclusion. This is misleading as it occurs even when the wallet successfully reaches the expected balance, implying a failure that did not occur. The warning should only be logged if the wallet fails to catch up after all retry attempts.
monero-harness/tests/wallet.rs#L128-L134
Bug: Percentage Scale Mismatch Causes Validation Errors
The percentage scale is inconsistent across the application. The GUI sends percentages in a 0-100 scale, but the LabeledMoneroAddress::new() function validates them as 0-1. Furthermore, the monero-sys distribute() function expects percentages that sum to 100. This mismatch leads to runtime validation errors when creating labeled addresses and distributing funds.
swap/src/monero.rs#L252-L267
src-gui/src/renderer/rpc.ts#L179-L196
BugBot free trial expires on July 22, 2025
You have used $0.00 of your $0.00 spend limit so far. Manage your spend limit in the Cursor dashboard.
Was this report helpful? Give feedback by reacting with 👍 or 👎
2 participants
Add a button/switch/... that enables users to voluntarily donate a small part (i.e. 0.1%) of their swap to the devs, only if it succeeds
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Database Migration
Bug Fixes
Build and Testing