Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces comprehensive Monero wallet integration, including transaction history, wallet state management, and event-driven UI updates across backend, database, and frontend layers. A new throttling utility crate is added. The GUI now features a Monero wallet page, transaction sending, approval dialogs, and real-time wallet status. Backend APIs, Tauri commands, and database migrations support wallet usage tracking and approval workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend (React)
participant Tauri as Tauri Backend
participant Swap as Rust Core/Swap
participant Monero as Monero Wallet/FFI
participant DB as SQLite DB
%% Initialization
UI->>Tauri: initialize_context()
Tauri->>Swap: ContextBuilder::build()
Swap->>DB: Database::new() / get_recent_wallets()
Swap->>UI: request_seed_selection_with_recent_wallets()
UI->>Swap: SeedChoice (RandomSeed/FromSeed/FromWalletPath)
Swap->>Monero: Open/Create Wallet (with password if needed)
Monero->>Swap: WalletHandle
Swap->>DB: record_wallet_access(wallet_path)
Swap->>Tauri: Context ready
%% Wallet State Fetch
UI->>Tauri: get_monero_main_address()
Tauri->>Swap: GetMoneroMainAddressArgs
Swap->>Monero: get_main_address()
Monero->>Swap: Address
Swap->>Tauri: Response
Tauri->>UI: Address
UI->>Tauri: get_monero_balance()
Tauri->>Swap: GetMoneroBalanceArgs
Swap->>Monero: get_balance()
Monero->>Swap: Balances
Swap->>Tauri: Response
Tauri->>UI: Balances
UI->>Tauri: get_monero_history()
Tauri->>Swap: GetMoneroHistoryArgs
Swap->>Monero: get_history()
Monero->>Swap: Transactions
Swap->>Tauri: Response
Tauri->>UI: Transactions
%% Sending Transaction
UI->>Tauri: send_monero(address, amount)
Tauri->>Swap: SendMoneroArgs
Swap->>UI: request_approval(SendMoneroDetails)
UI->>Swap: resolve_approval(request_id, accept/reject)
alt accepted
Swap->>Monero: transfer_with_approval()
Monero->>Swap: TxReceipt
Swap->>Tauri: SendMoneroResponse
Tauri->>UI: Success
else rejected
Swap->>Tauri: SendMoneroResponse (rejected)
Tauri->>UI: Failure
end
%% Wallet Event Updates
Monero-->>Swap: WalletListener: balance/sync/history update
Swap-->>Tauri: emit MoneroWalletUpdate event
Tauri-->>UI: MoneroWalletUpdate event (Redux update)
Poem
✨ Finishing Touches
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. 🪧 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 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: 4
🧹 Nitpick comments (3)
.vscode/settings.json (1)
75-76: No-op positional change – consider avoiding diff noiseRe-ordering keys inside the JSON object has zero functional impact but clutters the history. Unless a formatter enforces this ordering, you can drop this hunk to keep the PR focused on the Monero wallet feature.
src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx (1)
15-46: Clean up commented code for production readiness.The data fetching logic is well-implemented with proper error handling and loading states. However, consider:
- Remove the commented test error line (line 18) before production
- Either implement the transaction history feature or remove the commented code (lines 21-25, 34-35) to avoid confusion
Apply this diff to clean up the commented code:
- // throw new Error("test"); setIsLoading(true); setError(null); - /*const [historyResponse, addressResponse]: [GetMoneroHistoryResponse, GetMoneroMainAddressResponse] = await Promise.all([ - getMoneroHistory(), - getMoneroMainAddress(), - ]); - setHistory(historyResponse.transactions);*/ // Fetch all data in parallel if desired, or sequentially const mainAddressResponse = await getMoneroMainAddress(); setMainAddress(mainAddressResponse.address); const balanceResponse = await getMoneroBalance(); setBalance(balanceResponse); - // History fetching can be re-enabled when stable - // const historyResponse = await getMoneroHistory(); - // setHistory(historyResponse.transactions);swap/src/cli/api/request.rs (1)
501-530: Consider concurrent balance queries for better performance.The implementation is correct but could be optimized by making the balance queries concurrent since they're independent operations.
Apply this diff to make the balance queries concurrent:
- let total_balance = wallet.total_balance().await; - let unlocked_balance = wallet.unlocked_balance().await; + let (total_balance, unlocked_balance) = tokio::join!( + wallet.total_balance(), + wallet.unlocked_balance() + );This would reduce the total request time when both balances need to be fetched.
📜 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 (19)
.vscode/settings.json(1 hunks)justfile(2 hunks)monero-rpc/src/monerod.rs(1 hunks)monero-sys/Cargo.toml(1 hunks)monero-sys/src/bridge.h(1 hunks)monero-sys/src/bridge.rs(2 hunks)monero-sys/src/lib.rs(5 hunks)src-gui/package.json(1 hunks)src-gui/src/renderer/components/App.tsx(2 hunks)src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx(2 hunks)src-gui/src/renderer/components/modal/swap/TransactionInfoBox.tsx(2 hunks)src-gui/src/renderer/components/modal/swap/pages/in_progress/SwapSetupInflightPage.tsx(2 hunks)src-gui/src/renderer/components/navigation/NavigationHeader.tsx(2 hunks)src-gui/src/renderer/components/navigation/UnfinishedSwapsCountBadge.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx(1 hunks)src-gui/src/renderer/rpc.ts(2 hunks)src-tauri/src/lib.rs(4 hunks)swap/Cargo.toml(1 hunks)swap/src/cli/api/request.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Always verify method signatures in the Monero C++ headers (wallet2_api.h) before adding them to the Rust bridge to ensure correctness in function names, parameters, and return types.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When adding new functionality to the Rust/C++ bridge, the process should be: locate the function in wallet2_api.h, add its declaration to bridge.rs, implement the Rust wrapper in lib.rs, and add C++ helpers in bridge.h if needed for CXX limitations.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Rust wrapper types (such as WalletManager, Wallet) should provide idiomatic and safe interfaces, never expose raw pointers to users, implement Send and Sync as appropriate, and use Pin for C++ objects requiring stable memory addresses.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The bridge architecture between Rust and Monero C++ should use the cxx::bridge macro to define FFI interfaces, wrap static C++ methods as free functions in the C++ header, and use std::unique_ptr for string returns.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Memory safety is enforced by wrapping raw pointers in safe Rust types, using 'unsafe' only at the FFI boundary, and employing patterns like OnceLock to ensure singletons (e.g., WalletManager).
.vscode/settings.json (1)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
monero-sys/Cargo.toml (3)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The build process should use CMake to target the wallet_api, link both libwallet.a and libwallet_api.a, and configure CXX to bridge Rust and C++ code, ensuring all required static and dynamic libraries are linked.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The bridge architecture between Rust and Monero C++ should use the cxx::bridge macro to define FFI interfaces, wrap static C++ methods as free functions in the C++ header, and use std::unique_ptr for string returns.
src-gui/src/renderer/components/modal/swap/pages/in_progress/SwapSetupInflightPage.tsx (1)
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: src-gui/src/renderer/components/pages/help/SettingsBox.tsx:54-57
Timestamp: 2025-06-15T15:52:41.735Z
Learning: In the UnstoppableSwap GUI codebase, top-level await is used as a convention for infallible RPC calls like getTorForced() and getMatches(), as seen in files like SettingsBox.tsx and config.ts. This is an intentional architectural decision by the team.
src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx (1)
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: src-gui/src/renderer/components/pages/help/SettingsBox.tsx:54-57
Timestamp: 2025-06-15T15:52:41.735Z
Learning: In the UnstoppableSwap GUI codebase, top-level await is used as a convention for infallible RPC calls like getTorForced() and getMatches(), as seen in files like SettingsBox.tsx and config.ts. This is an intentional architectural decision by the team.
monero-rpc/src/monerod.rs (1)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
src-tauri/src/lib.rs (4)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Always verify method signatures in the Monero C++ headers (wallet2_api.h) before adding them to the Rust bridge to ensure correctness in function names, parameters, and return types.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When adding new functionality to the Rust/C++ bridge, the process should be: locate the function in wallet2_api.h, add its declaration to bridge.rs, implement the Rust wrapper in lib.rs, and add C++ helpers in bridge.h if needed for CXX limitations.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The bridge architecture between Rust and Monero C++ should use the cxx::bridge macro to define FFI interfaces, wrap static C++ methods as free functions in the C++ header, and use std::unique_ptr for string returns.
monero-sys/src/bridge.rs (6)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When adding new functionality to the Rust/C++ bridge, the process should be: locate the function in wallet2_api.h, add its declaration to bridge.rs, implement the Rust wrapper in lib.rs, and add C++ helpers in bridge.h if needed for CXX limitations.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Always verify method signatures in the Monero C++ headers (wallet2_api.h) before adding them to the Rust bridge to ensure correctness in function names, parameters, and return types.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The bridge architecture between Rust and Monero C++ should use the cxx::bridge macro to define FFI interfaces, wrap static C++ methods as free functions in the C++ header, and use std::unique_ptr for string returns.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Rust wrapper types (such as WalletManager, Wallet) should provide idiomatic and safe interfaces, never expose raw pointers to users, implement Send and Sync as appropriate, and use Pin for C++ objects requiring stable memory addresses.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Memory safety is enforced by wrapping raw pointers in safe Rust types, using 'unsafe' only at the FFI boundary, and employing patterns like OnceLock to ensure singletons (e.g., WalletManager).
monero-sys/src/bridge.h (3)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Always verify method signatures in the Monero C++ headers (wallet2_api.h) before adding them to the Rust bridge to ensure correctness in function names, parameters, and return types.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When adding new functionality to the Rust/C++ bridge, the process should be: locate the function in wallet2_api.h, add its declaration to bridge.rs, implement the Rust wrapper in lib.rs, and add C++ helpers in bridge.h if needed for CXX limitations.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The bridge architecture between Rust and Monero C++ should use the cxx::bridge macro to define FFI interfaces, wrap static C++ methods as free functions in the C++ header, and use std::unique_ptr for string returns.
src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx (1)
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: src-gui/src/renderer/components/pages/help/SettingsBox.tsx:54-57
Timestamp: 2025-06-15T15:52:41.735Z
Learning: In the UnstoppableSwap GUI codebase, top-level await is used as a convention for infallible RPC calls like getTorForced() and getMatches(), as seen in files like SettingsBox.tsx and config.ts. This is an intentional architectural decision by the team.
src-gui/src/renderer/rpc.ts (1)
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: src-gui/src/renderer/components/pages/help/SettingsBox.tsx:54-57
Timestamp: 2025-06-15T15:52:41.735Z
Learning: In the UnstoppableSwap GUI codebase, top-level await is used as a convention for infallible RPC calls like getTorForced() and getMatches(), as seen in files like SettingsBox.tsx and config.ts. This is an intentional architectural decision by the team.
monero-sys/src/lib.rs (6)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When adding new functionality to the Rust/C++ bridge, the process should be: locate the function in wallet2_api.h, add its declaration to bridge.rs, implement the Rust wrapper in lib.rs, and add C++ helpers in bridge.h if needed for CXX limitations.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Always verify method signatures in the Monero C++ headers (wallet2_api.h) before adding them to the Rust bridge to ensure correctness in function names, parameters, and return types.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Rust wrapper types (such as WalletManager, Wallet) should provide idiomatic and safe interfaces, never expose raw pointers to users, implement Send and Sync as appropriate, and use Pin for C++ objects requiring stable memory addresses.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: The bridge architecture between Rust and Monero C++ should use the cxx::bridge macro to define FFI interfaces, wrap static C++ methods as free functions in the C++ header, and use std::unique_ptr for string returns.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Memory safety is enforced by wrapping raw pointers in safe Rust types, using 'unsafe' only at the FFI boundary, and employing patterns like OnceLock to ensure singletons (e.g., WalletManager).
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When working with Rust FFI to C++ (specifically Monero), use the Rust 2021 edition and restrict the use of 'unsafe' to FFI interactions only.
swap/src/cli/api/request.rs (3)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Always verify method signatures in the Monero C++ headers (wallet2_api.h) before adding them to the Rust bridge to ensure correctness in function names, parameters, and return types.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: When adding new functionality to the Rust/C++ bridge, the process should be: locate the function in wallet2_api.h, add its declaration to bridge.rs, implement the Rust wrapper in lib.rs, and add C++ helpers in bridge.h if needed for CXX limitations.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-23T13:49:46.393Z
Learning: Rust wrapper types (such as WalletManager, Wallet) should provide idiomatic and safe interfaces, never expose raw pointers to users, implement Send and Sync as appropriate, and use Pin for C++ objects requiring stable memory addresses.
🧬 Code Graph Analysis (2)
src-gui/src/renderer/components/App.tsx (1)
src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx (1)
MoneroWalletPage(8-102)
monero-sys/src/bridge.rs (2)
monero-sys/src/bridge.h (2)
walletHistory(232-238)walletHistory(232-232)monero-sys/src/lib.rs (5)
count(1936-1940)transaction(1943-1958)amount(1963-1967)fee(1970-1974)confirmations(1977-1981)
🔇 Additional comments (36)
swap/Cargo.toml (1)
84-84: Verify the typeshare version update is necessary and compatible.Ensure that the typeshare version update from "1.0.3" to "1.0.4" is required for the new Monero wallet functionality and that there are no breaking changes.
#!/bin/bash # Description: Verify typeshare version 1.0.4 exists and check for any breaking changes # Check if version 1.0.4 exists on crates.io curl -s https://crates.io/api/v1/crates/typeshare | jq '.versions[] | select(.num == "1.0.4") | {num, yanked, created_at}' # Check recent versions and any potential breaking changes curl -s https://crates.io/api/v1/crates/typeshare | jq '.versions[] | select(.num | test("^1\\.0\\.[3-5]$")) | {num, yanked, created_at}' | head -10src-gui/src/renderer/components/navigation/NavigationHeader.tsx (2)
10-10: LGTM!The import for AttachMoneyIcon is correctly added and follows the established import pattern.
29-31: LGTM!The new "Monero Wallet" navigation item follows the established pattern and uses an appropriate icon. The route naming is consistent with the existing navigation structure.
src-gui/package.json (1)
7-9: LGTM!Adding the
../monero-sys/srcpath to the typeshare commands is necessary to generate TypeScript bindings for the new Monero wallet FFI types. The change is consistently applied across all related scripts.justfile (2)
29-29: LGTM!Adding the
--verboseflag will provide helpful debugging information during development of the new Monero wallet features.
108-108: LGTM!The trailing newline addition is a good formatting improvement.
src-gui/src/renderer/components/navigation/UnfinishedSwapsCountBadge.tsx (2)
1-1: LGTM!Adding the explicit React import improves compatibility and follows modern React best practices.
8-8: LGTM!Changing the type from
JSX.ElementtoReact.ReactElementprovides better type safety and is more specific. This aligns with similar improvements mentioned in other components.src-gui/src/renderer/components/modal/swap/pages/in_progress/SwapSetupInflightPage.tsx (2)
8-8: LGTM: Proper Theme type import added.Good addition of the explicit
Themeimport to support the type annotation improvement below.
484-484: Excellent type safety improvement.Replacing the
anytype with the specificThemetype from Material-UI provides better type safety and IDE support. This follows TypeScript best practices.monero-sys/Cargo.toml (3)
10-10: LGTM: Dependency formatting improvements.The formatting changes for the existing dependencies improve consistency and readability.
Also applies to: 12-12
11-11: New serde dependency aligns with serialization needs.The addition of
serdeversion 1.0.219 supports the new transaction history and wallet data serialization features mentioned in the AI summary.
14-14: New typeshare dependency supports type sharing.The addition of
typeshareversion 1.0.4 enables type sharing between Rust and TypeScript, which is essential for the new Monero wallet FFI bindings.src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx (2)
1-1: LGTM: Explicit React import added.Adding the explicit React import supports the return type annotation change and improves code clarity.
243-243: Good type annotation improvement.Changing from
JSX.Element | nulltoReact.ReactElement | nullprovides more explicit typing and aligns with the codebase consistency improvements mentioned in the AI summary.src-gui/src/renderer/components/App.tsx (2)
23-23: LGTM: MoneroWalletPage import added.The import is correctly placed and follows the existing import pattern for other page components.
75-75: Proper route integration for Monero wallet.The new route follows the established pattern and correctly integrates the MoneroWalletPage component into the application routing structure. Based on the relevant code snippet, the MoneroWalletPage displays wallet data including balance and main address.
src-gui/src/renderer/components/modal/swap/TransactionInfoBox.tsx (2)
1-1: LGTM: Import consolidation improves organization.Consolidating the React and ReactNode imports into a single import statement improves code organization and follows React best practices.
12-12: Good type specificity improvement.Changing the icon prop type from
JSX.ElementtoReact.ReactElementprovides more explicit typing and aligns with the broader type consistency improvements across the codebase.monero-rpc/src/monerod.rs (1)
232-232: LGTM: Simplified lifetime annotation.The anonymous lifetime
'_is appropriate here since the lifetime parameter isn't used elsewhere in the trait implementation. This simplification improves code readability.monero-sys/src/bridge.h (1)
229-238: LGTM: Well-implemented C++ bridge function.The
walletHistoryfunction follows the established patterns in this file:
- Proper null pointer check for defensive programming
- Correct use of
std::unique_ptrfor safe ownership transfer- Consistent with the bridge architecture for wrapping C++ methods
The implementation aligns with the learnings about using unique_ptr for string/object returns in the CXX bridge.
src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx (2)
1-14: LGTM: Proper imports and state management setup.The component follows React best practices with proper TypeScript typing for state variables and appropriate Material-UI imports.
48-102: Verify the transaction history table data mapping.The rendering logic is well-structured with proper conditional rendering and Material-UI usage. However, there's a potential data mapping issue:
Line 89 displays
tx.block_heightin the "Confirmations" column. Block height and confirmations are different values - confirmations typically represent the number of blocks since the transaction was mined.Please verify that
TransactionInfo.block_heightshould be displayed as confirmations, or if a separate confirmations field should be used.The rest of the component implementation looks solid with proper error handling and loading states.
src-gui/src/renderer/rpc.ts (2)
31-33: LGTM: Consistent import additions.The new import additions follow the established pattern and are properly typed.
392-402: LGTM: Well-implemented RPC functions.All three new functions follow the established patterns in this codebase:
- Consistent naming convention
- Proper use of
invokeNoArgshelper for parameter-less commands- Correct TypeScript return type annotations
- Consistent with existing RPC function implementations
src-tauri/src/lib.rs (4)
13-16: LGTM: Clean import additions.The new import additions are properly organized and follow the established pattern in the file.
198-200: LGTM: Proper command registration.The new Tauri commands are correctly added to the invoke_handler following the established pattern.
251-254: LGTM: Consistent tauri_command! usage.The new command declarations follow the established pattern using the
tauri_command!macro with theno_argsvariant, which is appropriate for commands that don't require parameters.
417-417: LGTM: Consistent command implementation.The
get_monero_balancecommand follows the same pattern as the other commands.monero-sys/src/bridge.rs (2)
57-61: LGTM: FFI type declarations follow established patterns.The new opaque types
TransactionHistoryandTransactionInfoare properly declared and will enable safe access to Monero transaction history functionality through the FFI bridge.
280-296: LGTM: FFI method signatures follow established safety patterns.The method signatures correctly follow the established FFI patterns:
walletHistoryproperly usesunsafeand returnsUniquePtrfor memory management- Methods taking references (
&TransactionHistory,&TransactionInfo) are safe- Return types are appropriate for their respective operations
The implementation aligns with the retrieved learnings about proper bridge architecture using the cxx::bridge macro and safe memory management patterns.
swap/src/cli/api/request.rs (2)
457-477: LGTM: Transaction history request implementation is correct.The implementation properly:
- Handles potential absence of monero manager with appropriate error context
- Follows the established async Request pattern
- Uses proper type annotations for frontend integration
479-499: LGTM: Main address request implementation is correct.The implementation follows the established pattern with proper error handling and type serialization for the Monero address.
monero-sys/src/lib.rs (3)
477-484: LGTM!The async
history()method correctly uses the established pattern to execute on the wallet thread and returns serializable data.
1703-1714: Appropriate use of unsafe for FFI interactionThe implementation correctly wraps the unsafe FFI call and immediately serializes the data, avoiding prolonged exposure of raw pointers. This follows the established pattern from the learnings about restricting unsafe to FFI boundaries.
2002-2023: Well-implemented custom serialization moduleThe
monero_serdemodule correctly handles serialization ofmonero::Amountas u64 piconeros, following serde best practices.
…of Monero wallet in GUI
|
For the wallet listener, we should do it like this:
|
binarybaron
force-pushed
the
feat/monero-wallet
branch
from
2cbe765 to
7b959f3
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Unintended Debug Logging Causes Performance Issues
The debug print statement std::cout << "WalletListener::Updated" << std::endl; was accidentally committed in the FunctionBasedListener::updated() method. This will cause excessive log output and could impact performance.
monero-sys/src/bridge.h#L344-L346
Lines 344 to 346 in 2e4cc7f
Bug: Redundant Refresh Causes Unnecessary API Calls
The resolveApproval function uses a setTimeout to refresh approvals again after already awaiting refreshApprovals(). This "just to make sure" pattern suggests a race condition workaround rather than a proper fix, leading to unnecessary API calls and masking underlying synchronization issues.
src-gui/src/renderer/rpc.ts#L574-L582
core/src-gui/src/renderer/rpc.ts
Lines 574 to 582 in 2e4cc7f
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
monero-sys/src/lib.rs (1)
1-2780: Consider breaking this large file into smaller modulesThis file has grown to nearly 3000 lines and contains many different responsibilities. Consider breaking it into logical modules to improve maintainability:
wallet_handle.rs- WalletHandle and related functionalitywallet_manager.rs- WalletManager implementationffi_wallet.rs- FfiWallet and FFI interactionslisteners.rs- Event listener implementationstransaction_history.rs- Transaction history handlingtypes.rs- Shared types and structsThis would improve:
- Code organization and readability
- Maintainability and testing
- Separation of concerns
- Easier code review in the future
♻️ Duplicate comments (13)
src-gui/src/renderer/components/pages/monero/components/WalletOverview.tsx (3)
33-34: Add null safety for balance calculation.The
pendingBalancecalculation accessesbalance.total_balanceandbalance.unlocked_balancewithout checking ifbalanceis defined, which could cause a runtime error.Add null safety:
const pendingBalance = - parseFloat(balance.total_balance) - parseFloat(balance.unlocked_balance); + balance ? parseFloat(balance.total_balance) - parseFloat(balance.unlocked_balance) : 0;
37-37: Add safety check for blocks left calculation.The calculation of
blocksLeftand its usage intoLocaleString()could fail ifsyncProgressproperties are undefined.Add safety checks:
- const blocksLeft = syncProgress?.target_block - syncProgress?.current_block; + const blocksLeft = syncProgress ? syncProgress.target_block - syncProgress.current_block : 0;And update the display:
{isSyncing && ( <Typography variant="body2" color="text.secondary"> - {blocksLeft.toLocaleString()} blocks left + {blocksLeft?.toLocaleString() || '0'} blocks left </Typography> )}Also applies to: 139-143
86-98: Fix padding inconsistency for "Pending" label.The "Pending" label has inconsistent padding compared to "Available Funds" as noted in previous feedback.
Ensure the "Pending" label aligns properly with "Available Funds" by adjusting the grid row alignment:
<Typography variant="body2" color="warning" sx={{ mb: 1, animation: "pulse 2s infinite", gridColumn: "2", gridRow: "1", - alignSelf: "end", }} > Pending </Typography>monero-sys/src/bridge.rs (1)
563-608: Consider reducing log levels for some eventsBased on the past review comment, some of these log events should be changed to trace level to reduce noise. The money-related events (spent, received, unconfirmed) are likely too verbose at info level.
Consider changing the log levels for frequent events to trace level as suggested in the past review comment.
swap/src/cli/api/request.rs (3)
565-632: Address past review comment about date parsingThe current implementation handles date parsing in Rust, but there's a past review comment suggesting this should be moved to the frontend. The date validation logic is appropriate for the backend.
The date parsing is handled correctly in Rust with proper validation, but consider the past review comment about moving date parsing to the frontend.
634-642: Add explanation comment for pause-stop-refresh sequenceThe sequence of pausing, stopping, and refreshing needs explanation as requested in the past review comment.
Add a comment explaining why we pause, stop, and then refresh again as requested in the past review comment.
675-677: Use .into() for amount conversionAs suggested in the past review comment, use
.into()instead of the explicit conversion method.Consider using
.into()for the amount conversion as suggested in the past review comment.monero-sys/src/lib.rs (2)
1031-1031: Create a constant for empty password to improve clarityThe empty string
""for password is used in multiple places but its purpose isn't immediately clear.As mentioned in the previous review, create a constant to make this more explicit:
impl WalletManager { + const EMPTY_PASSWORD: &str = ""; /// For now we don't support custom difficulty const DEFAULT_KDF_ROUNDS: u64 = 1; // ... later in the code - let_cxx_string!(password = password.map_or("", |s| s.as_str())); + let_cxx_string!(password = password.map_or(Self::EMPTY_PASSWORD, |s| s.as_str()));
2431-2461: Memory safety concern: Raw pointer usage without lifetime trackingThe
TransactionHistoryHandleandTransactionInfoHandlestructs hold raw pointers without lifetime guarantees, which creates potential use-after-free vulnerabilities.As mentioned in previous reviews, consider adding lifetime parameters or refactoring to eliminate these handle types:
-pub struct TransactionHistoryHandle(*mut TransactionHistory); -pub struct TransactionInfoHandle(*mut ffi::TransactionInfo); +pub struct TransactionHistoryHandle<'a> { + ptr: *mut TransactionHistory, + _phantom: std::marker::PhantomData<&'a TransactionHistory>, +}Alternatively, since the data is immediately serialized, consider removing these handle types entirely and directly converting to
TransactionInfowithin thehistory()method.src-gui/src/renderer/rpc.ts (2)
439-458: Address the unresolved feedback about using an enum for date format.The past review comment indicates this function should accept an enum
{ year: u32, month: u32, day: u32 }instead of a string. The current implementation correctly handles the Date object conversion, but this doesn't align with the requested enum format in the backend.Run the following script to verify the expected format in the backend:
#!/bin/bash # Description: Check the SetRestoreHeightArgs type definition in request.rs # Search for SetRestoreHeightArgs definition ast-grep --pattern 'struct SetRestoreHeightArgs { $$$ }' # Also check for any enum definitions related to restore height rg -A 10 "SetRestoreHeightArgs|RestoreHeight.*enum" swap/src/cli/api/request.rs
505-507: Error handling still only logs to console without user notification.The past review comment about propagating errors to the UI hasn't been addressed. Users won't know if wallet initialization fails.
swap/src/cli/api.rs (2)
333-337: Store the spawned task handle for proper cleanup.The spawned task for pool status updates should be stored to enable cleanup when the context is destroyed.
721-754: Add security controls to the password verification loop.The infinite retry loop still lacks security controls and could enable brute-force attempts.
🧹 Nitpick comments (5)
src-gui/src/renderer/components/pages/monero/SetRestoreHeightModal.tsx (1)
1-14: Consolidate Material UI imports.The Material UI imports are split across two statements. Consider consolidating them for better organization.
import { Accordion, AccordionDetails, AccordionSummary, Button, Dialog, DialogActions, DialogContent, + DialogTitle, TextField, Typography, Radio, } from "@mui/material"; - -import { DialogTitle } from "@mui/material";throttle/src/throttle.rs (2)
1-7: Remove commented out imports.Clean up the unused imports that are commented out.
-use std::time::{self, /* SystemTime, UNIX_EPOCH, */ Duration}; +use std::time::{self, Duration};
162-162: Translate test comment to English.Maintain consistent language in test comments.
- assert_eq!(*effect_run_times.lock().unwrap(), 2); // delay后执行最有一个参数 + assert_eq!(*effect_run_times.lock().unwrap(), 2); // Execute last parameter after delayswap/src/monero/wallet.rs (1)
214-214: Remove or document the commented code.This commented line should either be removed or have a clear explanation of why it's commented out.
fn on_refreshed(&self) { - //self.wallet.start_refresh_thread(); self.send_balance_update();monero-sys/src/lib.rs (1)
858-892: Static method doesn't need to be on WalletHandleThe
verify_wallet_passwordmethod is implemented as a static method onWalletHandlebut doesn't use any instance data. Consider moving this to a separate utility module or theWalletManagerstruct where it logically belongs.impl WalletHandle { - /// Verify the password for a wallet at the given path. - /// This function spawns a thread to perform the verification and returns the result via a oneshot channel. - /// Returns `Ok(true)` if the password is correct, `Ok(false)` if incorrect. - pub fn verify_wallet_password(path: String, password: String) -> anyhow::Result<bool> { - // ... implementation - } }And add it to
WalletManagerinstead:impl WalletManager { + /// Verify the password for a wallet at the given path. + /// Returns `Ok(true)` if the password is correct, `Ok(false)` if incorrect. + pub fn verify_wallet_password(path: String, password: String) -> anyhow::Result<bool> { + // ... implementation + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.locksrc-gui/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (68)
Cargo.toml(2 hunks)monero-harness/src/lib.rs(1 hunks)monero-rpc-pool/src/database.rs(1 hunks)monero-rpc-pool/src/lib.rs(3 hunks)monero-rpc-pool/src/proxy.rs(12 hunks)monero-sys/Cargo.toml(1 hunks)monero-sys/src/bridge.h(5 hunks)monero-sys/src/bridge.rs(9 hunks)monero-sys/src/lib.rs(42 hunks)monero-sys/tests/sign_message.rs(1 hunks)src-gui/package.json(2 hunks)src-gui/src/renderer/components/App.tsx(3 hunks)src-gui/src/renderer/components/alert/DaemonStatusAlert.tsx(1 hunks)src-gui/src/renderer/components/modal/password-entry/PasswordEntryDialog.tsx(1 hunks)src-gui/src/renderer/components/other/Units.tsx(3 hunks)src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/SendTransactionModal.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/SetRestoreHeightModal.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/SendAmountInput.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/SendApprovalContent.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/SendSuccessContent.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/SendTransactionContent.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/TransactionHistory.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/WalletActionButtons.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/WalletOverview.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/WalletPageLoadingState.tsx(1 hunks)src-gui/src/renderer/components/pages/monero/components/index.ts(1 hunks)src-gui/src/renderer/components/pages/swap/swap/init/InitPage.tsx(2 hunks)src-gui/src/renderer/rpc.ts(2 hunks)src-gui/src/store/features/walletSlice.ts(1 hunks)src-gui/src/utils/conversionUtils.ts(1 hunks)src-tauri/Cargo.toml(0 hunks)src-tauri/src/lib.rs(13 hunks)swap-env/Cargo.toml(1 hunks)swap-env/src/config.rs(2 hunks)swap-env/src/env.rs(1 hunks)swap-env/src/lib.rs(1 hunks)swap-fs/src/lib.rs(1 hunks)swap-serde/Cargo.toml(1 hunks)swap-serde/src/bitcoin.rs(2 hunks)swap-serde/src/electrum.rs(1 hunks)swap-serde/src/lib.rs(1 hunks)swap-serde/src/libp2p.rs(2 hunks)swap-serde/src/monero.rs(3 hunks)swap/Cargo.toml(1 hunks)swap/src/asb/command.rs(20 hunks)swap/src/bin/asb.rs(6 hunks)swap/src/bitcoin.rs(1 hunks)swap/src/cli/api.rs(17 hunks)swap/src/cli/api/request.rs(7 hunks)swap/src/cli/behaviour.rs(2 hunks)swap/src/cli/command.rs(1 hunks)swap/src/common/tracing_util.rs(1 hunks)swap/src/database.rs(1 hunks)swap/src/monero.rs(2 hunks)swap/src/monero/wallet.rs(6 hunks)swap/src/network/swap_setup/alice.rs(1 hunks)swap/src/network/swap_setup/bob.rs(1 hunks)swap/src/network/swarm.rs(1 hunks)swap/src/protocol/alice.rs(1 hunks)swap/src/protocol/alice/state.rs(1 hunks)swap/src/protocol/alice/swap.rs(1 hunks)swap/src/protocol/bob/state.rs(2 hunks)swap/src/protocol/bob/swap.rs(1 hunks)swap/src/seed.rs(6 hunks)swap/tests/harness/mod.rs(1 hunks)throttle/Cargo.toml(1 hunks)throttle/src/throttle.rs(1 hunks)
💤 Files with no reviewable changes (1)
- src-tauri/Cargo.toml
✅ Files skipped from review due to trivial changes (23)
- swap-serde/src/electrum.rs
- swap/src/network/swarm.rs
- swap/src/protocol/alice.rs
- swap-serde/src/bitcoin.rs
- swap-env/src/lib.rs
- swap/src/protocol/bob/swap.rs
- swap/src/bitcoin.rs
- swap-env/src/env.rs
- Cargo.toml
- swap-serde/src/libp2p.rs
- swap/src/database.rs
- swap/tests/harness/mod.rs
- swap/src/network/swap_setup/bob.rs
- swap/src/cli/behaviour.rs
- monero-rpc-pool/src/database.rs
- monero-sys/tests/sign_message.rs
- throttle/Cargo.toml
- swap-env/src/config.rs
- swap-serde/Cargo.toml
- swap-serde/src/lib.rs
- swap-env/Cargo.toml
- swap/src/protocol/alice/swap.rs
- src-gui/src/renderer/components/pages/monero/components/SendApprovalContent.tsx
🚧 Files skipped from review as they are similar to previous changes (18)
- monero-sys/Cargo.toml
- swap/Cargo.toml
- monero-harness/src/lib.rs
- src-gui/package.json
- src-gui/src/renderer/components/App.tsx
- src-gui/src/renderer/components/pages/swap/swap/init/InitPage.tsx
- src-gui/src/utils/conversionUtils.ts
- src-gui/src/renderer/components/pages/monero/MoneroWalletPage.tsx
- swap/src/monero.rs
- src-gui/src/renderer/components/modal/password-entry/PasswordEntryDialog.tsx
- swap/src/protocol/bob/state.rs
- swap/src/bin/asb.rs
- src-gui/src/renderer/components/pages/monero/components/WalletActionButtons.tsx
- src-gui/src/renderer/components/other/Units.tsx
- src-gui/src/store/features/walletSlice.ts
- src-gui/src/renderer/components/pages/monero/components/TransactionHistory.tsx
- src-gui/src/renderer/components/pages/monero/components/SendAmountInput.tsx
- src-gui/src/renderer/components/pages/monero/components/SendTransactionContent.tsx
🧰 Additional context used
📓 Path-based instructions (5)
monero-sys/**/bridge.h
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- monero-sys/CLAUDE.md
monero-sys/src/bridge.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- monero-sys/CLAUDE.md
monero-sys/{src/bridge.rs,src/lib.rs}
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- monero-sys/CLAUDE.md
monero-sys/**/*.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- monero-sys/CLAUDE.md
monero-sys/src/lib.rs
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- monero-sys/CLAUDE.md
🧬 Code Graph Analysis (4)
swap/src/cli/command.rs (1)
swap-serde/src/monero.rs (1)
validate_is_testnet(135-145)
swap/src/asb/command.rs (2)
swap/src/bin/asb.rs (1)
config(450-455)swap-env/src/config.rs (3)
get_config_file_defaults(18-18)get_config_file_defaults(32-46)get_config_file_defaults(50-64)
src-gui/src/renderer/components/pages/monero/components/SendSuccessContent.tsx (4)
src-gui/src/utils/conversionUtils.ts (1)
getMoneroTxExplorerUrl(40-45)src-gui/src/store/config.ts (1)
isTestnet(16-18)src-gui/src/renderer/components/other/Units.tsx (2)
PiconeroAmount(164-177)FiatPiconeroAmount(42-70)src-gui/src/renderer/components/other/MonospaceTextBox.tsx (1)
MonospaceTextBox(8-36)
monero-sys/src/bridge.h (3)
monero-sys/src/bridge.rs (43)
rescanBlockchainAsync(226-226)pauseRefresh(229-229)stop(232-232)startRefresh(188-188)pendingTransactionFee(291-291)pendingTransactionAmount(294-294)transactionInfoHash(337-337)on_new_block(491-491)on_new_block(528-530)on_new_block(591-591)on_updated(492-492)on_updated(532-534)on_updated(593-593)on_refreshed(493-493)on_refreshed(536-538)on_refreshed(595-595)on_reorg(420-425)on_reorg(473-480)on_reorg(494-494)on_reorg(540-543)on_reorg(597-605)on_pool_tx_removed(495-495)on_pool_tx_removed(545-547)on_pool_tx_removed(607-607)new_block(417-417)new_block(461-463)updated(418-418)updated(465-467)refreshed(419-419)refreshed(469-471)pool_tx_removed(426-426)pool_tx_removed(482-484)new(505-507)new(558-560)amount(325-325)money_spent(410-410)money_spent(449-451)money_received(411-411)money_received(453-455)unconfirmed_money_received(412-416)unconfirmed_money_received(457-459)create_rust_listener_adapter(440-442)destroy_rust_listener_adapter(444-444)monero-sys/src/lib.rs (21)
stop(554-556)stop(1633-1635)on_new_block(1324-1328)on_new_block(2550-2550)on_updated(1330-1334)on_updated(2552-2552)on_refreshed(1336-1340)on_refreshed(2554-2562)on_reorg(1342-1346)on_reorg(2564-2564)on_pool_tx_removed(1348-1352)on_pool_tx_removed(2566-2566)new(55-57)new(920-931)new(974-990)new(1276-1278)new(1359-1403)new(2329-2334)new(2383-2385)new(2537-2540)amount(2465-2467)swap/src/monero/wallet.rs (7)
on_new_block(203-207)on_updated(209-211)on_refreshed(213-218)on_reorg(220-224)on_pool_tx_removed(226-230)new(79-165)new(240-293)
🪛 Biome (1.9.4)
src-gui/src/renderer/components/pages/monero/components/WalletPageLoadingState.tsx
[error] 77-77: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🪛 ast-grep (0.38.6)
monero-rpc-pool/src/lib.rs
[warning] 107-109: Dangerously accepting invalid TLS
Context: reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(30))
.danger_accept_invalid_certs(true)
Note: [CWE-295]: Improper Certificate [REFERENCES]
- https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.danger_accept_invalid_hostnames
- https://docs.rs/reqwest/latest/reqwest/struct.ClientBuilder.html#method.danger_accept_invalid_certs
(reqwest-accept-invalid-rust)
⏰ 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: Cursor BugBot
🔇 Additional comments (53)
swap/src/protocol/alice/state.rs (1)
17-17: LGTM: Clean import reorganizationThe import reordering maintains functionality while improving code organization. This is consistent with similar import adjustments across other swap protocol files.
monero-rpc-pool/src/lib.rs (3)
42-42: LGTM: Good architectural improvement for connection reuse.Adding the shared HTTP client to AppState enables efficient connection pooling and reuse across proxy requests.
63-67: LGTM: Clean conversion implementation.The
Into<String>implementation forServerInfoprovides a convenient way to format server information as HTTP URLs.
117-120: LGTM: Proper AppState initialization.The AppState initialization correctly includes the new http_client field alongside the existing node_pool.
monero-rpc-pool/src/proxy.rs (7)
14-16: LGTM: Useful helper function for consistent node URL formatting.The
display_nodehelper function provides consistent formatting for node URLs across error messages and logs.
23-23: LGTM: Enhanced error classification with JsonRpcError variant.Adding the
JsonRpcErrorvariant toHandlerErrorenables better error categorization and handling, particularly useful for the fail-fast mechanism.Also applies to: 33-33
69-69: LGTM: Proper integration of shared HTTP client.The function signature changes to accept a shared
reqwest::Clientparameter eliminate redundant client creation and enable connection pooling benefits.Also applies to: 177-177
231-231: LGTM: Intelligent fail-fast mechanism for JSON-RPC errors.The implementation correctly distinguishes between network/connection errors (which may succeed on other nodes) and JSON-RPC errors (which are likely request-specific). The
MAX_JSONRPC_ERRORSthreshold of 3 provides reasonable balance between resilience and efficiency.Also applies to: 245-245
290-298: LGTM: Consistent use of shared HTTP client.The function call correctly uses the shared HTTP client from
state.http_client, enabling connection reuse and consistent configuration across all proxy requests.
327-359: LGTM: Well-implemented fail-fast logic with proper error tracking.The implementation correctly:
- Counts JSON-RPC errors using type-safe pattern matching
- Provides detailed logging for debugging
- Records failures only for non-JSON-RPC errors (since JSON-RPC errors aren't node failures)
- Returns comprehensive error information
This approach prevents unnecessary retries when the issue is with the request itself rather than node availability.
458-466: LGTM: Comprehensive error response handling.The new
JsonRpcErrorcase in the error response handling provides structured error information consistent with other error types, improving API usability and debugging capabilities.swap/src/network/swap_setup/alice.rs (1)
22-22: Import reorganization looks good.The
swap_env::envimport has been moved to maintain consistency with the import ordering standardization across the codebase.swap/src/asb/command.rs (2)
9-11: Import reorganization is consistent.The
swap_envrelated imports have been moved to maintain consistency with the import ordering across the codebase.
405-407: Improved readability with multi-line formatting.The chained method calls have been formatted across multiple lines for better readability, which is a good practice for complex expressions.
swap-fs/src/lib.rs (1)
23-32: New eigenwallet data directory function follows established patterns.The
system_data_dir_eigenwalletfunction correctly implements the same pattern assystem_data_dir, with appropriate testnet/mainnet directory selection and proper error handling.swap/src/common/tracing_util.rs (1)
70-79: Proper logging configuration for new workspace crates.The addition of "seed", "swap_env", "swap_fs", and "swap_serde" to OUR_CRATES ensures these new workspace crates are logged at the appropriate level alongside the existing "swap" and "asb" crates.
swap-serde/src/monero.rs (4)
1-1: Import reordering is fine.The change from
Network, AmounttoAmount, Networkis a minor formatting improvement.
14-14: Good practice to move hex import to top.Moving the
heximport to the top of the module's import list improves code organization.
146-146: Good practice to add newline at end of file.Adding a newline at the end of the file follows standard formatting conventions.
4-11: Keep thenetworkenum for serde’s remote mappingThe
pub enum networkinswap-serde/src/monero.rsis still required as the remote representation ofmonero::Networkfor Serde. It’s referenced via#[serde(with = "swap_serde::monero::network")]in multiple configs and environment structs, so it must remain:• swap-env/src/env.rs –
pub monero_network: monero::Network
• swap-env/src/config.rs –pub network: monero::Network
• swap/src/network/swap_setup.rs –pub monero: monero::NetworkNo removal is needed; this enum enables correct (de)serialization of the external
monero::Networktype.swap/src/cli/command.rs (1)
71-75: LGTM! Improved readability with multi-line formatting.The reformatting of the
validate_is_testnetcall improves code readability without changing functionality. This is a good practice for complex chained operations.src-gui/src/renderer/components/alert/DaemonStatusAlert.tsx (1)
190-193: Verify the UX impact of removing loading feedback.The change removes loading spinner feedback for
nullandNotInitializedstates. While this may reduce UI noise, ensure users have alternative feedback during daemon initialization to avoid confusion about whether the app is working.src-gui/src/renderer/components/pages/monero/components/index.ts (1)
1-5: LGTM! Clean barrel export pattern.This index file provides a clean centralized entry point for importing Monero wallet components. The barrel export pattern is a good practice for organizing related components.
src-gui/src/renderer/components/pages/monero/SendTransactionModal.tsx (1)
17-55: LGTM! Well-structured modal flow management.The component effectively manages the three-state flow (input → approval → success) for Monero transactions. The conditional rendering logic is clear, state management is proper, and the cleanup in
handleCloseensures no stale state persists.monero-sys/src/bridge.rs (11)
57-61: LGTM: Clean addition of transaction history typesThe addition of
TransactionHistoryandTransactionInfotypes follows the established pattern of mirroring C++ types in the FFI bridge.
104-104: LGTM: WalletListener type declarationThe WalletListener type is properly declared following the established FFI pattern.
123-136: LGTM: New wallet operations correctly declaredThe new wallet operations (
store,verifyWalletPassword,setSeedLanguage,setListener) follow the established pattern and mirror the C++ API correctly.
223-233: LGTM: Blockchain operations properly declaredThe blockchain operations (
getBlockchainHeightByDate,rescanBlockchainAsync,pauseRefresh,stop) are correctly declared following the established FFI pattern.
290-295: LGTM: Pending transaction operationsThe addition of
pendingTransactionFeeandpendingTransactionAmountfunctions follows the established pattern for pending transaction operations.
312-347: LGTM: Transaction history operationsThe transaction history operations (
history,count,transaction,refresh,amount,fee,confirmations,direction,transactionInfoHash) are properly declared and follow the established pattern.
402-446: LGTM: Well-designed wallet listener bridgeThe wallet listener bridge using cxx's virtual table approach is well-designed. The separation of concerns between the Rust trait and C++ adapter is clean and follows best practices for FFI.
448-484: LGTM: Callback bridge functionsThe callback bridge functions correctly convert between C++ types and Rust trait methods. The string conversion using
to_string()is appropriate here.
486-496: LGTM: Well-defined trait interfaceThe
WalletEventListenertrait is well-defined with appropriate bounds (Send + Sync) for thread safety. The method signatures are clean and consistent.
498-548: LGTM: Clean wrapper implementationThe
WalletListenerBoxwrapper is a clean solution for working with trait objects in CXX. The forwarding implementation is correct and the convenience methods are well-designed.
550-561: LGTM: TraceListener struct designThe
TraceListenerstruct is well-designed with a clear purpose and appropriate field for context logging.swap/src/cli/api/request.rs (10)
424-424: LGTM: Updated log directory pathThe change to use the provided logs directory or fall back to the context's log directory is correct and handles the data directory restructuring mentioned in the PR objectives.
458-482: LGTM: Clean GetRestoreHeight implementationThe implementation properly accesses the Monero wallet through the manager and includes appropriate error handling with context.
504-527: LGTM: Transaction history retrievalThe
GetMoneroHistoryArgsimplementation is clean and properly accesses the wallet to retrieve transaction history.
529-552: LGTM: Main address retrievalThe
GetMoneroMainAddressArgsimplementation correctly retrieves the main address from the wallet.
554-573: LGTM: Date structure definitionThe
Datestruct is well-defined with appropriate types for year, month, and day components.
648-680: LGTM: Monero balance retrievalThe
GetMoneroBalanceArgsimplementation properly retrieves both total and unlocked balances with appropriate error handling.
682-706: LGTM: SendMonero request structureThe
SendMoneroArgsandSendMoneroAmountenum are well-designed, supporting both specific amounts and sweep operations.
707-778: LGTM: Comprehensive send implementation with approval flowThe
SendMoneroArgsimplementation is comprehensive and well-designed:
- Proper address validation
- Async approval callback integration
- Clean error handling
- Appropriate use of Arc for thread safety
- Good separation of concerns between UI approval and wallet operations
The approval callback implementation is particularly well-designed, properly handling the async nature of user approval.
930-940: LGTM: Rejection approval request typesThe
RejectApprovalArgsandRejectApprovalResponsetypes are properly defined and follow the established pattern for API requests.
963-997: LGTM: Sync progress monitoringThe
GetMoneroSyncProgressArgsimplementation properly retrieves sync progress information and calculates the percentage correctly.monero-sys/src/lib.rs (6)
160-161: Field nameconfirmationscorrectly reflects the data storedGood correction from the previous misleading
block_heightfield name. The field now accurately represents the number of confirmations.
1364-1367: Good addition of thread-safe listener collectionThe addition of
Arc<Mutex<Vec<Box<dyn WalletEventListener>>>>for the listeners field properly handles thread safety for the listener collection.
1825-1834: Well-structured transaction method refactoringThe refactoring of the
transfermethod to use separatecreate_pending_transaction,publish_pending_transaction, anddispose_pending_transactionmethods improves code organization and enables the approval workflow functionality.
1305-1353: Well-implemented event listener forwardingThe implementation of
WalletEventListenerforArc<Mutex<Vec<Box<dyn WalletEventListener>>>>properly forwards events to all registered listeners with appropriate thread safety.
2569-2589: Good serde implementation for monero::AmountThe custom serde implementation properly handles Amount serialization/deserialization using piconero units, which is the correct approach for network compatibility.
2623-2623: Correct fix for inclusive range validationGood correction to use
(0.0..=1.0).contains(&p)instead of the previous exclusive range. This properly validates that percentages are between 0 and 1 inclusive.src-tauri/src/lib.rs (1)
96-119: Good refactoring of state management with internal synchronous locking.The new design with
RwLock<Option<Arc<Context>>>inside the State struct and synchronoustry_read()access pattern is cleaner and prevents potential deadlocks. The error messages are also clear and informative.monero-sys/src/bridge.h (1)
540-597: Well-implemented Rust listener adapter with proper ownership semantics.The
RustListenerAdapterclass correctly manages the Rust box ownership and forwards all callbacks appropriately. The RAII pattern with proper cleanup in the destroy function ensures no memory leaks.
| {Array.from({ length: 2 }).map((_) => ( | ||
| <Skeleton variant="rounded" sx={{ borderRadius: "100px" }}> | ||
| <Chip label="Loading..." variant="button" /> | ||
| </Skeleton> | ||
| ))} |
There was a problem hiding this comment.
Fix missing key prop in array mapping.
The static analysis correctly identifies a missing key prop for the mapped elements. This can cause React rendering issues and warnings.
Apply this fix:
- {Array.from({ length: 2 }).map((_) => (
- <Skeleton variant="rounded" sx={{ borderRadius: "100px" }}>
+ {Array.from({ length: 2 }).map((_, index) => (
+ <Skeleton key={index} variant="rounded" sx={{ borderRadius: "100px" }}>
<Chip label="Loading..." variant="button" />
</Skeleton>
))}📝 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.
| {Array.from({ length: 2 }).map((_) => ( | |
| <Skeleton variant="rounded" sx={{ borderRadius: "100px" }}> | |
| <Chip label="Loading..." variant="button" /> | |
| </Skeleton> | |
| ))} | |
| {Array.from({ length: 2 }).map((_, index) => ( | |
| <Skeleton key={index} variant="rounded" sx={{ borderRadius: "100px" }}> | |
| <Chip label="Loading..." variant="button" /> | |
| </Skeleton> | |
| ))} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 77-77: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🤖 Prompt for AI Agents
In
src-gui/src/renderer/components/pages/monero/components/WalletPageLoadingState.tsx
between lines 76 and 80, the elements generated by mapping over the array lack a
unique key prop, which can cause React rendering warnings. Add a unique key prop
to the Skeleton component inside the map function, for example by using the
index parameter of the map callback or a unique identifier, to ensure each
element has a stable key.
src-gui/src/renderer/components/pages/monero/components/SendSuccessContent.tsx
Show resolved
Hide resolved
| let monero_seed = | ||
| MoneroSeed::from_string(Language::English, Zeroizing::new(mnemonic.clone())).map_err( | ||
| |e| anyhow::anyhow!("Failed to parse seed from wallet (Error: {:?})", e), | ||
| )?; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling to preserve error details.
The error handling discards useful error information by converting to a generic message.
let monero_seed =
- MoneroSeed::from_string(Language::English, Zeroizing::new(mnemonic.clone())).map_err(
- |e| anyhow::anyhow!("Failed to parse seed from wallet (Error: {:?})", e),
- )?;
+ MoneroSeed::from_string(Language::English, Zeroizing::new(mnemonic.clone()))
+ .with_context(|| "Failed to parse seed from wallet")?;📝 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.
| let monero_seed = | |
| MoneroSeed::from_string(Language::English, Zeroizing::new(mnemonic.clone())).map_err( | |
| |e| anyhow::anyhow!("Failed to parse seed from wallet (Error: {:?})", e), | |
| )?; | |
| let monero_seed = | |
| MoneroSeed::from_string(Language::English, Zeroizing::new(mnemonic.clone())) | |
| .with_context(|| "Failed to parse seed from wallet")?; |
🤖 Prompt for AI Agents
In swap/src/seed.rs around lines 38 to 42, the error handling converts the
original error into a generic message, losing detailed error information. Modify
the error handling to preserve the original error context by using error
chaining or wrapping techniques, such as using the `context` or `with_context`
method from the anyhow crate, so the detailed error is retained and accessible
for debugging.
| async function refreshWalletDataAfterTransaction() { | ||
| try { | ||
| const [newBalance, newHistory] = await Promise.all([ | ||
| getMoneroBalance(), | ||
| getMoneroHistory(), | ||
| ]); | ||
| store.dispatch(setBalance(newBalance)); | ||
| store.dispatch(setHistory(newHistory)); | ||
| } catch (err) { | ||
| console.error("Failed to refresh wallet data after transaction:", err); | ||
| // Maybe show a non-blocking notification to user | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove unused function refreshWalletDataAfterTransaction.
This function is defined but never called anywhere in the code. The same logic is already implemented inline in sendMoneroTransaction (lines 517-526).
Apply this diff to remove the dead code:
-async function refreshWalletDataAfterTransaction() {
- try {
- const [newBalance, newHistory] = await Promise.all([
- getMoneroBalance(),
- getMoneroHistory(),
- ]);
- store.dispatch(setBalance(newBalance));
- store.dispatch(setHistory(newHistory));
- } catch (err) {
- console.error("Failed to refresh wallet data after transaction:", err);
- // Maybe show a non-blocking notification to user
- }
-}📝 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.
| async function refreshWalletDataAfterTransaction() { | |
| try { | |
| const [newBalance, newHistory] = await Promise.all([ | |
| getMoneroBalance(), | |
| getMoneroHistory(), | |
| ]); | |
| store.dispatch(setBalance(newBalance)); | |
| store.dispatch(setHistory(newHistory)); | |
| } catch (err) { | |
| console.error("Failed to refresh wallet data after transaction:", err); | |
| // Maybe show a non-blocking notification to user | |
| } | |
| } |
🤖 Prompt for AI Agents
In src-gui/src/renderer/rpc.ts around lines 535 to 547, the function
refreshWalletDataAfterTransaction is defined but never used, and its logic
duplicates code already present inline in sendMoneroTransaction (lines 517-526).
Remove the entire refreshWalletDataAfterTransaction function to eliminate dead
code and avoid redundancy.
| pub fn cleanup(&self) -> Result<()> { | ||
| // TODO: close all monero wallets | ||
|
|
||
| // call store(..) on all wallets | ||
| // if let Some(monero_manager) = &self.monero_manager { | ||
| // let wallet = monero_manager.main_wallet().await; | ||
| // wallet.store().await?; | ||
| // } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Implement proper wallet cleanup to prevent data loss.
The cleanup function has commented-out code that should save wallet state. Without this, wallet data may be lost on shutdown.
The TODO comment and past review indicate this needs implementation. Would you like me to help implement the proper cleanup logic that iterates through all wallets and calls store() on each?
🤖 Prompt for AI Agents
In swap/src/cli/api.rs around lines 554 to 562, the cleanup function currently
has commented-out code and a TODO for saving wallet states, which risks data
loss on shutdown. Implement the cleanup method to asynchronously iterate over
all wallets managed by monero_manager, calling the store() method on each wallet
to persist their state properly before returning Ok(()). Ensure to handle any
errors from store() calls appropriately to maintain robustness.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if let Err(err) = context.cleanup() { | ||
| println!("Cleanup failed {}", err); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use proper logging instead of println for cleanup errors.
Errors during cleanup should be logged properly using the logging framework rather than println.
Apply this diff to improve error logging:
- if let Err(err) = context.cleanup() {
- println!("Cleanup failed {}", err);
- }
+ if let Err(err) = context.cleanup() {
+ tracing::error!("Context cleanup failed: {}", err);
+ }📝 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.
| if let Err(err) = context.cleanup() { | |
| println!("Cleanup failed {}", err); | |
| } | |
| if let Err(err) = context.cleanup() { | |
| tracing::error!("Context cleanup failed: {}", err); | |
| } |
🤖 Prompt for AI Agents
In src-tauri/src/lib.rs around lines 232 to 234, replace the println! macro used
for logging cleanup errors with the appropriate logging framework method, such
as log::error! or another configured logger. This change ensures that cleanup
errors are recorded properly in the application's logs instead of just printing
to standard output.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src-gui/src/renderer/components/modal/seed-selection/SeedSelectionDialog.tsx (2)
72-81: Add file type filtering for wallet selection.The wallet file browser should filter for wallet files to improve UX and prevent selection of invalid files.
336-337: Fix cross-platform path handling.The path splitting assumes Unix-style paths and will fail on Windows systems.
🧹 Nitpick comments (1)
src-gui/src/renderer/components/modal/seed-selection/SeedSelectionDialog.tsx (1)
64-70: Improve dependency handling for recent wallets.The auto-selection logic is good for UX, but the dependency array
[recentWallets.length]won't trigger when wallet content changes with the same length. Consider using a more robust dependency:- }, [recentWallets.length]); + }, [recentWallets]);Or use
useMemoto create a stable reference if performance is a concern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-gui/src/renderer/components/modal/seed-selection/SeedSelectionDialog.tsx(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). (4)
- GitHub Check: build (aarch64-apple-darwin, macos-latest)
- GitHub Check: build (x86_64-apple-darwin, macos-13)
- GitHub Check: test (macos-latest)
- GitHub Check: Cursor BugBot
🔇 Additional comments (6)
src-gui/src/renderer/components/modal/seed-selection/SeedSelectionDialog.tsx (6)
12-31: LGTM - Import additions support the new functionality.The new imports are well-organized and necessary for the enhanced card-based UI, file selection, and async operations.
35-48: LGTM - Well-structured state management.The type-safe approach for
selectedOptionand proper extraction ofapprovalandrecentWalletswith appropriate guards is well implemented.
83-104: LGTM - Well-structured handler functions.The functions properly handle all seed choice types with appropriate error checking and type-safe construction of the SeedChoice object.
110-118: LGTM - Comprehensive validation logic.The validation properly handles all seed choice types with appropriate conditions for each scenario.
120-331: LGTM - Excellent UI redesign with improved UX.The card-based interface is intuitive and well-implemented with proper conditional rendering, styling, and accessibility considerations.
363-380: LGTM - Well-structured dialog actions.The button layout with Legacy option and proper async handling using PromiseInvokeButton is well-implemented.
There was a problem hiding this comment.
Bug: Monero Wallets Not Saved on Exit
Monero wallets are not saved on application shutdown. The wallet.store() call in the cleanup function is commented out. If uncommented, the await call would cause a compile error as cleanup is not an async function. This leads to potential data loss as wallet state is not persisted.
swap/src/cli/api.rs#L553-L562
Lines 553 to 562 in 89240c3
Bug: VList Height Mismatch Causes Overflow
The VList component's height was changed to 100vh, but its parent container is constrained by minHeight and maxHeight: minHeight. This mismatch causes the VList to overflow its parent, leading to layout issues and clipping. It should use height: "100%" to respect its parent's dimensions.
src-gui/src/renderer/components/other/ScrollablePaperTextBox.tsx#L83-L84
core/src-gui/src/renderer/components/other/ScrollablePaperTextBox.tsx
Lines 83 to 84 in 89240c3
Was this report helpful? Give feedback by reacting with 👍 or 👎
remove log in bridge.h cleanup
There was a problem hiding this comment.
Bug: VList Height Prop Ignored
The VList component's height is hardcoded to "100vh", ignoring the minHeight prop. This causes layout issues and inconsistent behavior, as the VList takes up the full viewport height while the parent Box correctly uses minHeight for its maxHeight and minHeight styles. It previously used MIN_HEIGHT.
src-gui/src/renderer/components/other/ScrollablePaperTextBox.tsx#L83-L84
core/src-gui/src/renderer/components/other/ScrollablePaperTextBox.tsx
Lines 83 to 84 in 08fe1d0
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
swap/src/cli/api.rs (3)
285-391: Verify the task handle storage for pool status updates.The spawned task for pool status updates should be stored to enable proper cleanup when the context is destroyed.
Consider storing the task handle as suggested in the previous review. The current implementation lacks proper task lifecycle management.
745-778: Add security controls to password verification loop.The infinite password retry loop lacks security controls and could enable brute-force attempts or trap users.
The previous review comment about adding maximum retry attempts, rate limiting, and cancellation options is still valid and should be implemented.
554-564: Implement proper wallet cleanup to prevent data loss.The cleanup function has been partially implemented but spawns a task without waiting for completion, which could lead to data loss if the process exits before the wallet is saved.
The current implementation doesn't guarantee the wallet is saved before the process exits. Consider making this synchronous or properly awaiting the task:
pub fn cleanup(&self) -> Result<()> { // TODO: close all monero wallets // call store(..) on all wallets - let monero_manager = self.monero_manager.clone(); - tokio::spawn(async move { - if let Some(monero_manager) = monero_manager { - let wallet = monero_manager.main_wallet().await; - wallet.store(None).await; - } - }); + // Use tokio::task::block_in_place to ensure wallet is saved before returning + if let Some(monero_manager) = &self.monero_manager { + let wallet = tokio::task::block_in_place(|| { + tokio::runtime::Handle::current().block_on(async { + monero_manager.main_wallet().await + }) + }); + tokio::task::block_in_place(|| { + tokio::runtime::Handle::current().block_on(async { + wallet.store(None).await + }) + }); + } Ok(()) }
🧹 Nitpick comments (1)
monero-sys/src/bridge.h (1)
400-418: Consider using safer factory function interface.The C-style factory functions expose raw pointers which could lead to memory leaks if not properly managed. Consider returning smart pointers or adding documentation about ownership.
Add documentation about ownership:
extern "C" { + // Returns a new FunctionBasedListener instance. + // Caller is responsible for calling destroy_listener() to avoid memory leaks. WalletListener* create_listener( CB_StringU64 on_spent, CB_StringU64 on_received, CB_StringU64 on_unconfirmed_received, CB_U64 on_new_block, CB_Void on_updated, CB_Void on_refreshed, CB_Reorg on_reorg, CB_String on_pool_tx_removed, CB_GetPassword on_get_password) { return new FunctionBasedListener( on_spent,on_received,on_unconfirmed_received,on_new_block, on_updated,on_refreshed,on_reorg,on_pool_tx_removed,on_get_password); } + // Destroys a FunctionBasedListener created by create_listener(). + // Pointer must not be used after this call. void destroy_listener(FunctionBasedListener* p) { delete p; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
monero-sys/src/bridge.h(5 hunks)swap-env/src/config.rs(4 hunks)swap/src/bin/asb.rs(5 hunks)swap/src/cli/api.rs(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- swap-env/src/config.rs
- swap/src/bin/asb.rs
🧰 Additional context used
📓 Path-based instructions (1)
monero-sys/**/bridge.h
Instructions used from:
Sources:
📄 CodeRabbit Inference Engine
- monero-sys/CLAUDE.md
🧬 Code Graph Analysis (1)
monero-sys/src/bridge.h (3)
monero-sys/src/bridge.rs (44)
rescanBlockchainAsync(226-226)pauseRefresh(229-229)stop(232-232)startRefresh(188-188)pendingTransactionFee(291-291)pendingTransactionAmount(294-294)transactionInfoHash(337-337)transactionInfoTimestamp(340-340)on_new_block(494-494)on_new_block(531-533)on_new_block(594-594)on_updated(495-495)on_updated(535-537)on_updated(596-596)on_refreshed(496-496)on_refreshed(539-541)on_refreshed(598-598)on_reorg(423-428)on_reorg(476-483)on_reorg(497-497)on_reorg(543-546)on_reorg(600-608)on_pool_tx_removed(498-498)on_pool_tx_removed(548-550)on_pool_tx_removed(610-610)new_block(420-420)new_block(464-466)updated(421-421)updated(468-470)refreshed(422-422)refreshed(472-474)pool_tx_removed(429-429)pool_tx_removed(485-487)new(508-510)new(561-563)amount(325-325)money_spent(413-413)money_spent(452-454)money_received(414-414)money_received(456-458)unconfirmed_money_received(415-419)unconfirmed_money_received(460-462)create_rust_listener_adapter(443-445)destroy_rust_listener_adapter(447-447)monero-sys/src/lib.rs (21)
stop(556-558)stop(1635-1637)on_new_block(1326-1330)on_new_block(2559-2559)on_updated(1332-1336)on_updated(2561-2561)on_refreshed(1338-1342)on_refreshed(2563-2571)on_reorg(1344-1348)on_reorg(2573-2573)on_pool_tx_removed(1350-1354)on_pool_tx_removed(2575-2575)new(55-57)new(922-933)new(976-992)new(1278-1280)new(1361-1405)new(2331-2336)new(2385-2387)new(2546-2549)amount(2467-2469)swap/src/monero/wallet.rs (7)
on_new_block(203-207)on_updated(209-211)on_refreshed(213-218)on_reorg(220-224)on_pool_tx_removed(226-230)new(79-165)new(240-293)
⏰ 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). (3)
- GitHub Check: test (macos-latest)
- GitHub Check: build (aarch64-apple-darwin, macos-latest)
- GitHub Check: build (x86_64-apple-darwin, macos-13)
🔇 Additional comments (9)
monero-sys/src/bridge.h (5)
19-21: Good documentation improvement for memory safety.The added attention note about
unique_ptrownership is important for preventing memory leaks and double-free issues in the FFI boundary.
64-82: LGTM - Clean wrapper functions for wallet control.These inline wrapper functions properly expose wallet control methods (
rescanBlockchainAsync,pauseRefresh,stop,startRefresh) to the FFI layer while maintaining the same signature semantics.
248-256: Good addition of transaction accessor functions.These functions provide clean access to
PendingTransactionproperties (fee,amount) that are needed for the approval workflow.
270-284: Proper TransactionInfo accessor implementation.The functions correctly wrap
TransactionInfomethods and handle the timestamp conversion safely with explicit casting.
545-604: Well-designed Rust listener adapter.The
RustListenerAdapterclass provides a clean bridge between C++ wallet listeners and Rust trait objects. The implementation correctly forwards all callbacks and handles therust::Boxownership properly.swap/src/cli/api.rs (4)
4-4: Good import addition for wallet selection.The
SeedChoiceimport is properly added to support the new interactive wallet selection functionality.
43-43: Proper addition of log directory field.The
log_dirfield is correctly added to theConfigstruct to support the logging initialization changes.
615-632: Good separation of legacy wallet handling.The
request_and_open_monero_wallet_legacyfunction properly handles the CLI-only wallet opening flow using the existingseed.pemapproach.
857-865: Good modular approach for eigenwallet data directory.The new
eigenwallet_datamodule provides a clean abstraction for the eigenwallet-specific data directory handling.
| class FunctionBasedListener final : public Monero::WalletListener { | ||
| public: | ||
| FunctionBasedListener( | ||
| CB_StringU64 on_spent, | ||
| CB_StringU64 on_received, | ||
| CB_StringU64 on_unconfirmed_received, | ||
| CB_U64 on_new_block, | ||
| CB_Void on_updated, | ||
| CB_Void on_refreshed, | ||
| CB_Reorg on_reorg, | ||
| CB_String on_pool_tx_removed, | ||
| CB_GetPassword on_get_password) | ||
| : | ||
| on_spent_(on_spent), | ||
| on_received_(on_received), | ||
| on_unconfirmed_received_(on_unconfirmed_received), | ||
| on_new_block_(on_new_block), | ||
| on_updated_(on_updated), | ||
| on_refreshed_(on_refreshed), | ||
| on_reorg_(on_reorg), | ||
| on_pool_tx_removed_(on_pool_tx_removed), | ||
| on_get_password_(on_get_password) {} | ||
|
|
||
| void moneySpent(const std::string& txid, uint64_t amt) override { | ||
| if (on_spent_) { | ||
| auto* spent = reinterpret_cast<void(*)(const std::string&, uint64_t)>(on_spent_); | ||
| spent(txid, amt); | ||
| } | ||
| } | ||
|
|
||
| void moneyReceived(const std::string& txid, uint64_t amt) override | ||
| { if (on_received_) { | ||
| auto* received = reinterpret_cast<void(*)(const std::string&, uint64_t)>(on_received_); | ||
| received(txid, amt); | ||
| } | ||
| } | ||
|
|
||
| void unconfirmedMoneyReceived(const std::string& txid, uint64_t amt) override | ||
| { if (on_unconfirmed_received_) { | ||
| auto* unconfirmed_received = reinterpret_cast<void(*)(const std::string&, uint64_t)>(on_unconfirmed_received_); | ||
| unconfirmed_received(txid, amt); | ||
| } | ||
| } | ||
|
|
||
| void newBlock(uint64_t h) override | ||
| { if (on_new_block_) { | ||
| auto* new_block = reinterpret_cast<void(*)(uint64_t)>(on_new_block_); | ||
| new_block(h); | ||
| } | ||
| } | ||
|
|
||
| void updated() override | ||
| { | ||
| if (on_updated_) { | ||
| auto* updated = reinterpret_cast<void(*)()>(on_updated_); | ||
| updated(); | ||
| } | ||
| } | ||
|
|
||
| void refreshed() override | ||
| { if (on_refreshed_) { | ||
| auto* refreshed = reinterpret_cast<void(*)()>(on_refreshed_); | ||
| refreshed(); | ||
| } | ||
| } | ||
|
|
||
| void onReorg(uint64_t h, uint64_t d, size_t t) override | ||
| { if (on_reorg_) { | ||
| auto* reorg = reinterpret_cast<void(*)(uint64_t, uint64_t, size_t)>(on_reorg_); | ||
| reorg(h, d, t); | ||
| } | ||
| } | ||
|
|
||
| void onPoolTxRemoved(const std::string& txid) override | ||
| { if (on_pool_tx_removed_) { | ||
| auto* pool_tx_removed = reinterpret_cast<void(*)(const std::string&)>(on_pool_tx_removed_); | ||
| pool_tx_removed(txid); | ||
| } | ||
| } | ||
|
|
||
| optional<std::string> onGetPassword(const char* reason) override { | ||
| if (on_get_password_) { | ||
| auto* get_password = reinterpret_cast<const char*(*)(const std::string&)>(on_get_password_); | ||
| return std::string(get_password(reason)); | ||
| } | ||
| return optional<std::string>(); | ||
| } | ||
|
|
||
| private: | ||
| CB_StringU64 on_spent_; | ||
| CB_StringU64 on_received_; | ||
| CB_StringU64 on_unconfirmed_received_; | ||
| CB_U64 on_new_block_; | ||
| CB_Void on_updated_; | ||
| CB_Void on_refreshed_; | ||
| CB_Reorg on_reorg_; | ||
| CB_String on_pool_tx_removed_; | ||
| CB_GetPassword on_get_password_; | ||
| }; |
There was a problem hiding this comment.
Security concern with raw function pointer casting.
The FunctionBasedListener class uses reinterpret_cast to convert opaque uintptr_t values to function pointers. This is inherently unsafe and could lead to crashes or code execution if incorrect pointers are provided.
Consider adding validation or using a safer callback mechanism:
- Add null pointer checks before casting
- Consider using a vtable-based approach instead of raw function pointers
- Add documentation about the expected function signatures
void moneySpent(const std::string& txid, uint64_t amt) override {
if (on_spent_) {
+ // Add basic validation
+ if (on_spent_ == 0) return;
auto* spent = reinterpret_cast<void(*)(const std::string&, uint64_t)>(on_spent_);
spent(txid, amt);
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In monero-sys/src/bridge.h between lines 300 and 398, the FunctionBasedListener
class unsafely uses reinterpret_cast to convert opaque uintptr_t values to
function pointers without validation, risking crashes or security issues. To fix
this, add explicit null pointer checks before each cast and invocation to ensure
the pointers are valid. Additionally, consider refactoring to use a safer
callback mechanism such as a vtable or std::function to avoid raw pointer
casting. Finally, document the expected function signatures clearly to guide
correct usage and prevent misuse.
| // bridge.h | ||
| #pragma once | ||
| #include <string> | ||
| #include <cstdint> | ||
| #include "wallet/api/wallet2_api.h" | ||
|
|
||
|
|
||
| using CB_StringU64 = uintptr_t; | ||
| using CB_U64 = uintptr_t; | ||
| using CB_Void = uintptr_t; | ||
| using CB_Reorg = uintptr_t; | ||
| using CB_String = uintptr_t; | ||
| using CB_GetPassword = uintptr_t; |
There was a problem hiding this comment.
Remove duplicate pragma and includes.
The file already has #pragma once at the top. These duplicate preprocessor directives and includes should be removed.
Apply this diff to remove the duplicates:
-// bridge.h
-#pragma once
-#include <string>
-#include <cstdint>
-#include "wallet/api/wallet2_api.h"
-
-📝 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.
| // bridge.h | |
| #pragma once | |
| #include <string> | |
| #include <cstdint> | |
| #include "wallet/api/wallet2_api.h" | |
| using CB_StringU64 = uintptr_t; | |
| using CB_U64 = uintptr_t; | |
| using CB_Void = uintptr_t; | |
| using CB_Reorg = uintptr_t; | |
| using CB_String = uintptr_t; | |
| using CB_GetPassword = uintptr_t; | |
| using CB_StringU64 = uintptr_t; | |
| using CB_U64 = uintptr_t; | |
| using CB_Void = uintptr_t; | |
| using CB_Reorg = uintptr_t; | |
| using CB_String = uintptr_t; | |
| using CB_GetPassword = uintptr_t; |
🤖 Prompt for AI Agents
In monero-sys/src/bridge.h around lines 286 to 298, there are duplicate #pragma
once directives and repeated include statements. Remove the redundant #pragma
once and duplicate #include lines so that each directive and include appears
only once at the top of the file to avoid unnecessary repetition.
| optional<std::string> onGetPassword(const char* reason) override { | ||
| if (on_get_password_) { | ||
| auto* get_password = reinterpret_cast<const char*(*)(const std::string&)>(on_get_password_); | ||
| return std::string(get_password(reason)); | ||
| } | ||
| return optional<std::string>(); | ||
| } |
There was a problem hiding this comment.
Potential memory safety issue with password callback.
The onGetPassword implementation assumes the returned const char* from the callback is valid and can be safely converted to std::string. This could cause undefined behavior if the callback returns an invalid pointer.
Add null pointer check and consider lifetime management:
optional<std::string> onGetPassword(const char* reason) override {
if (on_get_password_) {
auto* get_password = reinterpret_cast<const char*(*)(const std::string&)>(on_get_password_);
- return std::string(get_password(reason));
+ const char* result = get_password(reason);
+ if (result != nullptr) {
+ return std::string(result);
+ }
}
return optional<std::string>();
}📝 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.
| optional<std::string> onGetPassword(const char* reason) override { | |
| if (on_get_password_) { | |
| auto* get_password = reinterpret_cast<const char*(*)(const std::string&)>(on_get_password_); | |
| return std::string(get_password(reason)); | |
| } | |
| return optional<std::string>(); | |
| } | |
| optional<std::string> onGetPassword(const char* reason) override { | |
| if (on_get_password_) { | |
| auto* get_password = reinterpret_cast<const char*(*)(const std::string&)>(on_get_password_); | |
| const char* result = get_password(reason); | |
| if (result != nullptr) { | |
| return std::string(result); | |
| } | |
| } | |
| return optional<std::string>(); | |
| } |
🤖 Prompt for AI Agents
In monero-sys/src/bridge.h around lines 380 to 386, the onGetPassword method
assumes the callback returns a valid const char* pointer, which may lead to
undefined behavior if it is null or invalid. To fix this, add a null pointer
check after calling the callback before converting to std::string. If the
pointer is null, return an empty optional or handle the error safely. Also,
ensure the lifetime of the returned pointer is managed properly to avoid
dangling references.
| async fn request_and_open_monero_wallet( | ||
| tauri_handle: Option<TauriHandle>, | ||
| ) -> Result<Arc<Wallets>> { | ||
| let network = env_config.monero_network; | ||
| let wallet_dir = data_dir.join("monero").join("monero-data"); | ||
| eigenwallet_data_dir: &PathBuf, | ||
| legacy_data_dir: &PathBuf, | ||
| env_config: EnvConfig, | ||
| daemon: &monero_sys::Daemon, | ||
| wallet_database: &monero_sys::Database, | ||
| ) -> Result<(monero_sys::WalletHandle, Seed), Error> { | ||
| let eigenwallet_wallets_dir = eigenwallet_data_dir.join("wallets"); | ||
|
|
||
| let wallet = match tauri_handle { | ||
| Some(tauri_handle) => { | ||
| // Get recent wallets from database | ||
| let recent_wallets: Vec<String> = wallet_database | ||
| .get_recent_wallets(5) | ||
| .await | ||
| .unwrap_or_default() | ||
| .into_iter() | ||
| .map(|w| w.wallet_path) | ||
| .collect(); | ||
|
|
||
| // This loop continually requests the user to select a wallet file | ||
| // It then requests the user to provide a password. | ||
| // It repeats until the user provides a valid password or rejects the password request | ||
| // When the user rejects the password request, we prompt him to select a wallet again | ||
| loop { | ||
| let seed_choice = tauri_handle | ||
| .request_seed_selection_with_recent_wallets(recent_wallets.clone()) | ||
| .await?; | ||
|
|
||
| let daemon = monero_sys::Daemon { | ||
| address: monero_daemon_address, | ||
| ssl: false, | ||
| }; | ||
| let _monero_progress_handle = tauri_handle | ||
| .new_background_process_with_initial_progress( | ||
| TauriBackgroundProgress::OpeningMoneroWallet, | ||
| (), | ||
| ); | ||
|
|
||
| // This is the name of a wallet we only use for blockchain monitoring | ||
| const DEFAULT_WALLET: &str = "swap-tool-blockchain-monitoring-wallet"; | ||
|
|
||
| // Remove the monitoring wallet if it exists | ||
| // It doesn't contain any coins | ||
| // Deleting it ensures we never have issues at startup | ||
| // And we reset the restore height | ||
| let wallet_path = wallet_dir.join(DEFAULT_WALLET); | ||
| if wallet_path.exists() { | ||
| tracing::debug!( | ||
| wallet_path = %wallet_path.display(), | ||
| "Removing monitoring wallet" | ||
| ); | ||
| let _ = tokio::fs::remove_file(&wallet_path).await; | ||
| } | ||
| let keys_path = wallet_path.with_extension("keys"); | ||
| if keys_path.exists() { | ||
| tracing::debug!( | ||
| keys_path = %keys_path.display(), | ||
| "Removing monitoring wallet keys" | ||
| ); | ||
| let _ = tokio::fs::remove_file(keys_path).await; | ||
| } | ||
| fn new_wallet_path(eigenwallet_wallets_dir: &PathBuf) -> Result<PathBuf> { | ||
| let timestamp = std::time::SystemTime::now() | ||
| .duration_since(std::time::UNIX_EPOCH) | ||
| .unwrap() | ||
| .as_secs(); | ||
|
|
||
| let wallets = monero::Wallets::new( | ||
| wallet_dir, | ||
| DEFAULT_WALLET.to_string(), | ||
| daemon, | ||
| network, | ||
| false, | ||
| tauri_handle, | ||
| ) | ||
| .await | ||
| .context("Failed to initialize Monero wallets")?; | ||
| let wallet_path = eigenwallet_wallets_dir.join(format!("wallet_{}", timestamp)); | ||
|
|
||
| if let Some(parent) = wallet_path.parent() { | ||
| swap_fs::ensure_directory_exists(parent) | ||
| .context("Failed to create wallet directory")?; | ||
| } | ||
|
|
||
| Ok(wallet_path) | ||
| } | ||
|
|
||
| let wallet = match seed_choice { | ||
| SeedChoice::RandomSeed => { | ||
| // Create wallet with Unix timestamp as name | ||
| let wallet_path = new_wallet_path(&eigenwallet_wallets_dir) | ||
| .context("Failed to determine path for new wallet")?; | ||
|
|
||
| monero::Wallet::open_or_create( | ||
| wallet_path.display().to_string(), | ||
| daemon.clone(), | ||
| env_config.monero_network, | ||
| true, | ||
| ) | ||
| .await | ||
| .context("Failed to create wallet from random seed")? | ||
| } | ||
| SeedChoice::FromSeed { seed: mnemonic } => { | ||
| // Create wallet from provided seed | ||
| let wallet_path = new_wallet_path(&eigenwallet_wallets_dir) | ||
| .context("Failed to determine path for new wallet")?; | ||
|
|
||
| monero::Wallet::open_or_create_from_seed( | ||
| wallet_path.display().to_string(), | ||
| mnemonic, | ||
| env_config.monero_network, | ||
| 0, | ||
| true, | ||
| daemon.clone(), | ||
| ) | ||
| .await | ||
| .context("Failed to create wallet from provided seed")? | ||
| } | ||
| SeedChoice::FromWalletPath { wallet_path } => { | ||
| // Helper function to verify password | ||
| let verify_password = |password: String| -> Result<bool> { | ||
| monero_sys::WalletHandle::verify_wallet_password( | ||
| wallet_path.clone(), | ||
| password, | ||
| ) | ||
| .map_err(|e| anyhow::anyhow!("Failed to verify wallet password: {}", e)) | ||
| }; | ||
|
|
||
| // Request and verify password before opening wallet | ||
| let wallet_password: Option<String> = { | ||
| const WALLET_EMPTY_PASSWORD: &str = ""; | ||
|
|
||
| // First try empty password | ||
| if verify_password(WALLET_EMPTY_PASSWORD.to_string())? { | ||
| Some(WALLET_EMPTY_PASSWORD.to_string()) | ||
| } else { | ||
| // If empty password fails, ask user for password | ||
| loop { | ||
| // Request password from user | ||
| let password = tauri_handle | ||
| .request_password(wallet_path.clone()) | ||
| .await | ||
| .inspect_err(|e| { | ||
| tracing::error!( | ||
| "Failed to get password from user: {}", | ||
| e | ||
| ); | ||
| }) | ||
| .ok(); | ||
|
|
||
| // If the user rejects the password request (presses cancel) | ||
| // We prompt him to select a wallet again | ||
| let password = match password { | ||
| Some(password) => password, | ||
| None => break None, | ||
| }; | ||
|
|
||
| // Verify the password using the helper function | ||
| match verify_password(password.clone()) { | ||
| Ok(true) => { | ||
| break Some(password); | ||
| } | ||
| Ok(false) => { | ||
| // Continue loop to request password again | ||
| continue; | ||
| } | ||
| Err(e) => { | ||
| return Err(e); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| let password = match wallet_password { | ||
| Some(password) => password, | ||
| // None means the user rejected the password request | ||
| // We prompt him to select a wallet again | ||
| None => { | ||
| continue; | ||
| } | ||
| }; | ||
|
|
||
| // Open existing wallet with verified password | ||
| monero::Wallet::open_or_create_with_password( | ||
| wallet_path.clone(), | ||
| password, | ||
| daemon.clone(), | ||
| env_config.monero_network, | ||
| true, | ||
| ) | ||
| .await | ||
| .context("Failed to open wallet from provided path")? | ||
| } | ||
|
|
||
| SeedChoice::Legacy => { | ||
| let wallet = request_and_open_monero_wallet_legacy(legacy_data_dir, env_config, daemon).await?; | ||
| let seed = Seed::from_file_or_generate(legacy_data_dir) | ||
| .await | ||
| .context("Failed to extract seed from wallet")?; | ||
|
|
||
| break (wallet, seed); | ||
| } | ||
| }; | ||
|
|
||
| // Extract seed from the wallet | ||
| tracing::info!("Extracting seed from wallet directory: {}", legacy_data_dir.display()); | ||
| let seed = Seed::from_monero_wallet(&wallet) | ||
| .await | ||
| .context("Failed to extract seed from wallet")?; | ||
|
|
||
| break (wallet, seed); | ||
| } | ||
| } | ||
|
|
||
| Ok(Arc::new(wallets)) | ||
| // If we don't have a tauri handle, we use the seed.pem file | ||
| // This is used for the CLI to monitor the blockchain | ||
| None => { | ||
| let wallet = request_and_open_monero_wallet_legacy(legacy_data_dir, env_config, daemon).await?; | ||
| let seed = Seed::from_file_or_generate(legacy_data_dir) | ||
| .await | ||
| .context("Failed to extract seed from wallet")?; | ||
|
|
||
| (wallet, seed) | ||
| } | ||
| }; | ||
|
|
||
| Ok(wallet) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Complex wallet selection logic needs validation.
The request_and_open_monero_wallet function implements a comprehensive wallet selection flow but has several areas that need attention:
- Password verification loop security - Already flagged in previous reviews
- Error handling - Some error paths may not provide clear feedback to users
- Wallet path validation - No validation that wallet paths are secure/valid
Consider adding:
- Wallet path sanitization to prevent directory traversal
- Better error messages for users
- Validation of wallet file existence before attempting to open
- Timeout mechanism for the outer wallet selection loop
// Add path validation
+fn validate_wallet_path(path: &str) -> Result<PathBuf> {
+ let path = PathBuf::from(path);
+ let canonical = path.canonicalize()
+ .context("Invalid wallet path")?;
+
+ // Ensure path is within expected directory
+ if !canonical.starts_with(expected_wallet_dir) {
+ bail!("Wallet path is outside allowed directory");
+ }
+
+ Ok(canonical)
+}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In swap/src/cli/api.rs from lines 643 to 836, improve the wallet selection logic
by adding validation and security checks: sanitize wallet paths to prevent
directory traversal attacks before using them; check that wallet files exist
before attempting to open them and provide clear user-facing error messages if
not; enhance error handling to give more informative feedback on failures; and
implement a timeout or maximum retry count for the outer wallet selection loop
to avoid infinite looping if the user repeatedly cancels or fails password
verification.
There was a problem hiding this comment.
Bug: Badge Displays Incorrect Swap Count
The UnfinishedSwapsBadge component calculates displayedResumableSwapsCount (which subtracts 1 if a swap is running) and uses it to determine badge visibility. However, the badge content incorrectly displays the original resumableSwapsCount instead of the adjusted displayedResumableSwapsCount, leading to an inconsistent and unadjusted count being shown.
src-gui/src/renderer/components/navigation/UnfinishedSwapsCountBadge.tsx#L12-L20
Bug: ScrollablePaperTextBox Ignores MinHeight Prop
The VList height in ScrollablePaperTextBox is hardcoded to "100vh". This change, made to fix an undefined MIN_HEIGHT reference, causes the component to always occupy full viewport height, disregarding its minHeight prop and breaking its sizing contract. This can lead to layout issues and overflow when used in height-constrained containers.
src-gui/src/renderer/components/other/ScrollablePaperTextBox.tsx#L83-L84
core/src-gui/src/renderer/components/other/ScrollablePaperTextBox.tsx
Lines 83 to 84 in 3e605ac
Was this report helpful? Give feedback by reacting with 👍 or 👎
4 participants
This pull request will add support for the GUI to be used as a full functioning Monero wallet, similar to Feather Wallet or the Monero Wallet GUI.
TODO
WalletListenerand send update events from Rust to Frontendseed.pemto restore the walletMigration for oldI think we cannot cleanly migrate. Users have to rollback if they still have funds on the old version.seem.pemto our new system where the seed lives inside the Monero wallet file (Ifseed.pemexists, create a new walletautocreated_walletin the eigenwallet directory. Copy all files inside the top level data dir (sqlite, monero folder, ...) into the corresponding identities subdirectory, then create a.multi_wallet_migration_doneto let us skip this step later on)[ ] Allow setting restore height when creating wallet from existing seedvirtual bool store(const std::string &path) = 0;when we receive or send fundsargsfor commandsend_monero: invalid type: floating point135938976171.99998, expected u64")Some issues I encountered
thread 'wallet-swap-tool-blockchain-monitoring-wallet' panicked at monero-sys/src/lib.rs:728:18: failed to send result back to caller: Any { .. } note: run with RUST_BACKTRACE=1 environment variable to display a backtraceSummary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores
Documentation