fix(wallet): per-input signing after HD rotation

[pietro909]

Follow-up on top of #489.

Summary

• Route per-input signing through DescriptorProvider so rotated VTXOs (locked by index-N pubkeys) get signed by the right key instead of falling back to identity index-0.
• Persist metadata.signingDescriptor on rotated contracts and resolve each input's script to its contract at sign time. arkTx signing accepts a per-input source-script override because checkpoint witnessUtxo scripts don't match the source VTXO's contract.
• Forward walletMode to the service worker wallet so worker-mode wallets pick up the same HD signing path.

Test plan

• pnpm lint
• pnpm test:unit
• pnpm test:integration (HD rotation + send/renew)

Manual testing

1. mutinynet
2. brand new wallet with mnemonic phrase
3. first 2 transactions received through Arkade
4. third ($14.94) via Lightning
5. sent all the coins back (Arkade)
6. received from onchain (onboarding address)
7. sent a fraction via arkade

Contracts rotate, coins are spent with the correct scripts.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

• next-version

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b0a1b8f8-ed99-42c7-8db1-a1a2add9cd57

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/hd-wallet-via-contracts-sign

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[arkanaai]

Code Review — signInputsByOwner migration (PR #491)

Reviewer: Arkana (automated code review agent)
Verdict: Request changes — protocol-critical PR, requires human sign-off regardless

---

Summary

This PR replaces all identity.sign() / identity.signMultiple() call sites in Wallet with a new signInputsByOwner() private method that routes each PSBT input to either the identity's baseline key or a DescriptorProvider based on the owning contract's pubkey. This fixes the core HD-rotation-after-send bug (INVALID_PSBT_INPUT (5): missing tapscript spend sig). The approach is sound and well-documented.

✅ What looks correct

1. Signing migration is complete. Every identity.sign() call site for VTXO owner signing is migrated: settlement boarding, forfeits, all 3 intent proofs, checkpoint signing in finalizePendingTxs, and buildAndSubmitOffchainTx. The musig2 identity.signerSession() path correctly remains untouched (that's collaborative signing, not owner signing).

2. arkTx input positional mapping is safe. Verified the chain: inputs[] → buildOffchainTx → buildCheckpointTx.map() → buildVirtualTx sequential addInput(). No reordering anywhere. The arkTxOwnerScripts override map is correct.

3. Sequential threading preserves signatures. identity.sign() → returns new tx with identity sigs → passed to signWithDescriptor() → signTxWithKey clones (preserving prior sigs) → adds descriptor sigs. Accumulation is correct.

4. MissingSigningDescriptorError is the right call. Silently falling back to index-0 would reproduce the original bug with an opaque server error. Failing loudly with a typed error is correct.

5. Test coverage is thorough. ~550 new test lines covering: rotated intent proofs (all 3 types), mixed baseline+rotated, missing descriptor error, cosigner/connector skip, buildAndSubmitOffchainTx with rotation, baseline regression, provider opt-in.

6. signingDescriptor persistence in contract metadata (walletReceiveRotator.ts:537-544) — correct location, correct timing.

⚠️ Issues to address

P0 — Protocol correctness

1. Settlement forfeit signing — contract lookup may fail for boarding VTXOs
wallet.ts — signInputsByOwner called with [i] / [0] from the settlement handler. For boarding UTXOs, the code falls through to the boardingScriptHex comparison (line ~345 in signInputsByOwner). However, if a rotated wallet receives boarding UTXOs (which use the identity's base pubkey in the boarding tapscript), this still routes correctly through identity. Confirm: is it possible to have a boarding UTXO locked by a rotated pubkey? If yes, the boarding-script check only matches the baseline boarding script and would silently skip a rotated-boarding input. If no (boarding always uses baseline key), this is fine.

2. Batch signing regression is a real UX hit for hardware/external signers
The removal of isBatchSignable / signMultiple means external signers (Ledger, mobile wallet bridges) now get N+1 confirmation popups per send instead of 1. The comment references a future BatchSignableDescriptorProvider but there's no tracking issue. This should be:

• Documented in the PR description / CHANGELOG
• Tracked in a GitHub issue (not just a markdown file reference)
• Ideally: for the identity-sign group (baseline inputs), signMultiple could still be used when isBatchSignable is true, only falling back to sequential for descriptor groups

P1 — Type safety / robustness

3. Contract.metadata?.signingDescriptor has no compile-time safety
wallet.ts:~371 — metadata is Record<string, unknown>. A typo like signinDescriptor anywhere in the codebase would compile clean and silently produce undefined. Consider:

• Adding a ContractMetadata interface with optional signingDescriptor?: string and source?: string
• Or at minimum, extracting the key name to a shared constant

4. signWithDescriptor returns a clone — the sequential loop relies on this implicitly
wallet.ts:~405-413 — the contract between signInputsByOwner and signWithDescriptor is: "you clone my tx, sign the clone, return it." If any future DescriptorProvider implementation signs in-place and returns the same reference, signatures from earlier groups could be lost. The method comment should document this contract explicitly, or the threading should defensively clone before passing to signWithDescriptor.

P2 — Missing test coverage

5. No test for settlement/forfeit signing after rotation.
The settlement boarding path (handleSettlementFinalizationEvent) and forfeit signing path are both migrated to signInputsByOwner but have no rotation-specific tests. These are protocol-critical paths — if the contract lookup fails for a settlement input, the user can't settle and their funds are locked until CSV expiry. These are admittedly harder to test in unit tests, but at minimum a note/tracking issue for integration test coverage would be good.

🔍 Observations (non-blocking)

• isBatchSignable and combineTapscriptSigs are removed from wallet.ts imports but remain used elsewhere (staticDescriptorProvider.ts, boltz-swap). No cross-repo breakage.
• ServiceWorkerWalletMode type and walletMode passthrough are clean. No consumers yet outside the SDK itself.
• The ReceiveRotatorBootResult.provider field addition is backwards-compatible (additive).
• The lookupScriptOverrides parameter on signInputsByOwner is well-designed — clean separation between "what script owns this input" and "what witnessUtxo is on the PSBT".

🚨 Protocol-critical flag

This PR touches transaction signing, forfeit paths, and settlement flows. Even with the issues above resolved, this requires explicit human review and sign-off before merge per project policy.

---

🤖 Review by Arkana — automated code review agent

[Kukks]

This is super messy