consolidate PSBT exports to @caravan/psbt#466
consolidate PSBT exports to @caravan/psbt#466kunal-595 wants to merge 1 commit intocaravan-bitcoin:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 3fbc890 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR implements the first step of consolidating PSBT functionality by moving signature-related helpers from @caravan/bitcoin to @caravan/psbt. The refactoring is part of a larger effort to standardize on bitcoinjs-lib v6 and eliminate duplicate PSBT implementations.
Changes:
- Adds
addSignaturesToPSBT,parseSignaturesFromPSBT, andparseSignatureArrayFromPSBTfunctions to@caravan/psbtwith bitcoinjs-lib v6 support - Deprecates corresponding functions and constants in
@caravan/bitcoinwithout removing them - Updates the coordinator application to import PSBT signature functions from
@caravan/psbtinstead of@caravan/bitcoin
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/caravan-psbt/src/psbtv0/psbt.ts | Adds three PSBT signature helper functions and two private helper functions with v6 API compatibility |
| packages/caravan-psbt/src/psbtv0/psbt.test.ts | Adds minimal test coverage for the new functions (error cases only) |
| packages/caravan-bitcoin/src/psbt.ts | Adds @deprecated JSDoc tags to functions and constants being migrated |
| apps/coordinator/src/components/Wallet/WalletSign.jsx | Updates import to use @caravan/psbt |
| apps/coordinator/src/components/ScriptExplorer/Transaction.jsx | Updates import to use @caravan/psbt |
| apps/coordinator/src/components/ScriptExplorer/SignatureImporter.jsx | Updates import to use @caravan/psbt |
| apps/coordinator/src/components/Hermit/HermitSignatureImporterPsbt.jsx | Updates import to use @caravan/psbt |
| apps/coordinator/src/components/Hermit/HermitSignatureImporter.tsx | Updates import to use @caravan/psbt |
| .changeset/good-moons-cheat.md | Documents the changes with appropriate semver bumps (minor for psbt, patch for bitcoin) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| describe("parseSignaturesFromPSBT", () => { | ||
| it("should return null for invalid PSBT", () => { | ||
| expect(parseSignaturesFromPSBT("invalid")).toBeNull(); | ||
| }); | ||
|
|
||
| it("should return null for PSBT with no signatures", () => { | ||
| // Unsigned PSBT from test fixtures | ||
| const unsignedPsbt = TEST_FIXTURES.transactions[0].psbt; | ||
| if (unsignedPsbt) { | ||
| expect(parseSignaturesFromPSBT(unsignedPsbt)).toBeNull(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("parseSignatureArrayFromPSBT", () => { | ||
| it("should return null for invalid PSBT", () => { | ||
| expect(parseSignatureArrayFromPSBT("invalid")).toBeNull(); | ||
| }); | ||
|
|
||
| it("should return null for PSBT with no signatures", () => { | ||
| const unsignedPsbt = TEST_FIXTURES.transactions[0].psbt; | ||
| if (unsignedPsbt) { | ||
| expect(parseSignatureArrayFromPSBT(unsignedPsbt)).toBeNull(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("addSignaturesToPSBT", () => { | ||
| it("should return null for invalid PSBT", () => { | ||
| expect(addSignaturesToPSBT("testnet", "invalid", [], [])).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test coverage for the newly added functions is insufficient. While the existing tests in @caravan/bitcoin (packages/caravan-bitcoin/src/psbt.test.ts) include comprehensive tests for partially signed PSBTs, fully signed PSBTs with single and multiple inputs, the tests here only cover error cases (invalid PSBTs and PSBTs with no signatures). Consider adding tests that verify the happy path scenarios, such as:
- Successfully adding signatures to an unsigned PSBT
- Parsing signatures from partially signed PSBTs
- Parsing signatures from fully signed PSBTs with single and multiple inputs
- Verifying signature validation works correctly
These tests would ensure functional parity with the original implementation and provide confidence in the migration.
There was a problem hiding this comment.
@kunal-595 did you copy all the tests from packages/caravan-bitcoin/src/psbt.test.ts to packages/caravan-psbt/src/psbtv0/psbt.test.ts ?
There was a problem hiding this comment.
Not yet , i have only added error case coverage (invalid PSBTs, no signatures) .
the happy path tests from packages/caravan-bitcoin/src/psbt.test.ts including unsigned, partially signed, and fully signed PSBT fixtures (single and multi-input), along with signature parsing and addition have not been ported yet
i will move those next to reach parity with the original implemntation
Legend101Zz
left a comment
There was a problem hiding this comment.
Looks really good overall thanks for taking this up with, I think the approach you took would also not cause any breaks and maybe the comment I had earlier on the issue would have been wrong , I'll test it out once to also confirm eveyrthing works nicely :)
| }, | ||
| ]; | ||
| psbt.data.updateInput(inputIndex, { partialSig }); | ||
| const validator = (pk: any, msghash: any, sig: any): boolean => |
There was a problem hiding this comment.
maybe as we are moving things from @caravan/bitcoin to @caravan/psbt we can take this chance to also add some type defs if possible
There was a problem hiding this comment.
Great idea , i will add explicit types while migrating this code.
That includes removing any from the validator and defining clear parameter and return types for the signature helpers, i will include this in the same PR.
| * adds partial signature object(s) to each input and returns the PSBT with | ||
| * partial signature(s) included. | ||
| */ | ||
| export function addSignaturesToPSBT(network, psbt, pubkeys, signatures) { |
There was a problem hiding this comment.
same maybe some type defs
| describe("parseSignaturesFromPSBT", () => { | ||
| it("should return null for invalid PSBT", () => { | ||
| expect(parseSignaturesFromPSBT("invalid")).toBeNull(); | ||
| }); | ||
|
|
||
| it("should return null for PSBT with no signatures", () => { | ||
| // Unsigned PSBT from test fixtures | ||
| const unsignedPsbt = TEST_FIXTURES.transactions[0].psbt; | ||
| if (unsignedPsbt) { | ||
| expect(parseSignaturesFromPSBT(unsignedPsbt)).toBeNull(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("parseSignatureArrayFromPSBT", () => { | ||
| it("should return null for invalid PSBT", () => { | ||
| expect(parseSignatureArrayFromPSBT("invalid")).toBeNull(); | ||
| }); | ||
|
|
||
| it("should return null for PSBT with no signatures", () => { | ||
| const unsignedPsbt = TEST_FIXTURES.transactions[0].psbt; | ||
| if (unsignedPsbt) { | ||
| expect(parseSignatureArrayFromPSBT(unsignedPsbt)).toBeNull(); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| describe("addSignaturesToPSBT", () => { | ||
| it("should return null for invalid PSBT", () => { | ||
| expect(addSignaturesToPSBT("testnet", "invalid", [], [])).toBeNull(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
@kunal-595 did you copy all the tests from packages/caravan-bitcoin/src/psbt.test.ts to packages/caravan-psbt/src/psbtv0/psbt.test.ts ?
|
@bucko13 can you please approve the workflow , so we can test if this did not break any functionality ? |
Hi,@bucko13
What kind of change does this PR introduce?
Refactoring / internal maintenance
Issue Number:
Fixes #461
Snapshots/Videos:
N/A – internal refactoring, no UI changes
If relevant, did you update the documentation?
N/A – internal refactoring only
Summary
This PR implements Step 1 of #461 by consolidating PSBT ownership on
@caravan/psbt.Specifically, it:
@caravan/psbt@caravan/bitcoin@caravan/psbtThe change is intentionally scoped to avoid any modifications to cryptographic behavior, ECC initialization, or wallet signing logic. Existing PSBT construction and signing behavior is preserved, with regresion coverage added to ensure parity.
Does this PR introduce a breaking change?
No. This is an internal refactor with no user-visible or behavioral changes.
Checklist
npm run changeset).Other information
All tests pass locally (322 tests).
New test coverage added for:
addSignaturesToPSBTparseSignaturesFromPSBTparseSignatureArrayFromPSBTFollow-up work (ECC abstraction, bitcoinjs lib v6 migration, wallet updates) is intentionally deferred to separate PRs after this change lands.
Have you read the contributing guide?
Yes