-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Some functions cannot be used directly because they are not exported. #1862
Comments
Users should not import functions that are not exposed publicly. That is correct. This issue is making an implicit assumption that these functions MUST be exported, but they aren't. I think what you meant to say was "I think these functions are useful and would like to see them exported publicly in future versions. These are some of my use cases for them." |
Keep in mind, helper functions that we use internally are sometimes explicitly not exported because we don't want to commit to their interface. ie. If you import them like you do in your example, your app might break during a patch or minor upgrade of bitcoinjs-lib, since they are not a part of the public API, I could flip the order of the arguments if I wanted to, and I wouldn't need to bump the major version of this library. |
The example code is copy from the integration test, not written by me. import { toXOnly, tapTreeToList, tapTreeFromList } from '../../src/psbt/bip371';
import { witnessStackToScriptWitness } from '../../src/psbt/psbtutils'; These function are neccesseary when doing some functions which bitcoinjs-lib aim to providing to users., e.g. custom script with custom finalizer (both p2sh or p2wsh), and some taproot (p2tr) operations. I found that already had a opened issues on Nov 2, 2020 with the similar problem, but still not have any response. I think the best solution is making those function completely internal. That mean toXOnly, tapTreeToList, tapTreeFromList, witnessStackToScriptWitness are done internally. No longer need users to importing when writing a custom script with custom finalizer, and doing some taproot operations. |
re: the taproot functions, I defer to @motorina0 since I'll be pinging him whenever someone requires something changed to the public interface of these functions IF we make them public. re: @motorina0 thoughts overall? |
Most of people are just install bitcoinjs-lib using npm, the integration test should NOT including any hardcoded import files from node_modules directoy. Actually, the lastest bitcoinjs-lib installable version from npm does NOT supporting p2tr yet, because your integration test still need to requiring some functions which are users not easily finding it to import. About of the witnessStackToScriptWitness, I just using the method as same as the old issues (copy and patse the function) |
I usually copy the function (witnessStackToScriptWitness ) verbatim. I would kindly vote for exposing the function if you think it's safe enough (maybe add a new argument with the PSBT version that will throw if not V0 or V2). |
Where |
chiming in -- toXOnly would indeed be helpful to be exported to allow for going from seed phrase -> bip32 PK -> taproot address. |
I would be fine with merging a PR that exposes toXOnly. But...
So maybe PR merge for current major version that just exposes toXOnly, then in the next major version remove it in favor of Signer update maybe? Summer of Bitcoin mentee is currently working bottom-up on replacing Buffer with Uint8Array and supporting ESM. I will be publishing a major update then, so it might be fine to start working on these breaking-style changes to the interfaces. |
the main reason to expose it is so that user code doesn't depend on knowledge of the internals of how the key is represented. |
For example:
Users should not required to manually access the "node_modules" directory to get these functions after they are alreadly installed bitcoinjs-lib from npm.
bitcoinjs-lib should exporting it explicitly.
I hope I can import these functions directly on the next npm version of bitcoinjs-lib.
The text was updated successfully, but these errors were encountered: