Wallet: refactor to use Array unspents solely, deprecating unspentsMap

src/wallet.js

@@ -24,9 +24,12 @@ function Wallet(seed, network) {
   this.addresses = []
   this.changeAddresses = []
   this.network = network
+  this.unspents = []
+
+  // FIXME: remove in 2.0.0
   this.unspentMap = {}
 
-  // FIXME: remove in 2.x.y
+  // FIXME: remove in 2.0.0
   var me = this
   this.newMasterKey = function(seed) {
     console.warn('newMasterKey is deprecated, please make a new Wallet instance instead')
@@ -41,6 +44,7 @@ function Wallet(seed, network) {
     me.addresses = []
     me.changeAddresses = []
 
+    me.unspents = []
     me.unspentMap = {}
   }
 
@@ -50,27 +54,54 @@ function Wallet(seed, network) {
   this.getInternalAccount = function() { return internalAccount }
 }
 
-Wallet.prototype.createTx = function(to, value, fixedFee, changeAddress) {
+Wallet.prototype.createTransaction = function(to, value, options) {
+  // FIXME: remove in 2.0.0
+  if (typeof options !== 'object') {
+    if (options !== undefined) {
+      console.warn('Non options object parameters are deprecated, use options object instead')
+
+      options = {
+        fixedFee: arguments[2],
+        changeAddress: arguments[3]
+      }
+    }
+  }
+
+  options = options || {}
+
   assert(value > this.network.dustThreshold, value + ' must be above dust threshold (' + this.network.dustThreshold + ' Satoshis)')
 
-  var utxos = getCandidateOutputs(this.unspentMap, value)
+  var changeAddress = options.changeAddress
+  var fixedFee = options.fixedFee
+  var minConf = options.minConf === undefined ? 0 : options.minConf // FIXME: change minConf:1 by default in 2.0.0
+
+  // filter by minConf, then pending and sort by descending value
+  var unspents = this.unspents.filter(function(unspent) {
+    return unspent.confirmations >= minConf
+  }).filter(function(unspent) {
+    return !unspent.pending
+  }).sort(function(o1, o2) {
+    return o2.value - o1.value
+  })
+
   var accum = 0
-  var subTotal = value
   var addresses = []
+  var subTotal = value
 
   var txb = new TransactionBuilder()
   txb.addOutput(to, value)
 
-  for (var i = 0; i < utxos.length; ++i) {
+  for (var i = 0; i < unspents.length; ++i) {
-    var utxo = utxos[i]
+    var unspent = unspents[i]
-    addresses.push(utxo.address)
+    addresses.push(unspent.address)
 
-    txb.addInput(utxo.hash, utxo.index)
+    txb.addInput(unspent.txHash, unspent.index)
 
     var fee = fixedFee === undefined ? estimatePaddedFee(txb.buildIncomplete(), this.network) : fixedFee
 
-    accum += utxo.value
+    accum += unspent.value
     subTotal = value + fee
+
     if (accum >= subTotal) {
       var change = accum - subTotal
 
@@ -87,15 +118,20 @@ Wallet.prototype.createTx = function(to, value, fixedFee, changeAddress) {
   return this.signWith(txb, addresses).build()
 }
 
+// FIXME: remove in 2.0.0
 Wallet.prototype.processPendingTx = function(tx){
   this.__processTx(tx, true)
 }
 
+// FIXME: remove in 2.0.0
 Wallet.prototype.processConfirmedTx = function(tx){
   this.__processTx(tx, false)
 }
 
+// FIXME: remove in 2.0.0
 Wallet.prototype.__processTx = function(tx, isPending) {
+  console.warn('processTransaction is considered harmful, see issue #260 for more information')
+
   var txId = tx.getId()
   var txHash = tx.getHash()
 
@@ -110,31 +146,44 @@ Wallet.prototype.__processTx = function(tx, isPending) {
 
     var myAddresses = this.addresses.concat(this.changeAddresses)
     if (myAddresses.indexOf(address) > -1) {
-      var output = txId + ':' + i
+      var lookup = txId + ':' + i
+      if (lookup in this.unspentMap) return
 
-      this.unspentMap[output] = {
+      // its unique, add it
-        hash: txHash,
+      var unspent = {
-        index: i,
-        value: txOut.value,
         address: address,
+        confirmations: 0, // no way to determine this without more information
+        index: i,
+        txHash: txHash,
+        txId: txId,
+        value: txOut.value,
         pending: isPending
       }
+
+      this.unspentMap[lookup] = unspent
+      this.unspents.push(unspent)
     }
   }, this)
 
   tx.ins.forEach(function(txIn, i) {
     // copy and convert to big-endian hex
-    var txinId = bufferutils.reverse(txIn.hash).toString('hex')
+    var txInId = bufferutils.reverse(txIn.hash).toString('hex')
-    var output = txinId + ':' + txIn.index
 
-    if (!(output in this.unspentMap)) return
+    var lookup = txInId + ':' + txIn.index
+    if (!(lookup in this.unspentMap)) return
+
+    var unspent = this.unspentMap[lookup]
 
     if (isPending) {
-      this.unspentMap[output].pending = true
+      unspent.pending = true
-      this.unspentMap[output].spent = true
+      unspent.spent = true
 
     } else {
-      delete this.unspentMap[output]
+      delete this.unspentMap[lookup]
+
+      this.unspents = this.unspents.filter(function(unspent2) {
+        return unspent !== unspent2
+      })
     }
   }, this)
 }
@@ -157,9 +206,25 @@ Wallet.prototype.generateChangeAddress = function() {
   return this.getChangeAddress()
 }
 
-Wallet.prototype.getBalance = function() {
+Wallet.prototype.getAddress = function() {
-  return this.getUnspentOutputs().reduce(function(accum, output) {
+  if (this.addresses.length === 0) {
-    return accum + output.value
+    this.generateAddress()
+  }
+
+  return this.addresses[this.addresses.length - 1]
+}
+
+Wallet.prototype.getBalance = function(minConf) {
+  minConf = minConf || 0
+
+  return this.unspents.filter(function(unspent) {
+    return unspent.confirmations >= minConf
+
+  // FIXME: remove spent filter in 2.0.0
+  }).filter(function(unspent) {
+    return !unspent.spent
+  }).reduce(function(accum, unspent) {
+    return accum + unspent.value
   }, 0)
 }
 
@@ -193,65 +258,77 @@ Wallet.prototype.getPrivateKeyForAddress = function(address) {
   assert(false, 'Unknown address. Make sure the address is from the keychain and has been generated')
 }
 
-Wallet.prototype.getReceiveAddress = function() {
+Wallet.prototype.getUnspentOutputs = function(minConf) {
-  if (this.addresses.length === 0) {
+  minConf = minConf || 0
-    this.generateAddress()
-  }
 
-  return this.addresses[this.addresses.length - 1]
+  return this.unspents.filter(function(unspent) {
-}
+    return unspent.confirmations >= minConf
 
-Wallet.prototype.getUnspentOutputs = function() {
+  // FIXME: remove spent filter in 2.0.0
-  var utxos = []
+  }).filter(function(unspent) {
+    return !unspent.spent
+  }).map(function(unspent) {
+    return {
+      address: unspent.address,
+      confirmations: unspent.confirmations,
+      index: unspent.index,
+      txId: unspent.txId,
+      value: unspent.value,
 
-  for (var key in this.unspentMap) {
+      // FIXME: remove in 2.0.0
-    var output = this.unspentMap[key]
+      hash: unspent.txId,
-
+      pending: unspent.pending
-    // Don't include pending spent outputs
-    if (!output.spent) {
-      // hash is little-endian, we want big-endian
-      var txId = bufferutils.reverse(output.hash)
-
-      utxos.push({
-        hash: txId.toString('hex'),
-        index: output.index,
-        address: output.address,
-        value: output.value,
-        pending: output.pending
-      })
     }
-  }
+  })
-
-  return utxos
 }
 
-Wallet.prototype.setUnspentOutputs = function(utxos) {
+Wallet.prototype.setUnspentOutputs = function(unspents) {
-  utxos.forEach(function(utxo) {
+  this.unspentMap = {}
-    var txId = utxo.hash
+  this.unspents = unspents.map(function(unspent) {
-    assert.equal(typeof txId, 'string', 'Expected txId, got ' +  txId)
+    // FIXME: remove unspent.hash in 2.0.0
+    var txId = unspent.txId || unspent.hash
+    var index = unspent.index
 
-    var hash = bufferutils.reverse(new Buffer(txId, 'hex'))
+    // FIXME: remove in 2.0.0
-    var index = utxo.index
+    if (unspent.hash !== undefined) {
-    var address = utxo.address
+      console.warn('unspent.hash is deprecated, use unspent.txId instead')
-    var value = utxo.value
+    }
 
-    // FIXME: remove alternative in 2.x.y
+    // FIXME: remove in 2.0.0
-    if (index === undefined) index = utxo.outputIndex
+    if (index === undefined) {
+      console.warn('unspent.outputIndex is deprecated, use unspent.index instead')
+      index = utxo.outputIndex
+    }
 
-    assert.equal(hash.length, 32, 'Expected hash length of 32, got ' + hash.length)
+    assert.equal(typeof txId, 'string', 'Expected txId, got ' + txId)
+    assert.equal(txId.length, 64, 'Expected valid txId, got ' + txId)
+    assert.doesNotThrow(function() { Address.fromBase58Check(unspent.address) }, 'Expected Base58 Address, got ' + unspent.address)
     assert.equal(typeof index, 'number', 'Expected number index, got ' + index)
-    assert.doesNotThrow(function() { Address.fromBase58Check(address) }, 'Expected Base58 Address, got ' + address)
+    assert.equal(typeof unspent.value, 'number', 'Expected number value, got ' + unspent.value)
-    assert.equal(typeof value, 'number', 'Expected number value, got ' + value)
 
-    var output = txId + ':' + index
+    // FIXME: remove branch in 2.0.0
-
+    if (unspent.confirmations !== undefined) {
-    this.unspentMap[output] = {
+      assert.equal(typeof unspent.confirmations, 'number', 'Expected number confirmations, got ' + unspent.confirmations)
-      address: address,
-      hash: hash,
-      index: index,
-      pending: utxo.pending,
-      value: value
     }
+
+    var txHash = bufferutils.reverse(new Buffer(txId, 'hex'))
+
+    var unspent = {
+      address: unspent.address,
+      confirmations: unspent.confirmations || 0,
+      index: index,
+      txHash: txHash,
+      txId: txId,
+      value: unspent.value,
+
+      // FIXME: remove in 2.0.0
+      pending: unspent.pending || false
+    }
+
+    // FIXME: remove in 2.0.0
+    this.unspentMap[txId + ':' + index] = unspent
+
+    return unspent
   }, this)
 }
 
@@ -272,21 +349,8 @@ function estimatePaddedFee(tx, network) {
   return network.estimateFee(tmpTx)
 }
 
-function getCandidateOutputs(outputs/*, value*/) {
+// FIXME: 1.0.0 shims, remove in 2.0.0
-  var unspents = []
+Wallet.prototype.getReceiveAddress = Wallet.prototype.getAddress
-
+Wallet.prototype.createTx = Wallet.prototype.createTransaction
-  for (var key in outputs) {
-    var output = outputs[key]
-
-    if (!output.pending) {
-      unspents.push(output)
-    }
-  }
-
-  // sorted by descending value
-  return unspents.sort(function(o1, o2) {
-    return o2.value - o1.value
-  })
-}
 
 module.exports = Wallet

test/wallet.js

@@ -1,4 +1,5 @@
 var assert = require('assert')
+var bufferutils = require('../src/bufferutils')
 var crypto = require('../src/crypto')
 var networks = require('../src/networks')
 var sinon = require('sinon')
@@ -201,13 +202,12 @@ describe('Wallet', function() {
     beforeEach(function() {
       utxo = {
         "address" : "1AZpKpcfCzKDUeTFBQUL4MokQai3m3HMXv",
-        "hash": fakeTxId(6),
+        "confirmations": 1,
         "index": 0,
-        "pending": true,
+        "txId": fakeTxId(6),
-        "value": 20000
+        "value": 20000,
+        "pending": false
       }
-
-      expectedOutputKey = utxo.hash + ":" + utxo.index
     })
 
     describe('on construction', function() {
@@ -217,11 +217,10 @@ describe('Wallet', function() {
       })
 
       it('matches the expected behaviour', function() {
-        var output = wallet.unspentMap[expectedOutputKey]
+        var output = wallet.unspents[0]
 
-        assert(output)
-        assert.equal(output.value, utxo.value)
         assert.equal(output.address, utxo.address)
+        assert.equal(output.value, utxo.value)
       })
     })
 
@@ -245,20 +244,32 @@ describe('Wallet', function() {
         wallet.setUnspentOutputs([utxo])
       })
 
-      it('parses wallet outputs to the expected format', function() {
+      it('parses wallet unspents to the expected format', function() {
-        assert.deepEqual(wallet.getUnspentOutputs(), [utxo])
+        var outputs = wallet.getUnspentOutputs()
+        var output = outputs[0]
+
+        assert.equal(utxo.address, output.address)
+        assert.equal(utxo.index, output.index)
+        assert.equal(utxo.value, output.value)
+
+        // FIXME: remove in 2.0.0
+        assert.equal(utxo.txId, output.hash)
+        assert.equal(utxo.pending, output.pending)
+
+        // new in 2.0.0
+        assert.equal(utxo.txId, output.txId)
+        assert.equal(utxo.confirmations, output.confirmations)
       })
 
-      it("ignores pending spending outputs (outputs with 'spent' property)", function() {
+      it("ignores spent unspents (outputs with 'spent' property)", function() {
-        var output = wallet.unspentMap[expectedOutputKey]
+        var unspent = wallet.unspents[0]
-        output.pending = true
+        unspent.pending = true
-        output.spent = true
+        unspent.spent = true
         assert.deepEqual(wallet.getUnspentOutputs(), [])
       })
     })
   })
 
-  // FIXME: remove in 2.x.y
   describe('setUnspentOutputs', function() {
     var utxo
     var expectedOutputKey
@@ -271,16 +282,13 @@ describe('Wallet', function() {
         value: 500000
       }
 
-      expectedOutputKey = utxo.hash + ":" + utxo.index
-
       wallet = new Wallet(seed, networks.bitcoin)
     })
 
     it('matches the expected behaviour', function() {
       wallet.setUnspentOutputs([utxo])
 
-      var output = wallet.unspentMap[expectedOutputKey]
+      var output = wallet.unspents[0]
-      assert(output)
       assert.equal(output.value, utxo.value)
       assert.equal(output.address, utxo.address)
     })
@@ -340,13 +348,12 @@ describe('Wallet', function() {
           Array.prototype.reverse.call(txInId)
           txInId = txInId.toString('hex')
 
-          var key = txInId + ':' + txIn.index
+          var unspent = wallet.unspents[0]
-          var output = wallet.unspentMap[key]
+          assert(!unspent.pending)
 
-          assert(!output.pending)
           wallet.processPendingTx(spendTx)
-          assert(output.pending)
+          assert(unspent.pending)
-          assert(output.spent, true)
+          assert(unspent.spent, true)
         })
       })
     })
@@ -389,7 +396,7 @@ describe('Wallet', function() {
         }
       })
 
-      describe("when tx ins outpoint contains a known txhash:i", function() {
+      describe("when tx ins contains a known txhash:i", function() {
         var spendTx
         beforeEach(function() {
           wallet.addresses = [addresses[0]] // the address fixtureTx2 used as input
@@ -403,11 +410,9 @@ describe('Wallet', function() {
           assert.deepEqual(wallet.unspentMap, {})
         })
 
-        it("deletes corresponding 'output'", function() {
+        it("deletes corresponding 'unspent'", function() {
           var txIn = spendTx.ins[0]
-          var txInId = new Buffer(txIn.hash)
+          var txInId = bufferutils.reverse(txIn.hash).toString('hex')
-          Array.prototype.reverse.call(txInId)
-          txInId = txInId.toString('hex')
 
           var expected = txInId + ':' + txIn.index
           assert(expected in wallet.unspentMap)
@@ -416,19 +421,20 @@ describe('Wallet', function() {
           assert(!(expected in wallet.unspentMap))
         })
       })
-
-      it("does nothing when none of the involved addresses belong to the wallet", function() {
-        wallet.processConfirmedTx(tx)
-        assert.deepEqual(wallet.unspentMap, {})
-      })
     })
 
+    it("does nothing when none of the involved addresses belong to the wallet", function() {
+      wallet.processConfirmedTx(tx)
+      assert.deepEqual(wallet.unspentMap, {})
+    })
+
+
     function verifyOutputAdded(index, pending) {
       var txOut = tx.outs[index]
 
       var key = tx.getId() + ":" + index
       var output = wallet.unspentMap[key]
-      assert.deepEqual(output.hash, tx.getHash())
+      assert.deepEqual(output.txHash, tx.getHash())
       assert.equal(output.value, txOut.value)
       assert.equal(output.pending, pending)
 
@@ -512,13 +518,14 @@ describe('Wallet', function() {
       })
 
       function getFee(wallet, tx) {
-        var inputValue = tx.ins.reduce(function(memo, input){
+        var inputValue = tx.ins.reduce(function(accum, input) {
-          var id = Array.prototype.reverse.call(input.hash).toString('hex')
+          var txId = bufferutils.reverse(input.hash).toString('hex')
-          return memo + wallet.unspentMap[id + ':' + input.index].value
+
+          return accum + wallet.unspentMap[txId + ':' + input.index].value
         }, 0)
 
-        return tx.outs.reduce(function(memo, output){
+        return tx.outs.reduce(function(accum, output) {
-          return memo - output.value
+          return accum - output.value
         }, inputValue)
       }
     })