From 9f7729127ec26eacdc12615a0dc1712e762d0f52 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 13:10:19 +0500 Subject: [PATCH 1/9] added check for duplicate vote and updated corresponding tests --- logic/vote.js | 6 ++++++ test/api/delegates.js | 16 ++++++++-------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/logic/vote.js b/logic/vote.js index fbdcfd8d85b..2f504d9f99b 100644 --- a/logic/vote.js +++ b/logic/vote.js @@ -95,6 +95,12 @@ Vote.prototype.verify = function (trs, sender, cb) { return setImmediate(cb, 'Voting limit exceeded. Maximum is 33 votes per transaction'); } + for (var i = 0; i < trs.asset.votes.length; i++) { + if (trs.asset.votes.filter(function (v) { return v === trs.asset.votes[i]; }).length > 1) { + return setImmediate(cb, 'Duplicate votes are not allowed'); + } + } + async.eachSeries(trs.asset.votes, function (vote, eachSeriesCb) { self.verifyVote(vote, function (err) { if (err) { diff --git a/test/api/delegates.js b/test/api/delegates.js index 313e357230e..6580f33860f 100644 --- a/test/api/delegates.js +++ b/test/api/delegates.js @@ -102,37 +102,37 @@ describe('PUT /api/accounts/delegates with funds', function () { }); it('when upvoting same delegate multiple times should fail', function (done) { - var votedDelegate = '"+' + node.eAccount.publicKey + '","+' + node.eAccount.publicKey + '"'; + var votedDelegate = Array(2).fill('+' + node.eAccount.publicKey); putAccountsDelegates({ secret: account.password, - delegates: [votedDelegate] + delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.equal('Duplicate votes are not allowed'); done(); }); }); it('when downvoting same delegate multiple times should fail', function (done) { - var votedDelegate = '"+' + node.eAccount.publicKey + '","+' + node.eAccount.publicKey + '"'; + var votedDelegate = Array(2).fill('-' + node.eAccount.publicKey); putAccountsDelegates({ secret: account.password, - delegates: [votedDelegate] + delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.equal('Duplicate votes are not allowed'); done(); }); }); it('when upvoting and downvoting within same request should fail', function (done) { - var votedDelegate = '"+' + node.eAccount.publicKey + '","-' + node.eAccount.publicKey + '"'; + var votedDelegate = ['-' + node.eAccount.publicKey, '+' + node.eAccount.publicKey]; putAccountsDelegates({ secret: account.password, - delegates: [votedDelegate] + delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; node.expect(res.body).to.have.property('error'); From 876f2181f1b8130d6cbcef2242e9ea72674cb30d Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 13:36:43 +0500 Subject: [PATCH 2/9] refactored check for duplicate vote --- logic/vote.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/logic/vote.js b/logic/vote.js index 2f504d9f99b..49f4d3d159b 100644 --- a/logic/vote.js +++ b/logic/vote.js @@ -4,6 +4,7 @@ var async = require('async'); var constants = require('../helpers/constants.js'); var exceptions = require('../helpers/exceptions.js'); var Diff = require('../helpers/diff.js'); +var _ = require('lodash'); // Private fields var modules, library, self; @@ -95,10 +96,8 @@ Vote.prototype.verify = function (trs, sender, cb) { return setImmediate(cb, 'Voting limit exceeded. Maximum is 33 votes per transaction'); } - for (var i = 0; i < trs.asset.votes.length; i++) { - if (trs.asset.votes.filter(function (v) { return v === trs.asset.votes[i]; }).length > 1) { - return setImmediate(cb, 'Duplicate votes are not allowed'); - } + if (trs.asset.votes.length > _.uniq(trs.asset.votes).length) { + return setImmediate(cb, 'Duplicate votes are not allowed'); } async.eachSeries(trs.asset.votes, function (vote, eachSeriesCb) { From 64037276d50649d27a0075298094a1205e750a99 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 14:25:01 +0500 Subject: [PATCH 3/9] added specifc error message expectations for vote trs tests --- test/api/delegates.js | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/test/api/delegates.js b/test/api/delegates.js index 6580f33860f..074c45bee93 100644 --- a/test/api/delegates.js +++ b/test/api/delegates.js @@ -134,8 +134,13 @@ describe('PUT /api/accounts/delegates with funds', function () { secret: account.password, delegates: votedDelegate }, function (err, res) { + var expectedErrors = [ + 'Failed to remove vote, account has not voted for this delegate', + 'Failed to add vote, account has already voted for this delegate' + ]; node.expect(res.body).to.have.property('success').to.be.not.ok; node.expect(res.body).to.have.property('error'); + node.expect(expectedErrors).to.include(res.body.error); done(); }); }); @@ -200,7 +205,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: ['+' + node.eAccount.publicKey] }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.include('String is too short '); done(); }); }); @@ -211,7 +216,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: ['-' + node.eAccount.publicKey] }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.include('String is too short '); done(); }); }); @@ -222,7 +227,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: ['+'] }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.include('Invalid vote format'); done(); }); }); @@ -233,7 +238,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: ['-'] }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.include('Invalid vote format'); done(); }); }); @@ -244,7 +249,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: '' }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error'); + node.expect(res.body).to.have.property('error').to.equal('Invalid transaction asset'); done(); }); }); From 33879d0fe90034537a52775b9d46a337a7937e83 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 19:33:21 +0500 Subject: [PATCH 4/9] updated votes endpoint to check publicKey --- logic/vote.js | 4 ++-- test/api/delegates.js | 10 +++------- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/logic/vote.js b/logic/vote.js index 49f4d3d159b..03ac91ee1b0 100644 --- a/logic/vote.js +++ b/logic/vote.js @@ -96,8 +96,8 @@ Vote.prototype.verify = function (trs, sender, cb) { return setImmediate(cb, 'Voting limit exceeded. Maximum is 33 votes per transaction'); } - if (trs.asset.votes.length > _.uniq(trs.asset.votes).length) { - return setImmediate(cb, 'Duplicate votes are not allowed'); + if (trs.asset.votes.length > _.uniqBy(trs.asset.votes, function (v) { return v.slice(1); }).length) { + return setImmediate(cb, 'Multiple votes for same delegate are not allowed'); } async.eachSeries(trs.asset.votes, function (vote, eachSeriesCb) { diff --git a/test/api/delegates.js b/test/api/delegates.js index 074c45bee93..fdf94b51e41 100644 --- a/test/api/delegates.js +++ b/test/api/delegates.js @@ -109,7 +109,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error').to.equal('Duplicate votes are not allowed'); + node.expect(res.body).to.have.property('error').to.equal('Multiple votes for same delegate are not allowed'); done(); }); }); @@ -122,7 +122,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error').to.equal('Duplicate votes are not allowed'); + node.expect(res.body).to.have.property('error').to.equal('Multiple votes for same delegate are not allowed'); done(); }); }); @@ -134,13 +134,9 @@ describe('PUT /api/accounts/delegates with funds', function () { secret: account.password, delegates: votedDelegate }, function (err, res) { - var expectedErrors = [ - 'Failed to remove vote, account has not voted for this delegate', - 'Failed to add vote, account has already voted for this delegate' - ]; node.expect(res.body).to.have.property('success').to.be.not.ok; node.expect(res.body).to.have.property('error'); - node.expect(expectedErrors).to.include(res.body.error); + node.expect(res.body).to.have.property('error').to.equal('Multiple votes for same delegate are not allowed'); done(); }); }); From ed2ede4b160e1a28abad3e43287bff096e24ef09 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 21:14:09 +0500 Subject: [PATCH 5/9] changed the ordering of vote verification tests --- logic/vote.js | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/logic/vote.js b/logic/vote.js index 03ac91ee1b0..6d37a8d7d1b 100644 --- a/logic/vote.js +++ b/logic/vote.js @@ -96,19 +96,29 @@ Vote.prototype.verify = function (trs, sender, cb) { return setImmediate(cb, 'Voting limit exceeded. Maximum is 33 votes per transaction'); } - if (trs.asset.votes.length > _.uniqBy(trs.asset.votes, function (v) { return v.slice(1); }).length) { - return setImmediate(cb, 'Multiple votes for same delegate are not allowed'); - } - - async.eachSeries(trs.asset.votes, function (vote, eachSeriesCb) { - self.verifyVote(vote, function (err) { + async.applyEachSeries([ function (applyEachSeriesCb) { + async.each(trs.asset.votes, function (vote, eachCb) { + self.verifyVote(vote, function (err) { + if (err) { + return setImmediate(eachCb, ['Invalid vote at index', trs.asset.votes.indexOf(vote), '-', err].join(' ')); + } else { + return setImmediate(eachCb); + } + }); + }, function (err) { if (err) { - return setImmediate(eachSeriesCb, ['Invalid vote at index', trs.asset.votes.indexOf(vote), '-', err].join(' ')); + return setImmediate(applyEachSeriesCb, err); } else { - return setImmediate(eachSeriesCb); + return setImmediate(applyEachSeriesCb, null); } }); - }, function (err) { + }, function (applyEachSeriesCb) { + if (trs.asset.votes.length > _.uniqBy(trs.asset.votes, function (v) { return v.slice(1); }).length) { + return setImmediate(applyEachSeriesCb, 'Multiple votes for same delegate are not allowed'); + } else { + return setImmediate(applyEachSeriesCb); + } + }], function (err) { if (err) { return setImmediate(cb, err); } else { From 8c857a95d7f4c13822eceba6fcd66a4f55612300 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 22:13:13 +0500 Subject: [PATCH 6/9] added trs normalization in processAndVerification of trs --- logic/transactionPool.js | 8 ++++++++ test/api/delegates.js | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/logic/transactionPool.js b/logic/transactionPool.js index 11ea86cc765..9d8c4ebc3fb 100644 --- a/logic/transactionPool.js +++ b/logic/transactionPool.js @@ -723,6 +723,14 @@ __private.processVerifyTransaction = function (transaction, broadcast, cb) { } }); }, + function normalizeTransaction (sender, waterCb) { + try { + transaction = library.logic.transaction.objectNormalize(transaction); + return setImmediate(waterCb, null, sender); + } catch (err) { + return setImmediate(waterCb, err); + } + }, function verifyTransaction (sender, waterCb) { library.logic.transaction.verify(transaction, sender, function (err) { if (err) { diff --git a/test/api/delegates.js b/test/api/delegates.js index fdf94b51e41..d9a0b7f9656 100644 --- a/test/api/delegates.js +++ b/test/api/delegates.js @@ -109,7 +109,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error').to.equal('Multiple votes for same delegate are not allowed'); + node.expect(res.body).to.have.property('error').to.include('Failed to validate vote schema:'); done(); }); }); @@ -122,7 +122,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: votedDelegate }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error').to.equal('Multiple votes for same delegate are not allowed'); + node.expect(res.body).to.have.property('error').to.include('Failed to validate vote schema:'); done(); }); }); From ea87e102392d1cae38b55e5bb49314f1dd130ca4 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 22:27:14 +0500 Subject: [PATCH 7/9] removed applyEachSeries --- logic/vote.js | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/logic/vote.js b/logic/vote.js index 6d37a8d7d1b..9225c71e2f7 100644 --- a/logic/vote.js +++ b/logic/vote.js @@ -92,36 +92,23 @@ Vote.prototype.verify = function (trs, sender, cb) { return setImmediate(cb, 'Invalid votes. Must not be empty'); } - if (trs.asset.votes && trs.asset.votes.length > 33) { - return setImmediate(cb, 'Voting limit exceeded. Maximum is 33 votes per transaction'); - } - - async.applyEachSeries([ function (applyEachSeriesCb) { - async.each(trs.asset.votes, function (vote, eachCb) { - self.verifyVote(vote, function (err) { - if (err) { - return setImmediate(eachCb, ['Invalid vote at index', trs.asset.votes.indexOf(vote), '-', err].join(' ')); - } else { - return setImmediate(eachCb); - } - }); - }, function (err) { + async.eachSeries(trs.asset.votes, function (vote, eachSeriesCb) { + self.verifyVote(vote, function (err) { if (err) { - return setImmediate(applyEachSeriesCb, err); + return setImmediate(eachSeriesCb, ['Invalid vote at index', trs.asset.votes.indexOf(vote), '-', err].join(' ')); } else { - return setImmediate(applyEachSeriesCb, null); + return setImmediate(eachSeriesCb); } }); - }, function (applyEachSeriesCb) { - if (trs.asset.votes.length > _.uniqBy(trs.asset.votes, function (v) { return v.slice(1); }).length) { - return setImmediate(applyEachSeriesCb, 'Multiple votes for same delegate are not allowed'); - } else { - return setImmediate(applyEachSeriesCb); - } - }], function (err) { + }, function (err) { if (err) { return setImmediate(cb, err); } else { + + if (trs.asset.votes.length > _.uniqBy(trs.asset.votes, function (v) { return v.slice(1); }).length) { + return setImmediate(cb, 'Multiple votes for same delegate are not allowed'); + } + return self.checkConfirmedDelegates(trs, cb); } }); From c98601fb85b403127b6e0bbdd236c0748eea7805 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Thu, 22 Jun 2017 22:28:34 +0500 Subject: [PATCH 8/9] added accidentaly removed condition --- logic/vote.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/logic/vote.js b/logic/vote.js index 9225c71e2f7..2edf208381f 100644 --- a/logic/vote.js +++ b/logic/vote.js @@ -92,6 +92,10 @@ Vote.prototype.verify = function (trs, sender, cb) { return setImmediate(cb, 'Invalid votes. Must not be empty'); } + if (trs.asset.votes && trs.asset.votes.length > 33) { + return setImmediate(cb, 'Voting limit exceeded. Maximum is 33 votes per transaction'); + } + async.eachSeries(trs.asset.votes, function (vote, eachSeriesCb) { self.verifyVote(vote, function (err) { if (err) { From b804daa6dddd6c7cf0a97c507f4f78bc513b79a1 Mon Sep 17 00:00:00 2001 From: Usman Khan Date: Fri, 23 Jun 2017 05:02:11 +0500 Subject: [PATCH 9/9] resolved some failing tests --- test/api/delegates.js | 2 +- test/api/multisignatures.js | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/test/api/delegates.js b/test/api/delegates.js index d9a0b7f9656..ea434c9b9f5 100644 --- a/test/api/delegates.js +++ b/test/api/delegates.js @@ -245,7 +245,7 @@ describe('PUT /api/accounts/delegates with funds', function () { delegates: '' }, function (err, res) { node.expect(res.body).to.have.property('success').to.be.not.ok; - node.expect(res.body).to.have.property('error').to.equal('Invalid transaction asset'); + node.expect(res.body).to.have.property('error').to.equal('Failed to validate vote schema: Expected type array but found type string'); done(); }); }); diff --git a/test/api/multisignatures.js b/test/api/multisignatures.js index 45908dc4412..0f00d89a2e9 100644 --- a/test/api/multisignatures.js +++ b/test/api/multisignatures.js @@ -357,14 +357,12 @@ describe('GET /api/multisignatures/pending', function () { node.expect(pending.transaction).to.have.property('type').that.is.equal(node.txTypes.MULTI); node.expect(pending.transaction).to.have.property('amount').that.is.equal(0); node.expect(pending.transaction).to.have.property('senderPublicKey').that.is.equal(multisigAccount.publicKey); - node.expect(pending.transaction).to.have.property('requesterPublicKey').that.is.null; node.expect(pending.transaction).to.have.property('timestamp').that.is.a('number'); node.expect(pending.transaction).to.have.property('asset').that.is.an('object'); node.expect(pending.transaction.asset).to.have.property('multisignature').that.is.an('object'); node.expect(pending.transaction.asset.multisignature).to.have.property('min').that.is.a('number'); node.expect(pending.transaction.asset.multisignature).to.have.property('keysgroup').that.is.an('array'); node.expect(pending.transaction.asset.multisignature).to.have.property('lifetime').that.is.a('number'); - node.expect(pending.transaction).to.have.property('recipientId').that.is.null; node.expect(pending.transaction).to.have.property('signature').that.is.a('string'); node.expect(pending.transaction).to.have.property('id').that.is.equal(multiSigTx.txId); node.expect(pending.transaction).to.have.property('fee').that.is.equal(node.fees.multisignatureRegistrationFee * (Keys.length + 1));