feat(gui, cli): Request quotes concurrently at all sellers#429
feat(gui, cli): Request quotes concurrently at all sellers#429binarybaron merged 74 commits intomasterfrom
Conversation
- refactored file structure to match common projecte structure - implement step get bitcoin
binarybaron
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
swap/src/cli/api/request.rs (6)
594-595: Remove redundant span creation.A tracing span is already created at line 83. This additional span creation is redundant.
- let _span = get_swap_tracing_span(swap_id); -
659-666: Emit accurate initial values instead of hardcoded zeros.The initial
WaitingForBtcDepositevent uses hardcoded zero amounts which could confuse users. Consider fetching the actual wallet balance first.+ let initial_balance = bitcoin_wallet.balance().await?; + let initial_max_giveable = bitcoin_wallet.max_giveable(address_len).await?.0; + context.tauri_handle.emit_swap_progress_event( swap_id, TauriSwapProgressEvent::WaitingForBtcDeposit { deposit_address: bitcoin_change_address, - max_giveable: bitcoin::Amount::ZERO, - min_bitcoin_lock_tx_fee: bitcoin::Amount::ZERO, + max_giveable: initial_max_giveable, + min_bitcoin_lock_tx_fee: initial_balance.saturating_sub(initial_max_giveable), }, );
774-777: Remove duplicate event emission.The
ReceivedQuoteevent is emitted twice with the same data (lines 775 and 786).context.tauri_handle.emit_swap_progress_event( swap_id, TauriSwapProgressEvent::ReceivedQuote(quote.clone()), ); // Now create the event loop we use for the swap let (event_loop, event_loop_handle) = EventLoop::new(swap_id, swarm, seller_peer_id, context.db.clone())?; let event_loop = tokio::spawn(event_loop.run().in_current_span()); - context - .tauri_handle - .emit_swap_progress_event(swap_id, TauriSwapProgressEvent::ReceivedQuote(quote));Also applies to: 786-786
1507-1509: Properly clean up background tasks.The tasks are aborted but not awaited, which could lead to resource leaks.
quote_fetch_handle.abort(); wallet_refresh_handle.abort(); + // Wait for tasks to actually finish + let _ = quote_fetch_handle.await; + let _ = wallet_refresh_handle.await;
1304-1337: Improve error handling and make interval configurable.The function silently ignores sync errors and uses a hardcoded refresh interval.
pub async fn refresh_wallet_task<FMG, TMG, FB, TB, FS, TS>( max_giveable_fn: FMG, balance_fn: FB, sync_fn: FS, + refresh_interval: Duration, ) -> Result<( tokio::task::JoinHandle<()>, ::tokio::sync::watch::Receiver<(bitcoin::Amount, bitcoin::Amount)>, )> // ... generic constraints ... { let (tx, rx) = ::tokio::sync::watch::channel((bitcoin::Amount::ZERO, bitcoin::Amount::ZERO)); let handle = tokio::task::spawn(async move { loop { // Sync wallet before checking balance - let _ = sync_fn().await; + if let Err(e) = sync_fn().await { + tracing::warn!("Failed to sync wallet: {:#}", e); + // Continue anyway to try to get the balance + } if let (Ok(balance), Ok((max_giveable, _fee))) = (balance_fn().await, max_giveable_fn().await) { - let _ = tx.send((balance, max_giveable)); + if tx.send((balance, max_giveable)).is_err() { + tracing::debug!("Wallet refresh receiver dropped, stopping task"); + break; + } } - tokio::time::sleep(std::time::Duration::from_secs(5)).await; + tokio::time::sleep(refresh_interval).await; } }); Ok((handle, rx)) }
1262-1301: Add error handling and make the refresh interval configurable.The function has several issues:
- The 15-second interval is hardcoded
- No error handling in the spawned task
- No way to cleanly stop the task
pub async fn fetch_quotes_task( rendezvous_points: Vec<Multiaddr>, namespace: XmrBtcNamespace, sellers: Vec<Multiaddr>, identity: identity::Keypair, db: Option<Arc<dyn Database + Send + Sync>>, tor_client: Option<Arc<TorClient<TokioRustlsRuntime>>>, tauri_handle: Option<TauriHandle>, + refresh_interval: Duration, ) -> Result<( tokio::task::JoinHandle<()>, ::tokio::sync::watch::Receiver<Vec<SellerStatus>>, )> { // ... existing code ... let handle = tokio::task::spawn(async move { loop { - let sellers = fetch_fn().await; - let _ = tx.send(sellers); - tokio::time::sleep(std::time::Duration::from_secs(15)).await; + match fetch_fn().await { + Ok(sellers) => { + if tx.send(sellers).is_err() { + tracing::debug!("Quote fetch receiver dropped, stopping task"); + break; + } + } + Err(e) => { + tracing::error!("Failed to fetch quotes: {:#}", e); + } + } + tokio::time::sleep(refresh_interval).await; } }); Ok((handle, rx)) }
🧹 Nitpick comments (1)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx (1)
173-256: Remove unnecessary Fragment wrapper.The Fragment wrapper is unnecessary since it only contains one child element. This aligns with the static analysis hint from Biome.
- {pendingSelectMakerApprovals.length > 0 && ( - <> - <Box sx={{ display: "flex", flexDirection: "column", gap: 1 }}> + {pendingSelectMakerApprovals.length > 0 && ( + <Box sx={{ display: "flex", flexDirection: "column", gap: 1 }}> {pendingSelectMakerApprovals.map((makerApproval, index) => { // ... existing content })} - </Box> - </> - )} + </Box> + )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx(1 hunks)swap/src/cli/api/request.rs(10 hunks)swap/src/cli/api/tauri_bindings.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- swap/src/cli/api/tauri_bindings.rs
🧰 Additional context used
🪛 Biome (1.9.4)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx
[error] 173-256: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (6)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx (3)
17-135: Well-implemented progress indicator component.The
MakerDiscoveryStatuscomponent effectively communicates the background quote fetching process to users. The progress calculation logic and visual feedback are well-designed.
142-142: Good integration with the new hook.The component correctly integrates with the new
usePendingSelectMakerApprovalhook to handle multiple seller approvals.
175-253: Effective offer display and interaction design.The offer rendering logic is clean and provides good UX with clear maker information, pricing details, and actionable buttons. The approval handling through
resolveApprovalis properly implemented.swap/src/cli/api/request.rs (3)
59-63: Approve the multi-seller struct changes.The refactoring from a single
sellerfield torendezvous_pointsandsellersvectors correctly supports the new concurrent quote fetching architecture.
668-741: Well-designed concurrent architecture with proper cancellation.The new approach using
tokio::select!with background tasks and watch channels is well-implemented. The cancellation handling with swap lock suspension is robust.
1340-1517: Solid implementation of multi-seller approval logic.The
determine_btc_to_swapfunction effectively handles concurrent approvals with proper state management. The use ofFuturesUnorderedfor managing multiple approval requests is appropriate.
|
I think this leaks some objects in some cases (list_sellers event loop?) |
…/UnstoppableSwap/core into feat/concurrent-quotes-swap-init
There was a problem hiding this comment.
Actionable comments posted: 2
📜 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 (4)
src-gui/src/models/tauriModelExt.ts(1 hunks)src-gui/src/renderer/components/modal/swap/SwapDialog.tsx(2 hunks)src-gui/src/renderer/components/modal/swap/SwapStateStepper.tsx(1 hunks)swap/Cargo.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- swap/Cargo.toml
- src-gui/src/models/tauriModelExt.ts
🔇 Additional comments (7)
src-gui/src/renderer/components/modal/swap/SwapDialog.tsx (4)
16-17: LGTM: Clean imports for enhanced cancellation logic.The new imports support the improved cancellation flow by providing utility functions to check fund lock status and handle async swap suspension.
34-42: LGTM: Well-implemented async cancellation logic.The enhanced cancellation logic properly handles different scenarios:
- Shows suspend alert when funds are locked and swap is running
- Asynchronously suspends the swap before cleanup when funds aren't locked
- Maintains proper promise chain with cleanup after suspension
This is a significant improvement over immediate cancellation without proper resource cleanup.
16-17: Good addition of utility imports for enhanced cancellation logic.The new imports support the improved cancellation flow that differentiates between locked and unlocked fund states.
16-17: LGTM: Import statements are properly added.The new imports align with the enhanced swap suspension flow and fund locking checks.
src-gui/src/renderer/components/modal/swap/SwapStateStepper.tsx (3)
59-59: LGTM: Clear explicit handling for pre-fund-locking states.The explicit
nullreturn with a clear comment properly indicates that no stepper should be shown during these initial swap phases. This aligns well with thehaveFundsBeenLockedlogic used elsewhere and improves code clarity.
56-59: Excellent clarification of stepper behavior for early swap states.Explicitly returning
nullfor the early swap states (RequestingQuote,ReceivedQuote,WaitingForBtcDeposit) with a clear comment improves code readability and aligns with the newhaveFundsBeenLockedutility function logic. This ensures the stepper UI correctly reflects that no progress steps should be shown when funds haven't been locked yet.
56-59: LGTM: Logical consistency with fund locking states.The explicit
return nullfor early swap states correctly represents phases where no funds have been locked yet, which aligns with thehaveFundsBeenLockedutility function used inSwapDialog.tsx.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
swap/src/cli/api/request.rs (6)
603-604: Remove redundant span creation.A tracing span is already created in line 83 via
get_swap_tracing_span(swap_id). This additional span creation is redundant.- let _span = get_swap_tracing_span(swap_id); -
668-675: Remove placeholder values as requested.The initial
WaitingForBtcDepositevent uses hardcoded zero amounts which could confuse users. As noted in your comment, we shouldn't send any placeholder values.
783-796: Remove duplicate event emission.The
ReceivedQuoteevent is emitted twice with the same data (lines 785 and 795).- context - .tauri_handle - .emit_swap_progress_event(swap_id, TauriSwapProgressEvent::ReceivedQuote(quote));
1311-1345: Improve error handling and make interval configurable.The function has several issues that were previously identified:
- The 5-second interval is hardcoded
- No error handling in the spawned task
- No way to cleanly stop the task
pub async fn refresh_wallet_task<FMG, TMG, FB, TB, FS, TS>( max_giveable_fn: FMG, balance_fn: FB, sync_fn: FS, + refresh_interval: Duration, ) -> Result<( tokio::task::JoinHandle<()>, ::tokio::sync::watch::Receiver<(bitcoin::Amount, bitcoin::Amount)>, )> // ... generic constraints ... { let (tx, rx) = ::tokio::sync::watch::channel((bitcoin::Amount::ZERO, bitcoin::Amount::ZERO)); let handle = tokio::task::spawn(async move { loop { // Sync wallet before checking balance - let _ = sync_fn().await; + if let Err(e) = sync_fn().await { + tracing::warn!("Failed to sync wallet: {:#}", e); + // Continue anyway to try to get the balance + } if let (Ok(balance), Ok((max_giveable, _fee))) = (balance_fn().await, max_giveable_fn().await) { - let _ = tx.send((balance, max_giveable)); + if tx.send((balance, max_giveable)).is_err() { + tracing::debug!("Wallet refresh receiver dropped, stopping task"); + break; + } } - tokio::time::sleep(std::time::Duration::from_secs(5)).await; + tokio::time::sleep(refresh_interval).await; } }); Ok((handle, rx)) }
1515-1518: Properly clean up background tasks.The tasks are aborted but not awaited, which could lead to resource leaks.
- quote_fetch_handle.abort(); - wallet_refresh_handle.abort(); + quote_fetch_handle.abort(); + wallet_refresh_handle.abort(); + // Wait for tasks to actually finish + let _ = quote_fetch_handle.await; + let _ = wallet_refresh_handle.await;
1270-1309: Address hardcoded interval and use sellers parameter.The function has a hardcoded 15-second refresh interval and doesn't use the
sellersparameter as intended. As noted in your comment, this should "use the sellers that we pass in here, pass into list_sellers_init or let fetch_fn take a Vec".pub async fn fetch_quotes_task( rendezvous_points: Vec<Multiaddr>, namespace: XmrBtcNamespace, sellers: Vec<Multiaddr>, identity: identity::Keypair, db: Option<Arc<dyn Database + Send + Sync>>, tor_client: Option<Arc<TorClient<TokioRustlsRuntime>>>, tauri_handle: Option<TauriHandle>, + refresh_interval: Duration, ) -> Result<( tokio::task::JoinHandle<()>, ::tokio::sync::watch::Receiver<Vec<SellerStatus>>, )> { // ... existing code ... let handle = tokio::task::spawn(async move { loop { - let sellers = fetch_fn().await; - let _ = tx.send(sellers); - tokio::time::sleep(std::time::Duration::from_secs(15)).await; + match fetch_fn().await { + Ok(sellers) => { + if tx.send(sellers).is_err() { + tracing::debug!("Quote fetch receiver dropped, stopping task"); + break; + } + } + Err(e) => { + tracing::error!("Failed to fetch quotes: {:#}", e); + } + } + tokio::time::sleep(refresh_interval).await; } }); Ok((handle, rx)) }
🧹 Nitpick comments (2)
swap/src/cli/api/request.rs (2)
1347-1382: Consider refactoring to reduce complexity.The function signature has many generic parameters making it difficult to read and maintain. Consider:
- Extract a configuration struct for the function parameters:
pub struct SwapDeterminationConfig<FB, FMG, FS, FQ> { pub quote_fetch_tasks: FQ, pub balance_fn: FB, pub max_giveable_fn: FMG, pub sync_fn: FS, pub event_emitter: Option<TauriHandle>, pub swap_id: Uuid, }
- Split the function into smaller, more focused functions:
start_background_tasks()- Start quote and wallet refresh taskshandle_approval_loop()- Main approval handling logiccleanup_tasks()- Proper task cleanupThis would improve readability and testability.
1347-1525: Address potential resource leaks mentioned in PR description.As noted in the PR description, there might be object leaks in certain cases. Key areas to verify:
- Task lifecycle management - Ensure all spawned tasks are properly cleaned up
- Watch channel receivers - Verify that dropping receivers properly signals senders to stop
- Approval request handling - Ensure pending approvals are properly cancelled when balance/quotes change
#!/bin/bash # Search for potential resource leak patterns echo "=== Searching for task spawning without proper cleanup ===" rg -A 5 -B 2 "tokio::task::spawn" --type rust echo "=== Searching for watch channel usage ===" rg -A 3 -B 2 "watch::channel\|watch::Receiver\|watch::Sender" --type rust echo "=== Searching for abort calls without await ===" rg -A 2 -B 2 "\.abort\(\)" --type rust
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src-gui/src/renderer/components/modal/swap/SwapDialog.tsx(2 hunks)src-gui/src/renderer/rpc.ts(3 hunks)swap/src/cli/api/request.rs(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src-gui/src/renderer/components/modal/swap/SwapDialog.tsx
- src-gui/src/renderer/rpc.ts
🔇 Additional comments (3)
swap/src/cli/api/request.rs (3)
2-2: LGTM! Import changes support the new multi-seller functionality.The new imports for
futures,TorClient, and other dependencies are appropriately added to support the concurrent quote fetching and multi-seller swap functionality.Also applies to: 10-11, 14-14, 20-25, 34-34
60-64: LGTM! Struct change enables multi-seller functionality.The change from a single
sellerfield torendezvous_pointsandsellersvectors properly supports the new multi-seller approach with appropriate type annotations.
1248-1251: LGTM! Improved type safety with structured response.The change from returning raw JSON to a typed
GetCurrentSwapResponsestruct improves type safety and API consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx (2)
17-135: Consider optimizing progress calculations with useMemo.The progress calculations in the MakerDiscoveryStatus component run on every render. Consider memoizing these calculations to improve performance.
+import { useMemo } from 'react'; function MakerDiscoveryStatus() { const backgroundProcesses = usePendingBackgroundProcesses(); // Find active ListSellers processes const listSellersProcesses = backgroundProcesses.filter( ([, status]) => status.componentName === "ListSellers" && status.progress.type === "Pending", ); - const isActive = listSellersProcesses.length > 0; - - // Default values for inactive state - let progress = { - rendezvous_points_total: 0, - peers_discovered: 0, - rendezvous_points_connected: 0, - quotes_received: 0, - quotes_failed: 0, - }; - let progressValue = 0; - - if (isActive) { - // Use the first ListSellers process for display - const [, status] = listSellersProcesses[0]; - - // Type guard to ensure we have ListSellers progress - if ( - status.componentName === "ListSellers" && - status.progress.type === "Pending" - ) { - progress = status.progress.content; - - const totalExpected = - progress.rendezvous_points_total + progress.peers_discovered; - const totalCompleted = - progress.rendezvous_points_connected + - progress.quotes_received + - progress.quotes_failed; - progressValue = - totalExpected > 0 ? (totalCompleted / totalExpected) * 100 : 0; - } - } + const { isActive, progress, progressValue } = useMemo(() => { + const isActive = listSellersProcesses.length > 0; + + // Default values for inactive state + let progress = { + rendezvous_points_total: 0, + peers_discovered: 0, + rendezvous_points_connected: 0, + quotes_received: 0, + quotes_failed: 0, + }; + let progressValue = 0; + + if (isActive) { + // Use the first ListSellers process for display + const [, status] = listSellersProcesses[0]; + + // Type guard to ensure we have ListSellers progress + if ( + status.componentName === "ListSellers" && + status.progress.type === "Pending" + ) { + progress = status.progress.content; + + const totalExpected = + progress.rendezvous_points_total + progress.peers_discovered; + const totalCompleted = + progress.rendezvous_points_connected + + progress.quotes_received + + progress.quotes_failed; + progressValue = + totalExpected > 0 ? (totalCompleted / totalExpected) * 100 : 0; + } + } + + return { isActive, progress, progressValue }; + }, [listSellersProcesses]);
173-252: Remove unnecessary Fragment wrapper.The Fragment is unnecessary since it wraps multiple elements that can be rendered directly.
{pendingSelectMakerApprovals.length > 0 && ( - <> <Box sx={{ display: "flex", flexDirection: "column", gap: 1 }}> {pendingSelectMakerApprovals.map((makerApproval, index) => { const { request_id, details } = makerApproval.content; const { maker } = details.content; return ( <Box key={request_id}> <Box sx={{ display: "flex", alignItems: "center", gap: 2, py: 1.5, px: 2, backgroundColor: "background.default", borderRadius: 1, border: "1px solid", borderColor: "divider", }} > {/* Icon and ID */} <Box sx={{ display: "flex", alignItems: "center", gap: 1.5, flex: 1, }} > <IdentIcon value={maker.peer_id} size="2rem" /> <Box sx={{ minWidth: 0 }}> <Typography variant="body2" sx={{ fontWeight: "medium" }} > <TruncatedText limit={12} truncateMiddle> {maker.peer_id} </TruncatedText> </Typography> <Typography variant="caption" color="textSecondary"> v{maker.version} </Typography> </Box> </Box> {/* Price */} <Box sx={{ textAlign: "center", minWidth: 120 }}> <Typography variant="body2" sx={{ fontWeight: "medium" }} > <MoneroBitcoinExchangeRate rate={satsToBtc(maker.quote.price)} /> </Typography> <Typography variant="caption" color="textSecondary"> <SatsAmount amount={maker.quote.min_quantity} /> -{" "} <SatsAmount amount={maker.quote.max_quantity} /> </Typography> </Box> {/* Accept Button */} <PromiseInvokeButton variant="contained" color="success" size="large" sx={{ minWidth: 100, fontWeight: "bold" }} onInvoke={() => resolveApproval(request_id, true)} displayErrorSnackbar > Accept </PromiseInvokeButton> </Box> </Box> ); })} </Box> - </> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.cursor/rules/never-upgrade-deps-without-explicit-confirmation.mdc(1 hunks)src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/never-upgrade-deps-without-explicit-confirmation.mdc
🧰 Additional context used
🪛 Biome (1.9.4)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx
[error] 173-252: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
src-gui/src/renderer/components/modal/swap/pages/init/WaitingForBitcoinDepositPage.tsx
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Incorrect Swap ID Check in CancelButton
The CancelButton component incorrectly checks if (swapId !== null) to determine if a swap is active. The getCurrentSwapId() function returns a GetCurrentSwapResponse object with a nested swap_id field, not the swap ID directly. This causes the condition to always evaluate as true, even when no swap is running, leading to incorrect attempts to suspend a non-existent swap. The correct check should be if (swapId.swap_id !== null).
src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx#L14-L30
Bug: Error Handling Overlooked in New Function
The newly added list_sellers_init function uses .unwrap() on the results of swarm::cli() and db.get_all_peer_addresses(). This can cause the application to panic if these operations fail, despite TODO comments indicating awareness of the issue.
swap/src/cli/list_sellers.rs#L86-L96
Bug: Mutex Lock Failure Causes Stale Approvals
The ApprovalCleanupGuard::drop method uses try_lock() on the pending_approvals mutex. If the mutex is held by another task, try_lock() fails silently, preventing the pending approval from being removed from the map, the rejection response from being sent, and a rejection event from being emitted to the frontend. This results in stale approval requests and an inconsistent state between the frontend and backend.
swap/src/cli/api/tauri_bindings.rs#L798-L822
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Bug: Error Handling in Initialization
The list_sellers_init function uses unwrap() calls when creating the swarm and fetching peer addresses from the database. This can cause the application to panic if these operations fail, instead of handling errors gracefully. This is acknowledged by TODO comments in the code.
swap/src/cli/list_sellers.rs#L85-L97
Bug: Component Logic Errors Due to Incorrect Data Handling
The CancelButton component contains two bugs:
- The return value of
getCurrentSwapId()is an object{ swap_id: string | null }, but the code incorrectly treatsswapIdas a direct UUID ornullinstead of accessingswapId.swap_id. This causes theif (swapId !== null)condition to always be true and leads to incorrect logic whenswap_idisnull. - The
swap.stateobject from the Redux store can benull, butswap.state.curris accessed (e.g., inhaveFundsBeenLocked) without a null check, potentially causing a runtime error.
src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx#L15-L30
Bug: Function Parameter Type Mismatch
The getPageForState function's state parameter is incorrectly typed as SwapState. It is called with SwapState | null, which will cause a runtime error when null is passed, as the function's signature does not account for this possibility, despite an internal null check.
src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx#L28-L32
Bug: Mutex Lock Failure Blocks Approval Cleanup
The ApprovalCleanupGuard::drop implementation uses try_lock() on the pending_approvals mutex. If the mutex is already locked, try_lock() fails, causing the cleanup of approval requests to be skipped. This can lead to approval requests remaining indefinitely in the pending_approvals map, resulting in memory leaks and inconsistent state.
swap/src/cli/api/tauri_bindings.rs#L819-L830
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src-gui/src/renderer/components/pages/swap/swap/init/deposit_and_choose_offer/DepositAndChooseOfferPage.tsx (1)
130-155: Improve the key prop to use a stable identifier.While
startIndex + indexis better than justindex, it's still not a stable identifier that persists across re-renders and data changes. Use a unique property from the quote data instead:{paginatedOffers.map((quote, index) => { return ( <MakerOfferItem - key={startIndex + index} + key={quote.requestId || quote.quoteWithAddress.peer_id} quoteWithAddress={quote.quoteWithAddress} requestId={quote.requestId} /> ); })}This ensures React can properly track components across pagination and data updates.
🧹 Nitpick comments (2)
src-gui/src/renderer/components/pages/swap/swap/init/deposit_and_choose_offer/DepositAndChooseOfferPage.tsx (2)
20-39: Consider memoizing the data processing logic for better performance.The complex sorting and mapping operations run on every render. Consider wrapping this logic in
useMemoto optimize performance:+ import { useState, useMemo } from "react"; - const makerOffers = _.chain( + const makerOffers = useMemo(() => _.chain( sortBy( pendingSelectMakerApprovals, (approval) => -approval.content.expiration_ts, ), ) .map((approval) => ({ quoteWithAddress: approval.content.details.content.maker, requestId: approval.content.request_id, })) .concat( known_quotes.map((quote) => ({ quoteWithAddress: quote, requestId: null, })), ) .sortBy((quote) => quote.quoteWithAddress.quote.price) .sortBy((quote) => (quote.requestId ? 0 : 1)) // .uniqBy((quote) => quote.quoteWithAddress.peer_id) - .value(); + .value(), [pendingSelectMakerApprovals, known_quotes]);Note: The commented
.uniqBysuggests intentional duplicate handling - consider documenting why this is disabled.
157-171: Consider implementing the TODO for better loading state differentiation.The current implementation shows the same message whether makers are still being discovered or none were found. Consider implementing the TODO to provide more specific feedback to users about the actual state.
+ // You could add a prop or state to track discovery completion + {makerOffers.length === 0 && discoveryInProgress && ( + <Paper variant="outlined" sx={{ p: 3, textAlign: "center" }}> + <Typography variant="body1" color="textSecondary"> + Searching for available makers... + </Typography> + </Paper> + )} + + {makerOffers.length === 0 && !discoveryInProgress && ( + <Paper variant="outlined" sx={{ p: 3, textAlign: "center" }}> + <Typography variant="body1" color="textSecondary"> + No makers found. Please try again later. + </Typography> + </Paper> + )}Would you like me to help implement this loading state differentiation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src-gui/src/renderer/components/pages/swap/swap/init/deposit_and_choose_offer/DepositAndChooseOfferPage.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
PR: UnstoppableSwap/core#0
File: AGENT.md:0-0
Timestamp: 2025-06-30T08:53:49.168Z
Learning: Pull request titles must use the format `<type>(<scope>): <description>`, for example: `feat(gui): New feature`, `fix(swap): Issue fixed`, or `refactor(ci): Ci changes`.
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/pages/swap/swap/init/deposit_and_choose_offer/DepositAndChooseOfferPage.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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (4)
src-gui/src/renderer/components/pages/swap/swap/init/deposit_and_choose_offer/DepositAndChooseOfferPage.tsx (4)
1-9: Import structure looks good.All imports are properly organized and appear to be used within the component.
11-18: Component definition and state setup is well-structured.Good use of TypeScript generics for props typing and proper state management for pagination.
41-49: Pagination logic is correctly implemented.Standard and clean pagination calculations with proper event handling.
51-110: Layout structure and responsive design implementation looks excellent.Good use of Material-UI components with proper responsive breakpoints and consistent spacing throughout the deposit section.
There was a problem hiding this comment.
Bug: CancelButton: Null Handling & State Sync Issues
The CancelButton component has three issues:
- A null dereference when accessing
swap.state.currifswap.stateis null. - The
if (swapId !== null)condition is always true becausegetCurrentSwapId()returns an object, not theswap_iditself, making theelsebranch unreachable. It should checkswapId.swap_id !== null. - A state desynchronization between the backend's swap state (from
getCurrentSwapId) and the Redux store'sswap.state.curr, which can lead to incorrecthaveFundsBeenLockedchecks and missed suspend alerts.
src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx#L14-L31
Bug: Swarm Initialization and Peer Fetching Error Handling
The list_sellers_init function uses unwrap() when creating the swarm and when fetching peer addresses from the database. These unwrap() calls can lead to panics if the underlying operations fail, despite "TODO" comments acknowledging the issue. Proper error handling should be implemented.
swap/src/cli/list_sellers.rs#L85-L96
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
swap/src/cli/list_sellers.rs (1)
92-95: Handle swarm creation errors gracefully.The unwrap on swarm creation could cause a panic. This addresses the TODO comment and improves robustness.
Apply this diff to handle the error:
- // TODO: Dont use unwrap - let swarm = swarm::cli(identity, maybe_tor_client, behaviour) - .await - .unwrap(); + let swarm = match swarm::cli(identity, maybe_tor_client, behaviour).await { + Ok(swarm) => swarm, + Err(e) => { + tracing::error!("Failed to create swarm: {}", e); + return Vec::new(); // Return empty list on swarm creation failure + } + };swap/src/cli/api/request.rs (8)
614-615: Remove redundant span creation.A tracing span is already created in line 83 via
get_swap_tracing_span(swap_id). This additional span creation is redundant.- let _span = get_swap_tracing_span(swap_id); -
677-679: Acquire swap lock after seller selection to improve UX.The swap lock is acquired too early in the process, before the user has even selected a seller. This could block other swap attempts unnecessarily during the quote discovery and selection phase.
Consider moving the lock acquisition to after the user has selected a maker, just before starting the actual swap process.
783-786: Remove duplicate event emission.The
ReceivedQuoteevent is emitted twice with the same data (lines 783-786 and 795).- context.tauri_handle.emit_swap_progress_event( - swap_id, - TauriSwapProgressEvent::ReceivedQuote(quote.clone()), - );Remove one of the duplicate calls to avoid redundant event emission.
Also applies to: 795-795
1270-1311: Fix sellers parameter usage and improve error handling.The function accepts a
sellersparameter but doesn't use it properly, and has poor error handling with a hardcoded refresh interval.The issues include:
- Hardcoded 15-second refresh interval should be configurable
- Missing error handling in the spawned task
- The
sellersparameter should be passed to the quote fetching logic- No mechanism for graceful shutdown
These issues were identified in previous reviews and should be addressed.
1313-1347: Improve error handling and make interval configurable.The function silently ignores sync errors and uses a hardcoded refresh interval.
Issues that need to be addressed:
- Replace hardcoded 5-second interval with configurable parameter
- Properly handle or log errors from sync_fn instead of ignoring them
- Add mechanism for graceful shutdown when receiver is dropped
- Improve error handling for failed wallet operations
These issues were identified in previous reviews.
1532-1532: Remove unnecessary short sleep in tight loop.The 10ms sleep in the main loop is unnecessary and could impact responsiveness. The loop is already gated by
tokio::select!which will efficiently wait for events.- tokio::time::sleep(std::time::Duration::from_millis(10)).await;
1403-1534: Consider adding timeout or exit condition to prevent infinite loop.The function implements concurrent approval handling well, but the main loop could potentially run indefinitely if no approvals are received and no balance/quote changes occur.
Consider adding a timeout or maximum iteration count to prevent the function from blocking indefinitely. This was flagged in previous reviews.
1446-1483: Optimize approval request handling to avoid duplicates.The current implementation recreates approval requests for the same quotes in every loop iteration, potentially spamming the user with duplicate requests.
Consider tracking which quotes have already had approval requests sent to avoid creating duplicate requests for the same quote.
🧹 Nitpick comments (1)
swap/src/cli/list_sellers.rs (1)
45-59: Simplify the complex function signature with type aliases.The return type is quite verbose and hard to read. Consider introducing type aliases to improve readability.
+type SellerListClosure = impl Fn() -> std::pin::Pin< + Box<dyn std::future::Future<Output = Vec<SellerStatus>> + Send + 'static>, +> + Send + Sync + 'static; + pub async fn list_sellers_init( rendezvous_points: Vec<(PeerId, Multiaddr)>, namespace: XmrBtcNamespace, maybe_tor_client: Option<Arc<TorClient<TokioRustlsRuntime>>>, identity: identity::Keypair, db: Option<Arc<dyn Database + Send + Sync>>, tauri_handle: Option<TauriHandle>, sender: Option<::tokio::sync::watch::Sender<Vec<SellerStatus>>>, sellers: Vec<Multiaddr>, -) -> Result< - impl Fn() -> std::pin::Pin< - Box<dyn std::future::Future<Output = Vec<SellerStatus>> + Send + 'static>, - > + Send - + Sync - + 'static, -> { +) -> Result<SellerListClosure> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx(1 hunks)swap/src/cli/api/request.rs(14 hunks)swap/src/cli/api/tauri_bindings.rs(10 hunks)swap/src/cli/list_sellers.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx
- swap/src/cli/api/tauri_bindings.rs
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
PR: UnstoppableSwap/core#0
File: AGENT.md:0-0
Timestamp: 2025-06-30T08:53:49.168Z
Learning: Pull request titles must use the format `<type>(<scope>): <description>`, for example: `feat(gui): New feature`, `fix(swap): Issue fixed`, or `refactor(ci): Ci changes`.
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.
swap/src/cli/list_sellers.rs (5)
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: swap/src/asb/network.rs:61-77
Timestamp: 2025-06-15T15:55:03.118Z
Learning: In swap/src/asb/network.rs, write errors for the onion private key file are intentionally non-fatal and should be ignored using `let _ =` pattern.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : Implement `Send` and `Sync` for wrapper types in src/lib.rs.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : In src/lib.rs, provide idiomatic Rust interfaces to the C++ code, using wrapper types (WalletManager, Wallet) with safer interfaces, and handle memory management and safety concerns.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/bridge.rs : In src/bridge.rs, define the FFI interface using the `cxx::bridge` macro, declaring C++ types and functions to be accessed from Rust.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/bridge.rs : In src/bridge.rs, mirror functions from monero/src/wallet/api/wallet2_api.h by copying their definitions and letting CXX generate the bindings.
swap/src/cli/api/request.rs (16)
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : In src/lib.rs, provide idiomatic Rust interfaces to the C++ code, using wrapper types (WalletManager, Wallet) with safer interfaces, and handle memory management and safety concerns.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : Implement `Send` and `Sync` for wrapper types in src/lib.rs.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/{src/bridge.rs,src/lib.rs} : When adding a new function to the bridge, copy its definition from monero/src/wallet/api/wallet2_api.h into bridge.rs, then add wrapping logic in src/lib.rs.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/bridge.rs : In src/bridge.rs, mirror functions from monero/src/wallet/api/wallet2_api.h by copying their definitions and letting CXX generate the bindings.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/**/*.rs : Use Rust 2021 edition.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : Implement proper deref for wrapper types in src/lib.rs.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : Use the `OnceLock` pattern to ensure WalletManager is a singleton in src/lib.rs.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/bridge.rs : In src/bridge.rs, define the FFI interface using the `cxx::bridge` macro, declaring C++ types and functions to be accessed from Rust.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/src/lib.rs : Use `Pin` for C++ objects that require stable memory addresses in src/lib.rs.
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: swap/src/asb/network.rs:61-77
Timestamp: 2025-06-15T15:55:03.118Z
Learning: In swap/src/asb/network.rs, write errors for the onion private key file are intentionally non-fatal and should be ignored using `let _ =` pattern.
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.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: When implementing new wrapper functions: 1. Locate the function in wallet2_api.h, 2. Copy the exact method signature to bridge.rs, 3. Implement the Rust wrapper in lib.rs, 4. Run the build to ensure everything compiles.
Learnt from: nabijaczleweli
PR: UnstoppableSwap/core#391
File: swap/src/common/tor.rs:16-23
Timestamp: 2025-06-15T15:49:33.308Z
Learning: On Whonix systems in swap/src/common/tor.rs, using expect() on system_from_environment() is appropriate because Whonix systems don't have clearnet capabilities - they route all traffic through Tor only. Missing Tor environment variables on Whonix represents a fundamental system misconfiguration that should abort the program rather than attempting graceful degradation.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/{src/bridge.rs,src/lib.rs} : Raw pointers must be wrapped in safe Rust types, and `unsafe` is only used at the FFI boundary.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/**/build.rs : In build.rs, compile the Monero C++ code with CMake targeting wallet_api, set up include paths and library linking, configure CXX, and link required libraries.
Learnt from: CR
PR: UnstoppableSwap/core#0
File: monero-sys/CLAUDE.md:0-0
Timestamp: 2025-06-30T08:53:46.743Z
Learning: Applies to monero-sys/**/bridge.h : In bridge.h, provide helper functions to work around CXX limitations, including wrappers for static methods and handling string returns with std::unique_ptr.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (12)
swap/src/cli/list_sellers.rs (6)
100-106: LGTM! Database error handling has been improved.The error handling for database operations now properly handles failures instead of using unwrap, addressing the previous review feedback.
151-162: LGTM! Clean refactoring that maintains backward compatibility.The function now properly delegates to
list_sellers_initwhile maintaining the same API for existing callers.
167-167: LGTM! Added Deserialize for serialization consistency.Adding
DeserializealongsideSerializeensures the struct can be properly serialized and deserialized, which is important for the concurrent quote fetching workflow.
665-697: LGTM! Well-implemented partial update mechanism.The
build_current_sellersandemit_partial_updatemethods enable progressive UI updates during seller discovery. The implementation correctly filters out pending states and handles the optional sender gracefully.
699-699: LGTM! Clean integration of partial update functionality.The modified method signature enables partial updates while maintaining backward compatibility through the optional parameter.
909-909: LGTM! Appropriate placement of partial update emission.The partial update is correctly placed in the main loop to provide progressive feedback to the UI as seller discovery progresses.
swap/src/cli/api/request.rs (6)
2-5: LGTM! Import changes support concurrent functionality.The new imports properly support the concurrent quote fetching and task management features being introduced.
Also applies to: 7-7, 14-14, 17-17, 23-28, 37-38
64-67: LGTM! Structure correctly supports multiple sellers and rendezvous points.The change from single to multiple sellers/rendezvous points properly enables the concurrent quote fetching functionality described in the PR objectives.
318-321: LGTM! Response structures improved with better type safety.The changes provide more meaningful and type-safe responses, particularly the Optional swap_id which correctly represents cases where no swap is running.
Also applies to: 331-341
681-750: LGTM! Well-structured concurrent swap initiation.The tokio::select! implementation properly handles concurrent quote fetching, user approval, and shutdown signals. The closure-based approach for wallet functions is appropriate for the concurrent context and maintains proper resource management.
1248-1251: LGTM! Clean implementation with proper typing.The function correctly returns a typed response structure improving API consistency.
1417-1425: LGTM! Improved event emission with real balance data.The WaitingForBtcDeposit event now emits actual balance and max_giveable values instead of hardcoded zeros, providing accurate progress information to users.
There was a problem hiding this comment.
Bug: Blocking Lock in Async Drop Causes Issues
The ApprovalCleanupGuard::drop implementation uses blocking_lock() on a TokioMutex. As Drop implementations are synchronous but can be invoked from an async context, this can cause panics, deadlocks, or block the async runtime if the mutex is already held by an async task.
swap/src/cli/api/tauri_bindings.rs#L808-L832
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Bug: Mutex Poisoning Risk in Async Code
The code uses unwrap() on std::sync::Mutex::lock() calls, which can cause a panic and crash the application if the mutex becomes poisoned (e.g., if a thread holding the lock panics). This pattern is repeated and should be replaced with graceful error handling, especially given the async context and potential for complex error scenarios.
swap/src/cli/api/tauri_bindings.rs#L206-L210
Was this report helpful? Give feedback by reacting with 👍 or 👎
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx(4 hunks)src-gui/src/renderer/components/alert/SwapTxLockAlertsBox.tsx(1 hunks)src-gui/src/renderer/components/modal/SwapSuspendAlert.tsx(3 hunks)src-gui/src/renderer/components/modal/swap/pages/DebugPage.tsx(0 hunks)src-gui/src/renderer/components/pages/history/HistoryPage.tsx(0 hunks)src-gui/src/renderer/components/pages/history/table/HistoryRowActions.tsx(1 hunks)src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx(1 hunks)src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx(4 hunks)src-gui/src/renderer/components/pages/swap/swap/SwapWidget.tsx(1 hunks)src-gui/src/renderer/components/pages/swap/swap/components/TransactionInfoBox.tsx(1 hunks)src-gui/src/renderer/components/pages/swap/swap/in_progress/BitcoinLockTxInMempoolPage.tsx(3 hunks)src-gui/src/renderer/components/pages/swap/swap/in_progress/EncryptedSignatureSentPage.tsx(1 hunks)src-gui/src/renderer/components/pages/swap/swap/init/deposit_and_choose_offer/MakerOfferItem.tsx(1 hunks)src-gui/src/store/combinedReducer.ts(0 hunks)src-gui/src/store/features/torSlice.ts(0 hunks)src-gui/src/store/hooks.ts(3 hunks)src-gui/src/store/middleware/storeListener.ts(3 hunks)swap/src/cli/api/tauri_bindings.rs(11 hunks)
💤 Files with no reviewable changes (4)
- src-gui/src/renderer/components/pages/history/HistoryPage.tsx
- src-gui/src/renderer/components/modal/swap/pages/DebugPage.tsx
- src-gui/src/store/combinedReducer.ts
- src-gui/src/store/features/torSlice.ts
✅ Files skipped from review due to trivial changes (1)
- src-gui/src/renderer/components/pages/swap/swap/components/TransactionInfoBox.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- src-gui/src/renderer/components/pages/swap/swap/in_progress/EncryptedSignatureSentPage.tsx
- src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx
- src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx
- src-gui/src/store/hooks.ts
- src-gui/src/renderer/components/pages/swap/swap/init/deposit_and_choose_offer/MakerOfferItem.tsx
- swap/src/cli/api/tauri_bindings.rs
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: CR
PR: UnstoppableSwap/core#0
File: AGENT.md:0-0
Timestamp: 2025-06-30T08:53:49.168Z
Learning: Pull request titles must use the format `<type>(<scope>): <description>`, for example: `feat(gui): New feature`, `fix(swap): Issue fixed`, or `refactor(ci): Ci changes`.
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/SwapTxLockAlertsBox.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/modal/SwapSuspendAlert.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/store/middleware/storeListener.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.
src-gui/src/renderer/components/pages/history/table/HistoryRowActions.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/pages/swap/swap/in_progress/BitcoinLockTxInMempoolPage.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.
src-gui/src/renderer/components/pages/swap/swap/SwapWidget.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.
🧬 Code Graph Analysis (4)
src-gui/src/renderer/components/alert/SwapTxLockAlertsBox.tsx (1)
src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx (1)
SwapStatusAlert(235-305)
src-gui/src/store/middleware/storeListener.ts (1)
src-gui/src/renderer/rpc.ts (1)
getSwapInfo(148-157)
src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx (1)
src-gui/src/store/hooks.ts (1)
useIsSpecificSwapRunning(93-104)
src-gui/src/renderer/components/pages/swap/swap/SwapWidget.tsx (7)
src-gui/src/store/hooks.ts (2)
useAppSelector(30-30)useActiveSwapInfo(126-129)src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx (1)
SwapStatusAlert(235-305)src-gui/src/renderer/components/modal/swap/pages/DebugPage.tsx (1)
DebugPage(10-32)src-gui/src/renderer/components/pages/swap/swap/SwapStatePage.tsx (1)
SwapStatePage(135-141)src-gui/src/renderer/components/modal/swap/SwapStateStepper.tsx (1)
SwapStateStepper(177-196)src-gui/src/renderer/components/pages/swap/swap/CancelButton.tsx (1)
CancelButton(9-51)src-gui/src/renderer/components/modal/swap/pages/DebugPageSwitchBadge.tsx (1)
DebugPageSwitchBadge(5-27)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (23)
src-gui/src/renderer/components/pages/history/table/HistoryRowActions.tsx (4)
15-17: LGTM! Clean import additions for enhanced swap management.The new imports are well-organized and support the enhanced swap lifecycle management functionality.
26-32: LGTM! Sound logic for concurrent swap state management.The state checking logic correctly prevents resuming a swap that's already running and handles the case where another swap is running with locked funds. The comments clearly explain the business logic.
48-48: LGTM! Clear and informative tooltip logic.The conditional tooltip provides clear feedback to users about why the resume button might be disabled, enhancing the user experience.
52-61: LGTM! Well-structured button configuration with comprehensive disabled state logic.The button props correctly combine all the necessary conditions for disabling the resume functionality and provide appropriate user feedback through tooltips.
src-gui/src/renderer/components/alert/SwapTxLockAlertsBox.tsx (1)
14-14: LGTM! Good architectural improvement.Removing the explicit
isRunningprop aligns with the refactoredSwapStatusAlertcomponent that now internally determines the running state using theuseIsSpecificSwapRunninghook. This centralizes the logic and reduces coupling between components.src-gui/src/renderer/components/pages/swap/swap/in_progress/BitcoinLockTxInMempoolPage.tsx (2)
3-3: LGTM! Consistent import path.The absolute import path aligns with the consistent import path strategy used throughout the refactored swap components.
14-14: LGTM! Appropriate use of React fragment.Changing from
<Box>to React fragment (<>) removes an unnecessary wrapper element since the Box wasn't providing any styling or layout benefits here.Also applies to: 46-46
src-gui/src/renderer/components/modal/SwapSuspendAlert.tsx (4)
8-14: LGTM! Proper imports for enhanced UI.The additional Material-UI List components and CircleIcon are correctly imported to support the improved dialog structure.
29-29: LGTM! More descriptive dialog title.The title "Suspend running swap?" is much more specific and accurate than the previous generic title, clearly communicating the action to the user.
31-58: Excellent UX improvement with detailed explanations.The structured list with bullet points significantly improves the user experience by clearly explaining:
- What happens during suspension
- Important warnings about timelocks continuing to count down
- Guidance on monitoring and recovery options
The implementation properly uses Material-UI List components with consistent styling.
69-69: LGTM! More accurate button label."Suspend" is more precise than "Force stop" and aligns with the actual functionality being performed.
src-gui/src/renderer/components/pages/swap/swap/SwapWidget.tsx (5)
1-10: LGTM! Clean imports and proper organization.The imports follow the consistent absolute path strategy and include all necessary dependencies. The component structure is well-organized.
11-16: LGTM! Proper state management setup.The component correctly uses Redux hooks to access swap state and maintains local debug toggle state appropriately.
17-34: LGTM! Well-structured layout container.The outer Box and Paper components provide a clean, professional layout with:
- Consistent spacing using Material-UI's gap system
- Proper elevation and styling
- Responsive max-width constraint
- Centered alignment
35-43: LGTM! Clean conditional rendering.The debug/normal view toggle is implemented cleanly with appropriate conditional rendering between
DebugPageandSwapStatePage.
44-52: LGTM! Proper conditional UI elements.The conditional rendering of stepper and control elements only when swap state exists is appropriate, and the flexbox layout for the bottom controls provides good UX.
src-gui/src/renderer/components/alert/SwapStatusAlert/SwapStatusAlert.tsx (5)
17-17: LGTM! Proper import for centralized state logic.Adding the
useIsSpecificSwapRunninghook import supports the architectural improvement of centralizing running state detection within this component.
242-244: LGTM! Good defensive programming.The null check for the swap prop prevents potential runtime errors and makes the component more robust.
256-258: LGTM! Improved code readability.Extracting the unusual time calculation into a well-named constant (
hasUnusualAmountOfTimePassed) makes the code more readable and easier to understand.Also applies to: 263-263
268-268: LGTM! Excellent architectural improvement.Using the
useIsSpecificSwapRunninghook internally centralizes the running state logic within this component, eliminating the need for external components to pass this information as a prop. This reduces coupling and improves maintainability.
284-286: LGTM! Cleaner alert title logic.The alert title logic is now more readable by using the extracted
hasUnusualAmountOfTimePassedconstant, and the messages clearly differentiate between normal running state and unusual duration.src-gui/src/store/middleware/storeListener.ts (2)
2-2: LGTM: Necessary import for throttling functionality.The lodash import is correctly added to support the throttling and debouncing implementation.
85-88: Usage pattern is correct, but depends on fixing the throttling implementation.The integration of the throttled updater in the swap progress listener is well-implemented with appropriate logging. However, the effectiveness depends on resolving the throttle/debounce combination issues identified above.
src-gui/src/renderer/components/pages/history/table/HistoryRowActions.tsx
Show resolved
Hide resolved
…' into feat/concurrent-quotes-swap-init
|
bugbot run |
There was a problem hiding this comment.
Bug: Async Code Blocked by Sync Mutex
The pending_approvals field in TauriHandleInner was changed from tokio::sync::Mutex to std::sync::Mutex. This synchronous mutex is used in async functions (request_approval, resolve_approval), where its blocking .lock() call can block the async runtime, leading to performance issues or deadlocks.
swap/src/cli/api/tauri_bindings.rs#L137-L315
Was this report helpful? Give feedback by reacting with 👍 or 👎
3 participants
TODO:
[ ] Run setup_swap negotiation phase within the WaitingForBitcoinDepositPage (only transition transition into stepper flow once we have a final offer to show to the user)(out of scope)[ ] Only supply Monero receive address once the user select the maker (as part of(out of scope)ApprovalRequestDetails::SelectMaker). Allow changing / setting the address until final offer requested (field on WaitingForBitcoinDepositPage)Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Removals
Developer Notes