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

Transaction Builder #244

Merged
merged 16 commits into from
Aug 18, 2014
Merged

Transaction Builder #244

merged 16 commits into from
Aug 18, 2014

Conversation

dcousens
Copy link
Contributor

The aforementioned, and still in need of plenty of test fixtures, TransactionBuilder.

Anyone is welcome to throw fixtures at it, but, presuming you abide by the API (should be obvious), it should automagically sign them correctly with the private keys provided.
See tests for the intended behaviour.

All standard transaction types are supported including P2SH variants thereof.

It also needs a fromTransaction method for initializing it from an incomplete Transaction so you can pass a incomplete Transaction around between signers.

@kyledrake
Copy link
Contributor

Let's do a deprecation cycle, yeah. Use console.warn for it, it should work on everything.

@@ -162,7 +159,7 @@ Transaction.prototype.toHex = function() {
* hashType, serializes and finally hashes the result. This hash can then be
* used to sign the transaction input in question.
*/
Transaction.prototype.hashForSignature = function(prevOutScript, inIndex, hashType) {
Transaction.prototype.hashForSignature = function(inIndex, prevOutScript, hashType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we flipping this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consistency with all other input related function signatures.
On Jul 19, 2014 6:12 AM, "Kyle Drake" notifications@github.com wrote:

In src/transaction.js:

@@ -162,7 +159,7 @@ Transaction.prototype.toHex = function() {

  • hashType, serializes and finally hashes the result. This hash can then be
  • used to sign the transaction input in question.
    */
    -Transaction.prototype.hashForSignature = function(prevOutScript, inIndex, hashType) {
    +Transaction.prototype.hashForSignature = function(inIndex, prevOutScript, hashType) {

Why are we flipping this?


Reply to this email directly or view it on GitHub
https://github.com/bitcoinjs/bitcoinjs-lib/pull/244/files#r15131198.

@kyledrake
Copy link
Contributor

I'm +1 on this pending @weilu's review.

I want to tag this as a 1.1.0 release.

Not opposed to doing a console.warn deprecation cycle as proposed, and then we deprecate it in 1.2.0 or something.


var tx = wallet.createTx(to, value, fee)

assert(Transaction.prototype.sign.calledWith(0, wallet.getPrivateKeyForAddress(address2)))
assert(Transaction.prototype.sign.calledWith(1, wallet.getPrivateKeyForAddress(address1)))
assert.equal(tx.getId(), '1375c7841bdeca1471a97ed196ff729ff0ceee6ca711ee3c42992419d2f442ee')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this assert verifying "it signs the inputs with respective keys"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hash is dependent on the correct signatures for the hash to be correct. It is a more broad-reaching test than specifically mocking the API.

It wasn't simple at the time, but now this should probably revert to what was already there but using TxBuilder instead.

@dcousens
Copy link
Contributor Author

Rebased without removal of Transaction.prototype.* signing methods. Deprecated prototypes with console.warn. Retained use of sinon in Wallet for ensuring TransactionBuilder.sign is called, except a sinon.spy is used instead of a sinon.stub.

@dcousens
Copy link
Contributor Author

Added fromTransaction and signature verification.

Its a bit of a behemoth, and the error checking is not 100% consistent across all use cases simply because so much information gets lost in the TransactionBuilder -> Transaction -> TransactionBuilder pipeline... though I've done what I can to recover it.

@dcousens
Copy link
Contributor Author

For what its worth, this PR is chained on #247.

@dcousens
Copy link
Contributor Author

Asynchronous multi-signature signing is still not supported because you would still have to manually merge the signatures when joining (and in the right order!).
I'm undecided of how this process should happen... in the raw multisig case, I won't even have the original pubKeys so I can't even do a signature verification + re-ordering...

In-order synchronous signing is now quite easy to do though using TransactionBuilder.fromTransaction and Transaction as the serialization format (like bitcoin-qt).

@dcousens
Copy link
Contributor Author

Rebased and fixes applied as per the feedback. Thanks @weilu.

Now chained on #258.

@dcousens
Copy link
Contributor Author

Rebased and squashed a lot of the "in progress" commits.

edit: Sigh, coveralls crashing again. Continue forwards, the show must go on.

weilu added a commit that referenced this pull request Aug 18, 2014
@weilu weilu merged commit 106e00e into bitcoinjs:master Aug 18, 2014
@dcousens dcousens deleted the txbuilder branch August 18, 2014 10:20
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.

None yet

3 participants