-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spend to Taproot #91
Spend to Taproot #91
Conversation
🦋 Changeset detectedLatest commit: 24c5c5b The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good. Comments here are mostly nits. I'm still concerned with the major version increment I commented about on my previous review. Ideally, more changes (many) should be collected and released together as a major increment, or we consider changes to the serialized psbt (a v0-compatible psbt) as not being a breaking change.
import { bufferize } from "../functions"; | ||
import BigNumber from "bignumber.js"; | ||
|
||
bitcoin.initEccLib(ecc); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How long does this take? This line here will initialize ecc when any script runs which imports this module. Is that appropriate? Would it be better to only initialize this inside of a function or class initialization which would be using it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's a good question. I haven't noticed any performance impact and this is the way it's demoed in the bitcoinjs-lib docs. Let me see if initializing in the functions works. It feels clunkier having to do that every time but maybe worth it (and at least in our libs we can abstract that complexity from consumers of caravan).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to punt on this now if that's ok. given this is how it's done in the examples it should be ok. But when they hopefully fix some of the other issues with this initialization method, that might be a good opportunity to clean this up, maybe with a singleton factory that can check if it's already initialized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spend to taproot works and I haven't found any regressions. Nice work!
A spend to taproot test for the test suite would be nice but not a blocker.
Includes a few fixes:
TODO:
NOTE: the diff looks a little bigger than it is because the tiny-secp256k1 library was vendor-ized. See updated readme in @caravan/psbt for more details