[FIX] code review fixes

src/controllers/quote.js

@@ -119,7 +119,7 @@ exports.get = function * () {
   if (!this.query.destination_account && !this.query.destination_ledger) {
     throw new InvalidUriParameterError('Missing required parameter: destination_ledger or destination_account')
   }
-  this.body = yield model.getFullQuote(this.query, this.config)
+  this.body = yield model.getQuote(this.query, this.config)
 }
 
 function validateAmounts (sourceAmount, destinationAmount) {

src/controllers/routes.js

@@ -1,5 +1,7 @@
 'use strict'
 
+const co = require('co')
+const log = require('../common').log('routes')
 const requestUtil = require('five-bells-shared/utils/request')
 const routingTables = require('../services/routing-tables')
 const routeBroadcaster = require('../services/route-broadcaster')
@@ -15,8 +17,11 @@ exports.post = function * () {
 
   const connector = routes[0] && routes[0].connector
   if (connector && !knownConnectors[connector]) {
-    yield routeBroadcaster.broadcast()
     knownConnectors[connector] = true
+    co(routeBroadcaster.broadcast.bind(routeBroadcaster))
+      .catch(function (err) {
+        log.warn('error broadcasting routes: ' + err.message)
+      })
   }
 
   this.status = 200

src/lib/config.js

@@ -8,6 +8,11 @@ const _ = require('lodash')
 
 const envPrefix = 'CONNECTOR'
 
+const DEFAULT_MIN_MESSAGE_WINDOW = 1 // seconds
+const DEFAULT_MAX_HOLD_TIME = 10 // seconds
+const DEFAULT_FX_SPREAD = 0.002 // 0.2%
+const DEFAULT_SLIPPAGE = 0.001 // 0.1%
+
 function isRunningTests () {
   return (
     process.env.NODE_ENV === 'unit' ||
@@ -194,13 +199,13 @@ function getLocalConfig () {
 
   const expiry = {}
   expiry.minMessageWindow =
-    +Config.getEnv(envPrefix, 'MIN_MESSAGE_WINDOW') || 1 // seconds
-  expiry.maxHoldTime = +Config.getEnv(envPrefix, 'MAX_HOLD_TIME') || 10 // seconds
+    +Config.getEnv(envPrefix, 'MIN_MESSAGE_WINDOW') || DEFAULT_MIN_MESSAGE_WINDOW
+  expiry.maxHoldTime = +Config.getEnv(envPrefix, 'MAX_HOLD_TIME') || DEFAULT_MAX_HOLD_TIME
 
   // The spread is added to every quoted rate
-  const fxSpread = Number(Config.getEnv(envPrefix, 'FX_SPREAD')) || 0.002 // = .2%
+  const fxSpread = Number(Config.getEnv(envPrefix, 'FX_SPREAD')) || DEFAULT_FX_SPREAD
 
-  const slippage = +Config.getEnv(envPrefix, 'SLIPPAGE') || 0.001 // = 0.1%
+  const slippage = +Config.getEnv(envPrefix, 'SLIPPAGE') || DEFAULT_SLIPPAGE
 
   // BACKEND_URI must be defined for backends that connect to an external
   // component to retrieve the rate or amounts (it is therefore required

src/lib/route-broadcaster.js

@@ -5,6 +5,7 @@ const defer = require('co-defer')
 const _ = require('lodash')
 const request = require('co-request')
 const BROADCAST_INTERVAL = 30 * 1000 // milliseconds
+const CLEANUP_INTERVAL = 1000 // milliseconds
 
 class RouteBroadcaster {
   /**
@@ -34,7 +35,7 @@ class RouteBroadcaster {
     yield this.crawlLedgers()
     yield this.reloadLocalRoutes()
     yield this.broadcast()
-    setInterval(() => this.routingTables.removeExpiredRoutes(), 1000)
+    setInterval(() => this.routingTables.removeExpiredRoutes(), CLEANUP_INTERVAL)
     defer.setInterval(() => {
       return this.reloadLocalRoutes().then(this.broadcast.bind(this))
     }, BROADCAST_INTERVAL)

src/lib/route-builder.js

@@ -37,9 +37,9 @@ class RouteBuilder {
     const _nextHop = this._findNextHop(query)
     if (!_nextHop) throwAssetsNotTradedError()
     if (query.sourceAmount) {
-      _nextHop.finalAmount = (+_nextHop.finalAmount * (1 - this.slippage)).toFixed(15)
+      _nextHop.finalAmount = (new BigNumber(_nextHop.finalAmount)).times(1 - this.slippage).toString()
     } else { // fixed destinationAmount
-      _nextHop.sourceAmount = (+_nextHop.sourceAmount * (1 + this.slippage)).toFixed(15)
+      _nextHop.sourceAmount = (new BigNumber(_nextHop.sourceAmount)).times(1 + this.slippage).toString()
     }
     const nextHop = yield this._roundHop(_nextHop)
 
@@ -62,6 +62,14 @@ class RouteBuilder {
   }
 
   /**
+   * Given a source transfer with an embedded final transfer, get the next
+   * transfer in the chain.
+   *
+   * It works as follows:
+   * Given `sourceTransfer` A→C, find the next hop B on the route from A to C.
+   * If the next hop is the final one (B == C), return the final transfer.
+   * Otherwise, return a transfer at B, with the final transfer C embedded.
+   *
    * @param {Transfer} sourceTransfer
    * @returns {Transfer} destinationTransfer
    */
@@ -121,6 +129,9 @@ class RouteBuilder {
     return (new Date(sourceExpiryTime - minMessageWindow)).toISOString()
   }
 
+  /**
+   * Round the hop's amounts according to the corresponding ledgers' scales/precisions.
+   */
   * _roundHop (hop) {
     hop.sourceAmount = yield this._roundAmount('source', hop.sourceLedger, hop.sourceAmount)
     hop.destinationAmount = yield this._roundAmount('destination', hop.destinationLedger, hop.destinationAmount)

src/lib/routing-tables.js

@@ -31,7 +31,10 @@ class RoutingTables {
   }
 
   /**
-   * @param {Route} route
+   * Given a `route` B→C, create a route A→C for each source ledger A with a
+   * local route to B.
+   *
+   * @param {Route} route from ledger B→C
    */
   addRoute (route) {
     const ledgerB = route.source_ledger
@@ -47,12 +50,10 @@ class RoutingTables {
     this.eachSource((tableFromA, ledgerA) => {
       // Don't create A→B→A.
       if (ledgerA === ledgerC) return
-      const routesFromAToB = tableFromA.destinations.get(ledgerB)
-      if (!routesFromAToB) return
       // Don't create local route A→B→C if local route A→C already exists.
       if (this.baseURI === connectorFromBToC && this._getLocalRoute(ledgerA, ledgerC)) return
-      const routeFromAToB = routesFromAToB.get(this.baseURI)
-      // Don't create A→C→B when there is no local A→C.
+      // Don't create A→B→C when A→B is not a local pair.
+      const routeFromAToB = this._getLocalRoute(ledgerA, ledgerB)
       if (!routeFromAToB) return
       const routeFromAToC = routeFromAToB.join(routeFromBToC)
       routeFromAToC.info = makeRouteInfo(route, routeFromAToB.info.minMessageWindow)
@@ -147,9 +148,9 @@ class RoutingTables {
       connector: nextHop.bestHop,
       sourceLedger: ledgerA,
       // Prevent 'BigNumber Error: new BigNumber() number type has more than 15 significant digits'
-      sourceAmount: nextHop.bestCost.toFixed(15),
+      sourceAmount: nextHop.bestCost.toString(),
       destinationLedger: ledgerB,
-      destinationAmount: routeFromAToB.amountAt(nextHop.bestCost).toFixed(15),
+      destinationAmount: routeFromAToB.amountAt(nextHop.bestCost).toString(),
       destinationDebitAccount: this._getAccount(this.baseURI, ledgerB),
       destinationCreditAccount: isLocal ? null : this._getAccount(nextHop.bestHop, ledgerB),
       finalLedger: ledgerC,
@@ -175,25 +176,25 @@ class RoutingTables {
       sourceLedger: ledgerA,
       sourceAmount: sourceAmount,
       destinationLedger: ledgerB,
-      destinationAmount: routeFromAToB.amountAt(sourceAmount).toFixed(15),
+      destinationAmount: routeFromAToB.amountAt(sourceAmount).toString(),
       destinationDebitAccount: this._getAccount(this.baseURI, ledgerB),
       destinationCreditAccount: isLocal ? null : this._getAccount(nextHop.bestHop, ledgerB),
       finalLedger: ledgerC,
-      finalAmount: nextHop.bestValue.toFixed(15),
+      finalAmount: nextHop.bestValue.toString(),
       minMessageWindow: nextHop.info.minMessageWindow
     }
   }
-}
 
-;[
-  'findBestHopForSourceAmount',
-  'findBestHopForDestinationAmount'
-].forEach(function (fn) {
-  RoutingTables.prototype['_' + fn] = function (source, destination, amount) {
+  _findBestHopForSourceAmount (source, destination, amount) {
+    if (!this.sources[source]) return
+    return this.sources[source].findBestHopForSourceAmount(destination, amount)
+  }
+
+  _findBestHopForDestinationAmount (source, destination, amount) {
     if (!this.sources[source]) return
-    return this.sources[source][fn](destination, amount)
+    return this.sources[source].findBestHopForDestinationAmount(destination, amount)
   }
-})
+}
 
 function combineRoutesByConnector (routesByConnector) {
   let totalRoute = new routing.Route([])

src/models/quote.js

@@ -105,7 +105,7 @@ function getLocalTransfer (query) {
  * @param {Object} config
  * @returns {Transfer}
  */
-function * getFullQuote (params, config) {
+function * getQuote (params, config) {
   const query = yield makeQuoteQuery(params)
   if (query.sourceLedger === query.destinationLedger) {
     return getLocalTransfer(query)
@@ -129,4 +129,4 @@ function * getFullQuote (params, config) {
   return quote
 }
 
-module.exports = {getFullQuote}
+module.exports = {getQuote}

test/quoteSpec.js

@@ -3,6 +3,7 @@ const parseURL = require('url').parse
 const nock = require('nock')
 nock.enableNetConnect(['localhost'])
 const ratesResponse = require('./data/fxRates.json')
+const validate = require('five-bells-shared/services/validate')
 const appHelper = require('./helpers/app')
 const logger = require('five-bells-connector')._test.logger
 const balanceCache = require('five-bells-connector')._test.balanceCache
@@ -344,6 +345,21 @@ describe('Quotes', function () {
         .end()
     })
 
+    it('should return a valid Transfer Template object', function * () {
+      yield this.request()
+        .get('/quote?' +
+          'source_amount=100' +
+          '&source_ledger=http://eur-ledger.example/EUR' +
+          '&destination_ledger=http://usd-ledger.example/USD')
+        .expect(function (res) {
+          let validation = validate('TransferTemplate', res.body)
+          if (!validation.valid) {
+            throw new Error('Not a valid transfer template')
+          }
+        })
+        .end()
+    })
+
     it('should return quotes for fixed source amounts -- lower precision source_ledger', function * () {
       nock.cleanAll()
       // Increase scale

test/routingTablesSpec.js

@@ -317,9 +317,9 @@ describe('RoutingTables', function () {
         {
           connector: 'http://mary.example',
           sourceLedger: ledgerA,
-          sourceAmount: (100).toFixed(15),
+          sourceAmount: (100).toString(),
           destinationLedger: ledgerB,
-          destinationAmount: (50).toFixed(15),
+          destinationAmount: (50).toString(),
           destinationDebitAccount: ledgerB + '/accounts/mark',
           destinationCreditAccount: ledgerB + '/accounts/mary',
           finalLedger: ledgerC,
@@ -334,9 +334,9 @@ describe('RoutingTables', function () {
         {
           connector: 'http://mark.example',
           sourceLedger: ledgerA,
-          sourceAmount: (100).toFixed(15),
+          sourceAmount: (100).toString(),
           destinationLedger: ledgerB,
-          destinationAmount: (50).toFixed(15),
+          destinationAmount: (50).toString(),
           destinationDebitAccount: markB,
           destinationCreditAccount: null,
           finalLedger: ledgerB,
@@ -355,11 +355,11 @@ describe('RoutingTables', function () {
           sourceLedger: ledgerA,
           sourceAmount: '100',
           destinationLedger: ledgerB,
-          destinationAmount: (50).toFixed(15),
+          destinationAmount: (50).toString(),
           destinationDebitAccount: markB,
           destinationCreditAccount: null,
           finalLedger: ledgerB,
-          finalAmount: (50).toFixed(15),
+          finalAmount: (50).toString(),
           minMessageWindow: 1
         })
     })