soft settle #171
soft settle #171
Conversation
WalkthroughAdds utilities to identify and settle expiring or recoverable coins (getInputsToSettle, renewCoins), new UTXO/VTXO filters for expiration, replaces VtxoManager startup renewal with renewCoins, updates screens to use aggregated inputs for settlement eligibility, and removes/updates related tests. Changes
Sequence DiagramsequenceDiagram
participant Startup as WalletProvider Startup
participant Renew as renewCoins()
participant Inputs as getInputsToSettle()
participant VtxoFilter as getExpiringAndRecoverableVtxos()
participant UtxoFilter as getConfirmedAndNotExpiredUtxos()
participant Settle as settleVtxos()
Startup->>Renew: renewCoins(svcWallet)
Renew->>Inputs: getInputsToSettle(svcWallet)
Inputs->>VtxoFilter: getExpiringAndRecoverableVtxos(wallet)
VtxoFilter-->>Inputs: ExtendedVirtualCoin[]
Inputs->>UtxoFilter: getConfirmedAndNotExpiredUtxos(wallet)
UtxoFilter-->>Inputs: ExtendedCoin[]
Inputs-->>Renew: aggregated inputs
alt inputs exist
Renew->>Settle: settleVtxos(aggregated inputs)
Settle-->>Renew: success
else no inputs
Renew-->>Startup: no-op
end
Renew-->>Startup: complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Deploying wallet-signet with
|
| Latest commit: |
f89d6ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9cfec6a9.wallet-23u.pages.dev |
| Branch Preview URL: | https://soft-settle.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
f89d6ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6d364764.arkade-wallet.pages.dev |
| Branch Preview URL: | https://soft-settle.arkade-wallet.pages.dev |
Deploying wallet-bitcoin-v8 with
|
| Latest commit: |
f89d6ea
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://bd1e2e80.wallet-v8.pages.dev |
| Branch Preview URL: | https://soft-settle.wallet-v8.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
src/test/setup.ts (1)
9-9: Consider the test output impact.Uncommenting the console.log spy will enable console.log output during test execution. While this may be helpful for debugging, it could make test output noisy. Ensure this is intentional and consider whether selective enabling (e.g., via environment variable) would be preferable.
src/providers/wallet.tsx (1)
176-177: Consider logging renewal failures.The empty catch block silently swallows errors from
renewCoins, which could hide failures in the automatic renewal flow at startup. While this may be acceptable if renewal failures are non-critical, consider at least logging the error for observability.Apply this diff to add error logging:
// renew expiring coins on startup - renewCoins(svcWallet).catch(() => {}) + renewCoins(svcWallet).catch((err) => { + consoleError(err, 'Failed to renew coins on startup') + })src/screens/Wallet/Transaction.tsx (1)
162-162: Optional UX: explain when settlement is hidden.When hasInputsToSettle is false, no action appears. Consider a short hint like “Nothing to settle right now.”
src/screens/Settings/Vtxos.tsx (2)
178-183: Good gating; consider small UX hint when hidden.If hasInputsToSettle is false, consider showing a muted note (“No coins eligible for renewal.”).
64-71: Avoid duplication: extract a shared hook.Both screens duplicate the same eligibility logic. Consider a reusable hook to reduce drift:
// src/hooks/useSettleEligibility.ts import { useEffect, useState } from 'react' import { getInputsToSettle } from '../lib/asp' import type { IWallet } from '../lib/types' export function useSettleEligibility(svcWallet?: IWallet, deps: unknown[] = [], dust = 0) { const [hasInputs, setHasInputs] = useState(false) const [aboveDust, setAboveDust] = useState(false) const [total, setTotal] = useState(0) useEffect(() => { if (!svcWallet) return let cancelled = false ;(async () => { try { const inputs = await getInputsToSettle(svcWallet) if (cancelled) return const sum = inputs.reduce((a, v) => a + v.value, 0) || 0 setTotal(sum) setHasInputs(inputs.length > 0) setAboveDust(sum > dust) } catch { if (!cancelled) { setTotal(0); setHasInputs(false); setAboveDust(false) } } })() return () => { cancelled = true } // eslint-disable-next-line react-hooks/exhaustive-deps }, [svcWallet, dust, ...deps]) return { hasInputs, aboveDust, total } }Use in both screens to remove duplication.
Also applies to: 72-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/lib/asp.ts(2 hunks)src/lib/constants.ts(1 hunks)src/lib/utxo.ts(1 hunks)src/lib/vtxo.ts(1 hunks)src/providers/wallet.tsx(2 hunks)src/screens/Settings/Vtxos.tsx(5 hunks)src/screens/Wallet/Transaction.tsx(4 hunks)src/screens/Wallet/Unlock.tsx(1 hunks)src/test/screens/wallet/transaction.test.tsx(4 hunks)src/test/screens/wallet/unlock.test.tsx(0 hunks)src/test/setup.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/screens/wallet/unlock.test.tsx
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-06T09:53:28.428Z
Learnt from: bordalix
PR: arkade-os/wallet#134
File: src/screens/Settings/Password.tsx:34-39
Timestamp: 2025-08-06T09:53:28.428Z
Learning: In src/screens/Settings/Password.tsx, the saveNewPassword function doesn't need error handling for getPrivateKey because it's only called after authentication is verified. The useEffect validates the oldPassword using isValidPassword() which internally calls getPrivateKey(), and only sets authenticated=true if successful. The saveNewPassword function has a guard clause that returns early if not authenticated, so getPrivateKey(oldPassword) is guaranteed to work when called.
Applied to files:
src/screens/Wallet/Unlock.tsx
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#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/lib/asp.tssrc/providers/wallet.tsx
🧬 Code graph analysis (5)
src/lib/asp.ts (2)
src/lib/vtxo.ts (1)
getExpiringAndRecoverableVtxos(19-22)src/lib/utxo.ts (1)
getConfirmedAndNotExpiredUtxos(11-15)
src/lib/vtxo.ts (2)
src/lib/asp.ts (1)
getVtxos(124-133)src/lib/constants.ts (1)
maxPercentage(9-9)
src/providers/wallet.tsx (1)
src/lib/asp.ts (1)
renewCoins(186-189)
src/screens/Wallet/Transaction.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(39-53)src/lib/asp.ts (1)
getInputsToSettle(163-167)
src/screens/Settings/Vtxos.tsx (1)
src/lib/asp.ts (1)
getInputsToSettle(163-167)
🔇 Additional comments (10)
src/lib/asp.ts (1)
163-167: LGTM!The function correctly aggregates expiring/recoverable VTXOs and confirmed non-expired UTXOs into a unified list of settleable inputs.
src/lib/utxo.ts (1)
3-15: LGTM!The implementation correctly:
- Extracts and decodes the CSVMultisig tapscript from the UTXO's intentTapLeafScript
- Uses the SDK's
hasBoardingTxExpiredto determine expiration status- Filters for confirmed, non-expired UTXOs
The version byte removal on Line 5 assumes a fixed script structure, which should be stable given it's based on the SDK's CSVMultisigTapscript format.
src/lib/constants.ts (1)
9-9: LGTM!The constant provides a configurable threshold percentage with a sensible default value of 10. This is consumed by the VtxoManager in src/lib/vtxo.ts to determine expiring VTXOs.
src/lib/vtxo.ts (1)
6-22: LGTM!The implementation provides a comprehensive approach to identifying expiring and recoverable VTXOs:
getOrphanVtxosserves as a safety net for edge cases where VTXOs are expired but not sweptgetExpiringAndRecoverableVtxoscombines both VtxoManager-identified expiring VTXOs and orphaned VTXOsThe orphan detection logic correctly checks for unspent, expired, and not-swept conditions, and the use of the configurable
maxPercentagethreshold allows fine-tuning of expiry detection.src/screens/Wallet/Transaction.tsx (3)
25-25: Import looks good.
32-32: Context additions OK.
46-46: State addition OK.src/screens/Settings/Vtxos.tsx (3)
19-19: Import additions OK.
36-36: State addition OK.
49-51: Label logic OK.Compact and clear; no issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/screens/Settings/Vtxos.tsx (1)
64-71: Partial fix applied; cleanup and error handling still missing.The
aspInfonull-check and updated dependencies address part of the previous review feedback. However, the effect still lacks:
- Error handling (no
.catch()ortry/catch)- Cleanup to prevent state updates after unmount
These issues remain as described in the previous review comments.
src/screens/Wallet/Transaction.tsx (1)
72-80: Partial fix applied; cleanup and error handling still missing.The
aspInfonull-check and updated dependencies address part of the previous review feedback. However, the effect still lacks:
- Error handling (no
.catch()ortry/catch)- Cleanup to prevent state updates after unmount
These issues remain as described in the previous review comments.
🧹 Nitpick comments (1)
src/screens/Wallet/Transaction.tsx (1)
72-80: Extract duplicated settlement logic into a custom hook.This effect is nearly identical to the one in
src/screens/Settings/Vtxos.tsx(lines 64-71). Consider extracting the common pattern into a reusable custom hook such asuseSettlementEligibility()that returns{ hasInputsToSettle, amountAboveDust }.This would:
- Eliminate code duplication
- Centralize the fix for error handling and cleanup
- Improve maintainability
Example:
// src/hooks/useSettlementEligibility.ts export const useSettlementEligibility = (svcWallet, aspInfo, vtxos) => { const [hasInputsToSettle, setHasInputsToSettle] = useState(false) const [amountAboveDust, setAmountAboveDust] = useState(false) useEffect(() => { if (!aspInfo || !svcWallet) return let cancelled = false ;(async () => { try { const inputs = await getInputsToSettle(svcWallet) if (cancelled) return setHasInputsToSettle(inputs.length > 0) const totalAmount = inputs.reduce((a, v) => a + v.value, 0) || 0 setAmountAboveDust(totalAmount > (aspInfo?.dust ?? 0)) } catch { if (cancelled) return setHasInputsToSettle(false) setAmountAboveDust(false) } })() return () => { cancelled = true } }, [aspInfo, vtxos, svcWallet]) return { hasInputsToSettle, amountAboveDust } }Then use it in both files:
const { hasInputsToSettle, amountAboveDust } = useSettlementEligibility(svcWallet, aspInfo, vtxos)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/lib/asp.ts(2 hunks)src/screens/Settings/Vtxos.tsx(5 hunks)src/screens/Wallet/Transaction.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/asp.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/screens/Settings/Vtxos.tsx (1)
src/lib/asp.ts (1)
getInputsToSettle(163-167)
src/screens/Wallet/Transaction.tsx (2)
src/providers/wallet.tsx (1)
WalletContext(39-53)src/lib/asp.ts (1)
getInputsToSettle(163-167)
🔇 Additional comments (8)
src/screens/Settings/Vtxos.tsx (4)
19-19: LGTM: Import addition.The import of
getInputsToSettleis correct and aligns with the new settlement utility function.
36-36: LGTM: State addition.The new
hasInputsToSettlestate clearly tracks whether settleable inputs are available.
49-50: LGTM: Label logic update.The label correctly reflects the settlement state, showing 'Below dust limit' when the amount is insufficient. Dependencies are properly updated.
179-184: LGTM: Conditional rendering logic.The button visibility and disabled state are correctly determined. Note that the reliability of this UI depends on the effect at lines 64-71 being properly hardened.
src/screens/Wallet/Transaction.tsx (4)
25-25: LGTM: Import addition.The import is correct and consistent with the settlement refactor.
32-32: LGTM: Context destructuring update.The destructuring correctly includes
vtxosandsvcWalletfor the new settlement logic.
46-46: LGTM: State addition.The
hasInputsToSettlestate appropriately tracks settlement eligibility.
163-163: LGTM: Conditional logic update.The addition of
hasInputsToSettleto the settlement eligibility check is correct.
Signed-off-by: Marco Argentieri <3596602+tiero@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/lib/asp.ts (1)
186-186: Remove the unsupported callback parameter fromwallet.settle()call.This issue was previously flagged but remains unaddressed. The SDK's
settle()method signature issettle(params): Promise<string>and does not accept a callback parameter. Theconsole.logargument is ignored by the SDK.Apply this diff:
- await wallet.settle({ inputs, outputs }, console.log) + await wallet.settle({ inputs, outputs })
🧹 Nitpick comments (2)
src/lib/asp.ts (2)
177-177: Consider using BigInt arithmetic throughout for type consistency.The amount is computed using number arithmetic then converted to BigInt. While this works for Bitcoin amounts (within safe integer range), using BigInt throughout would be more type-consistent and future-proof.
Apply this diff for BigInt consistency:
- const amount = inputs.reduce((sum, input) => sum + input.value, 0) + const amount = inputs.reduce((sum, input) => sum + BigInt(input.value), BigInt(0))Then use
amountdirectly on line 182:const outputs = [ { address: await wallet.getAddress(), - amount: BigInt(amount), + amount, }, ]
189-192: Eliminate redundantgetInputsToSettlecall.
renewCoinscallsgetInputsToSettleto check for inputs, thensettleVtxoscalls it again (line 173). This duplicates potentially expensive wallet queries.Option 1: Pass inputs to
settleVtxos:Update
settleVtxossignature:export const settleVtxos = async (wallet: IWallet, inputs?: ExtendedCoin[]): Promise<void> => { const coins = inputs ?? await getInputsToSettle(wallet) // ... rest of logic }Update
renewCoins:export const renewCoins = async (wallet: IWallet): Promise<void> => { const inputs = await getInputsToSettle(wallet) - if (inputs.length > 0) await settleVtxos(wallet) + if (inputs.length > 0) await settleVtxos(wallet, inputs) }Option 2: Move the length check into
settleVtxosand let it return early:Update
settleVtxos:export const settleVtxos = async (wallet: IWallet): Promise<void> => { const inputs = await getInputsToSettle(wallet) - if (inputs.length === 0) throw new Error('No UTXOs or VTXOs eligible to settle') + if (inputs.length === 0) return // no inputs to settle // ... rest of logic }Update
renewCoins:export const renewCoins = async (wallet: IWallet): Promise<void> => { - const inputs = await getInputsToSettle(wallet) - if (inputs.length > 0) await settleVtxos(wallet) + await settleVtxos(wallet) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/lib/asp.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-30T18:33:29.839Z
Learnt from: bordalix
PR: arkade-os/wallet#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/lib/asp.ts
🧬 Code graph analysis (1)
src/lib/asp.ts (2)
src/lib/vtxo.ts (1)
getExpiringAndRecoverableVtxos(19-22)src/lib/utxo.ts (1)
getConfirmedAndNotExpiredUtxos(11-15)
🔇 Additional comments (2)
src/lib/asp.ts (2)
1-6: LGTM! Imports support the new settlement functionality.The added imports (ExtendedCoin, getConfirmedAndNotExpiredUtxos, getExpiringAndRecoverableVtxos) are necessary for the new input aggregation logic and are correctly used in the functions below.
166-170: LGTM! Clean input aggregation logic.The function correctly aggregates expiring/recoverable VTXOs and confirmed non-expired UTXOs, providing a single entry point for settlement-eligible inputs.
@tiero @louisinger please review
To work properly, it needs arkade-os/ts-sdk#197 and arkade-os/ts-sdk#199
Summary by CodeRabbit
New Features
Refactoring
Tests