fix bug in change calculation

docs/transaction.md

@@ -112,11 +112,11 @@ When outputs' value don't sum up to the same amount that inputs, the difference
 
 For this reason, some methods in the Transaction class are provided:
 
-* `change(address)`: Set up the change address. This will set an internal `_change` property that will store the change address.
+* `change(address)`: Set up the change address. This will set an internal `_changeScript` property that will store the change script associated with that address.
 * `fee(amount)`: Sets up the exact amount of fee to pay. If no change address is provided, this will raise an exception.
 * `getFee()`: returns the estimated fee amount to be paid, based on the size of the transaction, but disregarding the priority of the outputs.
 
-Internally, a `_changeOutput` property stores the index of the change output (so it can get updated when a new input or output is added).
+Internally, a `_changeIndex` property stores the index of the change output (so it can get updated when a new input or output is added).
 
 ## Multisig Transactions
 

lib/transaction/input/input.js

@@ -40,11 +40,10 @@ Input.prototype._fromObject = function(params) {
   this.prevTxId = params.prevTxId;
   this.outputIndex = params.outputIndex;
   this.sequenceNumber = params.sequenceNumber;
-  if (!_.isUndefined(params.script) || !_.isUndefined(params.scriptBuffer)) {
-    this.setScript(_.isUndefined(params.script) ? params.scriptBuffer : params.script);
-  } else {
+  if (_.isUndefined(params.script) && _.isUndefined(params.scriptBuffer)) {
     throw new errors.Transaction.Input.MissingScript();
   }
+  this.setScript(params.scriptBuffer || params.script);
   return this;
 };
 

lib/transaction/transaction.js

@@ -126,13 +126,12 @@ Transaction.prototype.uncheckedSerialize = function() {
 
 Transaction.prototype.checkedSerialize = Transaction.prototype.toString = function() {
   var feeError = this._validateFees();
-  if (feeError) {
-    var changeError = this._validateChange();
-    if (changeError) {
+  var missingChange = this._missingChange();
+  if (feeError && missingChange) {
     throw new errors.Transaction.ChangeAddressMissing();
-    } else {
-      throw new errors.Transaction.FeeError(feeError);
   }
+  if (feeError && !missingChange) {
+    throw new errors.Transaction.FeeError(feeError);
   }
   if (this._hasDustOutputs()) {
     throw new errors.Transaction.DustOutputs();
@@ -148,10 +147,8 @@ Transaction.prototype._validateFees = function() {
   }
 };
 
-Transaction.prototype._validateChange = function() {
-  if (!this._change) {
-    return 'Missing change address';
-  }
+Transaction.prototype._missingChange = function() {
+  return !this._changeScript;
 };
 
 Transaction.DUST_AMOUNT = 5460;
@@ -217,23 +214,7 @@ Transaction.prototype.fromJSON = function(json) {
   if (JSUtil.isValidJSON(json)) {
     json = JSON.parse(json);
   }
-  var self = this;
-  this.inputs = [];
-  var inputs = json.inputs || json.txins;
-  inputs.forEach(function(input) {
-    self.inputs.push(Input.fromJSON(input));
-  });
-  this.outputs = [];
-  var outputs = json.outputs || json.txouts;
-  outputs.forEach(function(output) {
-    self.outputs.push(Output.fromJSON(output));
-  });
-  if (json.change) {
-    this.change(json.change);
-  }
-  this.version = json.version;
-  this.nLockTime = json.nLockTime;
-  return this;
+  return this.fromObject(json);
 };
 
 Transaction.prototype.toObject = function toObject() {
@@ -246,7 +227,8 @@ Transaction.prototype.toObject = function toObject() {
     outputs.push(output.toObject());
   });
   return {
-    change: this._change ? this._change.toString() : undefined,
+    changeScript: this._changeScript ? this._changeScript.toString() : undefined,
+    changeIndex: !_.isUndefined(this._changeIndex) ? this._changeIndex : undefined,
     fee: this._fee ? this._fee : undefined,
     version: this.version,
     inputs: inputs,
@@ -275,14 +257,29 @@ Transaction.prototype.fromObject = function(transaction) {
   _.each(transaction.outputs, function(output) {
     self.addOutput(new Output(output));
   });
-  if (transaction.change) {
-    this.change(transaction.change);
+  if (transaction.changeIndex) {
+    this._changeIndex = transaction.changeIndex;
+  }
+  if (transaction.changeScript) {
+    this._changeScript = new Script(transaction.changeScript);
   }
   if (transaction.fee) {
     this.fee(transaction.fee);
   }
   this.nLockTime = transaction.nLockTime;
   this.version = transaction.version;
+  this._checkConsistency();
+  return this;
+};
+
+Transaction.prototype._checkConsistency = function() {
+  if (!_.isUndefined(this._changeIndex)) {
+    $.checkState(this._changeScript);
+    $.checkState(this.outputs[this._changeIndex]);
+    $.checkState(this.outputs[this._changeIndex].script.toString() ===
+      this._changeScript.toString());
+  }
+  // TODO: add other checks
 };
 
 /**
@@ -533,11 +530,22 @@ Transaction.prototype.fee = function(amount) {
  * @return {Transaction} this, for chaining
  */
 Transaction.prototype.change = function(address) {
-  this._change = new Address(address);
+  this._changeScript = Script.fromAddress(address);
   this._updateChangeOutput();
   return this;
 };
 
+
+/**
+ * @return {Output} change output, if it exists
+ */
+Transaction.prototype.getChangeOutput = function() {
+  if (!_.isUndefined(this._changeIndex)) {
+    return this.outputs[this._changeIndex];
+  }
+  return null;
+};
+
 /**
  * Add an output to the transaction.
  *
@@ -586,33 +594,46 @@ Transaction.prototype._addOutput = function(output) {
 };
 
 Transaction.prototype._updateChangeOutput = function() {
-  if (!this._change) {
+  if (!this._changeScript) {
     return;
   }
   this._clearSignatures();
-  if (!_.isUndefined(this._changeOutput)) {
-    this._removeOutput(this._changeOutput);
+  if (!_.isUndefined(this._changeIndex)) {
+    this._removeOutput(this._changeIndex);
   }
   var available = this._getUnspentValue();
   var fee = this.getFee();
-  if (available - fee > 0) {
-    this._changeOutput = this.outputs.length;
+  var changeAmount = available - fee;
+  if (changeAmount > 0) {
+    this._changeIndex = this.outputs.length;
     this._addOutput(new Output({
-      script: Script.fromAddress(this._change),
-      satoshis: available - fee
+      script: this._changeScript,
+      satoshis: changeAmount
     }));
   } else {
-    this._changeOutput = undefined;
+    this._changeIndex = undefined;
   }
 };
-
+/**
+ * Calculates the fees for the transaction.
+ *
+ * If there is no change output set, the fee will be the
+ * output amount minus the input amount.
+ * If there's a fixed fee set, return that
+ * If there's no fee set, estimate it based on size
+ * @return {Number} miner fee for this transaction in satoshis
+ */
 Transaction.prototype.getFee = function() {
-  if (!this._change) {
+  // if no change output is set, fees should equal all the unspent amount
+  if (!this._changeScript) {
     return this._getUnspentValue();
   }
   return this._fee || this._estimateFee();
 };
 
+/**
+ * Estimates fee from serialized transaction size in bytes.
+ */
 Transaction.prototype._estimateFee = function() {
   var estimatedSize = this._estimateSize();
   var available = this._getUnspentValue();
@@ -630,12 +651,12 @@ Transaction.prototype._clearSignatures = function() {
 };
 
 Transaction.FEE_PER_KB = 10000;
+// Safe upper bound for change address script
 Transaction.CHANGE_OUTPUT_MAX_SIZE = 20 + 4 + 34 + 4;
 
 Transaction._estimateFee = function(size, amountAvailable) {
   var fee = Math.ceil(size / Transaction.FEE_PER_KB);
   if (amountAvailable > fee) {
-    // Safe upper bound for change address script
     size += Transaction.CHANGE_OUTPUT_MAX_SIZE;
   }
   return Math.ceil(size / 1000) * Transaction.FEE_PER_KB;

test/transaction/transaction.js

@@ -142,7 +142,16 @@ describe('Transaction', function() {
       tx.inputs.length.should.equal(1);
     });
 
-    describe('not enough information errors', function() {
+    describe('isFullySigned', function() {
+      it('works for normal p2pkh', function() {
+        var transaction = new Transaction()
+          .from(simpleUtxoWith100000Satoshis)
+          .to(toAddress, 50000)
+          .change(changeAddress)
+          .sign(privateKey);
+        transaction.isFullySigned().should.equal(true);
+      });
+
       it('fails when Inputs are not subclassed and isFullySigned is called', function() {
         var tx = new Transaction(tx_1_hex);
         expect(function() {
@@ -172,6 +181,7 @@ describe('Transaction', function() {
       transaction.outputs[1].satoshis.should.equal(40000);
       transaction.outputs[1].script.toString()
         .should.equal(Script.fromAddress(changeAddress).toString());
+      transaction.getChangeOutput().script.should.deep.equal(Script.fromAddress(changeAddress));
     });
     it('accepts a P2SH address for change', function() {
       var transaction = new Transaction()
@@ -243,7 +253,8 @@ describe('Transaction', function() {
         .change(changeAddress)
         .toObject();
       var deserialized = new Transaction(serialized);
-      expect(deserialized._change.toString()).to.equal(changeAddress);
+      expect(deserialized._changeScript.toString()).to.equal(Script.fromAddress(changeAddress).toString());
+      expect(deserialized.getChangeOutput()).to.equal(null);
     });
     it('can avoid checked serialize', function() {
       var transaction = new Transaction()
@@ -309,13 +320,23 @@ describe('Transaction', function() {
 
   describe('to and from JSON', function() {
     it('takes a string that is a valid JSON and deserializes from it', function() {
-      var transaction = new Transaction();
-      expect(new Transaction(transaction.toJSON()).serialize()).to.equal(transaction.serialize());
+      var simple = new Transaction();
+      expect(new Transaction(simple.toJSON()).serialize()).to.equal(simple.serialize());
+      var complex = new Transaction()
+        .from(simpleUtxoWith100000Satoshis)
+        .to(toAddress, 50000)
+        .change(changeAddress)
+        .sign(privateKey);
+      var cj = complex.toJSON();
+      var ctx = new Transaction(cj);
+      expect(ctx.serialize()).to.equal(complex.serialize());
+
     });
     it('serializes the `change` information', function() {
       var transaction = new Transaction();
       transaction.change(changeAddress);
-      expect(JSON.parse(transaction.toJSON()).change).to.equal(changeAddress.toString());
+      expect(JSON.parse(transaction.toJSON()).changeScript).to.equal(Script.fromAddress(changeAddress).toString());
+      expect(new Transaction(transaction.toJSON()).serialize()).to.equal(transaction.serialize());
     });
     it('serializes correctly p2sh multisig signed tx', function() {
       var t = new Transaction(tx2hex);
@@ -413,6 +434,10 @@ describe('Transaction', function() {
     var timestamp = 1423504946;
     var blockHeight = 342734;
     var date = new Date(timestamp * MILLIS_IN_SECOND);
+    it('handles a null locktime', function() {
+      var transaction = new Transaction();
+      expect(transaction.getLockTime()).to.equal(null);
+    });
     it('handles a simple example', function() {
       var future = new Date(2025, 10, 30); // Sun Nov 30 2025
       var transaction = new Transaction()