Wallet clean

[dcousens]

This pull request attempts to clean the Wallet class so as to adhere to practices used throughout the rest of the repository.

It deprecates the following methods:

• newMasterKey --- considered harmful, and there is no reason not to just use new Wallet(...) instead, which is explicit and less ambiguous about what may happen.
• setUnspentOutputs --- this should only ever be done on initialization, and therefore has been included as an optional argument to the constructor. All other UTXO handling is done through processTx.

It also moves as many of the closure scoped methods to the prototype as possible without modifying/increasing the API.

As a side note, the testing infrastructure really needs to become data driven, at the moment it was simpler to use testnet more often as the test data was more readily available.

[dcousens]

I actually feel UTXO handling should be completely removed and handled solely by process*Tx. This is the most natural progression and means the user does not throw away information that will be useful for more advanced wallet designs in future.

This also means all discovery processes follow the same pattern and that unspents must be given in the form of their actual transaction (much safer and easier to reason with).

But that'll have to be for 2.0.0.

edit: See further comments.

[coveralls]

Coverage decreased (-0.09%) when pulling 6df785b on dcousens:wallclean into 09455a6 on bitcoinjs:master.

[coveralls]

Coverage decreased (-0.09%) when pulling c13177b on dcousens:wallclean into 09455a6 on bitcoinjs:master.

[coveralls]

Coverage decreased (-0.09%) when pulling 1ca6996 on dcousens:wallclean into 09455a6 on bitcoinjs:master.

[dcousens]

@weilu what are your thoughts on processTx actually over complicating things more than it should?

Why do we need anything other than the unspents?

The Wallet should probably just contain all unspents, and as unspents usually have an accompanying confirmations field, we could just feed that in and put in a parameterized minimum confirmations for the following functions:

• getBalance
• createTransaction

There is never going to be a 'catch-all' Wallet design, so maybe we should just make this one as simple as possible?

In what scenario is processTransaction actually useful unless we're including more advanced features like multisig in the Wallet by default?

edit: This is basically an exact contradiction to what I just mentioned above, but arguably I've made the issue more transparently 2-sided with this PR.

So I guess my question is, can we see ourselves wanting to support a more complicated Wallet design, or a P2PKH only design (not necessarily a bad thing).

Arguably anything more complicated would be context specific anyway.

[dcousens]

My biggest gripe with processTransaction right now is the fact that if you don't process transactions in exact chronological order, then there is the real possibility for missed spends and invalid unspent outputs to occur inside the Wallet.

The only way to work around this is to ensure that all transactions are correctly chained and processed in accordance to their graph-like structure. Thankfully its acyclic at least.

But still, it would require a lot more support code which we probably don't need to provide with bitcoinjs-lib. Therefore I vote to veto and remove processTransaction completely in 2.x.y, and completely contradict my self from above in saying unspents should be the only way to deal with this Wallet.