feat(wallet): HD receive rotation via contract repository (alt to #441)#473
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds HD receive-address rotation via WalletReceiveRotator, integrates provider-side rotator factories, widens ContractManager.refreshVtxos with includeInactive, wires lazy rotator installation into Wallet, and adds tests covering rotation and refresh scope behavior. ChangesHD Receive Rotation Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![arkanaai[bot]](https://avatars.githubusercontent.com/u/170820085?s=60&v=4){kind=small}
There was a problem hiding this comment.
Arkana Code Review — PR #473
Verdict: Request Changes — One bug that can cause state desync on failed contract registration, plus a multi-timelock gap on rotation. Architecture is solid, concurrency model is well-reasoned, test coverage is good. Fixing these two issues would make this mergeable.
🔴 BUG: rotateAndRegister updates tapscript before confirming contract registration
src/wallet/wallet.ts:1487-1509
rotateAndRegister mutates this.offchainTapscript (line 1487-1490) before calling manager.createContract() (line 1496). If createContract throws (network error, repo corruption, disk full), the wallet is left in an inconsistent state:
offchainTapscript/defaultContractScript/getAddress()all reflect the NEW key- The contract manager has no contract watching that new script
- VTXOs sent to the displayed address are invisible to the wallet until next boot
The HD index has also already been consumed via getNextSigningDescriptor(), so the damage is: one skipped index + a wallet showing an unwatched address.
Fix: Move the tapscript update AFTER createContract succeeds:
private async rotateAndRegister(): Promise<void> {
if (!this._hdProvider) return;
const descriptor = await this._hdProvider.getNextSigningDescriptor();
const pubKey = deriveLeafPubkey(descriptor);
const newTapscript = rebuildTapscript(this.offchainTapscript, pubKey);
const csvTimelock = newTapscript.options.csvTimelock
?? DefaultVtxo.Script.DEFAULT_TIMELOCK;
const manager = await this.getContractManager();
await manager.createContract({
type: "default",
params: {
pubKey: hex.encode(pubKey),
serverPubKey: hex.encode(newTapscript.options.serverPubKey),
csvTimelock: timelockToSequence(csvTimelock).toString(),
},
script: hex.encode(newTapscript.pkScript),
address: newTapscript.address(this.network.hrp, this.arkServerPublicKey).encode(),
state: "active",
});
// Only commit the new tapscript after the contract is persisted.
this.offchainTapscript = newTapscript;
}Note: this also fixes a subtle dependency — the current code uses this.defaultContractScript and this.getAddress() inside createContract, which read from the already-mutated offchainTapscript. The fix above computes script and address from the new tapscript directly, making the data flow explicit.
🟡 Multi-timelock gap on rotation
src/wallet/wallet.ts:1483-1509 vs 997-1057
initializeContractManager (line 1005) registers a default contract for every walletContractTimelocks entry at the current pubkey. But rotateAndRegister only registers one contract — at the primary csvTimelock.
If the wallet carries historical timelocks (from server unilateral-exit-delay changes), the rotated address is only watched at the current timelock. On next boot, initializeContractManager self-heals by registering all timelocks at the latest key. But between rotation and reboot, VTXOs created under a historical timelock at the new address would be invisible.
In practice this is low-risk (the server uses the current timelock for new rounds), but it's an inconsistency. Consider either:
- Iterating
walletContractTimelocksinrotateAndRegister(matches boot behavior), or - Documenting explicitly that rotation only watches the primary timelock and relies on boot to fill the gap.
🟡 Silent error swallowing in boot path
src/wallet/wallet.ts:1411-1416
} catch {
// Template not derivable, contract repo unavailable, or
// descriptor mismatch — fall back to the static path
// rather than fail wallet construction.
hdProvider = undefined;
}This catches all errors — including descriptor-mismatch ("Refusing to reuse HD state from a different identity"), which is a critical corruption signal. The wallet silently downgrades to a non-HD single-address mode with no logging, no event, no way for the application to detect the degradation.
Suggestion: at minimum, console.warn the error. Better: emit a wallet event or re-throw on descriptor-mismatch (which indicates seed/repo mismatch — a serious problem that shouldn't be papered over).
🟢 Observations (non-blocking)
Double rotation on rapid events (src/wallet/wallet.ts:1465-1474): The filter event.contractScript !== this.defaultContractScript runs synchronously when the event fires, before the chained rotation completes. If two vtxo_received events fire in the same tick for the same script, both pass the filter and queue two rotations. The second rotation advances the index an extra time. Not a funds-loss issue (old contracts stay active), but it wastes an HD index. A guard in rotateAndRegister that re-checks this.defaultContractScript against the event's script would prevent this.
looksLikeVanillaHDDescriptor (src/wallet/wallet.ts:175): endsWith("/0/*)") is loose — it matches non-BIP86 paths like tr([fp/48'/0'/0'/2']/0/*). Since SeedIdentity constructs BIP-86 descriptors by default, this is fine in practice, but the docstring overpromises.
offchainTapscript mutability on ReadonlyWallet (src/wallet/wallet.ts:281): The readonly → public change is on the base class whose name implies immutability. The field is only mutated from Wallet (subclass), so consider a protected modifier + a setter on Wallet to maintain the "readonly" contract for ReadonlyWallet consumers.
Test quality: The 9 tests are well-structured and cover the critical paths (boot, rotation, persistence, dispose, negative cases). Consider adding a test for the createContract failure scenario once the fix above lands.
⚠️ Protocol-Critical Flag
This PR touches offchain receive address derivation — the path that determines where VTXOs land. While the design is sound (old contracts stay active, no address reuse), the rotateAndRegister ordering bug above means a failed rotation could leave a wallet displaying an unwatched address. Requires human sign-off before merge.
Kukks
force-pushed
the
feat/hd-wallet-via-contracts
branch
4 times, most recently
from
cacf456 to
74ae558
Compare
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 8 commits)
Previous review on May 1 requested changes. This review covers the 4 new commits (May 7–8) that refactored into WalletReceiveRotator, added walletMode, metadata.source tagging, and includeInactive refresh.
The refactoring is a significant quality improvement — clean separation of concerns, proper walletMode polymorphism, and the metadata.source tagging fixes the ambient-contract-confusion bug from the original approach. Test coverage is solid (664 lines for rotation, 196 for includeInactive).
🔴 STILL OPEN: rotate() mutates tapscript before confirming contract registration
src/wallet/walletReceiveRotator.ts:282-842 (new location, same bug)
The original finding from my May 1 review was NOT addressed. rotate() still calls:
wallet.offchainTapscript = rebuildTapscript(wallet.offchainTapscript, pubKey); // line ~785
// ... then later ...
await manager.createContract({...}); // line ~806If createContract throws (network error, repo corruption, disk full):
offchainTapscript/defaultContractScript/getAddress()all reflect the NEW key- No contract is watching the new script
- The HD index is consumed via
getNextSigningDescriptor()but the binding was never persisted to the contract repo - Funds sent to the displayed address are invisible until the next boot (which would re-register via
initializeContractManager)
Now it's slightly worse: currentTaggedScript has NOT been updated (it's set after createContract on line 835), so a subsequent successful rotation would try to deactivate the CORRECT old contract — but the wallet is displaying an unwatched address in the meantime.
Fix remains the same: compute script/address from the new tapscript locally, call createContract, THEN commit wallet.offchainTapscript = newTapscript:
private async rotate(wallet: RotatableWallet): Promise<void> {
const descriptor = await this.provider.getNextSigningDescriptor();
const pubKey = deriveLeafPubkey(descriptor);
const newTapscript = rebuildTapscript(wallet.offchainTapscript, pubKey);
const manager = await wallet.getContractManager();
const csvTimelock = newTapscript.options.csvTimelock
?? DefaultVtxo.Script.DEFAULT_TIMELOCK;
const csvTimelockStr = timelockToSequence(csvTimelock).toString();
const serverPubKeyHex = hex.encode(newTapscript.options.serverPubKey);
const script = hex.encode(newTapscript.pkScript);
const address = newTapscript.address(/* ... */);
await manager.createContract({ script, address, ... });
// Only commit after persistence succeeds.
wallet.offchainTapscript = newTapscript;
const previousTagged = this.currentTaggedScript;
this.currentTaggedScript = script;
if (previousTagged && previousTagged !== script) {
await manager.setContractState(previousTagged, "inactive");
}
}This requires RotatableWallet to expose the network HRP + arkServerPublicKey (or a method to derive the address from a tapscript), but that's a small interface expansion for correctness.
🟡 setContractState failure leaves currentTaggedScript inconsistent
src/wallet/walletReceiveRotator.ts:834-841
currentTaggedScript is updated on line 835 BEFORE setContractState is called on line 840. If setContractState throws, the old tagged contract stays active in the repo. On the next rotation, the rotator tries to deactivate the NEW contract (which is the CURRENT one), not the stale one. The stale contract is orphaned as permanently-active.
Not a funds-loss issue (the stale contract just stays in the watched set forever), but it leaks watched scripts over time, degrading privacy and performance.
Fix: Swap the order — deactivate first, then update currentTaggedScript. Or wrap the deactivation in a try/catch that retains the old value on failure.
🟡 Multi-timelock gap on rotation (from previous review, still applicable)
initializeContractManager registers one contract per walletContractTimelocks entry. rotate() registers only ONE contract at the primary csvTimelock. Between rotation and reboot, a VTXO at a historical timelock on the new address is invisible. Low-risk (server uses current timelock for new rounds), but the inconsistency with the boot path remains.
🟢 New observations (non-blocking)
resolveDescriptorProvider under 'auto' mode (walletReceiveRotator.ts:450-460): The catch {} on HDDescriptorProvider.create still swallows descriptor-mismatch errors silently. This is improved by the existence of walletMode: 'hd' as an explicit strict alternative, which was my suggestion. Acceptable tradeoff for backwards compatibility. Would still benefit from a console.warn for debuggability.
includeInactive implementation is clean. The cursor-advancement logic correctly identifies includeInactive as a superset (cursor-safe). The contracts ? false : opts?.includeInactive guard in refreshVtxos properly prevents interaction with explicit scripts. The 5 new manager tests cover the key invariants. No issues found.
pickActiveReceive sorts by createdAt descending (walletReceiveRotator.ts:386): Correct, but relies on createdAt being monotonically increasing. Two rapid rotations in the same millisecond (possible with Date.now()) could return the wrong "newest." The serialization chain makes this unlikely in practice.
isHDCapableIdentity structural guard (hdCapableIdentity.ts:63-75): Clean type guard. Correctly checks all four required members.
Test quality: Expanded from 9 to ~20 tests. Good coverage of walletMode matrix, metadata.source tagging, baseline non-deactivation, and intruder-contract rejection. Still no test for createContract failure during rotation — please add one when the fix lands.
Cross-repo impact: Zero. No downstream consumers reference any of the new APIs (WalletMode, WalletReceiveRotator, includeInactive, createReceiveRotator). The public API additions are additive.
Bottom line: The refactoring is well-done — WalletReceiveRotator as a standalone class is cleaner than the inline approach, walletMode gives callers proper control, and metadata.source tagging eliminates the ambient-contract-confusion problem. But the original state-desync bug in rotate() persists and needs to be fixed before merge. The fix is straightforward (compute contract params from local newTapscript, persist, THEN commit to wallet state).
Kukks
force-pushed
the
feat/hd-wallet-via-contracts
branch
from
74ae558 to
dd115b9
Compare
There was a problem hiding this comment.
Arkana Code Review — feat(wallet): HD receive rotation via contract repository
Reviewer: Arkana (aggressive protocol review)
Verdict: Request changes — one high-severity boot-path bug, one medium concern on error handling in the rotation chain.
Overall the architecture is clean: the contract-repo-as-source-of-truth design, the WalletReceiveRotator lifecycle, and the ReceiveRotatorFactory extension point are well thought through. The test suite covers the happy path thoroughly. But I found a gap that would cause address drift on every wallet restart.
🔴 HIGH — Address drift on restart without rotation
src/wallet/walletReceiveRotator.ts:125-131 (resolveBoot) + pickActiveReceive (line 330)
pickActiveReceive filters by metadata.source === WALLET_RECEIVE_SOURCE (line 347). But the boot path never tags the initial allocation's contract with this source — initializeContractManager registers baseline contracts without metadata.source (confirmed by the test at test/walletHdRotation.test.ts line 1387: "does not tag boot baseline contracts as wallet-receive").
This means on every restart without rotation, pickActiveReceive returns undefined, and resolveBoot falls through to provider.getNextSigningDescriptor() (line 131), burning a new index:
Boot 1: pickActiveReceive → undefined → getNextSigningDescriptor → index 0 (lastIndexUsed=0)
Boot 2: pickActiveReceive → undefined → getNextSigningDescriptor → index 1 (lastIndexUsed=1)
Boot 3: pickActiveReceive → undefined → getNextSigningDescriptor → index 2 (lastIndexUsed=2)
Consequences:
- Display address changes on every restart without user action — breaks "share this address" UX.
- Index space burned on every restart;
lastIndexUsedratchets forward permanently. - Baseline contracts accumulate in the repo — one set per restart — growing the watcher's active set indefinitely (perf degradation).
- Not a direct fund-loss vector (old baselines stay
activeso VTXOs at old addresses are still detected), but the UX and state pollution are unacceptable.
Missing test: There's no test for "second boot on the same repos WITHOUT rotation preserves the same address." The existing persistence test ("second wallet on the same repos reads the rotated address") only covers the post-rotation case.
Suggested fix (pick one):
- Tag the boot allocation: In
resolveBoot, after allocating the first descriptor, register a tagged display contract immediately (metadata.source = WALLET_RECEIVE_SOURCE) so the next boot finds it. - Add a
getCurrentOrNextmethod to HDDescriptorProvider: IflastIndexUsedis defined, return the same index without incrementing; only allocate a fresh index when called explicitly for rotation. - Peek before allocating: In
defaultBoot, readlastIndexUsedfrom the wallet repo. If defined, re-derive the descriptor at that index without incrementing. Only callgetNextSigningDescriptorwhenlastIndexUsedisundefined.
Option 3 is cleanest because it keeps the provider's getNextSigningDescriptor pure (always advances) while making the boot path idempotent.
🟡 MEDIUM — Silent error swallowing in rotation chain burns an index
src/wallet/walletReceiveRotator.ts:157-159
this.chain = this.chain
.catch(() => undefined)
.then(() => this.rotate(wallet));If rotate() fails partway — specifically after getNextSigningDescriptor() (line 211) but before createContract() (line 247) completes — the allocated index is consumed permanently, the wallet's in-memory offchainTapscript is already swapped to the new pubkey (line 213-216), but no contract is registered for it. The .catch(() => undefined) swallows the error silently.
Post-failure state:
wallet.defaultContractScriptpoints to an address with no registered contract.- The
vtxo_receivedhandler compares against this new script, so events for the OLD script no longer trigger rotation. - The watcher doesn't know about the new script, so VTXOs sent to the displayed address are invisible.
- This is a fund-loss window: the user sees address N, shares it, someone sends funds, but the wallet never detects them.
On restart, pickActiveReceive would fall back to the previous tagged contract (or the baseline), so this is a transient issue within a single session. But the burned index is permanent.
Suggested fix: If createContract fails, roll back wallet.offchainTapscript to its pre-rotation value. Consider logging the error rather than swallowing it. The chain serialization is correct for the happy path, but the error path needs a compensating action.
🟢 Minor / Style
-
src/wallet/wallet.ts:196—offchainTapscriptvisibility:readonly→publiconReadonlyWalletis a wider surface than necessary. SinceRotatableWalletis the write interface, the mutation surface is architecturally contained, but any code with aReadonlyWalletreference can now mutate it at the type level. Consider a@internalJSDoc tag at minimum. -
src/wallet/walletReceiveRotator.ts:349—pickActiveReceivesort stability:.sort((a, b) => b.createdAt - a.createdAt)— if two contracts share the samecreatedAt(same millisecond), the sort is non-deterministic. Low probability in production, but could cause flaky test behavior. Add a tiebreaker (e.g., script comparison). -
Cross-repo impact: Minimal. No downstream repos in the org currently import
WalletConfig,RefreshVtxosOptions, oroffchainTapscript. The newWalletModeexport andincludeInactiveoption are additive. PR #440 (prerequisite) is merged; PR #441 (alternative) is closed. Clean dependency chain. -
includeInactivecursor logic (src/contracts/contractManager.ts): Correct. The diff properly passesincludeInactiveas a separate option tosyncContracts, keepingoptions.contracts === undefinedsomustUpdateCursorstays true for the superset case. The four tests covering cursor advancement / non-advancement are solid.
⚠️ Protocol-Critical Flag
This PR changes receive address derivation and rotation — how the wallet decides where to receive funds. While no VTXOs, signing, forfeit, or exit paths are modified, the receive-address lifecycle directly determines whether incoming funds are detected. Requesting human review before merge, per protocol-critical code policy.
TL;DR: The architecture and test coverage are strong. Fix the boot-path address-drift bug (HIGH) and the rotation-failure error handling (MEDIUM), and this is ready.
🤖 Generated with Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/identity/hdCapableIdentity.ts (1)
64-74: 💤 Low valueType guard claims
HDCapableIdentitybut validates only 4 of ~8 required members.
HDCapableIdentityextends bothReadonlyHDCapableIdentityandIdentity, meaning a trueHDCapableIdentityalso requiressignerSession,signMessage,sign,xOnlyPublicKey, andcompressedPublicKey. The guard checks onlydescriptor,isOurs,signWithDescriptor, andsignMessageWithDescriptor, so any object satisfying those 4 checks will be narrowed to the full interface — including the unchecked methods. Callers that use the narrowed type and invoke e.g.sign()orsignerSession()on such an object will get a runtime error.The docstring acknowledges this is intentional ("the four members the HD wallet flow relies on"), which is fine if the guard is only ever used as a discriminator on known concrete implementations. Consider either:
- Adding the missing method checks so the guard is complete, or
- Narrowing the return type to a minimal structural interface (e.g.
Pick<HDCapableIdentity, 'descriptor' | 'isOurs' | 'signWithDescriptor' | 'signMessageWithDescriptor'>) to accurately reflect what's actually validated.♻️ Option 2 — minimal return type (no behaviour change)
-export function isHDCapableIdentity( - value: unknown -): value is HDCapableIdentity { +type HDCapableIdentityCore = Pick< + HDCapableIdentity, + 'descriptor' | 'isOurs' | 'signWithDescriptor' | 'signMessageWithDescriptor' +>; + +export function isHDCapableIdentity( + value: unknown +): value is HDCapableIdentityCore & HDCapableIdentity {…or simply change the return annotation to
value is HDCapableIdentityCoreand let callers cast to the full type when they know the concrete implementation is complete.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/identity/hdCapableIdentity.ts` around lines 64 - 74, The type guard isHDCapableIdentity currently narrows to HDCapableIdentity but only checks descriptor, isOurs, signWithDescriptor and signMessageWithDescriptor; add checks for the remaining Identity members (signerSession, signMessage, sign, xOnlyPublicKey, compressedPublicKey) to make the guard complete, or change the function's return type to a narrower structural type (e.g. a new HDCapableIdentityCore or Pick<HDCapableIdentity, 'descriptor'|'isOurs'|'signWithDescriptor'|'signMessageWithDescriptor'>) so the signature matches the actual checks; update callers accordingly (cast to full HDCapableIdentity only when the concrete implementation is known).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/wallet.ts`:
- Around line 196-202: The shared mutable offchainTapscript can change
mid-operation causing incorrect change stamping; fix by capturing a
per-operation snapshot or serializing rotations: read this.offchainTapscript
into a local constant at the start of all send/settle flows and pass that
snapshot into updateDbAfterOffchainTx (add a tapscript parameter to
updateDbAfterOffchainTx and any callers), and/or ensure
Wallet.rebuildOffchainTapscript and rotation handlers (e.g., vtxo_received path)
acquire the existing _txLock so rotations are serialized against active
transaction flows; update callers and signatures accordingly so all DB-stamping
uses the captured tapscript, not the live mutable field.
In `@src/wallet/walletReceiveRotator.ts`:
- Around line 379-386: The bug is that this.currentTaggedScript is updated
before retiring the old one, which can orphan the previous contract if
manager.setContractState throws; change the order in the rotation logic so you
first capture previousTagged = this.currentTaggedScript, then if previousTagged
is defined and different from wallet.defaultContractScript call await
manager.setContractState(previousTagged, "inactive") (and let errors propagate
or handle them explicitly), and only after that assign this.currentTaggedScript
= wallet.defaultContractScript; update the code paths around
rotate()/walletReceiveRotator to follow this ordering to preserve the invariant.
- Around line 327-387: The rotate method mutates wallet.offchainTapscript (so
wallet.defaultContractScript and getAddress() reflect the new script) before
registering the new contract, risking an unregistered advertised address if
createContract or getContractManager fails; fix by computing the new
tapscript/pubkey/address/script locally (deriveLeafPubkey(descriptor) +
rebuildTapscript(...) -> localNewTapscript and localScript/address) and call
manager.createContract(...) using those local values first, then only after
successful createContract assign wallet.offchainTapscript = localNewTapscript
and update this.currentTaggedScript; reference rotate, deriveLeafPubkey,
rebuildTapscript, getContractManager, createContract, wallet.offchainTapscript,
wallet.defaultContractScript, and getAddress when making the change.
- Around line 274-277: The current promise chain on this.chain (and matching
.catch in drain/dispose) swallows errors from rotate(wallet) and related
functions (rotate, createContract, setContractState), so update those .catch(()
=> undefined) handlers to surface failures: log the error via the wallet logger
(e.g., wallet.logger.error or processLogger) and/or emit a wallet-level error
event (e.g., this.emit('error', err) or wallet.emitError) with the caught error
and context, then decide whether to rethrow or propagate a rejection so callers
can retry/handle; apply this change to the this.chain.catch(...) in the rotate
flow and the equivalent catch blocks in drain and dispose so rotation failures
are visible to operators.
- Around line 399-412: The error in deriveLeafPubkey currently interpolates the
full descriptor into the thrown Error; to defend against accidental secret
leakage (even if descriptors are normally xpub-only) change the error to avoid
printing the raw descriptor — e.g. throw with a constant/redacted placeholder
like "[REDACTED_DESCRIPTOR]" or include only a safe fingerprint/length instead;
update the throw in deriveLeafPubkey accordingly and ensure callers such as
HDDescriptorProvider.getNextSigningDescriptor / the descriptor property in
seedIdentity.ts still get useful context without exposing the descriptor
content.
In `@test/contracts/manager.test.ts`:
- Around line 474-476: The assertion is too permissive—replace the non-strict
comparison so the test fails if the cursor didn't advance: update the assertion
that checks stateAfter from expect((stateAfter?.lastSyncTime ?? 0) >=
SEEDED_CURSOR).toBe(true) to assert strict advancement (e.g.,
expect((stateAfter?.lastSyncTime ?? 0) > SEEDED_CURSOR).toBe(true)), referencing
walletRepo.getWalletState(), stateAfter.lastSyncTime and SEEDED_CURSOR to ensure
the cursor strictly moved forward.
In `@test/walletHdRotation.test.ts`:
- Around line 488-500: The seeded control contract currently uses a compressed
serverPubKey (starts with 02) so the test can be filtered out for the wrong
reason; update the seed passed to contractRepo.saveContract to match the real
wallet-owned default-contract shape by using the x-only serverPubKey (the
32-byte hex without the 0x02 prefix), ensure pubKey/serverPubKey values and
script match the valid default-contract format, and omit only metadata.source
(do not remove other required fields); reference contractRepo.saveContract,
serverPubKey, pubKey, script, address, state, and metadata.source when making
the change.
---
Nitpick comments:
In `@src/identity/hdCapableIdentity.ts`:
- Around line 64-74: The type guard isHDCapableIdentity currently narrows to
HDCapableIdentity but only checks descriptor, isOurs, signWithDescriptor and
signMessageWithDescriptor; add checks for the remaining Identity members
(signerSession, signMessage, sign, xOnlyPublicKey, compressedPublicKey) to make
the guard complete, or change the function's return type to a narrower
structural type (e.g. a new HDCapableIdentityCore or Pick<HDCapableIdentity,
'descriptor'|'isOurs'|'signWithDescriptor'|'signMessageWithDescriptor'>) so the
signature matches the actual checks; update callers accordingly (cast to full
HDCapableIdentity only when the concrete implementation is known).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 903c675b-9f25-47ad-bc47-449a44060bcf
📒 Files selected for processing (11)
src/contracts/contractManager.tssrc/identity/descriptorProvider.tssrc/identity/hdCapableIdentity.tssrc/identity/index.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/index.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/contracts/manager.test.tstest/walletHdRotation.test.ts
| this.chain = this.chain | ||
| .catch(() => undefined) | ||
| .then(() => this.rotate(wallet)); | ||
| }); |
There was a problem hiding this comment.
Rotation errors are silently swallowed — no log, no surface, no retry signal.
this.chain.catch(() => undefined).then(...) (and the matching .catch(() => undefined) in drain/dispose at lines 287 and 306) means a failure in rotate() (descriptor allocation, tapscript rebuild, createContract, or setContractState) disappears entirely. A user-visible symptom would be "rotation just stopped working" with no diagnostic trail, which is hard to triage in production.
At minimum, route the failure through a logger or a wallet-level error event so operators see it. Example:
Proposed change
- this.unsubscribe = manager.onContractEvent((event) => {
+ this.unsubscribe = manager.onContractEvent((event) => {
if (event.type !== "vtxo_received") return;
if (event.contractScript !== wallet.defaultContractScript) return;
this.chain = this.chain
.catch(() => undefined)
- .then(() => this.rotate(wallet));
+ .then(() => this.rotate(wallet))
+ .catch((err) => {
+ // Surface rather than swallow; the chain pattern
+ // already absorbs it on the next tick, but we want
+ // operators to see persistent rotation failures.
+ console.error("[walletReceiveRotator] rotate failed", err);
+ });
});Use whatever logger / event-emitter convention the wallet uses elsewhere; the key point is that today nothing observes these failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| this.chain = this.chain | |
| .catch(() => undefined) | |
| .then(() => this.rotate(wallet)); | |
| }); | |
| this.chain = this.chain | |
| .catch(() => undefined) | |
| .then(() => this.rotate(wallet)) | |
| .catch((err) => { | |
| // Surface rather than swallow; the chain pattern | |
| // already absorbs it on the next tick, but we want | |
| // operators to see persistent rotation failures. | |
| console.error("[walletReceiveRotator] rotate failed", err); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/wallet/walletReceiveRotator.ts` around lines 274 - 277, The current
promise chain on this.chain (and matching .catch in drain/dispose) swallows
errors from rotate(wallet) and related functions (rotate, createContract,
setContractState), so update those .catch(() => undefined) handlers to surface
failures: log the error via the wallet logger (e.g., wallet.logger.error or
processLogger) and/or emit a wallet-level error event (e.g., this.emit('error',
err) or wallet.emitError) with the caught error and context, then decide whether
to rethrow or propagate a rejection so callers can retry/handle; apply this
change to the this.chain.catch(...) in the rotate flow and the equivalent catch
blocks in drain and dispose so rotation failures are visible to operators.
| await contractRepo.saveContract({ | ||
| type: "default", | ||
| params: { | ||
| pubKey: "0000000000000000000000000000000000000000000000000000000000000000", | ||
| serverPubKey: | ||
| "0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798", | ||
| csvTimelock: "144", | ||
| }, | ||
| script: "ff".repeat(34), | ||
| address: "intruder", | ||
| state: "active", | ||
| createdAt: Date.now(), | ||
| }); |
There was a problem hiding this comment.
This control contract can be filtered out for the wrong reason.
The seeded row uses a compressed serverPubKey, while wallet-owned default contracts store the x-only server key. That means boot can ignore this contract even if the metadata.source filter is broken, so the test doesn't actually prove "missing tag only". Seed it with the same x-only serverPubKey/valid default-contract shape and omit only metadata.source.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/walletHdRotation.test.ts` around lines 488 - 500, The seeded control
contract currently uses a compressed serverPubKey (starts with 02) so the test
can be filtered out for the wrong reason; update the seed passed to
contractRepo.saveContract to match the real wallet-owned default-contract shape
by using the x-only serverPubKey (the 32-byte hex without the 0x02 prefix),
ensure pubKey/serverPubKey values and script match the valid default-contract
format, and omit only metadata.source (do not remove other required fields);
reference contractRepo.saveContract, serverPubKey, pubKey, script, address,
state, and metadata.source when making the change.
| * provider propagate — there is no silent fallback for an explicit | ||
| * provider. | ||
| */ | ||
| export type WalletMode = "auto" | "static" | "hd" | DescriptorProvider; |
There was a problem hiding this comment.
mmm seems to me a leaky abstraction to make people be even aware what HD is? What if the wallet is always in rotate mode, and advanced user may use lower level method to derive "static" address if ever needed?
There was a problem hiding this comment.
Agree on the HD visibility part: "auto" | "static" | "dynamic" would communicate the mode without having to know what "hd" means
Kukks
force-pushed
the
feat/hd-wallet-via-contracts
branch
from
dd115b9 to
ae1644e
Compare
|
Pushed 1.
|
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 11 force-push)
Reviewer: Arkana (aggressive protocol review)
Scope: Incremental — focusing on changes since last review (4-commit rebase: 6d7ade2..ae1644e)
What's fixed since last review ✅
The force-push addressed several critical findings from my previous reviews. Credit where due:
-
Rotation ordering —
rotate()now correctly capturespreviousTaggedbefore callingsetContractState, then updatescurrentTaggedScriptlast. IfsetContractStatethrows, the next rotation retries deactivating the same orphaned contract instead of racing forward. (walletReceiveRotator.ts:255-268) -
Mutation before createContract —
rotate()now builds the new tapscript + script + address entirely locally.wallet.offchainTapscript = newTapscriptonly runs AFTERcreateContractsucceeds. A regression test (createContract failure during rotation does NOT mutate the wallet's displayed tapscript) validates this. Clean fix. -
Index drift on restart —
getCurrentSigningDescriptor()onHDDescriptorProviderpeeks the already-allocated index when no tagged display contract exists. No more burning a fresh HD index per cold restart. Validated by thesecond boot WITHOUT rotation keeps the same addresstest. -
Source tagging —
metadata.source = 'wallet-receive'disambiguates rotated display contracts from baseline contracts.pickActiveReceivefilters on the tag, not "any active default contract." Eliminates the false-positive boot-pickup vector. -
Error surfacing — Rotation chain now calls
console.erroron failure instead of silently swallowing.
🔴 Critical: Rotation/transaction race (still open from prior review)
File: src/wallet/walletReceiveRotator.ts:243 + src/wallet/wallet.ts:2690-2705
rotate() writes wallet.offchainTapscript = newTapscript (walletReceiveRotator.ts:243) without acquiring _txLock. Meanwhile, updateDbAfterOffchainTx reads this.offchainTapscript across 4 separate field accesses (wallet.ts:2690-2705) under _txLock.
Concrete scenario: A vtxo_received event fires while a send() is in-flight. The rotation runs on the microtask queue (Promise chain, no lock), and writes wallet.offchainTapscript between the forfeit() read (line 2690) and the pkScript read (line 2705). The change VTXO is stamped with mixed scripts — forfeit from the OLD tapscript, pkScript from the NEW one. The VTXO becomes unspendable or bound to the wrong contract.
This is a fund-loss vector. The rotation event is triggered by VTXO reception, which is exactly when the wallet might also be in the middle of a send.
Fix options (pick one):
- (A) Snapshot
this.offchainTapscriptinto a local const at the top ofupdateDbAfterOffchainTxand use the snapshot for all 4 reads. Cheapest fix, no lock contention. - (B) Have
rotate()acquire_txLockso it serializes against send/settle. Semantically cleaner but adds latency to rotation.
Option A is strongly preferred — it's a 1-line change and makes the read side immune to any future mutation.
🟡 Medium: Descriptor leaked in error message
File: src/wallet/walletReceiveRotator.ts:283-286 (deriveLeafPubkey)
throw new Error(
`Cannot derive leaf pubkey from descriptor "${descriptor}": ...`
);The full descriptor string is interpolated into the error. Even though descriptors are normally xpub-based, a misconfigured caller could pass an xprv. Error messages surface in logs, crash reporters, and Sentry. Redact or truncate:
throw new Error(
`Cannot derive leaf pubkey from descriptor (length=${descriptor.length}): ...`
);🟡 Medium: Test cursor assertion is non-strict
File: test/contracts/manager.test.ts:475
expect((stateAfter?.lastSyncTime ?? 0) >= SEEDED_CURSOR).toBe(true);This passes even if the cursor didn't advance (equal case). The whole point of the test is proving the cursor moved forward. Use > or toBeGreaterThan:
expect(stateAfter?.lastSyncTime ?? 0).toBeGreaterThan(SEEDED_CURSOR);🟢 Observations (non-blocking)
-
isHDCapableIdentitytype guard (hdCapableIdentity.ts:64-74) — Still checks only 4 members but narrows to the fullHDCapableIdentitytype. Callers that destructure the remaining Identity members (sign,xOnlyPublicKey, etc.) would hit runtime errors on a partial implementation. Low risk in practice since onlyMnemonicIdentity/SeedIdentitypass the guard, but the type annotation overpromises. Consider narrowing the return type or adding a note. -
WalletModedesign — Clean. The'auto'silent-fallback vs'hd'loud-failure distinction is well-thought-out. The escape hatch of passing a rawDescriptorProvidercovers HSM/external-signer use cases without framework coupling. -
includeInactivecursor logic — Correct. ThemustUpdateCursorcheck (options.contracts === undefined && options.window === undefined) ensuresincludeInactive(which widens scope) is cursor-safe whilescripts(which narrows scope) is not. -
Test coverage — Significantly improved. The
createContract failureregression test directly validates the prior ordering fix. Thesecond boot WITHOUT rotationtest catches index drift. Thesecond rotation deactivates previous taggedtest validates the retire chain.
Verdict
The architecture is sound and the prior review findings are substantively addressed. The rotation/transaction race remains the sole blocker — it's a real fund-loss vector on any wallet that receives VTXOs while sending. Fix that (option A is a 1-line snapshot), fix the test assertion, and this is ready.
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 11 force-push)
Reviewer: Arkana (aggressive protocol review)
Scope: Incremental — focusing on changes since last review (4-commit rebase: 6d7ade2..ae1644e)
What's fixed since last review ✅
The force-push addressed several critical findings from my previous reviews. Credit where due:
-
Rotation ordering —
rotate()now correctly capturespreviousTaggedbefore callingsetContractState, then updatescurrentTaggedScriptlast. IfsetContractStatethrows, the next rotation retries deactivating the same orphaned contract instead of racing forward. (walletReceiveRotator.ts:255-268) -
Mutation before createContract —
rotate()now builds the new tapscript + script + address entirely locally.wallet.offchainTapscript = newTapscriptonly runs AFTERcreateContractsucceeds. A regression test (createContract failure during rotation does NOT mutate the wallet's displayed tapscript) validates this. Clean fix. -
Index drift on restart —
getCurrentSigningDescriptor()onHDDescriptorProviderpeeks the already-allocated index when no tagged display contract exists. No more burning a fresh HD index per cold restart. Validated by thesecond boot WITHOUT rotation keeps the same addresstest. -
Source tagging —
metadata.source = 'wallet-receive'disambiguates rotated display contracts from baseline contracts.pickActiveReceivefilters on the tag, not "any active default contract." Eliminates the false-positive boot-pickup vector. -
Error surfacing — Rotation chain now calls
console.erroron failure instead of silently swallowing.
🔴 Critical: Rotation/transaction race (still open from prior review)
File: src/wallet/walletReceiveRotator.ts:243 + src/wallet/wallet.ts:2690-2705
rotate() writes wallet.offchainTapscript = newTapscript (walletReceiveRotator.ts:243) without acquiring _txLock. Meanwhile, updateDbAfterOffchainTx reads this.offchainTapscript across 4 separate field accesses (wallet.ts:2690-2705) under _txLock.
Concrete scenario: A vtxo_received event fires while a send() is in-flight. The rotation runs on the microtask queue (Promise chain, no lock), and writes wallet.offchainTapscript between the forfeit() read (line 2690) and the pkScript read (line 2705). The change VTXO is stamped with mixed scripts — forfeit from the OLD tapscript, pkScript from the NEW one. The VTXO becomes unspendable or bound to the wrong contract.
This is a fund-loss vector. The rotation event is triggered by VTXO reception, which is exactly when the wallet might also be in the middle of a send.
Fix options (pick one):
- (A) Snapshot
this.offchainTapscriptinto a local const at the top ofupdateDbAfterOffchainTxand use the snapshot for all 4 reads. Cheapest fix, no lock contention. - (B) Have
rotate()acquire_txLockso it serializes against send/settle. Semantically cleaner but adds latency to rotation.
Option A is strongly preferred — it's a 1-line change and makes the read side immune to any future mutation.
🟡 Medium: Descriptor leaked in error message
File: src/wallet/walletReceiveRotator.ts:283-286 (deriveLeafPubkey)
throw new Error(
`Cannot derive leaf pubkey from descriptor "${descriptor}": ...`
);The full descriptor string is interpolated into the error. Even though descriptors are normally xpub-based, a misconfigured caller could pass an xprv. Error messages surface in logs, crash reporters, and Sentry. Redact or truncate:
throw new Error(
`Cannot derive leaf pubkey from descriptor (length=${descriptor.length}): ...`
);🟡 Medium: Test cursor assertion is non-strict
File: test/contracts/manager.test.ts:475
expect((stateAfter?.lastSyncTime ?? 0) >= SEEDED_CURSOR).toBe(true);This passes even if the cursor didn't advance (equal case). The whole point of the test is proving the cursor moved forward. Use > or toBeGreaterThan:
expect(stateAfter?.lastSyncTime ?? 0).toBeGreaterThan(SEEDED_CURSOR);🟢 Observations (non-blocking)
-
isHDCapableIdentitytype guard (hdCapableIdentity.ts:64-74) — Still checks only 4 members but narrows to the fullHDCapableIdentitytype. Callers that destructure the remaining Identity members (sign,xOnlyPublicKey, etc.) would hit runtime errors on a partial implementation. Low risk in practice since onlyMnemonicIdentity/SeedIdentitypass the guard, but the type annotation overpromises. Consider narrowing the return type or adding a note. -
WalletModedesign — Clean. The'auto'silent-fallback vs'hd'loud-failure distinction is well-thought-out. The escape hatch of passing a rawDescriptorProvidercovers HSM/external-signer use cases without framework coupling. -
includeInactivecursor logic — Correct. ThemustUpdateCursorcheck (options.contracts === undefined && options.window === undefined) ensuresincludeInactive(which widens scope) is cursor-safe whilescripts(which narrows scope) is not. -
Test coverage — Significantly improved. The
createContract failureregression test directly validates the prior ordering fix. Thesecond boot WITHOUT rotationtest catches index drift. Thesecond rotation deactivates previous taggedtest validates the retire chain. -
Cross-repo impact — Verified: no downstream consumers (banco, demos, asset-demos) import
WalletConfig,WalletMode,DescriptorProvider, or callrefreshVtxos. All reads ofoffchainTapscriptare read-only. Thereadonly → publicchange is backwards-compatible. No cross-repo breakage.
Verdict
The architecture is sound and the prior review findings are substantively addressed. The rotation/transaction race remains the sole blocker — it's a real fund-loss vector on any wallet that receives VTXOs while sending. Fix that (option A is a 1-line snapshot), fix the test assertion, and this is ready.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wallet/walletReceiveRotator.ts`:
- Around line 221-229: The catch block currently swallows all errors and returns
undefined; change it to only swallow the specific non-rangeable-descriptor
compatibility failure and rethrow every other error. Concretely, in the
try/catch around boot (the variable boot, the hasReceiveRotatorFactory check,
provider.createReceiveRotator and WalletReceiveRotator.defaultBoot), replace the
broad catch with logic that calls a predicate like isCompatibilityError(e) (or
checks e.name/e.code/e.message for the compatibility sentinel) and: if
allowSilentFallback && isCompatibilityError(e) return undefined; else throw e.
This ensures transient repo/storage or other failures are rethrown while
preserving the silent fallback only for the documented compatibility case.
- Around line 508-519: pickActiveReceive() currently merges "default" and
"delegate" contracts and can return a tagged contract from a different contract
family; restrict the candidate list to the same contract family as the repo's
current script to avoid cross-family restores. In the filtering chain (where
contractRepository.getContracts is used and you already compare
c.params.serverPubKey and metadata source), add a predicate that requires the
contract to match the repo's current script family—e.g., compare c.params.family
(or c.params.walletScriptFamily / c.params.scriptVersion if present) against
currentTaggedScript.family, or if no explicit family field exists, compare the
contract's script descriptor/pubkey fields to currentTaggedScript
(scriptPubKey/descriptor) so only contracts from the same tapscript family are
considered before sorting and picking the newest.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75bacf64-f3ae-4f43-923c-80b2ed82159d
📒 Files selected for processing (9)
src/contracts/contractManager.tssrc/identity/descriptorProvider.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/index.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/contracts/manager.test.tstest/walletHdRotation.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/identity/descriptorProvider.ts
- src/index.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/contracts/manager.test.ts
- src/contracts/contractManager.ts
- src/wallet/index.ts
- test/walletHdRotation.test.ts
- src/wallet/wallet.ts
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 2 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 15eb9056)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (ae1644e0 → 15eb9056)
New in 15eb9056 (CodeRabbit auto-fixes)
✅ Cross-family contract restore — pickActiveReceive now accepts expectedType and resolveBoot passes the correct family derived from the wallet's tapscript shape. Correct fix, no issues.
✅ Narrower error swallowing — resolveBoot catch block now only swallows errors matching "wildcard descriptor" under auto mode; everything else rethrows. Correct fix.
Outstanding findings (still open from prior reviews)
🔴 BLOCKER: Rotation/transaction race on offchainTapscript
Files: src/wallet/wallet.ts:2690-2705, src/wallet/walletReceiveRotator.ts:448
updateDbAfterOffchainTx reads this.offchainTapscript four times without snapshotting:
- L2690:
.forfeit() - L2691:
.forfeit() - L2694:
.encode() - L2705:
.pkScript
Meanwhile rotate() mutates wallet.offchainTapscript at L448. If a vtxo_received event fires mid-updateDbAfterOffchainTx, the change VTXO gets a Frankenstein tapscript — old forfeit leaf + new pkScript. The VTXO becomes unspendable.
Fix: Snapshot at the top of updateDbAfterOffchainTx:
const tapscript = this.offchainTapscript; // snapshot — rotation may swap this mid-methodThen use tapscript for all 4 reads. One-line fix.
🟡 Descriptor leaked in error message
File: src/wallet/walletReceiveRotator.ts:480
`Cannot derive leaf pubkey from descriptor "${descriptor}": ...`The full descriptor string is interpolated. A misconfigured caller passing an xprv leaks private key material into logs/crash reporters. Redact:
`Cannot derive leaf pubkey from descriptor (length=${descriptor.length}): ...`🟡 Test cursor assertion is non-strict
File: test/contracts/manager.test.ts:476
expect((stateAfter?.lastSyncTime ?? 0) >= SEEDED_CURSOR).toBe(true);Passes even when the cursor doesn't move (equal case). The test's purpose is proving the cursor advanced. Use:
expect(stateAfter?.lastSyncTime ?? 0).toBeGreaterThan(SEEDED_CURSOR);🟡 Multi-timelock gap on rotation
File: src/wallet/walletReceiveRotator.ts:388-440
rotate() registers one contract at the primary csvTimelock. initializeContractManager registers one per walletContractTimelocks entry. After rotation, only the primary timelock has the new pubkey; other timeslots point at stale keys until next boot. Self-heals on restart but creates a window where funds sent to non-primary-timelock rotated addresses aren't watched.
Low risk (most wallets have one timelock), but worth noting for multi-timelock configurations.
Verdict
The 15eb9056 fixes are correct and well-scoped. The rotation/transaction race remains the sole blocker — it's a real fund-loss vector. The other three are medium/low priority but should land before merge.
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 15eb9056)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (ae1644e0 → 15eb9056)
New in 15eb9056 (CodeRabbit auto-fixes)
✅ Cross-family contract restore — pickActiveReceive now accepts expectedType and resolveBoot passes the correct family derived from the wallet's tapscript shape. Correct fix, no issues.
✅ Narrower error swallowing — resolveBoot catch block now only swallows errors matching "wildcard descriptor" under auto mode; everything else rethrows. Correct fix.
Outstanding findings (still open from prior reviews)
🔴 BLOCKER: Rotation/transaction race on offchainTapscript
Files: src/wallet/wallet.ts:2690-2705, src/wallet/walletReceiveRotator.ts:448
updateDbAfterOffchainTx reads this.offchainTapscript four times without snapshotting:
- L2690:
.forfeit() - L2691:
.forfeit() - L2694:
.encode() - L2705:
.pkScript
Meanwhile rotate() mutates wallet.offchainTapscript at L448. If a vtxo_received event fires mid-updateDbAfterOffchainTx, the change VTXO gets a Frankenstein tapscript — old forfeit leaf + new pkScript. The VTXO becomes unspendable.
Fix: Snapshot at the top of updateDbAfterOffchainTx:
const tapscript = this.offchainTapscript; // snapshot — rotation may swap this mid-methodThen use tapscript for all 4 reads. One-line fix.
🟡 Descriptor leaked in error message
File: src/wallet/walletReceiveRotator.ts:480
`Cannot derive leaf pubkey from descriptor "${descriptor}": ...`The full descriptor string is interpolated. A misconfigured caller passing an xprv leaks private key material into logs/crash reporters. Redact:
`Cannot derive leaf pubkey from descriptor (length=${descriptor.length}): ...`🟡 Test cursor assertion is non-strict
File: test/contracts/manager.test.ts:476
expect((stateAfter?.lastSyncTime ?? 0) >= SEEDED_CURSOR).toBe(true);Passes even when the cursor doesn't move (equal case). The test's purpose is proving the cursor advanced. Use:
expect(stateAfter?.lastSyncTime ?? 0).toBeGreaterThan(SEEDED_CURSOR);🟡 Multi-timelock gap on rotation
File: src/wallet/walletReceiveRotator.ts:388-440
rotate() registers one contract at the primary csvTimelock. initializeContractManager registers one per walletContractTimelocks entry. After rotation, only the primary timelock has the new pubkey; other timeslots point at stale keys until next boot. Self-heals on restart but creates a window where funds sent to non-primary-timelock rotated addresses aren't watched.
Low risk (most wallets have one timelock), but worth noting for multi-timelock configurations.
Verdict
The 15eb9056 fixes are correct and well-scoped. The rotation/transaction race remains the sole blocker — it's a real fund-loss vector. The other three are medium/low priority but should land before merge.
Alternative to #441. Wires the HDDescriptorProvider into Wallet using the dotnet-sdk's design: the provider is a pure rotating allocator, and the contract repository is the source of truth for "what addresses am I currently bound to". Boot path - For SeedIdentity wallets with a vanilla BIP-86 descriptor, look up active default contracts whose serverPubKey matches the current server. If any exist, use the most recent contract's pubKey for offchainTapscript — no provider call, the wallet resumes from exactly where the previous session left off. - If no active default contract exists (fresh wallet, or repo cleared), call \`provider.getNextSigningDescriptor()\` once to allocate index 0 so the provider's persisted \`lastIndexUsed\` and the contract we're about to register stay in lockstep. - SingleKey wallets, and SeedIdentities with a non-vanilla descriptor, stay on the static path. Identical pre-PR behaviour. Rotation - Subscribe to \`vtxo_received\` on the contract manager. When the event fires for the currently-active default contract: 1. Allocate the next descriptor via the provider. 2. Rebuild offchainTapscript with the new pubkey. 3. Register the new default contract via \`ContractManager.createContract\`. The previous default contract entry stays active so previously-shared addresses keep crediting the wallet. - Rotations are serialised by an internal \`_hdRotationChain\` mutex so rapid-fire events can't overlap the allocate → rebuild → create sequence. Dispose - Tear down the rotation subscription first, then drain any in-flight rotation, then dispose the contract manager. A late \`vtxo_received\` event cannot queue work on a disposing wallet. Compared to #441 - No \`getCurrentReceivePubkey()\` accessor on the provider — the provider has no notion of "current". The wallet boot looks up active contracts in the repository instead. - No \`rotateReceive\` / \`getCurrentReceiveIndex\` either; the provider only exposes \`getNextSigningDescriptor\`. - \`offchainTapscript\` mutability widened (\`readonly\` → \`public\`) same as #441 — required to swap the script after rotation. Tests - \`test/walletHdRotation.test.ts\`: 9 tests — install conditioning (HD installed for vanilla, skipped for SingleKey, contract repo receives the boot allocation), rotation behaviour (rotates only on matching script + event type, leaves prior contract active), persistence (second wallet on same repos resumes the rotated address), dispose teardown.
Switch the boot-time \"what's my current display address?\" lookup from \"any active default contract\" to \"the active default contract *tagged* as the wallet's own receive address\". Borrowed from the btcpay-arkade plugin's pattern: every contract records where and why it was generated via \`metadata.source\`, and the wallet only cares about the ones it generated for itself. Why - The previous lookup (\"newest active default whose serverPubKey matches\") is fragile when the same contract repository ends up holding default contracts created by other code paths — legacy timelock variants, external integrations, future contract types. Picking \"newest\" risks resurrecting the wrong pubkey. - The tag makes ownership explicit: \`metadata.source === 'wallet-receive'\` is unambiguous and stable across timelock variants, server rotations, and external repo seeders. What changed (\`src/wallet/wallet.ts\`) - Add \`WALLET_RECEIVE_SOURCE = 'wallet-receive'\` constant. - \`pickActiveReceivePubkey\`: filter active contracts by \`metadata.source === WALLET_RECEIVE_SOURCE\` (and matching \`serverPubKey\`). Searches both \`default\` and \`delegate\` types so delegate wallets resolve correctly. - \`initializeContractManager\`: tag *only* the contract whose \`script\` matches \`this.defaultContractScript\` (the wallet's current display address). Past timelock variants and the default-companion of a delegate wallet stay untagged — they are watch entries, not the wallet's display. - \`rotateAndRegister\`: tag the freshly-rotated contract with \`source: WALLET_RECEIVE_SOURCE\`. Picks \`type: 'delegate'\` vs \`type: 'default'\` based on the wallet's tapscript shape so rotation works for both default and delegate wallets. Tests (\`test/walletHdRotation.test.ts\`) - Added \"tags the wallet's display contract with source=wallet-receive\" — verifies exactly one tagged contract is registered at boot, with the correct script. - Added \"second wallet ignores active default contracts without the source tag\" — pre-seeds an unrelated active default contract and asserts the boot path does NOT pick it up (instead allocates fresh).
Drop two ad-hoc helpers in favour of the canonical alternatives. - \`looksLikeVanillaHDDescriptor\` + \`config.identity instanceof SeedIdentity\` — replaced with the new \`isHDCapableIdentity()\` structural type guard. The guard checks the four members the HD wallet flow actually uses (\`descriptor\`, \`isOurs\`, \`signWithDescriptor\`, \`signMessageWithDescriptor\`) and narrows to \`HDCapableIdentity\` for downstream calls. The \"vanilla BIP-86\" string check is gone — if the descriptor isn't a ranged template, the provider's first \`getNextSigningDescriptor\` call throws and the surrounding try/catch falls back to the static path. - \`bytesEqual\` — replaced with \`equalBytes\` from \`@scure/btc-signer/utils.js\`. No more rolling our own. Type guard lives next to the interface in \`src/identity/hdCapableIdentity.ts\` and is re-exported through the identity barrel so other consumers can branch on HD capability without coupling to a concrete identity class.
Five related changes around HD receive rotation, kept together because
they share the rotator extraction:
* refactor: extract `WalletReceiveRotator` from wallet.ts. Owns the
`DescriptorProvider`, the `vtxo_received` subscription, the rotation
chain, the boot pubkey lookup (`pickActiveReceive`), and the
contract registration on rotate. wallet.ts shrinks ~225 lines: 1
field, ~10 lines in `Wallet.create`, 1 line in `dispose`.
* feat: polymorphic `walletMode: 'auto' | 'static' | 'hd' | DescriptorProvider`
on `WalletConfig`. `'static'` skips HD wiring, `'hd'` requires it
(throws on non-HD identity), passing a `DescriptorProvider` instance
drives rotation through it. `'auto'` (default) preserves today's
silent-fallback behaviour. The polymorphic field makes the
contradicting `static + provider` combo structurally unrepresentable.
* fix: keep index-0 baseline contracts active and UNTAGGED. The
`metadata.source = 'wallet-receive'` tag is now strictly the
"current rotated display" marker. Boot baseline contracts (default +
delegate × every walletContractTimelocks entry) stay registered as
always-active so addresses derived from index 0 keep crediting the
wallet forever.
* feat: on rotation, mark the previously-tagged display contract
`inactive`. The watcher's filter (`state === 'active' || lastKnownVtxos.size > 0`)
keeps watching it as long as it has unspent VTXOs, so funds in
flight at the old display address aren't lost — only the address
stops being advertised. The first rotation does NOT deactivate the
baseline (the rotator's `currentTaggedScript` is `undefined` until
the first tagged contract is created).
* feat: `refreshVtxos({ includeInactive: true })` escape hatch.
Default `refreshVtxos()` syncs only the watcher's active set;
`includeInactive` widens it to every contract in the repo for the
"did anyone send to a stale rotated address?" audit case. Cursor is
not advanced (targeted operation, same rule as the `scripts` path).
Ignored when `scripts` is also set.
Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <[email protected]>
Address overlapping Arkana / CodeRabbit findings on the rotator: * **Rotation/transaction race (Arkana 🔴)** — `updateDbAfterOffchainTx` used to read `this.offchainTapscript` across five field accesses (`forfeit`, `forfeit`, `encode`, `pkScript`, `arkAddress`-via-getter) while a concurrent `vtxo_received` could trigger `rotate()` and swap the field between reads, stamping the change VTXO with mixed scripts. Now snapshots `offchainTapscript` and the display address synchronously at function entry, before any `await`. Option (A) from Arkana's recommendation — a local snapshot, no extra lock contention. * **Descriptor leak in error message (Arkana / CodeRabbit 🟡)** — `deriveLeafPubkey` interpolated the full descriptor into its error, which would surface in logs / crash reporters. A misconfigured caller could pass an xprv. Redacted to `length=N`. * **Cursor assertion non-strict (Arkana 🟡)** — the `includeInactive` cursor-advance test used `>=` which passed even on the no-op case. Tightened to `toBeGreaterThan`.
Kukks
force-pushed
the
feat/hd-wallet-via-contracts
branch
from
15eb905 to
203fa4a
Compare
|
Rebased onto master ( Fixes
Rebase notesOne conflict in Verification
Integration CI rerunning on the rebased tip — the previous run's Still in the "non-blocking" pile from prior reviews
Happy to address any of these in this PR if you want, otherwise filing as follow-ups. |
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 203fa4a)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (15eb9056 → 203fa4a)
Previous findings — all resolved ✅
🔴 → ✅ Rotation/transaction race on offchainTapscript
File: src/wallet/wallet.ts:2638-2649
Correctly fixed. updateDbAfterOffchainTx now snapshots both offchainTapscript and primaryAddress synchronously at function entry (before any await), and all downstream reads use the snapshot. The comment explains the invariant clearly. The Frankenstein-tapscript vector is closed.
🟡 → ✅ Descriptor leaked in error message
File: src/wallet/walletReceiveRotator.ts:479-484
Now emits (length=${descriptor.length}) instead of the raw descriptor. Good.
🟡 → ✅ Test cursor assertion
File: test/contracts/manager.test.ts:474-476
Now uses toBeGreaterThan(SEEDED_CURSOR) — strict forward check.
New changes in this delta — reviewed
✅ ExtendedContractVtxo type (src/contracts/types.ts:85-101): Correctly mirrors the ExtendedVirtualCoin / VirtualCoin split. ContractVtxo keeps Partial<TapLeaves> for raw indexer data; ExtendedContractVtxo narrows to required after annotation. extraWitness is inherited from ExtendedVirtualCoin. Return types in contractManager.ts updated consistently across all methods (getVtxosForContracts, refreshContracts, fetchContractVtxosBulk, etc.). No breakage.
✅ isAnnotated type guard (src/wallet/delegator.ts:505-511): Runtime narrowing before passing ContractVtxo[] to makeDelegateForfeitTx. Correct — filters out unannotated vtxos that would blow up on tapTree access.
✅ extractArkProviderUrl + indexer resolution (src/wallet/wallet.ts:119-124, 269-286): Clean provider URL fallback chain: explicit indexerUrl > derived from arkProvider.serverUrl > arkadeServerUrl. Throws when a custom arkProvider has no discoverable URL and no explicit indexerUrl — prevents silent cross-server pairing. Good defensive behavior.
✅ Watcher enrichment tests (test/contracts/watcher.test.ts:255-390): Two new test cases: (1) delegate contract vtxos get correct tapscript, not default fallback; (2) unknown handler type falls back to raw vtxo + console.warn. Good coverage for the contractWatcher.ts catch-path improvement.
✅ VirtualCoin field reorder + JSDoc (src/wallet/index.ts:546-568): Fields reordered for logical grouping; JSDoc improved. No structural change — all fields identical.
✅ ESM import fixes (various files + scripts/add-extensions.js): .js extensions added to @scure/btc-signer sub-path imports. Build script extended to also fix .d.ts declaration files. Build order corrected (add-extensions runs after all tsc passes). Mechanical, correct.
✅ README cleanup: Removed hardcoded arkServerUrl from examples — consistent with DEFAULT_ARKADE_SERVER_URL default.
Still noted (unchanged, low-risk)
🟡 Multi-timelock gap on rotation (walletReceiveRotator.ts:388-440): rotate() still registers one contract at the primary csvTimelock only. Self-heals on next boot. Acknowledged as low-risk in the PR thread — tracking for future follow-up.
Verdict
All three prior findings fixed correctly. New code (type narrowing, provider resolution, watcher tests) is clean and well-structured. The rotation/transaction race — the only blocker — is closed.
Approving the code.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/wallet.ts (1)
1234-1257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRotator dispose error skips downstream teardown.
await this._receiveRotator?.dispose()runs before thetry/finallythat guards super.dispose() and vtxoManager.dispose() with the "best-effort teardown" comment. If the rotator throws while draining an in-flightcreateContract(it explicitly waits on those per the PR objectives), the contract manager and vtxoManager are leaked. Wrap it in the same defensive pattern so a throwing rotator still releases the rest.♻️ Proposed fix
override async dispose(): Promise<void> { // Tear down the rotation subscription + drain in-flight rotations // first so no late `vtxo_received` event can queue work on a // disposing wallet, and so any in-flight `createContract` call // finishes before we dispose the contract manager underneath it. - await this._receiveRotator?.dispose(); + try { + await this._receiveRotator?.dispose(); + } catch (err) { + console.warn("Wallet.dispose: receive rotator dispose failed", err); + } const manager = this._vtxoManager ?? (this._vtxoManagerInitializing ? await this._vtxoManagerInitializing.catch(() => undefined) : undefined);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/wallet/wallet.ts` around lines 1234 - 1257, The current dispose method calls await this._receiveRotator?.dispose() before entering the try/finally that guarantees teardown, so a thrown error from the rotator can skip disposing the contract/vtxo manager and super.dispose(); move the rotator disposal into the same defensive pattern or wrap it in its own try/catch inside the same try block: call await this._receiveRotator?.dispose() inside the try that also disposes manager (or catch and log/ignore errors from this._receiveRotator.dispose()) so that regardless of rotator failure the code still sets this._vtxoManager = undefined, clears this._vtxoManagerInitializing, attempts manager.dispose(), and always awaits super.dispose().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/wallet/wallet.ts`:
- Around line 1234-1257: The current dispose method calls await
this._receiveRotator?.dispose() before entering the try/finally that guarantees
teardown, so a thrown error from the rotator can skip disposing the
contract/vtxo manager and super.dispose(); move the rotator disposal into the
same defensive pattern or wrap it in its own try/catch inside the same try
block: call await this._receiveRotator?.dispose() inside the try that also
disposes manager (or catch and log/ignore errors from
this._receiveRotator.dispose()) so that regardless of rotator failure the code
still sets this._vtxoManager = undefined, clears this._vtxoManagerInitializing,
attempts manager.dispose(), and always awaits super.dispose().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b8e9c6c3-b16e-4015-bb95-dcb28c36abbf
📒 Files selected for processing (11)
src/contracts/contractManager.tssrc/identity/descriptorProvider.tssrc/identity/hdCapableIdentity.tssrc/identity/index.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/index.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/contracts/manager.test.tstest/walletHdRotation.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/identity/descriptorProvider.ts
- src/identity/index.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/index.ts
- src/wallet/index.ts
- src/identity/hdCapableIdentity.ts
- src/contracts/contractManager.ts
- src/wallet/hdDescriptorProvider.ts
- test/contracts/manager.test.ts
- test/walletHdRotation.test.ts
- src/wallet/walletReceiveRotator.ts
`initializeContractManager` used to construct the multi-timelock matrix (default + delegate × every `walletContractTimelocks` entry) at `this.offchainTapscript.options.pubKey` — which is the CURRENT display pubkey, including any HD rotation. On a wallet that's rotated past index 0, every boot re-registered the full matrix at the rotated pubkey, diluting the design's intent and turning every rotation into a permanent multi-timelock expansion. The matrix is bound to INDEX 0 by design: it's the permanent fallback set the wallet wants active forever, independent of any rotation. Rotated display contracts are intentionally single-timelock-single-pubkey at the current arkd delay, registered by `WalletReceiveRotator.rotate` and tagged with `metadata.source = WALLET_RECEIVE_SOURCE`. This commit anchors the matrix to `await this.identity.xOnlyPublicKey()` — the identity's static pubkey, equal to index 0 for HD wallets — so the baseline stays at index 0 across all boots regardless of rotation state. Regression test: `does NOT re-register the multi-timelock matrix at a rotated pubkey on reboot`. Rotates once, then boots a second wallet on the same repos and asserts that the rotated pubkey appears in exactly ONE tagged contract (the display), never duplicated across timelocks.
|
On the "multi-timelock gap on rotation" item — that was misframed as low-risk-self-heals. It's an actual design choice the boot path wasn't honoring. Fixed in The design (now explicit in code)
What the bug was
There was nothing to "self-heal" — there was something to NOT re-register. The fix
Regression test
|
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 ca2fa03)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (203fa4a → ca2fa03)
Changes reviewed
Single commit ca2fa03 — fix(wallet): bind baseline matrix to index 0, not the rotated pubkey
What it does: In initializeContractManager, the baseline multi-timelock contract matrix (default + delegate × every walletContractTimelocks entry) was previously using this.offchainTapscript.options.pubKey. After HD rotation + reboot, that field holds the rotated pubkey, so the matrix would silently shift away from index 0 — breaking the "index-0 stays active forever" invariant.
The fix introduces const baselinePubkey = await this.identity.xOnlyPublicKey() (which always returns indexZero.pubkey on HD identities) and uses it for both DefaultVtxo and DelegateVtxo script construction.
Verdict
✅ Correct fix. Verified:
ReadonlyDescriptorIdentity.xOnlyPublicKey()returnsthis.indexZero.pubkey— always index 0 regardless of HD state (seedIdentity.ts:505-508).serverPubKeyanddelegatePubKeystill read fromthis.offchainTapscript.options— correct, these are server-side params unaffected by rotation.- The comment block (
wallet.ts:937-949) accurately describes the design invariant. - New test (
walletHdRotation.test.ts:498-562) covers the exact regression: rotate → reboot → assert matrix stays at index 0, rotated pubkey only appears in the single tagged display contract, and contract count doesn't grow.
No open findings
All prior flagged items from my May 11/12 reviews have been addressed. No new issues in this delta.
🤖 Arkana
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 ca2fa03)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (203fa4a → ca2fa03)
Changes reviewed
Single commit ca2fa03 — fix(wallet): bind baseline matrix to index 0, not the rotated pubkey
What it does: In initializeContractManager, the baseline multi-timelock contract matrix (default + delegate × every walletContractTimelocks entry) was previously using this.offchainTapscript.options.pubKey. After HD rotation + reboot, that field holds the rotated pubkey, so the matrix would silently shift away from index 0 — breaking the "index-0 stays active forever" invariant.
The fix introduces const baselinePubkey = await this.identity.xOnlyPublicKey() (which always returns indexZero.pubkey on HD identities) and uses it for both DefaultVtxo and DelegateVtxo script construction.
Verdict
✅ Correct fix. Verified:
ReadonlyDescriptorIdentity.xOnlyPublicKey()returnsthis.indexZero.pubkey— always index 0 regardless of HD state (seedIdentity.ts:505-508).serverPubKeyanddelegatePubKeystill read fromthis.offchainTapscript.options— correct, these are server-side params unaffected by rotation.- The comment block (
wallet.ts:937-949) accurately describes the design invariant. - New test (
walletHdRotation.test.ts:498-562) covers the exact regression: rotate → reboot → assert matrix stays at index 0, rotated pubkey only appears in the single tagged display contract, and contract count doesn't grow.
No open findings
All prior flagged items from my May 11/12 reviews have been addressed. No new issues in this delta.
🤖 Arkana
`prepareUnrollTransaction` called `BigInt(feeRate)` where `feeRate` is a `number` returned by `OnchainProvider.getFeeRate()`. Esplora's `/fee-estimates` returns plain numbers and bitcoind regtest sometimes reports fractional sat/vB (e.g. 1.006 with a few low-fee txs in the mempool), making `BigInt(1.006)` throw `RangeError: cannot be converted to a BigInt because it is not an integer`. Round up before the BigInt conversion: pay at least the advertised rate (fractional sat/vB is meaningless on the wire) and satisfy BigInt's integer requirement. Surfaced by `should complete unroll after unilateral exit delay` going red in CI on a fee rate of 1.006. Pre-existing bug from the `prepareUnrollTransaction` extraction in #479 — master had the same code, master CI just hasn't rolled the unlucky regtest fee yet.
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 afb6def)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (ca2fa03 → afb6def)
Changes reviewed
Single commit afb6def — fix(unroll): Math.ceil feeRate before BigInt conversion
The bug: prepareUnrollTransaction called BigInt(feeRate) where feeRate comes from OnchainProvider.getFeeRate(). Esplora / bitcoind regtest can return fractional sat/vB (e.g. 1.006), causing BigInt(1.006) to throw RangeError. This is a real crash in the unilateral exit path — protocol-critical.
The fix: BigInt(Math.ceil(feeRate)) — rounds up before conversion. Correct direction (overpay by < 1 sat/vB, never underpay). Wallet.MIN_FEE_RATE = 1 is already an integer, so the floor guard doesn't bypass the ceil.
Verified
- ✅ Only call site for
.fee(BigInt(feeRate))in the codebase — no other instances of this bug pattern. - ✅
onchain.tshandlesfeeRatedifferently (stays asnumber, usesMath.ceil(feeRate * vsize)) — not affected. - ✅ Comment accurately describes the failure mode.
No findings
Clean, minimal, correct fix for a crash in the unilateral exit path.
🤖 Arkana
There was a problem hiding this comment.
Arkana Code Review — PR #473 (Update: May 12 afb6def)
Reviewer: Arkana (aggressive protocol review)
Scope: Delta since last review (ca2fa03 → afb6def)
Changes reviewed
Single commit afb6def — fix(unroll): Math.ceil feeRate before BigInt conversion
The bug: prepareUnrollTransaction called BigInt(feeRate) where feeRate comes from OnchainProvider.getFeeRate(). Esplora / bitcoind regtest can return fractional sat/vB (e.g. 1.006), causing BigInt(1.006) to throw RangeError. This is a real crash in the unilateral exit path — protocol-critical.
The fix: BigInt(Math.ceil(feeRate)) — rounds up before conversion. Correct direction (overpay by < 1 sat/vB, never underpay). Wallet.MIN_FEE_RATE = 1 is already an integer, so the floor guard doesn't bypass the ceil.
Verified
- ✅ Only call site for
.fee(BigInt(feeRate))in the codebase — no other instances of this bug pattern. - ✅
onchain.tshandlesfeeRatedifferently (stays asnumber, usesMath.ceil(feeRate * vsize)) — not affected. - ✅ Comment accurately describes the failure mode.
No findings
Clean, minimal, correct fix for a crash in the unilateral exit path.
🤖 Arkana
|
tested in the Wallet @Kukks: rotation works per-receive, DB updates. Let's go! EDIT: I've used arkade-os/wallet#545 which I also updated with latest master branch fyi 
|
…473) (#489) * feat(wallet): HD receive rotation backed by the contract repository Alternative to #441. Wires the HDDescriptorProvider into Wallet using the dotnet-sdk's design: the provider is a pure rotating allocator, and the contract repository is the source of truth for "what addresses am I currently bound to". Boot path - For SeedIdentity wallets with a vanilla BIP-86 descriptor, look up active default contracts whose serverPubKey matches the current server. If any exist, use the most recent contract's pubKey for offchainTapscript — no provider call, the wallet resumes from exactly where the previous session left off. - If no active default contract exists (fresh wallet, or repo cleared), call \`provider.getNextSigningDescriptor()\` once to allocate index 0 so the provider's persisted \`lastIndexUsed\` and the contract we're about to register stay in lockstep. - SingleKey wallets, and SeedIdentities with a non-vanilla descriptor, stay on the static path. Identical pre-PR behaviour. Rotation - Subscribe to \`vtxo_received\` on the contract manager. When the event fires for the currently-active default contract: 1. Allocate the next descriptor via the provider. 2. Rebuild offchainTapscript with the new pubkey. 3. Register the new default contract via \`ContractManager.createContract\`. The previous default contract entry stays active so previously-shared addresses keep crediting the wallet. - Rotations are serialised by an internal \`_hdRotationChain\` mutex so rapid-fire events can't overlap the allocate → rebuild → create sequence. Dispose - Tear down the rotation subscription first, then drain any in-flight rotation, then dispose the contract manager. A late \`vtxo_received\` event cannot queue work on a disposing wallet. Compared to #441 - No \`getCurrentReceivePubkey()\` accessor on the provider — the provider has no notion of "current". The wallet boot looks up active contracts in the repository instead. - No \`rotateReceive\` / \`getCurrentReceiveIndex\` either; the provider only exposes \`getNextSigningDescriptor\`. - \`offchainTapscript\` mutability widened (\`readonly\` → \`public\`) same as #441 — required to swap the script after rotation. Tests - \`test/walletHdRotation.test.ts\`: 9 tests — install conditioning (HD installed for vanilla, skipped for SingleKey, contract repo receives the boot allocation), rotation behaviour (rotates only on matching script + event type, leaves prior contract active), persistence (second wallet on same repos resumes the rotated address), dispose teardown. * refactor(wallet): tag wallet-owned contracts with metadata.source Switch the boot-time \"what's my current display address?\" lookup from \"any active default contract\" to \"the active default contract *tagged* as the wallet's own receive address\". Borrowed from the btcpay-arkade plugin's pattern: every contract records where and why it was generated via \`metadata.source\`, and the wallet only cares about the ones it generated for itself. Why - The previous lookup (\"newest active default whose serverPubKey matches\") is fragile when the same contract repository ends up holding default contracts created by other code paths — legacy timelock variants, external integrations, future contract types. Picking \"newest\" risks resurrecting the wrong pubkey. - The tag makes ownership explicit: \`metadata.source === 'wallet-receive'\` is unambiguous and stable across timelock variants, server rotations, and external repo seeders. What changed (\`src/wallet/wallet.ts\`) - Add \`WALLET_RECEIVE_SOURCE = 'wallet-receive'\` constant. - \`pickActiveReceivePubkey\`: filter active contracts by \`metadata.source === WALLET_RECEIVE_SOURCE\` (and matching \`serverPubKey\`). Searches both \`default\` and \`delegate\` types so delegate wallets resolve correctly. - \`initializeContractManager\`: tag *only* the contract whose \`script\` matches \`this.defaultContractScript\` (the wallet's current display address). Past timelock variants and the default-companion of a delegate wallet stay untagged — they are watch entries, not the wallet's display. - \`rotateAndRegister\`: tag the freshly-rotated contract with \`source: WALLET_RECEIVE_SOURCE\`. Picks \`type: 'delegate'\` vs \`type: 'default'\` based on the wallet's tapscript shape so rotation works for both default and delegate wallets. Tests (\`test/walletHdRotation.test.ts\`) - Added \"tags the wallet's display contract with source=wallet-receive\" — verifies exactly one tagged contract is registered at boot, with the correct script. - Added \"second wallet ignores active default contracts without the source tag\" — pre-seeds an unrelated active default contract and asserts the boot path does NOT pick it up (instead allocates fresh). * refactor(wallet): use HDCapableIdentity guard + scure equalBytes Drop two ad-hoc helpers in favour of the canonical alternatives. - \`looksLikeVanillaHDDescriptor\` + \`config.identity instanceof SeedIdentity\` — replaced with the new \`isHDCapableIdentity()\` structural type guard. The guard checks the four members the HD wallet flow actually uses (\`descriptor\`, \`isOurs\`, \`signWithDescriptor\`, \`signMessageWithDescriptor\`) and narrows to \`HDCapableIdentity\` for downstream calls. The \"vanilla BIP-86\" string check is gone — if the descriptor isn't a ranged template, the provider's first \`getNextSigningDescriptor\` call throws and the surrounding try/catch falls back to the static path. - \`bytesEqual\` — replaced with \`equalBytes\` from \`@scure/btc-signer/utils.js\`. No more rolling our own. Type guard lives next to the interface in \`src/identity/hdCapableIdentity.ts\` and is re-exported through the identity barrel so other consumers can branch on HD capability without coupling to a concrete identity class. * refactor(wallet): WalletReceiveRotator + walletMode + audit-sync flag Five related changes around HD receive rotation, kept together because they share the rotator extraction: * refactor: extract `WalletReceiveRotator` from wallet.ts. Owns the `DescriptorProvider`, the `vtxo_received` subscription, the rotation chain, the boot pubkey lookup (`pickActiveReceive`), and the contract registration on rotate. wallet.ts shrinks ~225 lines: 1 field, ~10 lines in `Wallet.create`, 1 line in `dispose`. * feat: polymorphic `walletMode: 'auto' | 'static' | 'hd' | DescriptorProvider` on `WalletConfig`. `'static'` skips HD wiring, `'hd'` requires it (throws on non-HD identity), passing a `DescriptorProvider` instance drives rotation through it. `'auto'` (default) preserves today's silent-fallback behaviour. The polymorphic field makes the contradicting `static + provider` combo structurally unrepresentable. * fix: keep index-0 baseline contracts active and UNTAGGED. The `metadata.source = 'wallet-receive'` tag is now strictly the "current rotated display" marker. Boot baseline contracts (default + delegate × every walletContractTimelocks entry) stay registered as always-active so addresses derived from index 0 keep crediting the wallet forever. * feat: on rotation, mark the previously-tagged display contract `inactive`. The watcher's filter (`state === 'active' || lastKnownVtxos.size > 0`) keeps watching it as long as it has unspent VTXOs, so funds in flight at the old display address aren't lost — only the address stops being advertised. The first rotation does NOT deactivate the baseline (the rotator's `currentTaggedScript` is `undefined` until the first tagged contract is created). * feat: `refreshVtxos({ includeInactive: true })` escape hatch. Default `refreshVtxos()` syncs only the watcher's active set; `includeInactive` widens it to every contract in the repo for the "did anyone send to a stale rotated address?" audit case. Cursor is not advanced (targeted operation, same rule as the `scripts` path). Ignored when `scripts` is also set. * fix: apply CodeRabbit auto-fixes Fixed 1 file(s) based on 2 unresolved review comments. Co-authored-by: CodeRabbit <[email protected]> * fix(wallet): rotation/transaction race + review nits Address overlapping Arkana / CodeRabbit findings on the rotator: * **Rotation/transaction race (Arkana 🔴)** — `updateDbAfterOffchainTx` used to read `this.offchainTapscript` across five field accesses (`forfeit`, `forfeit`, `encode`, `pkScript`, `arkAddress`-via-getter) while a concurrent `vtxo_received` could trigger `rotate()` and swap the field between reads, stamping the change VTXO with mixed scripts. Now snapshots `offchainTapscript` and the display address synchronously at function entry, before any `await`. Option (A) from Arkana's recommendation — a local snapshot, no extra lock contention. * **Descriptor leak in error message (Arkana / CodeRabbit 🟡)** — `deriveLeafPubkey` interpolated the full descriptor into its error, which would surface in logs / crash reporters. A misconfigured caller could pass an xprv. Redacted to `length=N`. * **Cursor assertion non-strict (Arkana 🟡)** — the `includeInactive` cursor-advance test used `>=` which passed even on the no-op case. Tightened to `toBeGreaterThan`. * fix(wallet): bind baseline matrix to index 0, not the rotated pubkey `initializeContractManager` used to construct the multi-timelock matrix (default + delegate × every `walletContractTimelocks` entry) at `this.offchainTapscript.options.pubKey` — which is the CURRENT display pubkey, including any HD rotation. On a wallet that's rotated past index 0, every boot re-registered the full matrix at the rotated pubkey, diluting the design's intent and turning every rotation into a permanent multi-timelock expansion. The matrix is bound to INDEX 0 by design: it's the permanent fallback set the wallet wants active forever, independent of any rotation. Rotated display contracts are intentionally single-timelock-single-pubkey at the current arkd delay, registered by `WalletReceiveRotator.rotate` and tagged with `metadata.source = WALLET_RECEIVE_SOURCE`. This commit anchors the matrix to `await this.identity.xOnlyPublicKey()` — the identity's static pubkey, equal to index 0 for HD wallets — so the baseline stays at index 0 across all boots regardless of rotation state. Regression test: `does NOT re-register the multi-timelock matrix at a rotated pubkey on reboot`. Rotates once, then boots a second wallet on the same repos and asserts that the rotated pubkey appears in exactly ONE tagged contract (the display), never duplicated across timelocks. * fix(unroll): Math.ceil feeRate before BigInt conversion `prepareUnrollTransaction` called `BigInt(feeRate)` where `feeRate` is a `number` returned by `OnchainProvider.getFeeRate()`. Esplora's `/fee-estimates` returns plain numbers and bitcoind regtest sometimes reports fractional sat/vB (e.g. 1.006 with a few low-fee txs in the mempool), making `BigInt(1.006)` throw `RangeError: cannot be converted to a BigInt because it is not an integer`. Round up before the BigInt conversion: pay at least the advertised rate (fractional sat/vB is meaningless on the wire) and satisfy BigInt's integer requirement. Surfaced by `should complete unroll after unilateral exit delay` going red in CI on a fee rate of 1.006. Pre-existing bug from the `prepareUnrollTransaction` extraction in #479 — master had the same code, master CI just hasn't rolled the unlucky regtest fee yet. * fix(wallet): walletMode 'auto' behaves like 'static' for now HD rotation is conservatively OFF by default until it has more soak time. Today, `walletMode: 'auto'` (the unset default) is identical to `walletMode: 'static'` — no HD rotation, no descriptor provider built. Opt into HD via `walletMode: 'hd'` or by passing a `DescriptorProvider` instance. The `'auto'` name is reserved for a future change that will re-enable identity-probing once the rotator has more field experience. Test `default ('auto') currently behaves like 'static' for HD-capable identities` locks this in — a future revert to identity-probing auto-detect would have to flip this test deliberately. * docs(wallet): mark walletMode 'auto'→'static' as explicitly short-term The previous commit kept the temporary-default behaviour but the comments said "for now" — soft enough to ossify into permanent default behaviour without anyone noticing. Strengthens the markers: * Add a `TODO(hd-maturation)` block in `resolveDescriptorProvider` listing the three concrete flip-back criteria (one consumer running HD on mainnet ≥ 1 month with no incidents, flip the lock-in test, update the WalletMode docstring). * `WalletMode` doc cross-references the `TODO`. * Lock-in test name renamed to include the `TODO(hd-maturation): flip me back when re-enabling auto-probe` marker so it shows up in grep + test reporter output. Behaviour unchanged from the previous commit (`4ed231a7`). * fix(wallet): close rotation race + harden init/dispose Address PR #489 review findings: * Snapshot `offchainTapscript` synchronously at `_txLock` entry in `_sendImpl` / `sendBitcoin`, derive `outputAddress` from it, and thread it through `updateDbAfterOffchainTx` so the change output's pkScript and the change-VTXO metadata can't drift across the offchain round-trip when `rotate()` fires mid-flight. * `getVtxoManager` now caches `_vtxoManager` only AFTER `_receiveRotator.install` resolves — a failing install no longer silently disables HD rotation for the wallet instance. * `dispose` wraps the rotator teardown in try/catch and rethrows after manager + super disposal, so a rotator failure can't leak the contract watcher. * Fix stale `{@link Wallet.rebuildOffchainTapscript}` → `WalletReceiveRotator.rotate`. * Dispose per-test `ContractManager`s in `manager.test.ts` so the fake-timer suite stops leaking watchers across cases. * refactor(wallet): make offchainTapscript read-only with internal setter PR #489 review #4 (Arkana 🟡): `offchainTapscript` was promoted to `public` (non-readonly) on `ReadonlyWallet` to let `WalletReceiveRotator.rotate` swap it after an HD rotation, but the side-effect was that any external code holding a wallet reference could overwrite it. Now `offchainTapscript` is a `public` getter over a `protected` backing field. The only sanctioned write path is `Wallet.setOffchainTapscriptForRotation`, marked `@internal` and added to the `RotatableWallet` surface the rotator consumes. The field is read-only from outside; the rotator's call site is the only one in the tree. * fix(wallet): rotator backoff, pluggable logger, typed compat error PR #489 review #6 + #9 + #10. All three live in `walletReceiveRotator.ts`: * **Backoff (#6, P3):** consecutive `rotate()` failures now gate future attempts behind exponential backoff (1s → 2s → … → 60s cap), so a broken provider can't make every `vtxo_received` re-hammer `getNextSigningDescriptor` + `createContract`. Counter resets on a successful rotation. * **Pluggable logger (#9, nit):** new `Logger` interface and `ReceiveRotatorBootOpts.logger` plumb a structured logger through the boot path. Default stays `console`, so existing callers are unaffected. * **Typed compatibility error (#10, nit):** new `NonRangeableDescriptorError` replaces the `err.message.includes("wildcard descriptor")` string match in the silent-fallback path. `deriveLeafPubkey` raises it instead of a generic `Error`. Regression test: rapid double `vtxo_received` events under a failing `createContract` now produce exactly one `createContract` call. * test(contracts): cover watcher-managed inactive transition PR #489 review #8 (CodeRabbit nit). The existing `refreshVtxos includeInactive > default path …` test seeded inactive rows directly into the repository via `seedRaw`, so it proved that unmanaged rows are ignored — but couldn't catch a regression where `manager.setContractState(..., "inactive")` accidentally left the contract in the watcher's tracked set. Adds a parallel test that creates a second contract via `manager.createContract` (so it's actually watched), transitions it to `inactive` via `setContractState`, then calls the default `refreshVtxos()` and asserts the inactive script is absent from the indexer query while the active one is present. * test(wallet): cover install-failure + dispose rethrow defensive paths Both paths come from PR #489 review fixes that landed in `1bee984f` but had no direct regression coverage. * `getVtxoManager` (#2): failing `WalletReceiveRotator.install` must leave `_vtxoManager` + `_receiveRotatorInstalled` untouched, so a subsequent `getVtxoManager()` call retries instead of returning a cached half-init'd manager with rotation silently disabled. * `dispose` (#3): a rotator-disposal rejection must NOT short-circuit the rest of teardown; the manager still disposes and the captured error rethrows at the end. * test(wallet): lock in snapshot binding in updateDbAfterOffchainTx Contract test for the PR #489 review #1 fix that landed in `1bee984f`. The existing tests passed `offchainTapscript` as a parameter but used the same mock everywhere, so they didn't prove the function actually derives the change-VTXO metadata from the snapshot rather than from `this.offchainTapscript`. This test wires two distinct `DefaultVtxo.Script` instances: the "wrong" one (`tapscriptOld`) is placed on `this.offchainTapscript`, the "right" one (`tapscriptNew`) is threaded through as the snapshot parameter. Assertions verify `script`, `tapTree`, `forfeitTapLeafScript`, `intentTapLeafScript`, AND the `primaryAddress` used for `saveTransactions` / `saveVtxos` keys all match `tapscriptNew` — a regression that re-reads `this.offchainTapscript` inside the function would fail the test. * fix(wallet): dispose manager on rotator install failure * Deprecate Wallet methods that can be used via DescriptorProvider * fix(worker): forward walletMode to service worker wallet * fix(wallet): per-input signing after HD rotation (#491) * fix(wallet): route per-input signing through DescriptorProvider after HD rotation Rotated VTXOs were locked by index-N pubkeys but every signing entry point still went through identity.sign (index-0), so post-rotation sends and renewals shipped unsigned PSBTs. Adds a signInputsByOwner helper that resolves each input's script to its contract and dispatches identity vs descriptor signing per group; rotated contracts now persist metadata.signingDescriptor so the helper can look the right descriptor up. arkTx signing takes a per-input source-script override because checkpoint witnessUtxo scripts don't match the source VTXO's contract. * fix(worker): forward walletMode to service worker wallet * refactor(wallet): extract InputSignerRouter from signInputsByOwner Move the per-input signer dispatch out of Wallet into a focused InputSignerRouter and route every legacy signInputsByOwner call site through it. Callers now hand the router explicit InputSigningJob[] derived from the source VTXO script, removing the lookupScriptOverrides footgun and the silent skip on missing overrides. Adds a typed DescriptorSigningProviderMissingError to replace the bare Error thrown deep inside the descriptor loop. * refactor(wallet): address PR #489 review nits - Re-export DescriptorSigningProviderMissingError from top-level barrel - Dedupe VtxoScript.decode in intentProofJobs - Preserve cause chain when wrapping HDDescriptorProvider.create errors - Clarify NonRangeableDescriptorError message for unexpected expansion shape - Make double-rotation behavior explicit in WalletReceiveRotator --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: CodeRabbit <[email protected]> Co-authored-by: Pietro Grandi <[email protected]> Co-authored-by: pietro909 <[email protected]>
3 participants
Stacked on #440 (HDDescriptorProvider as a pure rotating allocator). Alternative to #441.
Summary
Wires HD receive rotation into
Walletby following the dotnet-sdk's split of responsibilities:the
HDDescriptorProvideris a pure rotating allocator, and the contract repository is thesource of truth for "what addresses am I currently bound to". The provider has no notion of
"current" — the boot path looks up active default contracts in the repo to recover the wallet's
state, and rotation creates a new default contract while leaving the old one active.
What changed
src/wallet/wallet.tsBoot path (
Wallet.create): ForSeedIdentitywallets with a vanilla BIP-86 descriptor:defaultcontracts whoseserverPubKeymatches the current server.pubKeyfor the offchain tapscript. Noprovider call. The wallet resumes from exactly where the previous session left off.
provider.getNextSigningDescriptor()once, allocating index 0. Thecontract registered moments later by
initializeContractManagerrecords this binding,so storage and the contract repo stay in lockstep.
SingleKeywallets, andSeedIdentitywallets whose descriptor isn't a vanillatr(...path/0/*)template, stay on the static path unchanged.Rotation (
installHdRotationHandler+rotateAndRegister):vtxo_receivedon the contract manager. Filter for events on the currently-activedefault contract.
offchainTapscript, callmanager.createContract({ type: 'default', ... }). The previous default contract entry staysactiveso earlier addresses keep crediting the wallet._hdRotationChainso rapid-fire events can't interleave theallocate → rebuild → create sequence.
Dispose:
manager. A late
vtxo_receivedevent cannot queue work on a disposing wallet.offchainTapscriptmutability:readonly→public, same as #441 needed. Externalconsumers should still treat it as read-only (writes happen only in
rebuildOffchainTapscriptand the boot path).
test/walletHdRotation.test.ts— 9 testslastIndexUsed = 0).defaultContractScript.lastIndexUsedand the active script changes.active.vtxo_receivedevents do nothing.walletRepository+contractRepositoryreads the rotatedaddress by querying the contract repo for active default contracts (no event replay).
dispose(), late events do not call into the provider.Compared to #441
getCurrentReceivePubkey()accessor on the provider — the provider has no notion of"current". The wallet boot queries the contract repository instead.
rotateReceive/getCurrentReceiveIndex— the provider only exposesgetNextSigningDescriptor.accessor, allocation starts at the natural first index.
Blast radius
SingleKeywallets.MnemonicIdentity/SeedIdentityHD wallets, rotation now happens after eachvtxo_receivedon the active receive script. Old default contracts stayactive(no fundsloss).
settings.hdkey, written only on first allocation throughthe provider.
offchainTapscriptmutability widened only in the base class.Stacked PR order
DRAFT until #440 lands. Rebase target moves to
masterafter #440 merges.Summary by CodeRabbit
New Features
Documentation
Tests
Bug Fix