API Consistency

[dcousens]

A lot of the methods in this library used a mixed API, accepting byte arrays, hex strings and other special case strings.

Is there a particular guide line we are following here?
Do we want to more closely mimic formats that are used by the bitcoind RPC API?
Or do we use what is most convenient for cross-development where appropriate? (Byte arrays? Hex? Node Buffers? (using browserify))
Note, if we used Node Buffers, we'd get built-in import/export functions using the Buffers built-in .toString('hex') decoder.

This has been touched in a few issues, so I thought I'd ask it here.

[weilu]

Node buffers +1. It will save us from providing helper methods for different formats. We'll also be able to get rid of various hand-rolled methods in convert.js

[dcousens]

Speaking of consistency, I think is important we settle on 1 import style throughout the code base and use it.

At the moment we have:

• require(RELATIVE_FILEPATH)
• require(PACKAGE).Module

And some mixed versions of the above.

I personally prefer the latter require(PACKAGE).Module, because it gives us the most flexibility without having to collaborate the file paths with the package structure, despite almost 80% of the modules matching word for word.

If we opt for the first, it means that all the tests will use ../src/Module as their imports, which just seems like unnecessary coupling to the src directory.
Compared to: require('..').Module.

By doing require(PACKAGE), we can also opt to just doing:

var bjs = require('.')
var ECKey = bjs.ECKey
var BigInteger = bjs.BigInteger
...

Food for thought.

[weilu]

I'd go for require(RELATIVE_FILEPATH) because it's explicit about a file's dependencies. One can easily look at the top of a file and know what are its dependencies. If we do var bjs = require('.') one can call bjs.Address anywhere from the file. Unlike require("./address") in the middle of a file, bjs.Address would not be easily spotted.

It does couple dependency with filenames - if a file is moved we need to go into various places to update the requires. But how often do we move files anyway?

[dcousens]

I agree that in using bjs.* you would suddenly have two possible namespaces for using each module. (e.g Address and bjs.Address).
However that can be resolved by only using require(PACKAGE).Module for each module instead of the shorthand syntax Address = bjs.Address after a top level import.

[weilu]

Still require('./') is over-importing - require ALL THE THINGS but I only use Address.

[dcousens]

True.

But consider the current situation in tests/ where a module's file path is moved or changed: you would have to update not just the module's own tests, but every test that used that module or relied on its file path being the same.

Suddenly 1 change turns into many, simply because you used the file path to dictate package structure rather than the abstraction layer that a package provides.

[caedesvvv]

Maybe this would depend on crypto lib we use.

Imo buffers are shit atm because we're using bytes and wordarrays, at least when browserifying the buffer implementation looks like the horror... that's not going to go well. Also more difficult to manage from other apps, they don't have slice or splice.

CryptoJS also has a lot of encode decode, including hex of course, word representation looks like has to be more efficient.

just my 2c!

cheers!

[dcousens]

@caedesvvv I don't see how https://github.com/feross/buffer/blob/master/index.js is a bad implementation that looks like the horror?
The lack of slice and splice is the inherent nature of a buffer; I'm not sure what you were expecting here. The fact you have to make explicit copies allows you to build more performant applications.
The performance benefits in native/typed arrays (see above implementation) greatly outweigh the benefits of using Arrays, regardless of whether their contents are Numbers with arbitrary 8 or 32 bit limits.

Understandably migrating code is less fun, but it will be worthwhile in the long term.

[caedesvvv]

To me it does look like something terrible to be happening in all my arrays XD.

Is the plan to move everything to node crypto library? Also how do you guys coordinate is there any irc channel?

If we can use native crypto module from the browser that would also make it much better to do this, do you know if this is happening? (maybe it's going to go with: https://github.com/dominictarr/crypto-browserify)

[dcousens]

@caedesvvv we are currently using crypto-browserify for our local bitcoin.crypto module.

The local module supplies sha256, hash160, hash256 and sha1.
The hash160 implementation uses both CryptoJS (for RIPEMD160) and the crypto-browserify SHA256 implementation.

I think the plan is to move as much as possible to crypto-browserify, but naturally RIPEMD160 will still need to come from a different dependency.

I idle on #bitcoinjs-dev on Freenode, and I know @weilu was starting to as well.