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

BS58 module #228

Merged
merged 2 commits into from
Jun 26, 2014
Merged

BS58 module #228

merged 2 commits into from
Jun 26, 2014

Conversation

dcousens
Copy link
Contributor

This pull request removes src/base58.js and replaces it with https://github.com/cryptocoinjs/bs58.

All our changes and tests have been cross ported to that module, and there is little reason not to move it out given the API is not going to change (as far as I can see).

Its up for debate whether we should force users to use browserify when they might want to use bs58 themselves, or whether we should continue to export it as bitcoinjs.base58 = require('bs58').

I think we need a consistent policy here, because it currently impacts on BigInteger, ecurve and now base58.

The ideological solution would be that we just ask/force users to use browersify.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) when pulling 002c428 on dcousens:b58 into 0198477 on bitcoinjs:master.

@dcousens dcousens changed the title B58 module BS58 module Jun 26, 2014
@kyledrake
Copy link
Contributor

Force browserify is my strong opinion here. If there's no reason to expose an API for purposes of bitcoin work, let's not do it. People using these libraries below BitcoinJS is a smell that should be resolved by providing the things they need in the library directly.

kyledrake added a commit that referenced this pull request Jun 26, 2014
@kyledrake kyledrake merged commit 6596ca1 into bitcoinjs:master Jun 26, 2014
@Shic1983
Copy link

bitcoinlib-js currently offers no way for anyone to verify an uncompressed public key.

In the old version 0.1.3, you'd just do, var curve = getSECCurveByName("secp256k1");

In the new version there is just no way to do it that I can find, I have spend 2 days working on this.. simply trying to figure out how to verify an uncompressed public key;

Suggest to me so far has been; Browserify my code, which doesn't really work for me given I just want to include the bitcoinlib-js library and then perform this public key verification adhoc in some inline js tags...

The other way I thought was to basically provide an open channel to ecurve by adding in a custom function inside one of the existing files (eg: ecpubkey.js) to give me access to ecurve externally.

So far I have tried;
Bitcoin.ecdsa.verify('secp256k1', '04cecce48ed9b1865f68db5f57f34de979397664e69f7db2c02469abb4a15919fdf7b0fd3e03ae96b7f8f2af1881eb9010807bb9fe68f4c7fa995ccc7c0b4e00b7')
failed

Bitcoin.ECPubKey.fromBuffer('04cecce48ed9b1865f68db5f57f34de979397664e69f7db2c02469abb4a15919fdf7b0fd3e03ae96b7f8f2af1881eb9010807bb9fe68f4c7fa995ccc7c0b4e00b7')

also failed..

It just seems like with bitcoinlib-js ~ verifying an uncompressed public key should be easy.. but its not.. it feels really difficult and arkward..

@Shic1983
Copy link

@kyledrake I am strongly opposed to forcing browserify.
I think 99% of devs out there, who want to use a bitcoinlib-js library to just plug into to their browser, then start doing bitcoin related stuff... will more than likely be dumping it into the head then just trying to access it todo things from within their existing code either inline in the page... or in their existing code... etc... forcing people to go away and browserify their code just seems like its going to be alienating alot of people who dont have the time or will to bother to do such a thing but want to use a bitcoin javascript library.

@sidazhang
Copy link
Contributor

@NTOM is providing a precompiled browser version of bitcoinjs-lib a possible alternative. So developers that just want to get started quickly can simply download a bitcoinjs-lib.min.js from a CDN. A checksum can be provided against each version to verify the integrity of the source

@Shic1983
Copy link

@sidazhang yeah I'm all for using browserify as the package management thing for this library... I am just miffed because ecurve is used in bitcoinlib-js ~ yet I can't access it externally, which means i'd have to reinclude it separately, or alternatively browserify my whole code base... when all I want to do is just use it to verify a uncompressed public key...

@Shic1983
Copy link

In 0.1.3. .. one could do;

Crypto.SHA256(Crypto.SHA256(r,{asBytes: true}),{asBytes: true});
or
var curve = getSECCurveByName("secp256k1");

Both or which I think are useful and related to bitcoin, yet not included in the package cuz its browserifyied vs the original versions which were easier for 'normal' devs like me to just plug in and user without having to browserify one's code..

So instead I'd have to include Crypto-js as a seperate entity in my site.. even thought its already been included and utilized in bitcoinlib-js ..

I hope I am making sense here. 'Average joe' style comments from ur typical lay person who is looking to make use of the library and t he difficulties / challenges I am facing vs the prior version.

@dcousens dcousens deleted the b58 branch June 27, 2014 02:33
@dcousens
Copy link
Contributor Author

@NTOM you are being confused though, both examples you gave of not working were simply because you gave the completely wrong input.

var pub = ECPubKey.fromHex('04cecce48ed9b1865f68db5f57f34de979397664e69f7db2c02469abb4a15919fdf7b0fd3e03ae96b7f8f2af1881eb9010807bb9fe68f4c7fa995ccc7c0b4e00b7')

That will import the public key, but it is not verified as being on the secp256k1 curve.
If the point was compressed, it will be uncompressed as part of the import process and by way of derivation, there is no case where it isn't on the curve by default.

If you really need to check the point, you will need to use ecurve:

var ecurve = require('ecurve')
var curve = ecurve.getCurveByName('secp256k1')

curve.validate(pub.Q) // throws if false

The real issue you describe is that perhaps we should be providing better validation primitives in the ecurve and bitcoinjs-lib API a-like.

@kyledrake
Copy link
Contributor

Things people can't do with the library involving Bitcoin are a bug that needs to be fixed. Things people want to do that are out of scope for Bitcoin but happen to involve some of our dependencies are out of the scope of this library. I'm not keen on dumping our internal dependencies, and it makes it a lot harder for us to switch them for something else in the future.

I would much rather people include these libraries on their own projects directly if they need them.

@kyledrake
Copy link
Contributor

I'm also opposed to distributing a compiled copy of bitcoinjs-lib in our repository. Minified files are not auditable, and introduce security problems and weird diffs on commits, and also introduces a commit requirement to reminify for each commit.

If the problem here is you just want a file to download, it would be best to provide it as a separate thing. Maybe on the BitcoinJS web site I made, or maybe on a distribution server.

But if I compile this file for other developers, it means that you cannot audit my compile, and you have to trust that I (or an attacker) has not manipulated the file to steal your Bitcoins, or conduct other harmful activities. It feels like the wrong thing to do. Node.js works on all platforms, and you can very easily compile your own bitcoinjs-lib.min.js with just a few commands, so it's not that much more convenient.

If this is something you can't figure out how to do, you probably shouldn't be writing bitcoin code anyways. I'm opposed to providing a pre-compiled file. Use browserify. It's the future of JS programming.

@kyledrake
Copy link
Contributor

If you think "If this is something you can't figure out how to do, you probably shouldn't be writing bitcoin code anyways" is harsh, observe the incredibly simple process to compile your own minified bitcoinjs-lib:

$ git clone [email protected]:bitcoinjs/bitcoinjs-lib.git
$ cd bitcoinjs-lib
$ npm install
$ npm run-script compile

Is this just a documentation problem?

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

Successfully merging this pull request may close these issues.

5 participants