-
Notifications
You must be signed in to change notification settings - Fork 13
fix send/receive subdust #201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@bordalix has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughLimits provider now reads UTXO/VTXO min/max from environment variables and re-evaluates when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as Send/Receive Screen
participant Limits as LimitsContext
participant Provider as Limits Provider
Note over Provider: Read env vars for utxo/vtxo limits\nRecompute when [aspInfo.network, svcWallet, swapProvider] change
Provider->>Provider: Recompute limits
User->>UI: Enter amount (satoshis)
UI->>Limits: isBelowMin(satoshis)?
Limits-->>UI: true / false
alt amount below min
UI->>UI: Set label "Amount below min limit"
UI-->User: Disable action
else within limits
UI->>UI: Set normal label / enable action
UI-->User: Allow proceed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
There is nuance to this, cc@louisinger @altafan |
Deploying wallet-signet with
|
| Latest commit: |
692051d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://54a041d9.wallet-23u.pages.dev |
| Branch Preview URL: | https://fix-subdust.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
692051d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b77871b9.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-subdust.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
692051d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7804f666.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-subdust.wallet-bitcoin.pages.dev |
* UI tweaks on refunded failed submarine swaps * lint * update dependencies * update dependencies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/providers/limits.tsx (1)
131-145: Fix misleading comment about minimum calculation.The comment states "it should be the maximum between utxo and vtxo min amounts" (line 132), but the code correctly returns the minimum of the two values (line 144). Since users can send via either UTXO or VTXO, the effective minimum is the less restrictive option (smaller value). The matrix aligns with this logic.
Apply this diff to clarify the comment:
- // calculate absolute min sats available to send or receive - // it should be the maximum between utxo and vtxo min amounts, - // but we need to consider the special value -1 for 'no limits' + // Calculate absolute min sats available to send or receive. + // Returns the minimum between utxo and vtxo min amounts (the less restrictive limit), + // considering the special value -1 for 'no limits'.src/screens/Apps/Boltz/Swap.tsx (1)
102-106: Consider aligning refund UI color with SwapsList.The refunded indicator uses a green icon color (line 104), but in SwapsList.tsx the refunded status uses
'dark50'color (see colorDict on line 41). This creates an inconsistent visual representation of refunded swaps across the application. Refunds are typically not "success" states, so a neutral color might be more appropriate.Apply this diff to align the color scheme:
) : refunded ? ( <FlexRow alignItems='flex-start'> - <InfoIconDark color='green' /> + <InfoIconDark /> <TextSecondary>Swap refunded</TextSecondary> </FlexRow>This will use the default color for InfoIconDark, which should be consistent with the TextSecondary styling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(1 hunks)src/components/SwapsList.tsx(1 hunks)src/providers/limits.tsx(1 hunks)src/screens/Apps/Boltz/Swap.tsx(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: src/lib/asp.ts:0-0
Timestamp: 2025-06-30T18:33:29.839Z
Learning: In src/lib/asp.ts, only the collaborativeExit function should accept both IWallet and ServiceWorkerWallet types. Other wallet functions (getBalance, getTxHistory, getReceivingAddresses, redeemNotes, sendOnChain, settleVtxos) should maintain their original IWallet-only signatures and not be updated for consistency.
Applied to files:
src/providers/limits.tsx
🧬 Code graph analysis (2)
src/components/SwapsList.tsx (1)
src/components/Text.tsx (2)
Text(25-79)TextSecondary(91-97)
src/screens/Apps/Boltz/Swap.tsx (3)
src/components/FlexRow.tsx (1)
FlexRow(16-45)src/icons/Info.tsx (1)
InfoIconDark(16-27)src/components/Text.tsx (1)
TextSecondary(91-97)
⏰ 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: test
🔇 Additional comments (5)
src/providers/limits.tsx (1)
52-65: Update comment on line 52 to reflect current dependency chain. Verify that gating UTXO/VTXO limits onswapProvideris intentional.The first useEffect (lines 52-65) returns early if
swapProvideris unavailable (line 54), preventing UTXO and VTXO limits from being set at all. Since limit initialization defaultsutxo.maxandvtxo.maxtoBigInt(0), this leavesutxoTxsAllowed()andvtxoTxsAllowed()returning false whenever the swap provider is unavailable—effectively blocking all transaction types when Lightning connectivity fails.Update the comment on line 52 from "update limits when aspInfo or svcWallet changes" to include
swapProvider. If this dependency is intentional per design, document why UTXO/VTXO transactions should be gated on swap provider availability; otherwise, separate the effects so UTXO/VTXO limits can be set independently.src/screens/Apps/Boltz/Swap.tsx (3)
23-24: LGTM!The new imports are correctly used for rendering the refunded swap UI indicator.
45-45: LGTM!The refunded check correctly identifies submarine swaps that have been refunded, consistent with the logic in SwapsList.tsx.
56-56: Good defensive improvement.Displaying 'N/A' when the preimage is falsy improves the user experience by providing clear feedback instead of showing an empty value.
src/components/SwapsList.tsx (1)
63-63: Therefundedproperty is from the external@arkade-os/boltz-swappackage and cannot be verified in this environment.Since node_modules is unavailable in the sandbox, the actual type definitions for
PendingSubmarineSwapcannot be inspected. However, the code consistently accessesswap.refundedacross multiple files (SwapsList.tsx and Swap.tsx), suggesting it's a legitimate property on the type from the@arkade-os/boltz-swap@0.2.7package.The pattern of using
swap.refundedas a display override (line 69:{refunded ? 'Refunded' : status}) whilestatusDictmaps'transaction.refunded'to'Refunded'appears intentional—likely for handling intermediate states or edge cases specific to submarine swaps. Without access to the package documentation or the actual business logic explanation, the reviewer's suggestion for documentation is valid but cannot be definitively verified.
| "@arkade-os/boltz-swap": "0.2.7", | ||
| "@arkade-os/sdk": "0.3.4", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Verify dependency bumps are related to the subdust fix and don't introduce breaking changes.
Both dependencies are bumped by patch version (0.2.6 → 0.2.7 and 0.3.3 → 0.3.4), but I need to confirm these versions exist and understand their relationship to the limits provider fix described in the PR.
Please verify the following:
- Do these package versions exist on npm?
- What's in their changelogs/release notes—do they relate to the subdust limits fix?
- Are there any breaking changes or behavioral shifts in these releases?
You can check using:
Also note: Kukks flagged "nuance to this," which suggests there may be subtleties around the subdust handling that warrant careful testing of these new versions.
🌐 Web query:
@arkade-os/boltz-swap version 0.2.7 release notes
💡 Result:
I couldn't find any release notes or a published release for @arkade-os/boltz-swap version 0.2.7. The package registries and mirrors show other versions (e.g., npm shows 0.1.4; jsDelivr lists 0.2.6; socket.dev lists alpha/0.2.0-series). (npmjs.com)
Would you like me to (a) keep watching and notify if 0.2.7 appears, or (b) fetch the latest available release notes instead?
Citations:
🌐 Web query:
@arkade-os/sdk version 0.3.4 release notes
💡 Result:
I couldn't find a published "0.3.4" release note page or CHANGELOG entry. The package registry/CDN shows a 0.3.4 build exists, but the npm package page lists 0.2.3 as the published version and the project's GitHub releases/changelog pages show alpha/0.3.0-series releases instead. Sources: [jsDelivr (v0.3.4 exists)][1], [npm package page (version 0.2.3)][2], [GitHub releases/newreleases (v0.3.0-alpha series)][3].
If you want, I can:
- fetch the package contents for v0.3.4 (README / CHANGELOG) from the CDN and extract release notes, or
- search the repo tags/releases on GitHub for a v0.3.4 tag and return the notes. Which would you prefer?
These package versions cannot be verified to exist on npm registries—this is a blocking issue.
@arkade-os/boltz-swap 0.2.7 was not found on npm (npm shows 0.1.4; jsDelivr shows 0.2.6), and @arkade-os/sdk 0.3.4 is not published (npm shows 0.2.3; GitHub shows v0.3.0-alpha series).
- Are these versions built locally or referenced via git tags/branches rather than npm?
- Does your lockfile (
package-lock.jsonoryarn.lock) contain git references? - If these are pre-release versions, they should be pinned differently (e.g., with
npm linkor git refs in package.json).
Clarify the dependency resolution strategy before merging, as standard npm installs will fail with these versions.
🤖 Prompt for AI Agents
In package.json around lines 7-8, the pinned versions "@arkade-os/boltz-swap":
"0.2.7" and "@arkade-os/sdk": "0.3.4" do not exist on public registries; update
package.json to point to valid, resolvable sources: either change to the
published versions (e.g., boltz-swap 0.2.6/0.1.4 or sdk 0.2.3), replace them
with git refs (git+ssh or commit/tag) if you intend to consume a repository
version, or document and use npm link/local path for local builds; also update
or regenerate the lockfile (package-lock.json or yarn.lock) to reflect the
corrected references and add a short note in the PR describing whether these are
local/dev builds or repo refs so CI and reviewers know the dependency resolution
strategy.
| const refunded = swap.type === 'submarine' && swap.refunded | ||
|
|
||
| const Icon = iconDict[status] | ||
| const Kind = () => <Text thin>{direction}</Text> | ||
| const When = () => <TextSecondary>{when}</TextSecondary> | ||
| const Sats = () => <Text color={color}>{amount}</Text> | ||
| const Stat = () => <Text color={color}>{status}</Text> | ||
| const Stat = () => <Text color={color}>{refunded ? 'Refunded' : status}</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix color and icon inconsistency for refunded submarine swaps.
When swap.refunded is true but swap.status is not 'transaction.refunded', the text displays "Refunded" but uses the color and icon from the original status. For example, if the status is "Pending", it would show "Refunded" in yellow with a pending icon instead of dark50 color with a failed icon.
Apply this diff to ensure consistent color and icon for refunded swaps:
const sats = swap.type === 'reverse' ? swap.response.onchainAmount : swap.response.expectedAmount
const direction = swap.type === 'reverse' ? 'Lightning to Arkade' : 'Arkade to Lightning'
- const status: statusUI = statusDict[swap.status] || 'Pending'
- const color = colorDict[status]
const prefix = swap.type === 'reverse' ? '+' : '-'
const amount = `${prefix} ${config.showBalance ? prettyAmount(sats) : prettyHide(sats)}`
const when = window.innerWidth < 400 ? prettyAgo(swap.createdAt) : prettyDate(swap.createdAt)
const refunded = swap.type === 'submarine' && swap.refunded
+ const status: statusUI = refunded ? 'Refunded' : (statusDict[swap.status] || 'Pending')
+ const color = colorDict[status]
const Icon = iconDict[status]
const Kind = () => <Text thin>{direction}</Text>
const When = () => <TextSecondary>{when}</TextSecondary>
const Sats = () => <Text color={color}>{amount}</Text>
- const Stat = () => <Text color={color}>{refunded ? 'Refunded' : status}</Text>
+ const Stat = () => <Text color={color}>{status}</Text>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/SwapsList.tsx around lines 63 to 69, refunded submarine swaps
show "Refunded" text but still use the original status color and icon; change
the display logic so refunded swaps override status, color, and icon: compute a
displayStatus/displayColor/displayIcon (or conditionally set Icon and color) by
checking if swap.type === 'submarine' && swap.refunded, and when true set
displayStatus to 'Refunded' (or 'transaction.refunded' mapping), displayColor to
the dark50/failed color, and displayIcon to the failed icon; then use those
display* values in Kind/When/Sats/Stat/Icon so the text, color, and icon are
consistent for refunded swaps.
There was a subtle bug in limits provider which made him not update the min limits for vtxos.
This allowed sending and receiving below the operator announced vtxoMinAmount (in /v1/info).
Strangely, the server accepted those subdust payments and never complained 🤔
This PR fixes the bug and improves the UX to tell the user is trying to pay/receive below dust.
@Kukks please review.
Summary by CodeRabbit
New Features
Bug Fixes
Chores