-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Transaction Builder #244
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
Conversation
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" [email protected] 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.
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') |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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.
Rebased without removal of |
Added 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 |
For what its worth, this PR is chained on #247. |
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!). In-order synchronous signing is now quite easy to do though using |
Rebased and squashed a lot of the "in progress" commits. edit: Sigh, coveralls crashing again. Continue forwards, the show must go on. |
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 afromTransaction
method for initializing it from an incompleteTransaction
so you can pass a incompleteTransaction
around between signers.