Merge pull request #453 from isocolsky/ref/proposal-flow

Fix & improve new proposal flow

lib/common/defaults.js

@@ -5,7 +5,8 @@ var Defaults = {};
 Defaults.DEFAULT_FEE_PER_KB = 10000;
 Defaults.MIN_FEE_PER_KB = 0;
 Defaults.MAX_FEE_PER_KB = 1000000;
-Defaults.MAX_TX_FEE = 1 * 1e8;
+Defaults.MIN_TX_FEE = 0;
+Defaults.MAX_TX_FEE = 0.1 * 1e8;
 
 Defaults.MAX_KEYS = 100;
 

lib/model/txproposal.js

@@ -44,7 +44,7 @@ TxProposal.create = function(opts) {
   x.requiredRejections = Math.min(x.walletM, x.walletN - x.walletM + 1),
   x.status = 'temporary';
   x.actions = [];
-  x.fee = null;
+  x.fee = opts.fee;
   x.feePerKb = opts.feePerKb;
   x.excludeUnconfirmedUtxos = opts.excludeUnconfirmedUtxos;
 
@@ -74,6 +74,7 @@ TxProposal.fromObj = function(obj) {
   x.id = obj.id;
   x.walletId = obj.walletId;
   x.creatorId = obj.creatorId;
+  x.network = obj.network;
   x.outputs = obj.outputs;
   x.amount = obj.amount;
   x.message = obj.message;
@@ -93,7 +94,6 @@ TxProposal.fromObj = function(obj) {
   });
   x.outputOrder = obj.outputOrder;
   x.fee = obj.fee;
-  x.network = obj.network;
   x.feePerKb = obj.feePerKb;
   x.excludeUnconfirmedUtxos = obj.excludeUnconfirmedUtxos;
   x.addressType = obj.addressType;
@@ -209,6 +209,10 @@ TxProposal.prototype.getBitcoreTx = function() {
   return t;
 };
 
+TxProposal.prototype.getNetworkName = function() {
+  return this.network;
+};
+
 TxProposal.prototype.getRawTx = function() {
   var t = this.getBitcoreTx();
 
@@ -232,6 +236,7 @@ TxProposal.prototype.getEstimatedSize = function() {
 };
 
 TxProposal.prototype.estimateFee = function() {
+  $.checkState(_.isNumber(this.feePerKb));
   var fee = this.feePerKb * this.getEstimatedSize() / 1000;
   this.fee = parseInt(fee.toFixed(0));
 };

lib/server.js

@@ -1,4 +1,5 @@
 'use strict';
+
 var _ = require('lodash');
 var $ = require('preconditions').singleton();
 var async = require('async');
@@ -1219,8 +1220,11 @@ WalletService.prototype._checkTxAndEstimateFee = function(txp) {
     serializationOpts.disableLargeFees = true;
   }
 
-  try {
+  if (_.isNumber(txp.feePerKb)) {
     txp.estimateFee();
+  }
+
+  try {
     var bitcoreTx = txp.getBitcoreTx();
     bitcoreError = bitcoreTx.getSerializationError(serializationOpts);
     if (!bitcoreError) {
@@ -1535,7 +1539,8 @@ WalletService.prototype.createTxLegacy = function(opts, cb) {
  * @param {string} opts.outputs[].message - A message to attach to this output.
  * @param {string} opts.message - A message to attach to this transaction.
  * @param {Array} opts.inputs - Optional. Inputs for this TX
- * @param {string} opts.feePerKb - Optional. Use an alternative fee per KB for this TX
+ * @param {string} opts.fee - Optional. Use an alternative fee for this TX (mutually exclusive with feePerKb)
+ * @param {string} opts.feePerKb - Optional. Use an alternative fee per KB for this TX (mutually exclusive with fee)
  * @param {string} opts.payProUrl - Optional. Paypro URL for peers to verify TX
  * @param {string} opts.excludeUnconfirmedUtxos[=false] - Optional. Do not use UTXOs of unconfirmed transactions as inputs
  * @param {string} opts.validateOutputs[=true] - Optional. Perform validation on outputs.
@@ -1547,9 +1552,16 @@ WalletService.prototype.createTx = function(opts, cb) {
   if (!Utils.checkRequired(opts, ['outputs']))
     return cb(new ClientError('Required argument missing'));
 
-  var feePerKb = opts.feePerKb || Defaults.DEFAULT_FEE_PER_KB;
+  if (_.isNumber(opts.fee)) {
-  if (feePerKb < Defaults.MIN_FEE_PER_KB || feePerKb > Defaults.MAX_FEE_PER_KB)
+    opts.feePerKb = null;
-    return cb(new ClientError('Invalid fee per KB value'));
+    if (opts.fee < Defaults.MIN_TX_FEE || opts.fee > Defaults.MAX_TX_FEE)
+      return cb(new ClientError('Invalid fee'));
+  } else {
+    opts.fee = null;
+    opts.feePerKb = opts.feePerKb || Defaults.DEFAULT_FEE_PER_KB;
+    if (opts.feePerKb < Defaults.MIN_FEE_PER_KB || opts.feePerKb > Defaults.MAX_FEE_PER_KB)
+      return cb(new ClientError('Invalid fee per KB'));
+  }
 
   self._runLocked(cb, function(cb) {
     self.getWallet({}, function(err, wallet) {
@@ -1574,7 +1586,8 @@ WalletService.prototype.createTx = function(opts, cb) {
           outputs: opts.outputs,
           message: opts.message,
           changeAddress: wallet.createAddress(true),
-          feePerKb: feePerKb,
+          fee: opts.fee,
+          feePerKb: opts.feePerKb,
           payProUrl: opts.payProUrl,
           walletM: wallet.m,
           walletN: wallet.n,
@@ -1594,10 +1607,6 @@ WalletService.prototype.createTx = function(opts, cb) {
 
             self.storage.storeTx(wallet.id, txp, function(err) {
               if (err) return cb(err);
-
-              self._notify('NewTxProposal', {
-                amount: txp.getTotalAmount()
-              }, function() {
               return cb(null, txp);
             });
           });
@@ -1605,7 +1614,6 @@ WalletService.prototype.createTx = function(opts, cb) {
       });
     });
   });
-  });
 };
 
 WalletService.prototype._verifyRequestPubKey = function(requestPubKey, signature, xPubKey) {
@@ -1618,8 +1626,6 @@ WalletService.prototype._verifyRequestPubKey = function(requestPubKey, signature
  * @param {Object} opts
  * @param {string} opts.txProposalId - The tx id.
  * @param {string} opts.proposalSignature - S(raw tx). Used by other copayers to verify the proposal.
- * @param {string} opts.proposalSignaturePubKey - (Optional) An alternative public key used to verify the proposal signature.
- * @param {string} opts.proposalSignaturePubKeySig - (Optional) A signature used to validate the opts.proposalSignaturePubKey.
  */
 WalletService.prototype.publishTx = function(opts, cb) {
   var self = this;
@@ -1648,13 +1654,11 @@ WalletService.prototype.publishTx = function(opts, cb) {
           return cb(new ClientError('Invalid proposal signature'));
         }
 
-        // Verify signingKey
+        // Save signature info for other copayers to check
-        if (opts.proposalSignaturePubKey) {
+        txp.proposalSignature = opts.proposalSignature;
-          if (opts.proposalSignaturePubKey != signingKey ||
+        if (signingKey.selfSigned) {
-            !self._verifyRequestPubKey(opts.proposalSignaturePubKey, opts.proposalSignaturePubKeySig, copayer.xPubKey)
+          txp.proposalSignaturePubKey = signingKey.key;
-          ) {
+          txp.proposalSignaturePubKeySig = signingKey.signature;
-            return cb(new ClientError('Invalid proposal signing key'));
-          }
         }
 
         // Verify UTXOs are still available
@@ -1672,7 +1676,12 @@ WalletService.prototype.publishTx = function(opts, cb) {
           txp.status = 'pending';
           self.storage.storeTx(self.walletId, txp, function(err) {
             if (err) return cb(err);
-            return cb();
+
+            self._notify('NewTxProposal', {
+              amount: txp.getTotalAmount()
+            }, function() {
+              return cb(null, txp);
+            });
           });
         });
       });

test/integration/helpers.js

@@ -400,28 +400,6 @@ helpers.createExternalProposalOpts = function(toAddress, amount, signingKey, mor
 };
 
 
-helpers.createProposalOpts2 = function(outputs, moreOpts, inputs) {
-  _.each(outputs, function(output) {
-    output.amount = helpers.toSatoshi(output.amount);
-  });
-
-  var opts = {
-    outputs: outputs,
-    inputs: inputs || [],
-  };
-
-  if (moreOpts) {
-    moreOpts = _.pick(moreOpts, ['feePerKb', 'customData', 'message']);
-    opts = _.assign(opts, moreOpts);
-  }
-
-  opts = _.defaults(opts, {
-    message: null
-  });
-
-  return opts;
-};
-
 helpers.getProposalSignatureOpts = function(txp, signingKey) {
   var raw = txp.getRawTx();
   var proposalSignature = helpers.signMessage(raw, signingKey);
@@ -490,4 +468,15 @@ helpers.createAddresses = function(server, wallet, main, change, cb) {
   });
 };
 
+helpers.createAndPublishTx = function(server, txOpts, signingKey, cb) {
+  server.createTx(txOpts, function(err, txp) {
+    should.not.exist(err);
+    var publishOpts = helpers.getProposalSignatureOpts(txp, signingKey);
+    server.publishTx(publishOpts, function(err) {
+      should.not.exist(err);
+      return cb(txp);
+    });
+  });
+};
+
 module.exports = helpers;

test/integration/server.js

@@ -2693,13 +2693,14 @@ describe('Wallet service', function() {
 
       it('should create a tx', function(done) {
         helpers.stubUtxos(server, wallet, [1, 2], function() {
-          var txOpts = helpers.createProposalOpts2([{
+          var txOpts = {
+            outputs: [{
               toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-            amount: 0.8
+              amount: 0.8 * 1e8,
-          }], {
+            }],
             message: 'some message',
             customData: 'some custom data',
-          });
+          };
           server.createTx(txOpts, function(err, tx) {
             should.not.exist(err);
             should.exist(tx);
@@ -2721,15 +2722,53 @@ describe('Wallet service', function() {
         });
       });
 
-      it('should be able to send a temporary tx proposal', function(done) {
+      it('should be able to specify the final fee', function(done) {
         helpers.stubUtxos(server, wallet, [1, 2], function() {
-          var txOpts = helpers.createProposalOpts2([{
+          var txOpts = {
+            outputs: [{
               toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-            amount: 0.8
+              amount: 0.8 * 1e8,
-          }], {
+            }],
+            fee: 123400,
+          };
+          server.createTx(txOpts, function(err, tx) {
+            should.not.exist(err);
+            should.exist(tx);
+            tx.fee.should.equal(123400);
+            var t = tx.getBitcoreTx();
+            t.getFee().should.equal(123400);
+            done();
+          });
+        });
+      });
+
+      it('should check explicit fee to be below max', function(done) {
+        helpers.stubUtxos(server, wallet, [1, 2], function() {
+          var txOpts = {
+            outputs: [{
+              toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+              amount: 0.8 * 1e8,
+            }],
+            fee: 1e8,
+          };
+          server.createTx(txOpts, function(err, tx) {
+            should.exist(err);
+            err.message.should.contain('fee');
+            done();
+          });
+        });
+      });
+
+      it('should be able to publish a temporary tx proposal', function(done) {
+        helpers.stubUtxos(server, wallet, [1, 2], function() {
+          var txOpts = {
+            outputs: [{
+              toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+              amount: 0.8 * 1e8,
+            }],
             message: 'some message',
             customData: 'some custom data',
-          });
+          };
           server.createTx(txOpts, function(err, txp) {
             should.not.exist(err);
             should.exist(txp);
@@ -2739,6 +2778,7 @@ describe('Wallet service', function() {
               server.getPendingTxs({}, function(err, txs) {
                 should.not.exist(err);
                 txs.length.should.equal(1);
+                should.exist(txs[0].proposalSignature);
                 done();
               });
             });
@@ -2746,7 +2786,36 @@ describe('Wallet service', function() {
         });
       });
 
-      it('should fail to send non-existent tx proposal', function(done) {
+      it('should delay NewTxProposal notification until published', function(done) {
+        helpers.stubUtxos(server, wallet, [1, 2], function() {
+          var txOpts = {
+            outputs: [{
+              toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+              amount: 0.8 * 1e8,
+            }],
+            message: 'some message',
+          };
+          server.createTx(txOpts, function(err, txp) {
+            should.not.exist(err);
+            should.exist(txp);
+            server.getNotifications({}, function(err, notifications) {
+              should.not.exist(err);
+              _.pluck(notifications, 'type').should.not.contain('NewTxProposal');
+              var publishOpts = helpers.getProposalSignatureOpts(txp, TestData.copayers[0].privKey_1H_0);
+              server.publishTx(publishOpts, function(err) {
+                should.not.exist(err);
+                server.getNotifications({}, function(err, notifications) {
+                  should.not.exist(err);
+                  _.pluck(notifications, 'type').should.contain('NewTxProposal');
+                  done();
+                });
+              });
+            });
+          });
+        });
+      });
+
+      it('should fail to publish non-existent tx proposal', function(done) {
         server.publishTx({
           txProposalId: 'wrong-id',
           proposalSignature: 'dummy',
@@ -2760,14 +2829,15 @@ describe('Wallet service', function() {
         });
       });
 
-      it('should fail to send tx proposal with wrong signature', function(done) {
+      it('should fail to publish tx proposal with wrong signature', function(done) {
         helpers.stubUtxos(server, wallet, [1, 2], function() {
-          var txOpts = helpers.createProposalOpts2([{
+          var txOpts = {
+            outputs: [{
               toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-            amount: 0.8
+              amount: 0.8 * 1e8,
-          }], {
+            }],
             message: 'some message',
-          });
+          };
           server.createTx(txOpts, function(err, txp) {
             should.not.exist(err);
             should.exist(txp);
@@ -2783,33 +2853,27 @@ describe('Wallet service', function() {
         });
       });
 
-      it('should fail to send tx proposal not signed by the creator', function(done) {
+      it('should fail to publish tx proposal not signed by the creator', function(done) {
         helpers.stubUtxos(server, wallet, [1, 2], function() {
-          var txOpts = helpers.createProposalOpts2([{
+          var txOpts = {
+            outputs: [{
               toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-            amount: 0.8
+              amount: 0.8 * 1e8,
-          }], {
+            }],
             message: 'some message',
-          });
+          };
           server.createTx(txOpts, function(err, txp) {
             should.not.exist(err);
             should.exist(txp);
 
-            var raw = txp.getRawTx();
-            var proposalSignature = helpers.signMessage(raw, TestData.copayers[0].privKey_1H_0);
-            var pubKey = new Bitcore.PrivateKey(TestData.copayers[0].privKey_1H_0).toPublicKey().toString();
-            var pubKeySig = helpers.signMessage(pubKey, TestData.copayers[1].privKey_1H_0);
-
             var publishOpts = {
               txProposalId: txp.id,
-              proposalSignature: proposalSignature,
+              proposalSignature: helpers.signMessage(txp.getRawTx(), TestData.copayers[1].privKey_1H_0),
-              proposalSignaturePubKey: pubKey,
-              proposalSignaturePubKeySig: pubKeySig,
             }
 
             server.publishTx(publishOpts, function(err) {
               should.exist(err);
-              err.message.should.contain('Invalid proposal signing key');
+              err.message.should.contain('Invalid proposal signature');
               done();
             });
           });
@@ -2818,27 +2882,27 @@ describe('Wallet service', function() {
 
       it('should accept a tx proposal signed with a custom key', function(done) {
         var reqPrivKey = new Bitcore.PrivateKey();
-        var reqPubKey = reqPrivKey.toPublicKey();
+        var reqPubKey = reqPrivKey.toPublicKey().toString();
 
         var xPrivKey = TestData.copayers[0].xPrivKey_44H_0H_0H;
-        var sig = helpers.signRequestPubKey(reqPubKey, xPrivKey);
 
-        var opts = {
+        var accessOpts = {
           copayerId: TestData.copayers[0].id44,
           requestPubKey: reqPubKey,
-          signature: sig,
+          signature: helpers.signRequestPubKey(reqPubKey, xPrivKey),
         };
 
-        server.addAccess(opts, function(err) {
+        server.addAccess(accessOpts, function(err) {
           should.not.exist(err);
 
           helpers.stubUtxos(server, wallet, [1, 2], function() {
-            var txOpts = helpers.createProposalOpts2([{
+            var txOpts = {
+              outputs: [{
                 toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-              amount: 0.8
+                amount: 0.8 * 1e8,
-            }], {
+              }],
               message: 'some message',
-            });
+            };
             server.createTx(txOpts, function(err, txp) {
               should.not.exist(err);
               should.exist(txp);
@@ -2846,29 +2910,35 @@ describe('Wallet service', function() {
               var publishOpts = {
                 txProposalId: txp.id,
                 proposalSignature: helpers.signMessage(txp.getRawTx(), reqPrivKey),
-                proposalSignaturePubKey: reqPubKey,
-                proposalSignaturePubKeySig: sig,
               }
 
               server.publishTx(publishOpts, function(err) {
-                should.exist(err);
+                should.not.exist(err);
-                err.message.should.contain('Invalid proposal signing key');
+                server.getTx({
+                  txProposalId: txp.id
+                }, function(err, x) {
+                  should.not.exist(err);
+                  x.proposalSignature.should.equal(publishOpts.proposalSignature);
+                  x.proposalSignaturePubKey.should.equal(accessOpts.requestPubKey);
+                  x.proposalSignaturePubKeySig.should.equal(accessOpts.signature);
                   done();
                 });
               });
             });
           });
         });
-
-      it('should fail to send a temporary tx proposal if utxos are unavailable', function(done) {
-        var txp1, txp2;
-        var txOpts = helpers.createProposalOpts2([{
-          toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-          amount: 0.8
-        }], {
-          message: 'some message',
       });
 
+      it('should fail to publish a temporary tx proposal if utxos are unavailable', function(done) {
+        var txp1, txp2;
+        var txOpts = {
+          outputs: [{
+            toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+            amount: 0.8 * 1e8,
+          }],
+          message: 'some message',
+        };
+
         async.waterfall([
 
           function(next) {
@@ -2890,7 +2960,7 @@ describe('Wallet service', function() {
             var publishOpts = helpers.getProposalSignatureOpts(txp1, TestData.copayers[0].privKey_1H_0);
             server.publishTx(publishOpts, next);
           },
-          function(next) {
+          function(txp, next) {
             var publishOpts = helpers.getProposalSignatureOpts(txp2, TestData.copayers[0].privKey_1H_0);
             server.publishTx(publishOpts, function(err) {
               should.exist(err);
@@ -2914,7 +2984,7 @@ describe('Wallet service', function() {
             var publishOpts = helpers.getProposalSignatureOpts(txp3, TestData.copayers[0].privKey_1H_0);
             server.publishTx(publishOpts, next);
           },
-          function(next) {
+          function(txp, next) {
             server.getPendingTxs({}, function(err, txs) {
               should.not.exist(err);
               txs.length.should.equal(2);
@@ -2929,13 +2999,14 @@ describe('Wallet service', function() {
 
       it('should fail to list pending proposals from legacy client', function(done) {
         helpers.stubUtxos(server, wallet, [1, 2], function() {
-          var txOpts = helpers.createProposalOpts2([{
+          var txOpts = {
+            outputs: [{
               toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
-            amount: 0.8
+              amount: 0.8 * 1e8,
-          }], {
+            }],
             message: 'some message',
             customData: 'some custom data',
-          });
+          };
           server.createTx(txOpts, function(err, txp) {
             should.not.exist(err);
             should.exist(txp);
@@ -3387,6 +3458,8 @@ describe('Wallet service', function() {
 
   describe('#broadcastTx & #broadcastRawTx', function() {
     var server, wallet, txpid, txid;
+    describe('Legacy', function() {
+
       beforeEach(function(done) {
         helpers.createAndJoinWallet(1, 1, function(s, w) {
           server = s;
@@ -3505,10 +3578,6 @@ describe('Wallet service', function() {
         });
       });
 
-
-
-
-
       it('should fail to brodcast a not yet accepted tx', function(done) {
         helpers.stubBroadcast();
         var txOpts = helpers.createSimpleProposalOpts('18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7', 9, TestData.copayers[0].privKey_1H_0, {
@@ -3591,6 +3660,63 @@ describe('Wallet service', function() {
       });
     });
 
+    describe('New', function() {
+      beforeEach(function(done) {
+        helpers.createAndJoinWallet(1, 1, function(s, w) {
+          server = s;
+          wallet = w;
+          helpers.stubUtxos(server, wallet, [10, 10], function() {
+            var txOpts = {
+              outputs: [{
+                toAddress: '18PzpUFkFZE8zKWUPvfykkTxmB9oMR8qP7',
+                amount: 9e8,
+              }],
+              message: 'some message',
+            };
+            helpers.createAndPublishTx(server, txOpts, TestData.copayers[0].privKey_1H_0, function(txp) {
+              should.exist(txp);
+              var signatures = helpers.clientSign(txp, TestData.copayers[0].xPrivKey_44H_0H_0H);
+              server.signTx({
+                txProposalId: txp.id,
+                signatures: signatures,
+              }, function(err, txp) {
+                should.not.exist(err);
+                should.exist(txp);
+                txp.isAccepted().should.be.true;
+                txp.isBroadcasted().should.be.false;
+                txid = txp.txid;
+                txpid = txp.id;
+                done();
+              });
+            });
+          });
+        });
+      });
+
+      it('should broadcast a tx', function(done) {
+        var clock = sinon.useFakeTimers(1234000, 'Date');
+        helpers.stubBroadcast();
+        server.broadcastTx({
+          txProposalId: txpid
+        }, function(err) {
+          should.not.exist(err);
+          server.getTx({
+            txProposalId: txpid
+          }, function(err, txp) {
+            should.not.exist(err);
+            should.not.exist(txp.raw);
+            txp.txid.should.equal(txid);
+            txp.isBroadcasted().should.be.true;
+            txp.broadcastedOn.should.equal(1234);
+            clock.restore();
+            done();
+          });
+        });
+      });
+
+    });
+  });
+
   describe('Tx proposal workflow', function() {
     var server, wallet;
     beforeEach(function(done) {