feat(wallet): Retry logic for Monero wallet#417
Conversation
WalkthroughThis update introduces retry logic with exponential backoff for key wallet operations, enhances logging for wallet actions and transaction confirmations, and adds a method to retrieve the wallet filename via FFI. It also updates the wallet initialization to accept an optional Tauri handle and improves confirmation progress reporting. Changes
Sequence Diagram(s)sequenceDiagram
participant UI/Tauri
participant Wallets
participant FfiWallet
participant MoneroNetwork
UI/Tauri->>Wallets: new(data_dir, daemon_address, env_config, tauri_handle)
Wallets->>FfiWallet: initialize()
loop Retry with backoff
FfiWallet->>MoneroNetwork: init()
alt Success
FfiWallet-->>Wallets: Ok
else Failure
FfiWallet-->>Wallets: Error (retry)
end
end
Wallets-->>UI/Tauri: Wallets instance
sequenceDiagram
participant User
participant WalletHandle
participant FfiWallet
participant MoneroNetwork
User->>WalletHandle: transfer/sweep()
loop Retry with backoff
WalletHandle->>FfiWallet: transfer/sweep()
FfiWallet->>MoneroNetwork: send transaction
alt Success
FfiWallet-->>WalletHandle: TxReceipt/Success
else Failure
FfiWallet-->>WalletHandle: Error (retry)
end
end
WalletHandle-->>User: Result
sequenceDiagram
participant Protocol
participant WalletHandle
Protocol->>WalletHandle: wait_until_confirmed(txid, tx_key, address, amount, confirmations, listener)
loop Until confirmations reached
WalletHandle->>WalletHandle: poll confirmation status
WalletHandle-->>Protocol: listener(confirmations)
end
WalletHandle-->>Protocol: Ok
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
monero-sys/src/lib.rs (1)
585-636:wait_until_confirmedleaks sensitive data & may spam listener
Logging the full
txidand recipient address atinfo!level exposes PII on production logs.
Mask at least the middle of the hash or downgrade todebug!.
listener(tx_status.confirmations)is invoked every polling tick, even when the confirmations count has not changed.
An idempotent listener may still perform heavy UI updates or I/O.- if let Some(listener) = &listener { - listener(tx_status.confirmations); + if tx_status.confirmations > last_seen && let Some(listener) = &listener { + listener(tx_status.confirmations); + last_seen = tx_status.confirmations; }Add
let mut last_seen = 0;before the loop.
🧹 Nitpick comments (9)
swap/src/bin/asb.rs (1)
465-472: Pass the actual UI handle where available
Wallets::new(.., false, None)hard-codesNone, meaning the UI can never receive progress updates from CLI mode even when compiled with the Tauri feature.If the CLI can receive a
TauriHandle(e.g. throughenv_configor an optional CLI flag) forward it here; otherwise document why the handle is always unavailable.monero-sys/src/bridge.h (1)
168-171: Correct wrapper but missing doc-comment parity
walletFilenamefollows the existing wrapper pattern. Consider adding a one-liner comment (like the neighbouring wrappers) for consistency/readability.swap/src/monero/wallet.rs (1)
15-16: Unused imports will triggerunused_importswarnings
TauriBackgroundProgress,TauriEmitterandTauriHandleare imported but onlyTauriHandleis stored.
Either remove/#[allow(unused_imports)]the extras or use them; many CI pipelines treat warnings as errors.swap/src/protocol/alice/swap.rs (1)
11-12: Remove now-unusedno_listenerimportWith the new listener closure passed to
wait_until_confirmed,no_listeneris no longer referenced in this module. Keeping an unused import will triggerunused_importswarnings (and breaks the build if#![deny(warnings)]is enabled).-use crate::monero::wallet::no_listener;swap/src/protocol/alice/state.rs (1)
578-583: Per-confirmation debug log may be too chattySame remark as in
swap.rs: the closure passed here fires once per new confirmation while waiting for 10 blocks.
If log volume becomes a concern you may want to:Some(|c| { if c % 5 == 0 { // every 5 confirmations tracing::info!(%c, "Monero lock tx confirmations"); } })or downgrade to
trace!.
Purely cosmetic; functionality is correct.swap/tests/harness/mod.rs (1)
310-317: Hard-codedtrueflag – use a named builder for clarityPassing magic booleans (
true,None) intoWallets::newmakes the call hard to read later.
Consider exposing a small builder/helper:Wallets::builder() .regtest(true) .tauri_handle(None) .open(monero_wallet_dir, "main", monero_daemon, monero::Network::Mainnet) .await?Not critical for test code, but aids readability.
monero-sys/src/lib.rs (3)
20-22: Remove unusedretryimport
retryis imported but never referenced after the latest changes. This will trigger adead_code/unused_importslint incargo check.-use backoff::{future, retry, retry_notify}; +use backoff::{future, retry_notify};
1002-1013:retry_notifycaptureswalletby mutable borrow – safe but brittleThe closure mutably borrows
walletacross retry attempts.
While this compiles (because the closure isFnMut), a non-obvious lifetime arises: if a future insideinitholds a borrow across an.await, subsequent retries will dead-lock borrow checking at compile time.Recommend keeping the mutable borrow inside the closure:
- || { - wallet - .init(&daemon.address, daemon.ssl) + || { + let result = wallet.init(&daemon.address, daemon.ssl); + // never holds the &mut borrow across .await boundaries + resultNo functional change, but easier to reason about.
1667-1683: Localbackoff()helper shadows thebackoffcrate – rename for clarityHaving both
use backoff::…and a free function calledbackoffin the same module is confusing for readers and IDE tooling.-fn backoff( +fn build_backoff(Update the three call-sites accordingly.
📜 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 (11)
.vscode/settings.json(0 hunks)monero-sys/Cargo.toml(1 hunks)monero-sys/src/bridge.h(1 hunks)monero-sys/src/bridge.rs(1 hunks)monero-sys/src/lib.rs(21 hunks)swap/src/bin/asb.rs(1 hunks)swap/src/cli/api.rs(2 hunks)swap/src/monero/wallet.rs(3 hunks)swap/src/protocol/alice/state.rs(1 hunks)swap/src/protocol/alice/swap.rs(1 hunks)swap/tests/harness/mod.rs(3 hunks)
💤 Files with no reviewable changes (1)
- .vscode/settings.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
monero-sys/src/bridge.rs (1)
monero-sys/src/bridge.h (2)
walletFilename(168-171)walletFilename(168-168)
swap/tests/harness/mod.rs (2)
swap/src/monero/wallet.rs (1)
new(64-97)monero-harness/src/lib.rs (4)
new(57-134)new(355-377)new(414-439)wallet(140-148)
🔇 Additional comments (6)
monero-sys/Cargo.toml (1)
8-8: Pinning to0.4.0may break builds on crates-io mirrors
backoff = "0.4.0"is valid today, but several mirrors only host the latest patch (0.4.x).
Suggest relaxing to the compatible spec to avoid build failures and automatically receive patch-level fixes:-backoff = "0.4.0" +backoff = "0.4"Please run
cargo update -p backoffafterwards to lock the latest patch inCargo.lock.monero-sys/src/bridge.rs (1)
129-131: Looks good – FFI surface stays consistent
walletFilenamemirrorswalletPathand returns an ownedCxxString, matching CXX expectations.
No issues spotted.swap/src/monero/wallet.rs (1)
70-71: ```shell
#!/bin/bashEscape parentheses in the rg pattern to avoid the “unclosed group” error
rg --context 2 'Wallets::new(' swap | grep -v ', None'
</details> <details> <summary>swap/src/protocol/alice/swap.rs (1)</summary> `304-311`: **Logging callback looks good, but consider throttling** The added info/debug logs are helpful, however `wait_until_confirmed` polls frequently and the closure will emit a `tracing::debug!` on *every* confirmation height update (every block). If many swaps run concurrently this can flood logs. Two lightweight options: 1. Emit only on meaningful milestones, e.g. every `n` confirmations. 2. Swap `debug!` for `trace!` to reduce default verbosity. No functional issue—purely a log-noise consideration. [ suggest_nitpick ] </details> <details> <summary>swap/src/cli/api.rs (2)</summary> `583-590`: **Propagating `tauri_handle` into `Wallets::new` looks correct** Good to see the handle forwarded; this keeps UI progress consistent with Bitcoin wallet initialisation. --- `538-540`: **Parameter rename is fine – check callers for outdated `_tauri_handle`** You renamed the argument to `tauri_handle`. All in-repo callers seem updated, but please verify with: ```shell rg -n "_tauri_handle" swap | headto ensure no stale call sites remain.
binarybaron
changed the title
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores
backoffcrate.