bitcoinjs / bitcoinjs-lib

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

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

Closed totty90 closed 9 years ago

totty90 commented 9 years ago

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: https://github.com/bitcoinjs/bitcoinjs-lib/pull/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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

Please see https://github.com/bitcoinjs/bitcoinjs-lib/pull/252, https://github.com/bitcoinjs/bitcoinjs-lib/pull/231, https://github.com/bitcoinjs/bitcoinjs-lib/pull/257, https://github.com/bitcoinjs/bitcoinjs-lib/pull/308, parts of https://github.com/bitcoinjs/bitcoinjs-lib/pull/228 and https://github.com/bitcoinjs/bitcoinjs-lib/issues/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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

Please see https://github.com/bitcoinjs/bitcoinjs-lib/pull/308#issuecomment-60174661 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 commented 9 years ago

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 https://github.com/bitcoinjs/bitcoinjs-lib/pull/308#issuecomment-60174661 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:

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 https://github.com/bitcoinjs/bitcoinjs-lib/pull/231 create a dist for browserify use without the required components built in, for some advanced use.

dcousens commented 9 years ago

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 commented 9 years ago

ping @jprichardson

jprichardson commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

+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 commented 9 years ago

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 commented 9 years ago

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 https://github.com/bitcoinjs/bitcoinjs-lib/issues/314#issuecomment-64534913 .

kyledrake commented 9 years ago

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 commented 9 years ago

@kyledrake the README suggestion was being discussed in https://github.com/bitcoinjs/bitcoinjs-lib/pull/315. We haven't reached consensus yet on whether we should hand hold people through the use of browserify in the rudimentary case.

davidapple commented 5 years ago

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 commented 5 years ago

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

dcousens commented 5 years ago

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

jprichardson commented 5 years ago

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.