bitcoinjs / bitcoinjs-lib

A javascript Bitcoin library for node.js and browsers.
MIT License
5.6k stars 2.08k forks source link

Some functions cannot be used directly because they are not exported. #1862

Open h76oeI6pMxU9g4p8aCpc6Q opened 1 year ago

h76oeI6pMxU9g4p8aCpc6Q commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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. https://github.com/bitcoinjs/bitcoinjs-lib/issues/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 commented 1 year ago

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 commented 1 year ago

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) https://github.com/bitcoinjs/bitcoinjs-lib/issues/1648

landabaso commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

[RaySwag52]()