From 00578735552fbc1387d1d5613c5359cc247f2453 Mon Sep 17 00:00:00 2001 From: Alan Cohen Date: Thu, 19 Mar 2015 12:13:37 -0700 Subject: [PATCH] [FIX] Correctly determine when to set SendMax (RLJS-272) - SendMax should not be set when the source and destination currencies are the same and the issuers are "any that the source is able to use and the destination is able to receive" e.g., source_amount: { value: '0.0003272934271954', currency: 'GWD', issuer: '' }, destination_amount: { value: '0.0003272934271954', currency: 'GWD', issuer: 'r4p4gZaWSq8Cs1d8mn1jaGqVU1HUns1ek3' } - SendMax documentation: https://ripple.com/build/transactions/#special-issuer-values-for-sendmax-and-amount --- api/lib/rest-to-tx-converter.js | 72 ++++++++++++++++++---------- test/fixtures/addresses.js | 8 +++- test/unit/fixtures/rest-converter.js | 35 ++++++++++---- test/unit/rest-converter-test.js | 64 ++++++++++++++++++++++--- 4 files changed, 136 insertions(+), 43 deletions(-) diff --git a/api/lib/rest-to-tx-converter.js b/api/lib/rest-to-tx-converter.js index 7aebd4dc..0651876f 100644 --- a/api/lib/rest-to-tx-converter.js +++ b/api/lib/rest-to-tx-converter.js @@ -2,7 +2,7 @@ 'use strict'; var _ = require('lodash'); var ripple = require('ripple-lib'); -var bignum = require('bignumber.js'); +var BigNumber = require('bignumber.js'); var utils = require('./utils'); function RestToTxConverter() {} @@ -19,12 +19,38 @@ function RestToTxConverter() {} * @param {ripple-lib Transaction} transaction */ RestToTxConverter.prototype.convert = function(payment, callback) { + function isSendMaxRequired() { + var src = payment.source_account; + + var srcAmt = payment.source_amount; + var dstAmt = payment.destination_amount; + + // Don't set SendMax for XRP->XRP payment + if (!srcAmt || srcAmt.currency === 'XRP' && dstAmt.currency === 'XRP') { + return false; + } + + // only set send max when: + // - source and destination currencies are same + // and source issuer is not source account + // - source amount and destination issuers are different + // + if (srcAmt.currency === dstAmt.currency) { + if (srcAmt.issuer !== src) { + return true; + } + } else if (srcAmt.issuer !== dstAmt.issuer) { + return true; + } + return false; + } + try { - // Convert blank issuer to sender's address - // (Ripple convention for 'any issuer') - // https://ripple.com/build/transactions/ - // #special-issuer-values-for-sendmax-and-amount - // https://ripple.com/build/ripple-rest/#counterparties-in-payments + // Convert blank issuer to sender's address + // (Ripple convention for 'any issuer') + // https://ripple.com/build/transactions/ + // #special-issuer-values-for-sendmax-and-amount + // https://ripple.com/build/ripple-rest/#counterparties-in-payments if (payment.source_amount && payment.source_amount.currency !== 'XRP' && payment.source_amount.issuer === '') { payment.source_amount.issuer = payment.source_account; @@ -43,11 +69,11 @@ RestToTxConverter.prototype.convert = function(payment, callback) { // Uppercase currency codes if (payment.source_amount) { payment.source_amount.currency = - payment.source_amount.currency.toUpperCase(); + payment.source_amount.currency.toUpperCase(); } if (payment.destination_amount) { payment.destination_amount.currency = - payment.destination_amount.currency.toUpperCase(); + payment.destination_amount.currency.toUpperCase(); } /* Construct payment */ var transaction = new ripple.Transaction(); @@ -70,24 +96,20 @@ RestToTxConverter.prototype.convert = function(payment, callback) { if (payment.destination_tag) { transaction.destinationTag(parseInt(payment.destination_tag, 10)); } + // SendMax - if (payment.source_amount) { - // Only set send max if source and destination currencies are different - if (!(payment.source_amount.currency - === payment.destination_amount.currency - && payment.source_amount.issuer - === payment.destination_amount.issuer)) { - var max_value = bignum(payment.source_amount.value) - .plus(payment.source_slippage || 0).toString(); - if (payment.source_amount.currency === 'XRP') { - transaction.sendMax(utils.xrpToDrops(max_value)); - } else { - transaction.sendMax({ - value: max_value, - currency: payment.source_amount.currency, - issuer: payment.source_amount.issuer - }); - } + if (isSendMaxRequired()) { + var max_value = new BigNumber(payment.source_amount.value) + .plus(payment.source_slippage || 0).toString(); + + if (payment.source_amount.currency === 'XRP') { + transaction.sendMax(utils.xrpToDrops(max_value)); + } else { + transaction.sendMax({ + value: max_value, + currency: payment.source_amount.currency, + issuer: payment.source_amount.issuer + }); } } diff --git a/test/fixtures/addresses.js b/test/fixtures/addresses.js index b2e7ddbd..3ef57d74 100644 --- a/test/fixtures/addresses.js +++ b/test/fixtures/addresses.js @@ -1,7 +1,11 @@ +'use strict'; + module.exports = { VALID: 'r3GgMwvgvP8h4yVWvjH1dPZNvC37TjzBBE', ISSUER: 'r3PDtZSa5LiYp1Ysn1vMuMzB59RzV3W9QH', SECRET: 'shsWGZcmZz6YsWWmcnpfr6fLTdtFV', INVALID: 'rxxLy6UWsjzxsQrTATf1bwDYSaJMoTGvfY2Q', - COUNTERPARTY: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B' -}; \ No newline at end of file + COUNTERPARTY: 'rvYAfWj5gh67oV6fW32ZzP3Aw4Eubs59B', + VALID2: 'rHrGzm86u7fwsickgTghsS9D7sUuC3VKwE', + ISSUER2: 'rBXMZX16zyqthrECN4AY4wxbug7335tCKh' +}; diff --git a/test/unit/fixtures/rest-converter.js b/test/unit/fixtures/rest-converter.js index a0123490..41efcab6 100644 --- a/test/unit/fixtures/rest-converter.js +++ b/test/unit/fixtures/rest-converter.js @@ -25,6 +25,21 @@ module.exports.paymentRestXRP = { } }; +module.exports.paymentRestXRPtoXRP = { + 'source_account': addresses.VALID, + 'source_amount': { + 'value': '1', + 'currency': 'XRP', + 'issuer': '' + }, + 'destination_account': addresses.COUNTERPARTY, + 'destination_amount': { + 'value': '1', + 'currency': 'XRP', + 'issuer': '' + } +}; + module.exports.paymentRestComplex = { 'source_account': addresses.VALID, 'source_tag': '', @@ -118,9 +133,11 @@ module.exports.exportsPaymentRestIssuers = function(options) { _.defaults(options, { sourceAccount: addresses.VALID, destinationAccount: addresses.COUNTERPARTY, - sourceIssuer: addresses.VALID, - destinationIssuer: addresses.COUNTERPARTY, - sourceValue: '10' + sourceIssuer: '', + destinationIssuer: '', + sourceValue: '10', + destinationCurrency: 'USD', + sourceCurrency: 'USD' }); return { @@ -128,16 +145,16 @@ module.exports.exportsPaymentRestIssuers = function(options) { source_tag: '', source_amount: { value: options.sourceValue, - currency: 'USD', + currency: options.sourceCurrency, issuer: options.sourceIssuer }, source_slippage: options.sourceSlippage, - destination_account: options.destinationAccount, - destination_tag: '', - destination_amount: { + destination_account: options.destinationAccount, + destination_tag: '', + destination_amount: { value: '10', - currency: 'USD', - issuer: options.destinationIssuer + currency: options.destinationCurrency, + issuer: options.destinationIssuer }, invoice_id: '', paths: '[]', diff --git a/test/unit/rest-converter-test.js b/test/unit/rest-converter-test.js index bb7d9567..dae5127b 100644 --- a/test/unit/rest-converter-test.js +++ b/test/unit/rest-converter-test.js @@ -17,14 +17,24 @@ suite('unit - converter - Rest to Tx', function() { }); }); - test('convert() -- payment with XRP', function(done) { + test('convert() -- payment with XRP and no source amount', function(done) { restToTxConverter.convert(fixtures.paymentRestXRP, function(err, transaction) { assert.strictEqual(err, null); assert.deepEqual(transaction.summary(), fixtures.paymentTxXRP); + assert.strictEqual(transaction.tx_json.SendMax, undefined); done(); }); }); + test('convert() -- payment XRP to XRP', function(done) { + restToTxConverter.convert(fixtures.paymentRestXRPtoXRP, function(err, transaction) { + assert.strictEqual(err, null); + assert.strictEqual(transaction.tx_json.SendMax, undefined); + done(); + }); + }); + + test('convert() -- payment with additional flags', function(done) { restToTxConverter.convert(fixtures.paymentRestComplex, function(err, transaction) { assert.strictEqual(err, null); @@ -46,14 +56,16 @@ suite('unit - converter - Rest to Tx', function() { test('convert() -- payment with currency that has different issuers for source and destination amount', function(done) { restToTxConverter.convert(fixtures.exportsPaymentRestIssuers({ - sourceIssuer: addresses.VALID, - destinationIssuer: addresses.COUNTERPARTY + sourceAccount: addresses.VALID, + destinationAccount: addresses.COUNTERPARTY, + sourceIssuer: addresses.VALID2, + destinationIssuer: addresses.ISSUER2 }), function(err, transaction) { assert.strictEqual(err, null); assert.deepEqual(transaction.tx_json.SendMax, { value: '10', currency: 'USD', - issuer: addresses.VALID + issuer: addresses.VALID2 }); done(); }); @@ -61,8 +73,10 @@ suite('unit - converter - Rest to Tx', function() { test('convert() -- payment with currency that has different issuers for source and destination amount and a source_slippage of 0.1', function(done) { restToTxConverter.convert(fixtures.exportsPaymentRestIssuers({ - sourceIssuer: addresses.VALID, - destinationIssuer: addresses.COUNTERPARTY, + sourceAccount: addresses.VALID, + destinationAccount: addresses.COUNTERPARTY, + sourceIssuer: addresses.VALID2, + destinationIssuer: addresses.ISSUER2, sourceSlippage: '0.1', sourceAmount: '10' }), function(err, transaction) { @@ -70,10 +84,46 @@ suite('unit - converter - Rest to Tx', function() { assert.deepEqual(transaction.tx_json.SendMax, { value: '10.1', currency: 'USD', - issuer: addresses.VALID + issuer: addresses.VALID2 }); done(); }); }); + test('convert() -- payment with same currency for source and destination, no issuer for source amount', function(done) { + restToTxConverter.convert(fixtures.exportsPaymentRestIssuers({ + sourceIssuer: '', + destinationAccount: addresses.COUNTERPARTY, + destinationIssuer: addresses.COUNTERPARTY + }), function(err, transaction) { + assert.strictEqual(err, null); + assert.strictEqual(transaction.tx_json.SendMax, undefined); + done(); + }); + }); + + test('convert() -- payment with same currency for source and destination, no issuer for source and destination amount', function(done) { + restToTxConverter.convert(fixtures.exportsPaymentRestIssuers({ + sourceIssuer: '', + destinationAccount: addresses.COUNTERPARTY, + destinationIssuer: '' + }), function(err, transaction) { + assert.strictEqual(err, null); + assert.strictEqual(transaction.tx_json.SendMax, undefined); + done(); + }); + }); + + test('convert() -- payment with same currency for source and destination, no issuer for destination amount', function(done) { + restToTxConverter.convert(fixtures.exportsPaymentRestIssuers({ + sourceIssuer: addresses.VALID, + destinationAccount: addresses.COUNTERPARTY, + destinationIssuer: '' + }), function(err, transaction) { + assert.strictEqual(transaction.tx_json.SendMax, undefined); + done(err); + }); + }); + + });