upgrade to caravan from (deprecated) unchained-bitcoin, add taproot support#61
upgrade to caravan from (deprecated) unchained-bitcoin, add taproot support#61
Conversation
There was a problem hiding this comment.
PR Overview
This PR upgrades the xpub-lib package from the deprecated unchained-bitcoin to caravan-bitcoin and adds Taproot tests. Key changes include updating all import references and constants, revising test cases to support Taproot addresses, and updating documentation accordingly.
Reviewed Changes
| File | Description |
|---|---|
| packages/xpub-lib/src/index.js | Updated export references to use caravan-bitcoin |
| packages/xpub-tool/src/components/layout.js | Updated link to reference caravan instead of unchained-bitcoin |
| packages/xpub-lib/src/conversion.test.js | Replaced NETWORKS with Network in test cases |
| packages/xpub-lib/src/metadata.js | Updated import and constant usage for network values |
| packages/xpub-lib/src/conversion.js | Replaced constant usage with caravan's Network |
| packages/xpub-lib/src/paths.test.js | Replaced NETWORKS with Network in derivation path tests |
| README.md | Updated documentation to refer to caravan |
| packages/xpub-lib/src/paths.js | Updated constants in derivation path logic |
| packages/xpub-lib/src/validation.js | Updated import reference from unchained-bitcoin to caravan |
| packages/xpub-tool/gatsby-config.js | Updated site description to reference caravan |
| packages/xpub-lib/src/validation.test.js | Updated tests with caravan Network and added Taproot tests |
| packages/xpub-lib/README.md | Updated documentation to reference caravan |
| packages/xpub-lib/src/metadata.test.js | Updated constant usage for network tests |
| packages/xpub-lib/test/fixtures.js | Added TAPROOT test keys |
| packages/xpub-lib/src/derivation.js | Replaced default network constant with caravan's Network |
| packages/xpub-lib/src/derivation.test.js | Replaced NETWORKS with Network in derivation tests |
Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/xpub-lib/src/validation.test.js:162
- [nitpick] The test description 'valid bech32 v1 (P2TR) address on mainnet' uses a testnet key and network. Consider renaming it to 'valid bech32 v1 (P2TR) address on testnet' to accurately reflect the context.
test("valid bech32 v1 (P2TR) address on mainnet", () => {
|
Note that I've added tests to validate taproot addresses in this PR. I made those same changes on master to prove the failures: test runs on master$ yarn test
yarn run v1.22.22
$ yarn run compile
$ babel -d lib/ src/
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db
Successfully compiled 16 files with Babel (406ms).
$ jest lib
PASS lib/utils.test.js
PASS src/utils.test.js
PASS lib/paths.test.js
PASS src/metadata.test.js
PASS src/conversion.test.js
PASS lib/conversion.test.js
PASS src/paths.test.js
PASS lib/metadata.test.js
FAIL src/validation.test.js
● isValidAddress › valid bech32 v1 (P2TR) address on mainnet
expect(received).toBeTruthy()
Received: false
161 | })
162 | test("valid bech32 v1 (P2TR) address on mainnet", () => {
> 163 | expect(isValidAddress(KEY.MAIN.TAPROOT, NETWORKS.MAINNET)).toBeTruthy()
| ^
164 | })
165 |
166 | // TESTNET
at Object.<anonymous> (src/validation.test.js:163:64)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
● isValidAddress › valid bech32 v1 (P2TR) address on mainnet
expect(received).toBeTruthy()
Received: false
175 | })
176 | test("valid bech32 v1 (P2TR) address on mainnet", () => {
> 177 | expect(isValidAddress(KEY.TEST.TAPROOT, NETWORKS.TESTNET)).toBeTruthy()
| ^
178 | })
179 |
180 | // INVALID: NETWORK MISMATCH
at Object.<anonymous> (src/validation.test.js:177:64)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
FAIL lib/validation.test.js
● isValidAddress › valid bech32 v1 (P2TR) address on mainnet
expect(received).toBeTruthy()
Received: false
137 | });
138 | test("valid bech32 v1 (P2TR) address on mainnet", function () {
> 139 | expect((0, _validation.isValidAddress)(_fixtures.KEY.MAIN.TAPROOT, _unchainedBitcoin.NETWORKS.MAINNET)).toBeTruthy();
| ^
140 | }); // TESTNET
141 |
142 | test("valid legacy (P2PKH) address on testnet", function () {
at Object.<anonymous> (lib/validation.test.js:139:109)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
● isValidAddress › valid bech32 v1 (P2TR) address on mainnet
expect(received).toBeTruthy()
Received: false
150 | });
151 | test("valid bech32 v1 (P2TR) address on mainnet", function () {
> 152 | expect((0, _validation.isValidAddress)(_fixtures.KEY.TEST.TAPROOT, _unchainedBitcoin.NETWORKS.TESTNET)).toBeTruthy();
| ^
153 | }); // INVALID: NETWORK MISMATCH
154 |
155 | test("invalid legacy (P2PKH) address (wrong network)", function () {
at Object.<anonymous> (lib/validation.test.js:152:109)
at processTicksAndRejections (node:internal/process/task_queues:95:5)
PASS src/derivation.test.js (26.307 s)
PASS lib/derivation.test.js (26.561 s)
Test Suites: 2 failed, 10 passed, 12 total
Tests: 4 failed, 200 passed, 204 totaltest runs on this branch$ yarn test
yarn run v1.22.22
$ yarn run compile
$ babel -d lib/ src/
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db
Successfully compiled 16 files with Babel (436ms).
$ jest lib
PASS lib/utils.test.js
PASS src/utils.test.js
PASS lib/paths.test.js
PASS lib/conversion.test.js
PASS src/paths.test.js
PASS src/metadata.test.js
PASS src/conversion.test.js
PASS lib/validation.test.js
PASS lib/metadata.test.js
PASS src/validation.test.js
PASS lib/derivation.test.js (25.172 s)
PASS src/derivation.test.js (25.493 s)
Test Suites: 12 passed, 12 total
Tests: 204 passed, 204 total |
|
blocked by #62 |
…injs lib, updated derivation functions to support taproot
|
@turkycat as discussed, I added a bunch of changes:
Things left to update:
|
This is expected as there are no extended key formats that imply taproot. I also think that concept is dead, everyone just uses xpub these days anyway Thanks for the additions! |
There was a problem hiding this comment.
PR Overview
This PR upgrades the codebase to use the Caravan library instead of the deprecated unchained-bitcoin and adds taproot (p2tr) support. Additionally, it removes various legacy React components from the xpub-components-bootstrap package.
- Updated imports and references from unchained-bitcoin to Caravan in both library and CLI code.
- Added taproot support by introducing a new "p2tr" derivation case and updating corresponding help texts and tests.
- Removed deprecated bootstrap components to streamline the UI components.
Reviewed Changes
| File | Description |
|---|---|
| packages/xpub-lib/jest.config.js | Adjusts Jest config to ignore most node_modules except for valibot |
| packages/xpub-cli/src/xpub.js | Migrates import style and adds taproot support; renames the account-related option |
| packages/xpub-lib/src/conversion.test.js | Updates tests to reflect the new Caravan-based imports |
| packages/xpub-lib/src/conversion.js | Updates conversion function to use Caravan’s Network |
| README.md & packages/xpub-lib/README.md | Updates dependency references and documentation for the new library and added taproot support |
| packages/xpub-components-bootstrap/* | Removes several deprecated React components |
Copilot reviewed 48 out of 48 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
packages/xpub-cli/src/xpub.js:128
- The network variable is reassigned later in the code even though it was declared earlier. Consider reusing the existing variable to avoid redundancy.
const network = cmdObj.testnet ? Network.TESTNET : Network.MAINNET
|
copilot's last comment is incorrect, the file contains two different |
d28b536 to
dec8e96
Compare
| return bc1Address | ||
| return bc1qAddress | ||
| } | ||
| case Purpose.P2TR: { |
ramontayag
left a comment
There was a problem hiding this comment.
Looks ok to me, though my headspace isn't here currently so I suggest getting others to review as well
bertonjulian
left a comment
There was a problem hiding this comment.
LGTM. Tested the CLI against an xpub & tpub from a test Sparrow wallet and compared p2tr addresses generated. As discussed offline, we will likely remove getPurposeFromExtPubKey as it would produce incorrect results for p2tr. But that might happen in a separate PR.
Sorry to nitpick, but for the PR history I want to be clear that it isn't really an incorrect result for |
I compiled this locally, ran the tests, validate the taproot behavior of the cli tool and library against my own extended key tools (different impl) to verify our own test keys and addresses.
I also validated against the BIP86 test vectors
BIP86 test vectors
$ ./xpub.js derive -p p2tr -v xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ { path: "m/86'/0'/0'/0/0", address: 'bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20cac6yqjjwudpxqkedrcr' } $ ./xpub.js derive -p p2tr -v xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ -i 1 { path: "m/86'/0'/0'/0/1", address: 'bc1p4qhjn9zdvkux4e44uhx8tc55attvtyu358kutcqkudyccelu0was9fqzwh' } $ ./xpub.js derive -p p2tr -v xpub6BgBgsespWvERF3LHQu6CnqdvfEvtMcQjYrcRzx53QJjSxarj2afYWcLteoGVky7D3UKDP9QyrLprQ3VCECoY49yfdDEHGCtMMj92pReUsQ -c 1 { path: "m/86'/0'/0'/1/0", address: 'bc1p3qkhfews2uk44qtvauqyr2ttdsw7svhkl9nkm9s9c3x4ax5h60wqwruhk7' } $ $ ./xpub.js validate -v -a bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20cac6yqjjwudpxqkedrcr valid address bc1p5cyxnuxmeuwuvkwfem96lqzszd02n6xdcjrs20cac6yqjjwudpxqkedrcr $ ./xpub.js validate -v -a bc1p4qhjn9zdvkux4e44uhx8tc55attvtyu358kutcqkudyccelu0was9fqzwh valid address bc1p4qhjn9zdvkux4e44uhx8tc55attvtyu358kutcqkudyccelu0was9fqzwh $ ./xpub.js validate -v -a bc1p3qkhfews2uk44qtvauqyr2ttdsw7svhkl9nkm9s9c3x4ax5h60wqwruhk7 valid address bc1p3qkhfews2uk44qtvauqyr2ttdsw7svhkl9nkm9s9c3x4ax5h60wqwruhk7contributes to PSEC-1321 CUS-395