feat(wallet): HD receive rotation via contract repository (reopen of #473)#489
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:
WalkthroughAdds HD wallet receive-address rotation and provider hooks, wires rotation into Wallet with tapscript snapshotting and descriptor-aware signing, introduces RefreshVtxosOptions.includeInactive for wider contract refreshes, fixes Esplora fee-rate rounding, and adds tests covering rotation, persistence, signing, and includeInactive behavior. ChangesHD Receive Rotation & Contract Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 — feat(wallet): HD receive rotation via contract repository
Thorough review of the full diff (1889 additions across 12 files). This is protocol-critical: it touches VTXO handling, change VTXO construction, contract lifecycle, and the display address derivation. Human sign-off required even if all feedback below is addressed.
🔴 P1 — Residual race between rotation and change VTXO metadata
src/wallet/wallet.ts (diff hunk at _applyServerResponse)
The PR correctly identifies the race where WalletReceiveRotator.rotate() mutates this.offchainTapscript without acquiring _txLock, and adds a snapshot at _applyServerResponse entry:
const offchainTapscript = this.offchainTapscript;
const primaryAddress = this.arkAddress.encode();However, this snapshot is at the wrong level. The change output's pkScript is computed earlier in the calling method:
sendBitcoin(line ~1372):this.arkAddress.pkScript→ used in the transaction output sent to the server_sendImpl(line ~2260/2437):outputAddress.pkScript(derived fromthis.getAddress()) → used in change output
These reads happen before the first await (correct), but _applyServerResponse is called after await buildAndSubmitOffchainTx(). If rotation completes during that network round-trip, the _applyServerResponse snapshot captures the rotated tapscript while the actual server-side change VTXO was built with the pre-rotation pkScript. Result:
forfeitTapLeafScript,intentTapLeafScript,tapTree, andscriptin the local VTXO record all point to the new rotated identity- The server's VTXO is locked to the old identity's pkScript
- Spending the change VTXO would construct incorrect forfeit transactions → server rejects
Not fund-loss (next refreshVtxos from the indexer reconciles), but causes temporary inability to spend change and incorrect balance.
Fix: Snapshot offchainTapscript at _txLock acquisition (top of sendBitcoin/_sendImpl) and thread it through to _applyServerResponse, or make rotate() acquire _txLock so it serializes with sends.
🟡 P2 — Stale {@link} in JSDoc
src/wallet/wallet.ts:209 (diff line for offchainTapscript declaration):
* read-only — writes happen exclusively in
* {@link Wallet.rebuildOffchainTapscript} and the boot path.Wallet.rebuildOffchainTapscript does not exist. The actual write site is WalletReceiveRotator.rotate() (line ~1044 of walletReceiveRotator.ts: wallet.offchainTapscript = newTapscript). Broken {@link} misleads anyone tracing the mutation site.
🟡 P2 — offchainTapscript visibility weakening
src/wallet/wallet.ts:210: Changed from readonly to public on ReadonlyWallet. This is a public API surface — external code (e.g. wallet/expo/wallet.ts reads .offchainTapscript.options.csvTimelock) can now accidentally overwrite it. Consider:
- A
protected set+public getpattern, or - A narrower
/** @internal */-annotated setter that the rotator calls, keeping the public propertyreadonly
🟢 P3 — No backoff on repeated rotation failures
src/wallet/walletReceiveRotator.ts:179-185: If rotate() throws (e.g. persistent repo or provider failure), every subsequent vtxo_received retries immediately:
this.chain = this.chain
.catch(() => undefined)
.then(() => this.rotate(wallet))
.catch((err) => {
console.error("WalletReceiveRotator: rotation failed", err);
});Under a broken provider, every incoming VTXO event hammers getNextSigningDescriptor + createContract. Consider a simple attempt counter with exponential backoff, or a circuit breaker that stops retrying after N consecutive failures.
✅ Things that look correct
-
unroll.tsfee-rate fix:BigInt(Math.ceil(feeRate))— correct direction (rounds up = overpays rather than underpays). Fixes realRangeErroron fractional esplora rates. ✓ -
Boot index stability:
getCurrentSigningDescriptor()peeks the current index before falling through togetNextSigningDescriptor(). No index drift across restarts without rotation. Test coverage confirms. ✓ -
Cursor invariant with
includeInactive: ThemustUpdateCursorguard correctly allows cursor advancement whenincludeInactivewidens the query (superset ⊇ watched set). Windowed queries still skip cursor update. Tests cover both paths. ✓ -
Deactivation ordering in
rotate(): Registers new contract → commitswallet.offchainTapscript→ deactivates previous tagged contract → updatescurrentTaggedScript. IfsetContractStatethrows, next rotation retries the same orphan. Baseline (index-0) is never deactivated. ✓ -
Dispose ordering: Rotator unsubscribes + drains before contract manager disposes. Prevents late events from racing disposal. ✓
-
deriveLeafPubkeyerror message: Redacts the descriptor (could contain xprv), logs only.length. Good security hygiene. ✓ -
isHDCapableIdentitystructural guard: Checks exactly the 4 members the HD flow uses. Appropriate for the construction-time probe. ✓ -
includeInactiveinteraction withscripts:contracts ? false : opts?.includeInactive— explicitscriptsalways wins. Documented and tested. ✓ -
Test coverage: 828-line integration test file (
walletHdRotation.test.ts) covers rotation, persistence, disposal,createContractfailure,walletModematrix, and index drift regression.manager.test.tsadds 188 lines forincludeInactivecursor behavior. Solid. ✓
Verdict: Request changes for the P1 race condition fix. The current snapshot approach in _applyServerResponse is necessary but insufficient — the snapshot must be taken at the same point where the change output's pkScript is captured (top of the _txLock lambda) and threaded through.
🚨 Protocol-critical: This PR modifies VTXO change construction, contract lifecycle, and the display address. Requires human review and sign-off regardless of code feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
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)
1239-1260:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep teardown progressing if rotator disposal fails.
A rejection from
_receiveRotator.dispose()exits beforemanager.dispose()andsuper.dispose(), which leaks the contract manager/watcher cleanup on the exact failure path where best-effort teardown matters most.Suggested 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(); + let rotatorError: unknown; + try { + // 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(); + } catch (error) { + rotatorError = error; + } const manager = this._vtxoManager ?? (this._vtxoManagerInitializing ? await this._vtxoManagerInitializing.catch(() => undefined) @@ } finally { this._vtxoManager = undefined; this._vtxoManagerInitializing = undefined; await super.dispose(); } + + if (rotatorError) { + throw rotatorError; + } }🤖 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 1239 - 1260, The call to await this._receiveRotator?.dispose() inside Wallet.dispose can throw and short-circuit the rest of teardown; wrap the rotator disposal in a try/catch (or Promise.catch) so failures are swallowed/handled and do not prevent the subsequent vtxo manager cleanup and super.dispose() from running. Specifically, in the dispose() method, protect the this._receiveRotator?.dispose() call with error handling, then proceed to resolve/await the existing logic that checks this._vtxoManager and this._vtxoManagerInitializing and calls manager.dispose(), finally calling await super.dispose() regardless of rotator errors.
🧹 Nitpick comments (3)
test/contracts/manager.test.ts (1)
359-372: ⚡ Quick winExercise the actual inactive-transition path here.
seedRaw()never registers the contract with the watcher, so this assertion only proves that unmanaged repository rows are ignored. IfsetContractState(..., "inactive")accidentally left a contract in the watched set, this test would still pass. Seed a second valid contract throughmanager.createContract(...)and transition it to"inactive"before asserting the default refresh skips it.Also applies to: 392-405
🤖 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/contracts/manager.test.ts` around lines 359 - 372, The test currently uses seedRaw (which calls repository.saveContract) and therefore never registers the contract with the watcher; change the test to exercise the real inactive-transition path by creating a second, normal contract via manager.createContract(...) (so it gets registered with the watcher), then transition that created contract to "inactive" using manager.setContractState(..., "inactive") (or the manager method that performs the state transition) before asserting that the default refresh skips inactive contracts; keep the existing seedRaw usage for the unmanaged-row scenario but add the manager.createContract + manager.setContractState sequence to validate the watcher-managed inactive path.src/wallet/walletReceiveRotator.ts (2)
337-337: 💤 Low valueConsider structured logging instead of console.error.
Line 337 uses
console.errorto surface rotation failures. While this ensures errors are visible, a structured logging approach (e.g., a logger instance passed via constructor or config) would provide:
- Consistent log format across the SDK
- Easier filtering/searching in production
- Potential integration with error tracking services (Sentry, etc.)
- Control over log levels
For now,
console.erroris acceptable, but consider introducing a logging abstraction if the SDK grows.🤖 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` at line 337, The code in WalletReceiveRotator uses console.error("WalletReceiveRotator: rotation failed", err) which is unstructured; replace this with a pluggable logger instance (e.g., this.logger.error) or accept a logger in the WalletReceiveRotator constructor/config and call that instead so logs follow SDK conventions and support levels/tracing; update the class constructor to store the logger, replace the console.error call in the rotation failure path with logger.error including the same message and the error object, and ensure any unit-tests/mocks are updated to provide a logger.
239-247: 💤 Low valueConsider more robust error detection for compatibility fallback.
The
isCompatibilityErrorhelper relies on string matching (err.message.includes("wildcard descriptor")), which is fragile if the error message changes inexpand()or related libraries. For a compatibility shim, this is acceptable, but a more robust approach would be to:
- Define a custom error class for non-rangeable descriptors in the descriptor expansion logic
- Check
err instanceof NonRangeableDescriptorErrorinstead of string matchingThis would make the fallback logic more maintainable and less prone to breaking if error messages evolve.
🤖 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 239 - 247, The current compatibility fallback in walletReceiveRotator.ts uses fragile string matching in isCompatibilityError (err.message.includes("wildcard descriptor")); change this to use a dedicated error class: define a NonRangeableDescriptorError (thrown from the descriptor expansion logic / expand() path when a descriptor is non-rangeable) and update isCompatibilityError to return err instanceof NonRangeableDescriptorError; keep the existing allowSilentFallback check and return undefined on that instance, otherwise rethrow the error.
🤖 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 1218-1229: Currently the code assigns this._vtxoManager and sets
this._receiveRotatorInstalled before awaiting
this._receiveRotator.install(this), so a failing install leaves the manager
cached and prevents future retries; change the flow in getVtxoManager(): await
this._vtxoManagerInitializing to obtain manager but do NOT assign
this._vtxoManager or set this._receiveRotatorInstalled until after the install()
call completes successfully — call await this._receiveRotator.install(this)
first (when this._receiveRotator exists and this._receiveRotatorInstalled is
false), only then set this._vtxoManager = manager and
this._receiveRotatorInstalled = true, and let install rejections propagate so
callers can retry later.
In `@test/contracts/manager.test.ts`:
- Around line 436-467: The test creates background watchers via
ContractManager.create which leak mockIndexer.getVtxos calls; ensure each
per-test manager is disposed by wrapping the test code that calls
ContractManager.create(...) and uses mgr in a try/finally and calling
mgr.dispose() in the finally block (apply the same fix for the other occurrence
around lines 489-518), so the watcher started by ContractManager.create is
stopped and mockIndexer calls do not leak into subsequent tests.
---
Outside diff comments:
In `@src/wallet/wallet.ts`:
- Around line 1239-1260: The call to await this._receiveRotator?.dispose()
inside Wallet.dispose can throw and short-circuit the rest of teardown; wrap the
rotator disposal in a try/catch (or Promise.catch) so failures are
swallowed/handled and do not prevent the subsequent vtxo manager cleanup and
super.dispose() from running. Specifically, in the dispose() method, protect the
this._receiveRotator?.dispose() call with error handling, then proceed to
resolve/await the existing logic that checks this._vtxoManager and
this._vtxoManagerInitializing and calls manager.dispose(), finally calling await
super.dispose() regardless of rotator errors.
---
Nitpick comments:
In `@src/wallet/walletReceiveRotator.ts`:
- Line 337: The code in WalletReceiveRotator uses
console.error("WalletReceiveRotator: rotation failed", err) which is
unstructured; replace this with a pluggable logger instance (e.g.,
this.logger.error) or accept a logger in the WalletReceiveRotator
constructor/config and call that instead so logs follow SDK conventions and
support levels/tracing; update the class constructor to store the logger,
replace the console.error call in the rotation failure path with logger.error
including the same message and the error object, and ensure any unit-tests/mocks
are updated to provide a logger.
- Around line 239-247: The current compatibility fallback in
walletReceiveRotator.ts uses fragile string matching in isCompatibilityError
(err.message.includes("wildcard descriptor")); change this to use a dedicated
error class: define a NonRangeableDescriptorError (thrown from the descriptor
expansion logic / expand() path when a descriptor is non-rangeable) and update
isCompatibilityError to return err instanceof NonRangeableDescriptorError; keep
the existing allowSilentFallback check and return undefined on that instance,
otherwise rethrow the error.
In `@test/contracts/manager.test.ts`:
- Around line 359-372: The test currently uses seedRaw (which calls
repository.saveContract) and therefore never registers the contract with the
watcher; change the test to exercise the real inactive-transition path by
creating a second, normal contract via manager.createContract(...) (so it gets
registered with the watcher), then transition that created contract to
"inactive" using manager.setContractState(..., "inactive") (or the manager
method that performs the state transition) before asserting that the default
refresh skips inactive contracts; keep the existing seedRaw usage for the
unmanaged-row scenario but add the manager.createContract +
manager.setContractState sequence to validate the watcher-managed inactive path.
🪄 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: 34b9191c-6105-4c26-b7b6-730d8b8e9e9e
📒 Files selected for processing (12)
src/contracts/contractManager.tssrc/identity/descriptorProvider.tssrc/identity/hdCapableIdentity.tssrc/identity/index.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/index.tssrc/wallet/unroll.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/contracts/manager.test.tstest/walletHdRotation.test.ts
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.
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.
pietro909
force-pushed
the
feat/hd-wallet-via-contracts
branch
from
46becd1 to
1e0dc37
Compare
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.
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.
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.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/walletHdRotation.test.ts (1)
893-919: 💤 Low valuePrefer
as unknown as DescriptorProvideroveras neverfor the supplied provider.
walletMode: provider as never(also at line 938) is a TypeScript anti-pattern —neveris meant for unreachable values, not for bypassing structural-type checks. IfDescriptorProviderever adds a method, this cast silently keeps the partial fake compiling while drifting from the real interface. Casting throughunknownor making the fake implement the interface keeps the test honest if the surface evolves.♻️ Suggested change
+import type { DescriptorProvider } from "../src/identity/descriptorProvider"; @@ - walletMode: provider as never, + walletMode: provider as unknown as DescriptorProvider,Apply the same change at line 938.
🤖 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 893 - 919, Replace the unsafe TypeScript cast "as never" used when passing the fake provider into Wallet.create with a proper "as unknown as DescriptorProvider" to avoid abusing the never type; specifically update the `walletMode: provider as never` usage (the fake provider created via `fakeProvider([PUBKEY_A, PUBKEY_B])`) to `walletMode: provider as unknown as DescriptorProvider` and make the same replacement for the other occurrence so the test's `provider`, `DescriptorProvider` and `walletMode` typing remains honest if the interface later changes.
🤖 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.
Nitpick comments:
In `@test/walletHdRotation.test.ts`:
- Around line 893-919: Replace the unsafe TypeScript cast "as never" used when
passing the fake provider into Wallet.create with a proper "as unknown as
DescriptorProvider" to avoid abusing the never type; specifically update the
`walletMode: provider as never` usage (the fake provider created via
`fakeProvider([PUBKEY_A, PUBKEY_B])`) to `walletMode: provider as unknown as
DescriptorProvider` and make the same replacement for the other occurrence so
the test's `provider`, `DescriptorProvider` and `walletMode` typing remains
honest if the interface later changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0487889a-8b49-42e4-a3be-4336c8e40d15
📒 Files selected for processing (13)
src/contracts/contractManager.tssrc/identity/descriptorProvider.tssrc/identity/hdCapableIdentity.tssrc/identity/index.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/index.tssrc/wallet/unroll.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/contracts/manager.test.tstest/wallet.test.tstest/walletHdRotation.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/identity/index.ts
- src/identity/descriptorProvider.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/identity/hdCapableIdentity.ts
- src/index.ts
- src/wallet/unroll.ts
- test/contracts/manager.test.ts
- src/wallet/index.ts
- src/wallet/hdDescriptorProvider.ts
- src/contracts/contractManager.ts
- src/wallet/walletReceiveRotator.ts
- src/wallet/wallet.ts
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 <noreply@coderabbit.ai>
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`.
`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.
`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.
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.
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`).
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.
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.
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.
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.
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.
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.
pietro909
force-pushed
the
feat/hd-wallet-via-contracts
branch
from
1e0dc37 to
8139970
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
1340-1368:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRestore the active rotated receive contract for readonly boot too.
This new boot path rehydrates a full
Walletfrom the repository’s active wallet-tagged contract, butReadonlyWallet.create()still always usessetup.offchainTapscript. A fresh readonly instance will therefore fall back to the permanent index-0 address after rotation, makinggetAddress()anddefaultContractScriptinconsistent with the repository source of truth.🤖 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 1340 - 1368, ReadonlyWallet.create currently ignores the resolved boot and always uses setup.offchainTapscript, which breaks rotated receive contracts; update ReadonlyWallet.create to call WalletReceiveRotator.resolveBoot(config, setup) (or accept the resolved boot) and pass boot?.offchainTapscript ?? setup.offchainTapscript for the offchain tapscript and boot?.rotator as the rotator argument (mirroring the Wallet constructor usage) so the readonly instance restores the active rotated receive contract from the repository.
🤖 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 1242-1260: When calling await this._receiveRotator.install(this)
after obtaining the constructed VtxoManager (local variable manager from
this._vtxoManagerInitializing), wrap the install call in a try/catch; on
rejection ensure you dispose the freshly created manager (e.g., await
manager.dispose() or manager.dispose() if synchronous) before rethrowing, and do
not set this._vtxoManager or flip this._receiveRotatorInstalled on failure. Keep
the assignment this._vtxoManager = manager and this._receiveRotatorInstalled =
true only after install succeeds.
---
Outside diff comments:
In `@src/wallet/wallet.ts`:
- Around line 1340-1368: ReadonlyWallet.create currently ignores the resolved
boot and always uses setup.offchainTapscript, which breaks rotated receive
contracts; update ReadonlyWallet.create to call
WalletReceiveRotator.resolveBoot(config, setup) (or accept the resolved boot)
and pass boot?.offchainTapscript ?? setup.offchainTapscript for the offchain
tapscript and boot?.rotator as the rotator argument (mirroring the Wallet
constructor usage) so the readonly instance restores the active rotated receive
contract from the repository.
🪄 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: 8a65676d-794c-48fd-8b0c-7c7199ef117b
📒 Files selected for processing (13)
src/contracts/contractManager.tssrc/identity/descriptorProvider.tssrc/identity/hdCapableIdentity.tssrc/identity/index.tssrc/index.tssrc/wallet/hdDescriptorProvider.tssrc/wallet/index.tssrc/wallet/unroll.tssrc/wallet/wallet.tssrc/wallet/walletReceiveRotator.tstest/contracts/manager.test.tstest/wallet.test.tstest/walletHdRotation.test.ts
✅ Files skipped from review due to trivial changes (2)
- src/identity/index.ts
- src/identity/descriptorProvider.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- src/wallet/index.ts
- src/identity/hdCapableIdentity.ts
- src/index.ts
- src/wallet/unroll.ts
- src/contracts/contractManager.ts
- test/contracts/manager.test.ts
- test/wallet.test.ts
- src/wallet/hdDescriptorProvider.ts
- src/wallet/walletReceiveRotator.ts
- test/walletHdRotation.test.ts
There was a problem hiding this comment.
Arkana Re-Review — Post-fix commit sweep
Reviewed all 6 fix commits pushed since my 2026-05-13 review (9eb8b351 through cc73774e).
✅ P1 Race Condition — FIXED
The tapscript snapshot is now taken at the correct level:
_sendImpl(wallet.ts:2380):const offchainTapscript = this.offchainTapscript;is the first line inside the_withTxLockcallback, before anyawait. Threaded as an explicit parameter toupdateDbAfterOffchainTx(wallet.ts:2607).sendBitcoin(wallet.ts:1448): Same pattern — snapshot at top of_withTxLock, threaded through at line 1510.updateDbAfterOffchainTx(wallet.ts:2708-2718): TakesoffchainTapscriptas a parameter. All change-VTXO metadata (forfeitTapLeafScript,intentTapLeafScript,tapTree,script) derived from the parameter, not fromthis.offchainTapscript.WalletReceiveRotator.rotate(walletReceiveRotator.ts:505-577): Builds new tapscript locally → registers contract → only then commits viawallet.setOffchainTapscriptForRotation(newTapscript)at line 565. Concurrent sends are safe because they already captured their snapshot.
Regression test at test/wallet.test.ts:1959 explicitly verifies the snapshot binding. Solid.
✅ P2 Stale {@link} — FIXED
No references to Wallet.rebuildOffchainTapscript remain. Links now point to Wallet.setOffchainTapscriptForRotation and WalletReceiveRotator.rotate.
✅ P2 offchainTapscript Visibility — FIXED
_offchainTapscriptis aprotectedbacking field (wallet.ts:206).- Read-only getter
get offchainTapscript()is public (wallet.ts:257-259). - Sole write path is
setOffchainTapscriptForRotation()(wallet.ts:1101-1104) —publicfor theRotatableWalletinterface but marked@internal. TypeScript enforces no direct assignment via the getter-only property.
✅ P3 No Backoff — FIXED
runRotateWithBackoff() (walletReceiveRotator.ts:424-456) implements exponential backoff:
min(2^consecutiveFailures * 1000ms, 60_000ms), exponent capped at 16.- Events during backoff window are skipped with log.
- Counter resets on success.
✅ cc73774e — Manager dispose on install failure
wallet.ts:1240-1247: If _receiveRotator.install() throws, the manager is disposed and the error re-thrown, leaving both _vtxoManager and _receiveRotatorInstalled unset. Allows clean retry on next getVtxoManager() call. Correct.
🟢 P3 (new, non-blocking) — _settleImpl address snapshot inconsistency
wallet.ts:1633 and wallet.ts:1695 both call await this.getAddress() without snapshotting. If rotation fires between them and the inputs carry assets, findDestinationOutputIndex returns -1 → throws "Cannot assign assets: no output matches the destination address" (line 1703).
Not fund-loss (throws before signing), but for consistency with the _sendImpl fix, consider snapshotting the address once at the top of _settleImpl.
Test coverage
Strong. walletHdRotation.test.ts (949 lines) covers installation, rotation, persistence, disposal, backoff, and walletMode matrix. wallet.test.ts adds a direct snapshot-binding regression test. manager.test.ts adds 7 includeInactive cursor-invariant tests.
Verdict: All P1/P2/P3 findings properly addressed. Approving with the minor _settleImpl note above (can be a follow-up).
🚨 Protocol-critical reminder: This PR modifies VTXO change construction, contract lifecycle, and display address derivation. Human sign-off still required before merge.
There was a problem hiding this comment.
Arkana Re-Review — 2820d97a (deprecation annotations)
Single new commit since my last review (cc73774e):
2820d97a — Deprecate Wallet methods that can be used via DescriptorProvider
Doc-only change. Adds @deprecated JSDoc tags to:
ReadonlyHDCapableIdentity.isOurs()→ useDescriptorProvider.isOurs()HDCapableIdentity.signWithDescriptor()→ useDescriptorProvider.signWithDescriptor()HDCapableIdentity.signMessageWithDescriptor()→ useDescriptorProvider.signMessageWithDescriptor()- Corresponding implementations in
SeedIdentityandReadonlyDescriptorIdentity
✅ Verified: HDDescriptorProvider, StaticDescriptorProvider, and the DescriptorProvider interface all exist and expose the referenced replacement methods. Deprecation guidance is accurate.
No logic changes. No runtime impact. LGTM.
Previous _settleImpl address-snapshot note (P3, non-blocking) from my last review still applies — can land as a follow-up.
🚨 Protocol-critical reminder remains: This PR modifies VTXO change construction, contract lifecycle, and display address derivation. Human sign-off required before merge.
* 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
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.
|
@coderabbitai please review carefully the refactor performed in bd48a46 |
There was a problem hiding this comment.
Code Review — PR #489: HD receive rotation via contract repository
Overall assessment: this is a well-designed and carefully implemented feature. The protocol-critical paths have been thought through and most of the hard edge cases are handled. I have a few concerns that need addressing before merge, one of which touches a protocol-critical code path.
PROTOCOL-CRITICAL — Forfeit and Settlement signing: verify lookupScript mapping
The PR migrates forfeit-tx signing from this.identity.sign(forfeitTx, [0]) to this._signerRouter.sign(forfeitTx, [{ index: 0, lookupScript: VtxoScript.decode(input.tapTree).pkScript }]). This is correct in intent, but warrants extra scrutiny:
Forfeit path: The connector occupies input index 0 and the VTXO is index 1 in a standard forfeit transaction. The comment in the old code says "do not sign the connector input" and signs [0]. Please confirm whether input 0 is the VTXO or the connector in the forfeit tx structure — if the VTXO is at index 1 and the connector at index 0, the signing job's index: 0 may be incorrect. The InputSignerRouter silently skips inputs it cannot classify, so a wrong index would produce an unsigned PSBT that the server rejects, which is recoverable — but this needs to be verified with certainty.
Settlement boarding path: Uses this.signInputsByOwner(settlementPsbt, [i]) (the existing inline routing helper). The new InputSignerRouter is a separate class and is NOT used here — the PR uses signInputsByOwner for settlement boarding. This is fine since signInputsByOwner has equivalent logic, but the dual-router situation means two copies of the routing logic exist. Not a blocker but worth flagging.
Potential Race: rotate() listens to wallet.defaultContractScript which derives from offchainTapscript
In walletReceiveRotator.ts, install() checks event.contractScript !== wallet.defaultContractScript to decide whether to rotate. defaultContractScript derives from the live offchainTapscript field (which setOffchainTapscriptForRotation mutates). This means:
- Event arrives for script A (currently displayed)
rotate()begins, gets descriptor, builds new tapscript B- Before
createContractreturns, a second event for A fires - The second event's guard checks
event.contractScript !== wallet.defaultContractScript— A is still the displayed script — so it enqueues another rotation - First rotation commits, setting
offchainTapscript = BandcurrentTaggedScript = B - Second rotation also runs, allocating index 2
The serialized chain prevents interleaving, so step 4's check happens synchronously and sees A before rotation 1 commits. So actually both events pass the guard and get enqueued. After rotation 1 completes, rotation 2 will call rotate() again — but now wallet.defaultContractScript is B, and the guard would have already passed (it was evaluated when the event fired, not inside chain). The chain serializes execution, not enqueueing.
This means two rapid arrivals on the same script can burn two consecutive HD indices. The first PR test ("skips subsequent events within the backoff window") only covers the failure case. The success case of two rapid events could still double-rotate.
This is the expected behavior for a BIP-44 receive rotation scheme (each receive triggers a new address), so it may be intentional. But the comment "Serialise rotations: two rapid vtxo_received events on the same contract must not interleave" implies they shouldn't both trigger a rotation. Please clarify and add a test for the double-event-success case.
InputSignerRouter exists in diff but not used in the merged wallet.ts
The diff introduces src/wallet/inputSignerRouter.ts as a new standalone module and imports it into wallet.ts, along with a _signerRouter field. However, looking at the actual wallet.ts in the repo post-#488 base, the wallet uses signInputsByOwner (an inline private method) rather than delegating to InputSignerRouter. The diff shows replacing several this.identity.sign() calls with this._signerRouter.sign().
This is fine as an approach — the InputSignerRouter and signInputsByOwner appear to have equivalent routing logic. However, there are now two implementations of the same routing logic in the codebase: InputSignerRouter and signInputsByOwner. The settlement boarding path still uses signInputsByOwner (not _signerRouter).
Suggestion: consolidate onto one path — either remove InputSignerRouter and have signInputsByOwner call through, or migrate signInputsByOwner to delegate to InputSignerRouter. Having two parallel implementations of signing routing is a maintenance hazard.
Non-rangeable descriptor handling in deriveLeafPubkey
deriveLeafPubkey throws NonRangeableDescriptorError when expansion.expansionMap?.["@0"]?.pubkey is missing. However, expand() may return an empty expansionMap for valid non-HD tr(xpub) descriptors where the pubkey is in a different slot. This is probably fine for the wallet's use cases, but the error message ("ensure the descriptor is materialized (no wildcard)") could mislead operators when the descriptor is valid but uses an unexpected structure. Low severity.
pickActiveReceive sorts by createdAt timestamp
pickActiveReceive sorts candidates by b.createdAt - a.createdAt to pick the newest. This relies on monotonic createdAt timestamps from the contract repository. InMemoryContractRepository uses Date.now() which is fine, but custom repository implementations might populate createdAt differently. If two rotations happen within the same millisecond (possible in tests), the "newest" selection is non-deterministic. Consider using a sequence number or per-session rotation counter as a tiebreaker.
resolveDescriptorProvider wraps HDDescriptorProvider.create errors
The walletMode: 'hd' path wraps the HDDescriptorProvider.create error in a new Error with a generic prefix message, losing the original stack trace (it's set as message text but not as cause). Use { cause: e } to preserve the chain:
throw new Error("walletMode 'hd' failed to initialize: ...", { cause: e });Math.ceil(feeRate) fix in unroll.ts
The BigInt(Math.ceil(feeRate)) fix is correct and addresses a real regtest flake. Clean.
includeInactive cursor advancement logic
The cursor advancement logic is correct: includeInactive with no scripts and no window is treated as a superset of the watched set, so the cursor advances. The comment and test coverage are thorough. The mustUpdateCursor condition options.contracts === undefined && options.window === undefined && (window.after ?? 0) <= cursor is unchanged and correctly covers the includeInactive case without modification.
Test coverage
The test suite is excellent:
walletHdRotation.test.ts(1496 lines) covers installation, rotation, persistence, backoff, and the race-condition snapshot fixinputSignerRouter.test.tscovers all routing branches including uppercase pubkey normalization, duplicate contracts, and mixed identity/descriptor signingmanager.test.tsadditions cover all fourincludeInactivecases including cursor advancement
The snapshot-binding regression test (binds the change VTXO metadata to the snapshot, not to this.offchainTapscript) directly validates the core race fix. Well done.
Summary
| Finding | Severity | Blocking |
|---|---|---|
| Forfeit tx: confirm input index 0 is VTXO not connector | Protocol-critical | Yes — needs human verification |
| Two rapid vtxo_received events can double-rotate | Logic question | Clarification needed |
| Dual routing implementations (InputSignerRouter + signInputsByOwner) | Maintainability | No, but should be tracked |
resolveDescriptorProvider loses error cause chain |
Low | No |
pickActiveReceive non-deterministic sort on same-ms createdAt |
Low | No |
The first item must have human eyes on the forfeit transaction structure before merge. Everything else is either clarification or polish.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch the details of commit 🏁 Script executed: Length of output: 51617
SummaryThe commit extracts Positive observations
Issues found1.
|
There was a problem hiding this comment.
Re-reviewing against current tip (bd48a46) after author corrections on my previous review. Three items corrected:
1. ✅ Dual routing — concern was stale
My previous flag about signInputsByOwner co-existing with InputSignerRouter was written against pre-bd48a46 state. Commit bd48a46 (the HEAD) extracted InputSignerRouter and removed signInputsByOwner entirely. wallet.ts now routes every signing site — settlement, boarding, forfeit, intent proofs, virtual tx, checkpoints — through _signerRouter.sign(...). No parallel implementation exists. Concern withdrawn.
2. ✅ Forfeit index: 0 — confirmed correct
buildForfeitTx is called with [vtxoInput, connectorInput] (wallet.ts ~1967–1988), so input 0 = VTXO, input 1 = connector. The signer job uses index: 0 + lookupScript: VtxoScript.decode(input.tapTree).pkScript — maps cleanly to the VTXO's contract in the repository. The connector is intentionally skipped (no job submitted for it). No bug.
3. ✅ pickActiveReceive tiebreaker — non-determinism accepted
.sort((a, b) => b.createdAt - a.createdAt) in walletReceiveRotator.ts:693 is a ms-precision sort; equal timestamps are non-deterministic. Author confirms this is acceptable. The metadata.source = 'wallet-receive' tag + rotation serialisation make it practically impossible to have two simultaneously-tagged contracts with the same ms timestamp in production.
InputSignerRouter — fresh eyes on bd48a46
The new router is clean:
- Baseline-owned contracts (
params.pubKey === identity.xOnlyPublicKey) → identity sign. Correct. - Non-baseline
default/delegatecontracts → descriptor sign viasigningDescriptorfrom contract metadata. Hard fail (MissingSigningDescriptorError) if the descriptor is absent — no silent partial signing. ✅ - Boarding script → identity sign. ✅
- No matching contract, not boarding → silently skipped (cosigner/connector inputs). Matches historical behaviour, intentional. ✅
DescriptorSigningProviderMissingErrorthrown if descriptor groups exist but no provider is wired. ✅- Descriptor groups sorted alphabetically before signing for determinism. ✅
pubKeylowercased before comparison to guard against uppercase persistence in custom repo adapters — good defensive touch. ✅
Remaining protocol-critical flag (human sign-off still required):
This PR touches the full VTXO signing path, forfeit construction, HD rotation serialisation, and dispose ordering. The code is correct to the best of my review, but per Ark Labs protocol: protocol-critical PRs require explicit human sign-off before merge. Flagging for kukks / team.
APPROVED — all prior blocking items resolved. 🟢
⚠️ Protocol-critical: please confirm human sign-off before merging.
- 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
There was a problem hiding this comment.
Arkana Re-Review — b07b66b0 (review nits)
Reviewing the single new commit since my last approval (bd48a46 → b07b66b0).
Five changes, all cosmetic/quality-of-life — no behavioral or protocol-critical delta:
| # | File | Change | Verdict |
|---|---|---|---|
| 1 | src/index.ts, src/wallet/wallet.ts |
Re-export DescriptorSigningProviderMissingError from barrel |
✅ |
| 2 | src/wallet/wallet.ts:128-133 |
Dedupe VtxoScript.decode in intentProofJobs — build coinJobs first, reuse [0].lookupScript for index: 0 |
✅ |
| 3 | src/wallet/walletReceiveRotator.ts:633-638 |
NonRangeableDescriptorError message now explains missing @0 key; still doesn't leak descriptor value |
✅ |
| 4 | src/wallet/walletReceiveRotator.ts:766-770 |
{ cause: e } on wrapping Error in resolveDescriptorProvider — standard error chain preservation |
✅ |
| 5 | src/wallet/walletReceiveRotator.ts:404-414 |
Comment clarification: two rapid events intentionally burn two HD indices (BIP-44 style), chain prevents interleaving not deduplication | ✅ |
No new protocol-critical surface. Prior approval stands.
⚠️ Protocol-critical flag from prior review still applies: human sign-off required before merge.
There was a problem hiding this comment.
Arkana Re-Review — b07b66b0 (review nits)
Reviewing the single new commit since my last approval (bd48a46 → b07b66b0).
Five changes, all cosmetic/quality-of-life — no behavioral or protocol-critical delta:
| # | File | Change | Verdict |
|---|---|---|---|
| 1 | src/index.ts, src/wallet/wallet.ts |
Re-export DescriptorSigningProviderMissingError from barrel |
✅ |
| 2 | src/wallet/wallet.ts:128-133 |
Dedupe VtxoScript.decode in intentProofJobs — build coinJobs first, reuse [0].lookupScript for index: 0 |
✅ |
| 3 | src/wallet/walletReceiveRotator.ts:633-638 |
NonRangeableDescriptorError message now explains missing @0 key; still doesn't leak descriptor value |
✅ |
| 4 | src/wallet/walletReceiveRotator.ts:766-770 |
{ cause: e } on wrapping Error in resolveDescriptorProvider — standard error chain preservation |
✅ |
| 5 | src/wallet/walletReceiveRotator.ts:404-414 |
Comment clarification: two rapid events intentionally burn two HD indices (BIP-44 style), chain prevents interleaving not deduplication | ✅ |
No new protocol-critical surface. Prior approval stands.
⚠️ Protocol-critical flag from prior review still applies: human sign-off required before merge.
2 participants
Reopens #473 — the original PR was squash-merged prematurely (`4eec1120`) and is being reverted via #488.
Branch ref is unchanged from #473's last tip (`afb6def2`); same review history applies. Diff will appear empty against master until #488 lands, then will surface the full change again.
See #473 for the full design + review thread.
Quick recap of what's in this PR
WalletReceiveRotatorextraction from `wallet.ts` (~225 lines moved)All findings from prior Arkana / CodeRabbit reviews addressed in the original PR.
Order of operations
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests