feat(wallets): add BC-UR xpub import support - #522
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b059ec8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
Shadouts
left a comment
Shadouts
left a comment
There was a problem hiding this comment.
Mostly requests for cleanup and deduplication of some logic available in the @caravan/bitcoin library
| network: BitcoinNetwork | ||
| ): Network.MAINNET | Network.TESTNET { | ||
| return network === Network.MAINNET ? Network.MAINNET : Network.TESTNET; | ||
| } |
There was a problem hiding this comment.
I don't think this is the way to go. Does ExtendedPublicKey not already properly handle test networks like regtest? I don't think you have to transform the value before passing it to ExtendedPublicKey. If you find that you have to, then it smells like something else might be off. See my other comment about validateExtendedPublicKeyForNetwork()
| const decodedResult = this.handleDecodedResult( | ||
| result.type, | ||
| Buffer.from(result.cbor.buffer), | ||
| Buffer.from(result.cbor), |
There was a problem hiding this comment.
what's requiring this change?
| expect(decoder.getError()).toBe(PASSPORT_MULTISIG_EXPORT_ERROR); | ||
| }); | ||
|
|
||
| it("rejects a key that does not represent m/45'", () => { |
There was a problem hiding this comment.
Should this actually be a constraint? If so, why?
| import { bip32SerializationNetwork } from "./utils"; | ||
|
|
||
| const PASSPORT_MULTISIG_PATH = "m/45'"; | ||
| const PASSPORT_MULTISIG_INDEX = 0x8000002d; |
There was a problem hiding this comment.
Maybe collect device-specific values together into a config object.
| const PASSPORT_MULTISIG_INDEX = 0x8000002d; | ||
| const FINGERPRINT_PATTERN = /^[0-9a-f]{8}$/i; | ||
|
|
||
| export const PASSPORT_MULTISIG_EXPORT_ERROR = |
There was a problem hiding this comment.
Don't export just to get a test to work. This isn't really being used anywhere else.
| import { BCUR2Decoder, ExtendedPublicKeyData } from "./decoder"; | ||
| import { bip32SerializationNetwork } from "./utils"; | ||
|
|
||
| const PASSPORT_MULTISIG_PATH = "m/45'"; |
There was a problem hiding this comment.
Use MULTISIG_ROOT from constants.ts
| import { bip32SerializationNetwork } from "./utils"; | ||
|
|
||
| const PASSPORT_MULTISIG_PATH = "m/45'"; | ||
| const PASSPORT_MULTISIG_INDEX = 0x8000002d; |
There was a problem hiding this comment.
Use hardenedBIP32Index() from @caravan/bitcoin
|
|
||
| const PASSPORT_MULTISIG_PATH = "m/45'"; | ||
| const PASSPORT_MULTISIG_INDEX = 0x8000002d; | ||
| const FINGERPRINT_PATTERN = /^[0-9a-f]{8}$/i; |
There was a problem hiding this comment.
Use validateRootFingerprint from @caravan/bitcoin
| const serializationNetwork = bip32SerializationNetwork(network); | ||
| const expectedVersion = | ||
| serializationNetwork === Network.MAINNET | ||
| ? EXTENDED_PUBLIC_KEY_VERSIONS.xpub | ||
| : EXTENDED_PUBLIC_KEY_VERSIONS.tpub; | ||
|
|
||
| let key: ExtendedPublicKey; | ||
| try { | ||
| key = ExtendedPublicKey.fromBase58(value.p2sh); | ||
| } catch (_error) { | ||
| throw new Error(PASSPORT_MULTISIG_EXPORT_ERROR); | ||
| } | ||
|
|
||
| if ( | ||
| key.version !== expectedVersion || |
There was a problem hiding this comment.
Some of this can be handled simply by using validateExtendedPublicKeyForNetwork() from @caravan/biitcoin
| value.xfp.toLowerCase() | ||
| ) { | ||
| throw new Error(PASSPORT_MULTISIG_EXPORT_ERROR); | ||
| } |
There was a problem hiding this comment.
There is some more validation logic that can be replaced with stuff we have in @caravan/bitcoin
2 participants
What kind of change does this PR introduce?
Issue Number:
Fixes #
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Checklist
npm run changeset)Other information
Have you read the contributing guide?
For information on creating and using changesets, please refer to our documentation on changesets.