Skip to content
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

Open
h76oeI6pMxU9g4p8aCpc6Q opened this issue Jan 10, 2023 · 11 comments

Comments

@h76oeI6pMxU9g4p8aCpc6Q
Copy link

h76oeI6pMxU9g4p8aCpc6Q commented Jan 10, 2023

For example:

import { toXOnly, tapTreeToList, tapTreeFromList } from '../../src/psbt/bip371';
import { witnessStackToScriptWitness } from '../../src/psbt/psbtutils';

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.

@junderw
Copy link
Member

junderw commented Jan 10, 2023

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."

@junderw
Copy link
Member

junderw commented Jan 10, 2023

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.

@h76oeI6pMxU9g4p8aCpc6Q
Copy link
Author

h76oeI6pMxU9g4p8aCpc6Q commented Jan 10, 2023

The example code is copy from the integration test, not written by me.
https://github.com/bitcoinjs/bitcoinjs-lib/blob/master/test/integration/taproot.spec.ts

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.
#1648

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.

@junderw
Copy link
Member

junderw commented Jan 11, 2023

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: witnessStackToScriptWitness I was against making this public, since I was afraid that some new PSBT version would come along, be more popular, and then use some different method of finalization for the witness stack... but seeing as PSBT v2 has been out for a while and adoption hasn't really taken on, nor has it modified the scriptWitness, it might make sense to expose it.

@motorina0 thoughts overall?

@h76oeI6pMxU9g4p8aCpc6Q
Copy link
Author

h76oeI6pMxU9g4p8aCpc6Q commented Jan 11, 2023

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)
#1648

@landabaso
Copy link

landabaso commented Feb 1, 2023

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).

@RaySwag522
Copy link

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) #1648

Where

@RaySwag522
Copy link

RaySwag52

@JeremyRubin
Copy link

chiming in -- toXOnly would indeed be helpful to be exported to allow for going from seed phrase -> bip32 PK -> taproot address.

@junderw
Copy link
Member

junderw commented Jul 21, 2024

I would be fine with merging a PR that exposes toXOnly.

But...

  1. It's so simple I am not sure how necessary it is.
  2. Arguably it should maybe be in an updated version of the Signer interfaces. (and thus implemented on bip32 and ecpair).

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.

@JeremyRubin
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants