Repository navigation
Use VtxoManager instance from service worker - #442
Conversation
Deploying wallet-mutinynet with
|
| Latest commit: |
349b48a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://75b0862a.arkade-wallet.pages.dev |
| Branch Preview URL: | https://create-vtxo-manager-instance.arkade-wallet.pages.dev |
pietro909
changed the title
|
@arkanaai review |
ReviewWhat & Why: Stops instantiating a fresh Questions / nits:
Otherwise looks clean. Replaces #410. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThreads an injected IVtxoManager through VTXO settlement flows: adds IVtxoManager imports and state, removes the old vtxo helper, updates signatures for getInputsToSettle/settleVtxos/renewCoins, and updates provider and UI call sites to pass vtxoManager. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as "Settings / Transaction UI"
participant WP as "WalletProvider"
participant ASP as "asp lib"
participant VM as "IVtxoManager"
User->>UI: trigger rollover / settle action
UI->>WP: read wallet, svcWallet, vtxoManager
UI->>ASP: getInputsToSettle(wallet, vtxoManager, thresholdMs)
ASP->>VM: getExpiringVtxos(thresholdMs)
VM-->>ASP: expiring vtxos
ASP-->>UI: inputs to settle
UI->>ASP: settleVtxos(wallet, vtxoManager, dustAmount, thresholdMs)
ASP->>VM: execute settlement / queries
VM-->>ASP: progress/result
ASP-->>UI: settlement result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-bitcoin with
|
| Latest commit: |
349b48a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f0aa9eb5.wallet-bitcoin.pages.dev |
| Branch Preview URL: | https://create-vtxo-manager-instance.wallet-bitcoin.pages.dev |
|
Iterative review — new commits Good direction: Bug: missing Sentry import in
Fix: add Everything else in this iteration looks clean — |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/screens/Settings/Vtxos.tsx`:
- Around line 142-144: The file is calling Sentry.captureException in the
renewVtxos:handleRollover error handler but never imports Sentry, causing a
runtime error; add the missing import (e.g., import * as Sentry from
'@sentry/react' or import Sentry from '@sentry/react' depending on project
style) at the top of the file so the Sentry identifier used in
renewVtxos:handleRollover is defined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e71e9f59-ee2f-4d51-9169-fc7239fff351
📒 Files selected for processing (5)
src/lib/asp.tssrc/lib/vtxo.tssrc/providers/wallet.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Transaction.tsx
💤 Files with no reviewable changes (1)
- src/lib/vtxo.ts
🔍 Arkana — Iterative Review (commit
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/screens/Settings/Vtxos.tsx`:
- Around line 113-121: The effect currently calls getInputsToSettle as a
fire-and-forget promise and omits wallet.thresholdMs from the dependency array,
which can produce stale state and unhandled rejections; change the effect to an
async function (or async IIFE) that awaits getInputsToSettle, wraps the call in
try/catch to handle and log errors, and uses a cancellation flag or
AbortController to ignore results from stale responses before calling
setHasBoardingUtxosToSettle, setHasInputsToSettle, setHasVtxosToSettle and
setAboveDust; also include wallet.thresholdMs (or the correct threshold prop) in
the effect dependency array so the effect reruns when threshold changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0eb16f6a-7250-4327-ab9e-f20b845e5b4d
📒 Files selected for processing (1)
src/screens/Settings/Vtxos.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/playwright.yml:
- Line 10: The workflow timeout was lowered to "timeout-minutes: 30" which will
likely cause flaky CI failures given Playwright settings (playwright.config.ts:
retries=5, workers=1, 2 projects) and long test timeouts in
src/test/e2e/swap.test.ts; fix by either restoring a safer per-job timeout
(e.g., increase timeout-minutes back to a larger value such as 60–90) or split
Mobile/Desktop into separate jobs (or a job matrix) so each job runs one
Playwright project and keep a reasonable timeout per job; update the
.github/workflows/playwright.yml entry "timeout-minutes: 30" accordingly and, if
splitting, create two jobs that invoke the same Playwright run but restrict to a
single project each.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23094b9c-a0fa-4dfc-b41c-788cd85253cc
📒 Files selected for processing (1)
.github/workflows/playwright.yml
Not sure what's wrong with the integration tests: they all fail here, but locally they are all passing. |
|
Followup review (commit Three solid improvements to
All three are correctness fixes. No concerns with this commit. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/screens/Settings/Vtxos.tsx`:
- Around line 113-127: When getInputsToSettle in fetchInputs throws, reset the
settlement-derived UI state so stale values don't keep the CTA enabled: inside
the catch (after the cancelled guard) call setHasBoardingUtxosToSettle(false),
setHasInputsToSettle(false), setHasVtxosToSettle(false) and setAboveDust(false)
(or setAboveDust(0 > aspInfo.dust) if you prefer a deterministic boolean) before
logging the error via consoleError(err); keep the cancelled checks and do not
alter behaviour in the success path or change getInputsToSettle itself.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4bda0af5-7fbc-42ab-8ff1-b4fa265aa8a2
📒 Files selected for processing (1)
src/screens/Settings/Vtxos.tsx
| if (!aspInfo || !svcWallet || !vtxoManager) return | ||
| let cancelled = false | ||
| const fetchInputs = async () => { | ||
| try { | ||
| const { boardingUtxos, inputs, vtxos } = await getInputsToSettle(svcWallet, vtxoManager, wallet.thresholdMs) | ||
| if (cancelled) return | ||
| setHasBoardingUtxosToSettle(boardingUtxos.length > 0) | ||
| setHasInputsToSettle(inputs.length > 0) | ||
| setHasVtxosToSettle(vtxos.length > 0) | ||
| const amount = inputs.reduce((a, v) => a + v.value, 0) || 0 | ||
| setAboveDust(amount > aspInfo.dust) | ||
| } catch (err) { | ||
| if (cancelled) return | ||
| consoleError(err) | ||
| } |
There was a problem hiding this comment.
Reset settlement-derived UI state when getInputsToSettle fails.
On Line 124, the catch path only logs and leaves previous has*ToSettle / aboveDust values intact, which can keep the renew CTA enabled from stale data.
💡 Suggested fix
const fetchInputs = async () => {
try {
const { boardingUtxos, inputs, vtxos } = await getInputsToSettle(svcWallet, vtxoManager, wallet.thresholdMs)
if (cancelled) return
+ setError('')
setHasBoardingUtxosToSettle(boardingUtxos.length > 0)
setHasInputsToSettle(inputs.length > 0)
setHasVtxosToSettle(vtxos.length > 0)
const amount = inputs.reduce((a, v) => a + v.value, 0) || 0
setAboveDust(amount > aspInfo.dust)
} catch (err) {
if (cancelled) return
consoleError(err)
+ setHasBoardingUtxosToSettle(false)
+ setHasInputsToSettle(false)
+ setHasVtxosToSettle(false)
+ setAboveDust(false)
+ setError(extractError(err))
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/screens/Settings/Vtxos.tsx` around lines 113 - 127, When
getInputsToSettle in fetchInputs throws, reset the settlement-derived UI state
so stale values don't keep the CTA enabled: inside the catch (after the
cancelled guard) call setHasBoardingUtxosToSettle(false),
setHasInputsToSettle(false), setHasVtxosToSettle(false) and setAboveDust(false)
(or setAboveDust(0 > aspInfo.dust) if you prefer a deterministic boolean) before
logging the error via consoleError(err); keep the cancelled checks and do not
alter behaviour in the success path or change getInputsToSettle itself.
d3b6d2d to
626fb98
Compare
|
🔍 Incremental review (rebased, new head: Changes since last review look solid. Key observations: Cancellation pattern in Vtxos.tsx — The
Sentry capture in
One thing to watch: The No blocking issues. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/screens/Settings/Vtxos.tsx (1)
124-127:⚠️ Potential issue | 🟠 MajorReset settlement-derived flags when input fetch fails.
On failure, the effect logs but keeps prior
has*ToSettle/aboveDustvalues, which can leave CTA state stale.💡 Proposed fix
} catch (err) { if (cancelled) return consoleError(err) + setHasBoardingUtxosToSettle(false) + setHasInputsToSettle(false) + setHasVtxosToSettle(false) + setAboveDust(false) + setError(extractError(err)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/screens/Settings/Vtxos.tsx` around lines 124 - 127, The catch block that handles input fetch failures currently only logs the error and leaves prior settlement flags stale; inside the catch (err) { ... } for the input-fetching effect in Vtxos.tsx, after verifying not cancelled, reset all settlement-derived state (the has*ToSettle and aboveDust flags referenced in the component) to their safe fallback values (e.g., false or null as your state types expect) before calling consoleError(err) so the CTA state cannot remain stale on fetch failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/asp.ts`:
- Around line 249-251: Remove the truthy gate on thresholdMs so
vtxoManager.getExpiringVtxos is always invoked (pass thresholdMs even if
undefined or 0); replace the conditional expression that sets vtxos with a
direct await vtxoManager.getExpiringVtxos(thresholdMs) call and keep the
existing boardingUtxos retrieval via getConfirmedAndNotExpiredUtxos(wallet),
ensuring getExpiringVtxos handles falsy thresholds gracefully and returns an
empty array when appropriate.
In `@src/providers/wallet.tsx`:
- Around line 453-456: The settlePreconfirmed function currently awaits
settleVtxos and then calls notifyTxSettled but does not capture or tag errors;
wrap the await call in a try/catch inside settlePreconfirmed, catch any error
thrown by settleVtxos (or any subsequent step), send a telemetry/diagnostic
event with contextual tags (e.g., function: "settlePreconfirmed", svcWallet id
or descriptor, vtxoManager state, aspInfo.dust, wallet.thresholdMs, and the
error message/stack), then rethrow the caught error so callers still observe
failure; ensure references to settlePreconfirmed, settleVtxos, notifyTxSettled,
svcWallet, vtxoManager, aspInfo.dust, and wallet.thresholdMs are used when
building the telemetry payload.
- Line 357: The call to renewCoins(svcWallet, vtxoMgr, aspInfo.dust,
wallet.thresholdMs) currently swallows all errors via .catch(() => {}); change
this so startup renewal failures are surfaced: either await the promise and let
the error propagate, or replace the empty catch with a handler that logs the
error (include error details) using the existing logger and rethrows or triggers
process exit; update the call site around renewCoins so failures are not
silently ignored and include references to svcWallet, vtxoMgr, aspInfo.dust and
wallet.thresholdMs in the log context.
---
Duplicate comments:
In `@src/screens/Settings/Vtxos.tsx`:
- Around line 124-127: The catch block that handles input fetch failures
currently only logs the error and leaves prior settlement flags stale; inside
the catch (err) { ... } for the input-fetching effect in Vtxos.tsx, after
verifying not cancelled, reset all settlement-derived state (the has*ToSettle
and aboveDust flags referenced in the component) to their safe fallback values
(e.g., false or null as your state types expect) before calling
consoleError(err) so the CTA state cannot remain stale on fetch failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a2fe0f89-2f50-4e05-abbc-c0969d0c55b0
📒 Files selected for processing (6)
.github/workflows/playwright.ymlsrc/lib/asp.tssrc/lib/vtxo.tssrc/providers/wallet.tsxsrc/screens/Settings/Vtxos.tsxsrc/screens/Wallet/Transaction.tsx
💤 Files with no reviewable changes (1)
- src/lib/vtxo.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/playwright.yml
626fb98 to
8ff2f32
Compare
Followup Review — New commits since last reviewNew commit: Good improvements in this iteration: ✅ Stale response cancellation (Vtxos.tsx): The ✅ Error handling (Vtxos.tsx): ✅ Missing dependency: ✅ Sentry tag on handleRollover: One note on the overall PR: The No blocking issues in this iteration. |
8ff2f32 to
8427eb3
Compare
🔍 Arkana Review —
|
8427eb3 to
5c7d3e6
Compare
🔍 Review — Use VtxoManager instance from service workerApproach: Replaces per-call What works well
Items to watch
Cross-repo context
Overall clean refactor. The threshold alignment question is the main thing to resolve before or shortly after merge. |
5c7d3e6 to
593adec
Compare
🔍 Arkana — Iterative Review (rebase
|
🔍 Arkana — Iterative Review (commit
|
33f0c83 to
349b48a
Compare
🔍 Arkana — Iterative Review (commit
|
ghost
mentioned this pull request
Use VtxoManager from the Service Worker wallet, and use that instance instead of creating a new one on every call.
Replaces #410
Depends on #450
Possible behavioral changes
Orphan VTXO recovery [preserved]
The SDK's
getExpiringVtxos()covers the orphan case and more. It returns VTXOs matching any of:isSpendable&&isExpired— this is the orphan case (unspent + expired)isRecoverable— swept + unspentisSubdust— below dust amount (bonus the old code didn't have)Sentry error capture [changed, but preserved]
The old
captureSettleError()was sending settle errors to Sentry with rich context (amount, dust threshold, inputs, wallet address). Updated with:renewVtxos:startupsettlePreconfirmed(wallet.tsx:453): now wraps in try/catch, captures to Sentry with tagrenewVtxos:settlePreconfirmed, then re-throws so callers still see the errorhandleRollover(Vtxos.tsx:141): added Sentry.captureException in the existing catch block with tagrenewVtxos:handleRolloverRisky behavioral changes (postponed)
settlePreconfirmedThe function name says "settle" (on-chain) but now calls vtxoManager.renewVtxos() which does a virtual renewal (off-chain). Are these supposed to be the same thing, or was the on-chain settle intentional?
thresholdMsno longer passed torenewVtxos()Wallet's threshold (wallet.thresholdMs): computed dynamically as
batchLifetimeMs * 10 / 100— i.e., 10% of the batch lifetime. For example, if the batch lifetime is 7 days, the threshold would be ~16.8 hours.SDK's default (renewVtxos() with no args): falls through to
DEFAULT_RENEWAL_CONFIG.thresholdMs = 3 days(since no renewalConfig or settlementConfig is passed by the wallet)These don't match. The wallet was using a dynamic, much shorter threshold (10% of batch lifetime), while the SDK defaults to a static 3 days. The renewVtxos() call will renew VTXOs that are within 3 days of expiry, whereas the old code would only renew those within ~10% of the batch lifetime.
Summary by CodeRabbit
Refactor
UI / Wallet
Bug Fix / Reliability