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

The minified file used for the client side should be included in this lib #314

Closed
totty90 opened this issue Nov 17, 2014 · 21 comments
Closed

Comments

@totty90
Copy link

totty90 commented Nov 17, 2014

As every proper repo, the min file should be already included. Don't expect every dev can build it (might use win and missing some build tools).

Put the min file in this folder '/dist' and can be named 'bitcoinjs-lib.min.js'.

Edit: Seems like you have already talked about this: #252 but you closed. I have to say that I've never found a bower component that requires any build step before use. Take a look at every client side javascript lib (not only bower) and you will see that they all have a min file bundled. You can create a script to run every time you want to publish a new version that creates the min file and do other stuff you need.

If you don't put a min file then many client side dev will not use this (I don't know if you care or not, but I hope you do).

@weilu
Copy link
Contributor

weilu commented Nov 17, 2014

My opnion hasn't changed. If a dev choose not to use this library because they are unable/unwilling to build the bundle file themselves, perhaps this library isn't for them.

@totty90
Copy link
Author

totty90 commented Nov 17, 2014

I understand your point of view, but what is the point of making everyone's life (of whom uses it on the browser) a pain? I think you should lower the barrier of entry not making it more difficult to use. If someone makes something easier to use why should I use your lib when both do the same?

The problem is not only I have to have npm (that not every user has) but your docs are not even correct.

npm -g install bitcoinjs-lib browserify uglify-js
browserify -r bitcoinjs-lib -s bitcoin | uglifyjs > bitcoinjs.min.js

Will not work you have to do like this:

npm install bitcoinjs-lib
npm install -g browserify uglify-js
browserify -r bitcoinjs-lib -s bitcoin | uglifyjs > bitcoinjs.min.js

bitcoinjs-lib should not be local.

But I even wonder why you would put that documentation at all? If a user can't figure out, perhaps this library isn't for them. Now I understand why you put the error in your docs, so it make more difficult to get started, great idea!

I understand you might not like to make the build every time you publish a new version. Or is another cause?

@dcousens
Copy link
Contributor

Please see #252, #231, #257, #308, parts of #228 and #148.

tl;dr its not necessary.

Its also not possible to use this library correctly fully without either creating a bundle which includes ecurve, bigi and browserify Buffer's, as they are all part of our public API.

It would also be difficult for us to create an easily audit-able (that is, for security reasons) minified file without locking all dependencies and all sub dependency versions due to the nature of browserify.

Thanks for pointing out a potential error in the documentation. Perhaps your right, maybe we should just link to the browserify documentation instead.

@dcousens
Copy link
Contributor

If you have any potential solutions we can use that solve the problems above, then I'd be happy to investigate them with you and merge them, as this is [evidently] a concern for several users.

@totty90
Copy link
Author

totty90 commented Nov 17, 2014

Its also not possible to use this library correctly without either creating a bundle which includes ecurve, bigi and browserify Buffer's, as they are all part of our public API.

So your docs aren't right? I can't use on browser directly after I build it?

@dcousens
Copy link
Contributor

It is not possible to use this library correctly in its entirety without using browserify yourself.

Please see #308 (comment) for more information on how you might go about being able to use all aspects of the bitcoinjs-lib API in the browser.

Perhaps our documentation needs to be more clear around this. Pull requests are appreciated.

@totty90
Copy link
Author

totty90 commented Nov 17, 2014

Then is even more complicated to use on browser than I thought (was not clear in the doc, it just said you have to do this and that to use it).

If you have any potential solutions we can use that solve the problems above, then I'd be happy to investigate them with you and merge them, as this is [evidently] a concern for several users.

Do what says here #308 (comment) and build a bundle with all the necessary deps to be used out of the box on the client. Best would not even have to depend on browserify (some use requirejs) and export in 3 types:

  • Browserify export;
  • Requirejs export;
  • Global variable export;

In this way you make happy every client side dev. I doubt you have any client side dev using your lib with all those difficulties to make it work.

Also make like you said here #231 create a dist for browserify use without the required components built in, for some advanced use.

@dcousens
Copy link
Contributor

The only compromise that would still be verifiable would be to commit a npm shrinkwrap JSON file with each release to allow 3rd parties to verify the browserified dist file themselves.

I'm still [personally] a fan of just leaving all the build process up to users and just move all the responsibility of teaching users how to use browserify, to the browserify documentation.

Lets not act like a browser module, and instead just note that we are compatible with browserify?

@weilu thoughts?

@dcousens
Copy link
Contributor

ping @jprichardson

@jprichardson
Copy link
Member

I understand your point of view, but what is the point of making everyone's life (of whom uses it on the browser) a pain?

This is clearly not the intention of the authors of bitcoinjs-lib. I feel like I've discussed this issue and related issues ad nauseam with the bitcoinjs-lib devs. The conclusion that keeps surfacing is that we're developing financial applications here; as such, we must take the extra precautions to ensure that our applications work as expected and don't contain any malware. I truly believe that this is an important principle to stick to.

However, this doesn't mean that we go out of our way to make using the library difficult. I also believe that the developers have shown their willingness to work with the community and improving these sorts of issues.

It would also be difficult for us to create an easily audit-able (that is, for security reasons) minified file without locking all dependencies and all sub dependency versions due to the nature of Browserify.

I think this is one of the most important points. Related to above.


What if there was a build file that was included that was NOT minified and is a UMD (commonjs, amd, script)? A build script could add the UMD part. Also, since most developers like to use their own minifier and may create their own builds, maybe this is the best of all of both worlds (non-minified, but browser ready)? It is additional work, that's for sure, but maybe it would satisfy the needs of browser devs that don't use Browserify (As an aside, Browserify rocks :p )?

@weilu
Copy link
Contributor

weilu commented Nov 18, 2014

How is this for the browser section?

Browser

bitcoinjs-lib works with browserify. If you want to compile a minified version with a global object in the browser, here is how.

From the repository:

# Checkout the latest tagged version of the source code
git clone https://github.com/bitcoinjs/bitcoinjs-lib.git
cd bitcoinjs-lib
git co `git describe`

# Install dependencies & compile `bitcoinjs-min.js`
npm install
npm run-script compile

Alternatively, from NPM:

npm install bitcoinjs-lib
cd node_modules/bitcoinjs-lib
npm run-script compile

After loading the compiled bitcoinjs-min.js file in your browser, you will be able to use the global bitcoin object.

If you want to customize exported modules in the bundled, update src/index.js and run npm run-script compile.

@totty90
Copy link
Author

totty90 commented Nov 18, 2014

The conclusion that keeps surfacing is that we're developing financial applications here; as such, we must take the extra precautions to ensure that our applications work as expected and don't contain any malware.
It would also be difficult for us to create an easily audit-able (that is, for security reasons) minified file without locking all dependencies and all sub dependency versions due to the nature of Browserify

Doing so, you will just make the developer responsable for the security of your library. You have to understand that most devs will make it wrong and if there is a malware in npm then all your users will be affected, none will even notice.

All this doesn't increase the security of the final release, it will make it less secure. If you guys would make the build, of course you would waste more time, would certainly more secure, at least you check for malware and if you hear something about any of the deps you can fastly update the required deps and make a new dist which will be downloaded from now on.

Also, since most developers like to use their own minifier and may create their own builds, maybe this is the best of all of both worlds (non-minified, but browser ready)?

Yes some, but others might not have (just look at almost all the others libs, why you want to change that? Are all of them wrong? Or just because if a user don't know how to use a build process, this lib isn't for them?).

All this to say that I will not insist anymore, you already made your mind and is hard to change. (I also never needed the client side version, but wanted to improve your lib.)

@kyledrake
Copy link
Contributor

+1 @weilu's README suggestion. +1 to staying the course.

Random thought: We -could- create a dist repo, and sign all commits to it as versions (bitcoinjs-2.0.min.js) with our PGP keys. You wouldn't be able to easily audit the provided file, but you could at least have a way to verify that one of us did it.

Comedy option: We create a dist repo, and create two compiled versions of each release. One of them contains theftware that sends all Bitcoins to the BitcoinJS multi-sig address, and the other does not, and you have a 50/50 chance of choosing the "correct" one.

An amendment to the README to include the explanation for why we do not put dists into bitcoinjs-lib is probably a good idea.

@totty90
Copy link
Author

totty90 commented Nov 26, 2014

Comedy option: We create a dist repo, and create two compiled versions of each release. One of them contains theftware that sends all Bitcoins to the BitcoinJS multi-sig address, and the other does not, and you have a 50/50 chance of choosing the "correct" one.

What is the benefit?

Random thought: We -could- create a dist repo, and sign all commits to it as versions (bitcoinjs-2.0.min.js) with our PGP keys. You wouldn't be able to easily audit the provided file, but you could at least have a way to verify that one of us did it.

If is already in this repo we already can be sure that they have done.

@dcousens
Copy link
Contributor

Agree with @totty90 that the extra repo adds nothing but misdirection.
On Nov 26, 2014 8:18 PM, "TT TY" notifications@github.com wrote:

Comedy option: We create a dist repo, and create two compiled versions of
each release. One of them contains theftware that sends all Bitcoins to the
BitcoinJS multi-sig address, and the other does not, and you have a 50/50
chance of choosing the "correct" one.

What is the benefit?

Random thought: We -could- create a dist repo, and sign all commits to it
as versions (bitcoinjs-2.0.min.js) with our PGP keys. You wouldn't be able
to easily audit the provided file, but you could at least have a way to
verify that one of us did it.

If is already in this repo we already can be sure that they have done.


Reply to this email directly or view it on GitHub
#314 (comment)
.

@kyledrake
Copy link
Contributor

kyledrake commented Nov 27, 2014

What is the benefit?

Humor, education, the introduction of healthy paranoia to developers, and something to link to the next time this is filed as an issue.

There seems to be consensus on this issue from all the developers that this is a security principle worth maintaining, so I'm closing.

If anyone wants a pre-built BitcoinJS, just send them here <redacted by @dcousens>

@dcousens
Copy link
Contributor

@kyledrake the README suggestion was being discussed in #315.
We haven't reached consensus yet on whether we should hand hold people through the use of browserify in the rudimentary case.

@davidapple
Copy link

davidapple commented Sep 17, 2018

I understand your security concerns. However, having to compile this locally is a massive pain. I am a proficient web developer, however, I routinely work on machines that throw errors when running npm. This is a massive barrier to entry for a lot of developers and a huge time sponge.

Please make a compiled version of bitcoinjs-lib v4.0.2 available from somewhere. Even if it's from another repository. Nearly all other js libraries include a dist folder with a minified version of the library pre-compiled. It's standard and saves a huge amount of time.

I'm sure people using this for serious financial applications are aware of the risks. But forcing people to compile every time is blocking the majority of developers that want to play.

Not publishing a minified version of this somewhere will force developers to search the web, finding compiled versions of earlier versions from unreliable sources.

@dcousens
Copy link
Contributor

Half the libraries interface is using a Buffer. How do you expect to use it?

@dcousens
Copy link
Contributor

I don't think any new argument has been presented.
Should we lock this one?

@jprichardson
Copy link
Member

Should we lock this one?

Yes.

@davidapple A new issue should be created to discuss this. Including a bundled version carries an additional audit burden for consumers of the package. Also, there's a lot of packaged overlap due to the reliance upon Node.js specific libs which means the payload size would generally be quite large and unnecessary for most. I realize it's painful for you, but these days it's inescapable for web devs to avoid using npm; so much tooling depends upon it.

@bitcoinjs bitcoinjs locked as resolved and limited conversation to collaborators Sep 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants