overwrite btc-signer Transaction

[louisinger]

overwrite the default options of btc-signer Transaction.

thus, this:

import { Transaction } from "@scure/btc-signer" // from btc-signer

const arkTx = Transaction.fromPsbt(b64, { allowUnknown: true })

is equivalent to:

import { Transaction } from "@arkade-os/sdk" // from sdk

const arkTx = Transaction.fromPsbt(b64)

@bordalix @tiero please review

Summary by CodeRabbit

• Refactor

  • Transaction handling consolidated into a local utility with consistent defaults; imports standardized across the codebase.

• Behavior Changes

  • Stricter handling of unknown inputs/outputs for safer, more predictable transaction construction while preserving existing error behavior and user flows.

• Tests

  • End-to-end tests updated to match the new transaction utility and simplified creation/parsing APIs.

[coderabbitai]

Walkthrough

Adds a local Transaction wrapper (src/utils/transaction.ts), switches project imports to it, and removes per-call allowUnknown*, allowUnknownInputs/Outputs flags across transaction construction/parsing. Types that referenced the external Transaction now reference the local wrapper.

Changes

Cohort / File(s)	Summary
Local Transaction wrapper src/utils/transaction.ts	Adds Transaction class extending @scure/btc-signer Transaction with static ARK_TX_OPTS, withArkOpts helper, and fromPSBT/fromRaw/constructor that merge Ark defaults.
Import migration to local Transaction src/index.ts, src/identity/index.ts, src/identity/singleKey.ts, src/intent/index.ts, src/utils/arkTransaction.ts, src/wallet/onchain.ts, src/wallet/unroll.ts, src/forfeit.ts, test/e2e/vhtlc.test.ts	Replace imports from @scure/btc-signer or @scure/btc-signer/transaction.js with the local src/utils/transaction (and re-export from src/index.ts), updating call sites accordingly.
Remove per-call unknown flags* src/forfeit.ts, src/intent/index.ts, src/utils/arkTransaction.ts, src/wallet/onchain.ts, src/wallet/unroll.ts, test/e2e/vhtlc.test.ts	Drop allowUnknown, allowUnknownOutputs, allowUnknownInputs from Transaction constructors and fromPSBT calls; keep other options (e.g., version, allowLegacyWitnessUtxo) where present.
Public type binding updates src/intent/index.ts, src/utils/arkTransaction.ts	Types (Proof, ArkTxInput, OffchainTx) now reference the local Transaction type instead of the external @scure/btc-signer Transaction; no function signatures changed.
Behavioral surface unchanged except options src/forfeit.ts, src/wallet/onchain.ts, src/wallet/unroll.ts	Core logic and control flow unchanged; error behavior (e.g., missing witnessUtxo) preserved; only construction/parsing option handling altered.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Caller
  participant Tx as utils/transaction.Transaction
  participant Btc as @scure/btc-signer.Transaction

  Note over Tx,Btc: Creation/parsing now goes through local wrapper
  Caller->>Tx: new Transaction(opts?) / fromPSBT(psbt, opts?) / fromRaw(raw, opts?)
  activate Tx
  Tx->>Tx: Merge opts with ARK_TX_OPTS (default unknown* = true)
  Tx->>Btc: Delegate construct/parse with merged opts
  activate Btc
  Btc-->>Tx: Base Transaction instance
  deactivate Btc
  Tx-->>Caller: Wrapped Transaction instance
  deactivate Tx

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• Update to arkd v0.7.0 #121: Introduces/adjusts the same local Transaction wrapper and updates imports across modules; overlaps directly with this migration.
• intent proof of ownership rework #153: Modifies allowUnknown*/allowUnknownInputs flags and touches src/forfeit.ts, intersecting with this PR’s option changes.
• ServiceWorkerWallet.sign : allow unknown PSBT fields #127: Alters Transaction.fromPSBT/constructor behavior around allowing unknown PSBT fields/inputs, related to this PR’s removal of per-call flags.

Suggested reviewers

• bordalix
• tiero

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check	❓ Inconclusive	The title “overwrite btc-signer Transaction” is vague and does not clearly convey that the pull request is modifying the default Transaction options in the btc-signer wrapper, so reviewers may not immediately understand the main intent of the change.	Please update the title to explicitly reference overriding default Transaction options, for example “Override default transaction options in btc-signer Transaction wrapper,” so that the primary change is clear to anyone scanning the PR.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 314e48d and 083a49f.

📒 Files selected for processing (1)

• src/utils/transaction.ts (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/utils/transaction.ts

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: test

---

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.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8d3c8a0 and 314e48d.

📒 Files selected for processing (10)

• src/forfeit.ts (1 hunks)
• src/identity/index.ts (1 hunks)
• src/identity/singleKey.ts (1 hunks)
• src/index.ts (1 hunks)
• src/intent/index.ts (1 hunks)
• src/utils/arkTransaction.ts (2 hunks)
• src/utils/transaction.ts (1 hunks)
• src/wallet/onchain.ts (3 hunks)
• src/wallet/unroll.ts (4 hunks)
• test/e2e/vhtlc.test.ts (3 hunks)

🧰 Additional context used

🧬 Code graph analysis (3)

src/wallet/unroll.ts (1)

src/utils/transaction.ts (1)

• Transaction (9-27)

src/utils/transaction.ts (1)

src/index.ts (1)

• Transaction (203-203)

test/e2e/vhtlc.test.ts (2)

src/index.ts (1)

• Transaction (203-203)

src/utils/transaction.ts (1)

• Transaction (9-27)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: test

🔇 Additional comments (19)

src/identity/singleKey.ts (1)

6-6: LGTM! Import paths updated correctly.

The import changes align with the project-wide adoption of the local Transaction wrapper while maintaining all existing functionality.

Also applies to: 9-9

src/utils/transaction.ts (2)

29-31: User options can override Ark defaults.

The withArkOpts function applies user options after Ark options ({ ...Transaction.ARK_TX_OPTS, ...opts }), allowing users to override the Ark defaults. While this provides flexibility, it means users could pass { allowUnknown: false } and disable the Ark-specific behavior, potentially defeating the purpose of this wrapper.

Consider whether this is intentional. If Ark options should always be enforced, reverse the merge order to { ...opts, ...Transaction.ARK_TX_OPTS }.

---

20-22: Confirm fromPSBT’s polymorphic return type

Verify that @scure/btc-signer’s Transaction.fromPSBT is declared to return this (i.e. the subclass) rather than always the base type; if it doesn’t, you’ll need to cast or override to preserve subclass behavior.

src/identity/index.ts (1)

1-1: LGTM! Import path updated correctly.

The import change aligns with the project-wide adoption of the local Transaction wrapper.

src/index.ts (1)

1-1: LGTM! Import path updated correctly.

The import change aligns with the local Transaction wrapper implementation and maintains the same public API surface.

src/forfeit.ts (1)

1-1: LGTM! Correctly uses the new Transaction wrapper.

The removal of explicit allowUnknown* options is correct since they're now provided by default through the ARK_TX_OPTS in the local Transaction wrapper. The function logic remains unchanged.

Also applies to: 10-13

src/wallet/unroll.ts (3)

2-2: LGTM! Import paths updated correctly.

The import changes align with the project-wide adoption of the local Transaction wrapper.

Also applies to: 15-15

---

170-170: LGTM! Correctly uses Transaction.fromPSBT with default options.

The removal of explicit options is correct since allowUnknown: true is now provided by default through the wrapper's ARK_TX_OPTS.

---

295-295: LGTM! Correctly uses Transaction constructor with default options.

The removal of allowUnknownInputs: true is correct since it's now provided by default through the wrapper's ARK_TX_OPTS. The version: 2 option is properly preserved.

src/wallet/onchain.ts (2)

13-13: LGTM! Import path updated correctly.

The import change aligns with the project-wide adoption of the local Transaction wrapper.

---

162-165: LGTM! Correctly uses Transaction constructor with appropriate options.

The removal of allowUnknownInputs: true is correct since it's now provided by default through the wrapper's ARK_TX_OPTS. The allowLegacyWitnessUtxo: true option is properly preserved as it's not part of the Ark defaults and is specific to this use case.

test/e2e/vhtlc.test.ts (3)

17-17: LGTM! Import path updated correctly.

The import change aligns with the project-wide adoption of the local Transaction wrapper, now imported from the main SDK export.

---

141-141: LGTM! Correctly uses Transaction.fromPSBT with default options.

The removal of explicit { allowUnknown: true } is correct since this option is now provided by default through the wrapper's ARK_TX_OPTS.

---

234-234: LGTM! Correctly uses Transaction constructor with default options.

The removal of { allowUnknownInputs: true } is correct since this option is now provided by default through the wrapper's ARK_TX_OPTS.

src/intent/index.ts (3)

116-140: LGTM! Constructor simplified by relying on wrapper defaults.

The craftToSpendTx function now uses the local Transaction wrapper without explicit allowUnknown* options. This is consistent with the PR's goal to set these options as defaults in the wrapper.

---

143-199: LGTM! Constructor simplified by relying on wrapper defaults.

The craftToSignTx function now uses the local Transaction wrapper without explicit allowUnknown* options, consistent with the refactor goal.

---

1-5: Transaction wrapper correctly defaults allowUnknown options

ARK_TX_OPTS sets allowUnknown, allowUnknownInputs, and allowUnknownOutputs to true by default, so no further changes are needed.

src/utils/arkTransaction.ts (2)

1-22: LGTM! Import changed to use local Transaction wrapper.

The Transaction import has been moved from the external @scure/btc-signer to the local ./transaction wrapper. This change is consistent with the refactor in src/intent/index.ts and aligns with the PR objective to standardize Transaction options.

Note: Verification of the wrapper implementation is requested in the review comment for src/intent/index.ts.

---

69-120: LGTM! Constructor simplified in buildVirtualTx.

The buildVirtualTx function now creates transactions using the local wrapper without explicit allowUnknown* options. This change propagates through the checkpoint transaction creation flow (via buildCheckpointTx on line 139), ensuring consistent behavior across all Ark transactions.