Fix coin control #229
Fix coin control #229
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds thresholdMs-based VTXO settlement lifecycle: computes batch lifetime via an Indexer, threads thresholdMs through asp/vtxo/wallet flows and UI, updates provider state handling, adds Indexer, adjusts several function signatures, adds UI props, bumps Changes
Sequence Diagram(s)sequenceDiagram
participant WP as Wallet Provider / UI
participant IDX as Indexer
participant ASP as ASP API
participant DB as IndexedDB
participant SVC as Service Worker / Settlement
WP->>WP: calcBatchLifetimeMs(aspInfo, vtxos)
WP->>WP: pick sample vtxo (batchExpiry + commitmentTxId)
WP->>IDX: getCommitmentTxCreatedAt(txid)
IDX->>DB: query 'commitmentTxs' collection (by txid)
alt cached
DB-->>IDX: CommitmentTxRecord {when}
else not cached
IDX->>ASP: fetch commitmentTx(txid)
ASP-->>IDX: commitmentTx (may include endedAt)
IDX->>DB: save {txid, when}
DB-->>IDX: saved
end
IDX-->>WP: when (batchStart)
WP->>WP: batchLifetimeMs = batchExpiry - batchStart
WP->>WP: thresholdMs = batchLifetimeMs * maxPercentage
WP->>SVC: renewCoins(wallet, dust, thresholdMs)
SVC->>SVC: getInputsToSettle(wallet, thresholdMs)
SVC-->>SVC: returns { inputs, vtxos, boardingUtxos }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-06-30T18:33:29.839ZApplied to files:
🧬 Code graph analysis (1)src/screens/Settings/Vtxos.tsx (2)
⏰ 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)
🔇 Additional comments (11)
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: |
8478276
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://55e8bda6.wallet-23u.pages.dev |
| Branch Preview URL: | https://fix-coin-control.wallet-23u.pages.dev |
Deploying wallet-mutinynet with
|
| Latest commit: |
8478276
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://61e2fe3b.arkade-wallet.pages.dev |
| Branch Preview URL: | https://fix-coin-control.arkade-wallet.pages.dev |
Deploying wallet-bitcoin with
|
| Latest commit: |
8478276
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7654886b.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://fix-coin-control.wallet-bitcoin.pages.dev |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes coin control functionality by introducing a threshold mechanism to determine which coins should be settled. The key changes involve calculating a thresholdMs value based on batch lifetime and using it to identify expiring VTXOs.
Key Changes
- Added
thresholdMsto the Wallet type and calculated it based on batch lifetime percentage - Updated
getInputsToSettleto return separate arrays for vtxos, boarding UTXOs, and combined inputs - Modified
calcNextRolloverto handle cases when there are no VTXOs by falling back to boarding UTXOs - Added new
Indexerclass to fetch commitment transaction timestamps for batch lifetime calculations - Updated SDK from version 0.3.5 to 0.3.6
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/indexer.ts | New indexer class for fetching commitment transaction data |
| src/lib/wallet.ts | Updated rollover calculation logic and added batch lifetime calculation |
| src/lib/types.ts | Added optional thresholdMs field to Wallet type |
| src/lib/vtxo.ts | Updated to use thresholdMs parameter instead of percentage |
| src/lib/asp.ts | Modified getInputsToSettle to return structured response with separate coin types |
| src/providers/wallet.tsx | Added calculation of thresholdMs and integration with rollover logic |
| src/screens/Settings/Vtxos.tsx | Enhanced UI to show different settlement actions and expiring coins |
| src/screens/Wallet/Transaction.tsx | Updated to use new getInputsToSettle response structure |
| package.json | Updated SDK dependency to 0.3.6 |
| public/wallet-service-worker.mjs | Minified service worker bundle with variable renaming |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
public/wallet-service-worker.mjs (1)
10020-10090: Don’t record a change VTXO when no change output exists.
Lpalready subtracts the target amount and returns a positivechangeAmounteven when the change was sent via the sub‑dust branch (i.e., no corresponding output was actually added). The block starting at Line 10041 always persists a preconfirmed change VTXO wheneverchangeAmount > 0n, so if the change fell below the dust threshold andsubdustPkScriptwas used, we end up tracking a virtual coin that does not exist on-chain. This desynchronises the wallet state from reality and can corrupt future settlements.Guard the persistence block with a check that the “normal” change output was added (i.e., skip when the sub-dust branch was taken), or otherwise persist only when the output really exists. citeturn0file0
src/providers/wallet.tsx (1)
20-23: Restore the requiredthresholdMsdefault.
Walletnow exposes athresholdMsfield, yetdefaultWalletomits it. With the currentWallettyping this fails to compile (Property 'thresholdMs' is missing…) and also leaves context consumers readingwallet.thresholdMsasundefined. Please seed the default with a numeric value (e.g.,0) so the app builds and downstream logic has a safe baseline.Apply this diff:
const defaultWallet: Wallet = { network: '', nextRollover: 0, + thresholdMs: 0, }
🧹 Nitpick comments (1)
src/lib/vtxo.ts (1)
22-23: Parallelize the independent awaits.
manager.getExpiringVtxos()andgetOrphanVtxos(wallet)don’t depend on each other, so we can fetch them concurrently to trim the response time of this helper.- return [...(await manager.getExpiringVtxos()), ...(await getOrphanVtxos(wallet))] + const [expiring, orphan] = await Promise.all([ + manager.getExpiringVtxos(), + getOrphanVtxos(wallet), + ]) + return [...expiring, ...orphan]
📜 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 (10)
package.json(1 hunks)public/wallet-service-worker.mjs(123 hunks)src/lib/asp.ts(3 hunks)src/lib/indexer.ts(1 hunks)src/lib/types.ts(1 hunks)src/lib/vtxo.ts(1 hunks)src/lib/wallet.ts(2 hunks)src/providers/wallet.tsx(4 hunks)src/screens/Settings/Vtxos.tsx(10 hunks)src/screens/Wallet/Transaction.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/screens/Wallet/Transaction.tsxsrc/lib/types.tssrc/lib/vtxo.tssrc/lib/wallet.tssrc/screens/Settings/Vtxos.tsxsrc/lib/asp.tspublic/wallet-service-worker.mjssrc/providers/wallet.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/lib/vtxo.tspublic/wallet-service-worker.mjssrc/providers/wallet.tsx
📚 Learning: 2025-08-28T15:58:17.245Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 142
File: src/lib/bip21.ts:7-13
Timestamp: 2025-08-28T15:58:17.245Z
Learning: In src/lib/bip21.ts, the encodeBip21 function is used by the wallet which always has an arkAddress available to pass to the function, while decodeBip21 is used to parse received possible BIP21 URIs which may not contain arkAddress. This explains why arkAddress is required in encodeBip21 but optional in the Bip21Decoded interface.
Applied to files:
src/lib/asp.ts
🧬 Code graph analysis (6)
src/screens/Wallet/Transaction.tsx (1)
src/lib/asp.ts (1)
getInputsToSettle(172-179)
src/lib/wallet.ts (4)
src/lib/types.ts (1)
Vtxo(94-94)src/providers/asp.tsx (1)
AspInfo(6-6)src/lib/utxo.ts (1)
getConfirmedAndNotExpiredUtxos(11-15)src/lib/indexer.ts (1)
Indexer(10-38)
src/lib/indexer.ts (1)
src/providers/asp.tsx (1)
AspInfo(6-6)
src/screens/Settings/Vtxos.tsx (2)
src/lib/asp.ts (2)
getInputsToSettle(172-179)settleVtxos(181-198)src/lib/format.ts (1)
prettyDelta(36-55)
src/lib/asp.ts (2)
src/lib/vtxo.ts (1)
getExpiringAndRecoverableVtxos(18-24)src/lib/utxo.ts (1)
getConfirmedAndNotExpiredUtxos(11-15)
src/providers/wallet.tsx (3)
src/lib/wallet.ts (2)
calcBatchLifetimeMs(37-48)calcNextRollover(21-35)src/lib/constants.ts (1)
maxPercentage(9-9)src/lib/asp.ts (2)
renewCoins(200-203)settleVtxos(181-198)
🔇 Additional comments (1)
package.json (1)
8-8: Confirm @arkade-os/sdk 0.3.6 is published before merging.I haven’t been able to confirm that 0.3.6 is available on the npm registry. If it’s not published yet, installs will fail as soon as this lands. Please double-check the registry status before merging.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/lib/indexer.ts (1)
26-33: Persist the commitment start timestampLine 28 stores
commitmentTx.endedAt, butwallet.tsreads this as the batch start. We already guardstartedAt; saving the end time collapses the computed lifetime toward zero and breaks the new threshold logic.- if (!commitmentTx?.endedAt) return null - const when = Number(commitmentTx.endedAt) + if (!commitmentTx?.startedAt) return null + const when = Number(commitmentTx.startedAt)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/Button.tsx(3 hunks)src/components/FlexRow.tsx(3 hunks)src/lib/indexer.ts(1 hunks)src/providers/wallet.tsx(5 hunks)src/screens/Settings/Vtxos.tsx(11 hunks)src/screens/Wallet/Index.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 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/lib/indexer.tssrc/providers/wallet.tsxsrc/screens/Settings/Vtxos.tsx
📚 Learning: 2025-06-30T18:23:34.163Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 114
File: public/wallet-service-worker.mjs:6350-6353
Timestamp: 2025-06-30T18:23:34.163Z
Learning: Do not review `public/wallet-service-worker.mjs` as it is a built file generated by Vite.
Applied to files:
src/providers/wallet.tsx
📚 Learning: 2025-07-21T18:25:36.391Z
Learnt from: bordalix
Repo: arkade-os/wallet PR: 121
File: src/screens/Init/Restore.tsx:58-58
Timestamp: 2025-07-21T18:25:36.391Z
Learning: In src/screens/Init/Restore.tsx, the Input component for private key restoration intentionally omits the `value` prop to make it uncontrolled. This is a security feature to prevent the private key from being displayed if users navigate away and return to the screen, avoiding potential exposure of sensitive data in the UI.
Applied to files:
src/screens/Wallet/Index.tsx
🧬 Code graph analysis (5)
src/lib/indexer.ts (1)
src/providers/asp.tsx (1)
AspInfo(6-6)
src/components/Button.tsx (1)
src/components/FlexRow.tsx (1)
FlexRow(17-48)
src/providers/wallet.tsx (5)
src/lib/types.ts (1)
Wallet(96-103)src/lib/wallet.ts (2)
calcBatchLifetimeMs(37-48)calcNextRollover(21-35)src/lib/constants.ts (1)
maxPercentage(9-9)src/lib/asp.ts (2)
renewCoins(200-203)settleVtxos(181-198)src/lib/storage.ts (1)
saveWalletToStorage(31-33)
src/screens/Settings/Vtxos.tsx (2)
src/lib/asp.ts (2)
getInputsToSettle(172-179)settleVtxos(181-198)src/lib/format.ts (1)
prettyDelta(36-55)
src/screens/Wallet/Index.tsx (1)
src/components/Button.tsx (1)
Button(21-64)
⏰ 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
Summary by CodeRabbit
Chores
New Features
Style