diff --git a/src/lib/route.js b/src/lib/route.js index 0e7394d..2b77dc9 100644 --- a/src/lib/route.js +++ b/src/lib/route.js @@ -66,7 +66,7 @@ class Route { // are not included in this.paths, so for instance a path: // * localLedger -> trustLine -> destinationLedger has this.paths === [ [] ], and path length 2 // * localLedger -> trustLine -> anotherTrustLine -> destinationLedger has this.paths === [ ['anotherTrustLine'] ] and path length 3 - return max + 2 + return this.nextLedger === this.destinationLedger ? max + 2 : max + 3 } /** diff --git a/src/lib/routing-table.js b/src/lib/routing-table.js index 9ba0647..958838a 100644 --- a/src/lib/routing-table.js +++ b/src/lib/routing-table.js @@ -70,29 +70,31 @@ class RoutingTable { return undefined } - let bestHop = null - let bestValue = null - let bestRoute = null + let bestPath routes.forEach((route, nextHop) => { - const value = route.amountAt(sourceAmount) // If we have a route but not a curve, pick a route at random // and get a remote quote. In the future we may refactor this // so that multiple next hop options can be returned, and all // can be asked for a quote, but for now, it's just the last. - if (value === undefined || !bestValue || value.gt(bestValue)) { - bestHop = nextHop - bestValue = value && value.toString() - bestRoute = route - } + bestPath = getBetterPath(bestPath, { + nextHop, + route, + value: route.amountAt(sourceAmount), + pathLength: route.maxPathLength() + }) }) - if (bestHop) { - debug('findBestHopForSourceAmount to ' + destination + ' for ' + sourceAmount + ' found route through ' + bestHop) + if (bestPath) { + debug('findBestHopForSourceAmount to ' + destination + ' for ' + sourceAmount + ' found route through ' + bestPath.nextHop) } else { debug('findBestHopForSourceAmount could not find route to ' + destination + ' for ' + sourceAmount + '. Current routing table: ' + JSON.stringify(this.destinations.toJSON())) } - return { bestHop, bestValue, bestRoute } + return { + bestHop: bestPath.nextHop, + bestValue: bestPath.value && bestPath.value.toString(), + bestRoute: bestPath.route + } } findBestHopForDestinationAmount (destination, destinationAmount) { @@ -103,30 +105,56 @@ class RoutingTable { return undefined } - let bestHop = null - let bestCost = Infinity - let bestRoute = null + let bestPath routes.forEach((route, nextHop) => { const cost = route.amountReverse(destinationAmount) + if (cost.equals(Infinity)) return // If we have a route but not a curve, pick a route at random // and get a remote quote. In the future we may refactor this // so that multiple next hop options can be returned, and all // can be asked for a quote, but for now, it's just the last. - if (cost === undefined || cost.lt(bestCost)) { - bestHop = nextHop - bestCost = cost && cost.toString() - bestRoute = route - } + bestPath = getBetterPath(bestPath, { + nextHop, + route, + cost, + pathLength: route.maxPathLength() + }) }) - if (bestHop) { - debug('findBestHopForDestinationAmount to ' + destination + ' for ' + destinationAmount + ' found route through ' + bestHop) - return { bestHop, bestCost, bestRoute } + if (bestPath) { + debug('findBestHopForDestinationAmount to ' + destination + ' for ' + destinationAmount + ' found route through ' + bestPath.nextHop) + return { + bestHop: bestPath.nextHop, + bestCost: bestPath.cost && bestPath.cost.toString(), + bestRoute: bestPath.route + } } else { debug('findBestHopForDestinationAmount could not find route to ' + destination + ' for ' + destinationAmount + '. Current routing table: ' + JSON.stringify(this.destinations.toJSON())) return undefined } } + + static _getBetterPath (currentPath, otherPath) { return getBetterPath(currentPath, otherPath) } +} + +/** + * If both hops score equally, return `currentPath`. + * It doesn't actually matter which is returned in that case, so long as it is consistent. + */ +function getBetterPath (currentPath, otherPath) { + if (!currentPath) return otherPath + if (currentPath.pathLength < otherPath.pathLength) return currentPath + if (otherPath.pathLength < currentPath.pathLength) return otherPath + if (otherPath.value !== undefined) { + if (!currentPath.value) return otherPath + return otherPath.value.gt(currentPath.value) ? otherPath : currentPath + } + if (otherPath.cost !== undefined) { + if (!currentPath.cost) return otherPath + return otherPath.cost.lt(currentPath.cost) ? otherPath : currentPath + } + // No curve + return currentPath } module.exports = RoutingTable diff --git a/test/routing-table.test.js b/test/routing-table.test.js index 4e49d0a..340dcde 100644 --- a/test/routing-table.test.js +++ b/test/routing-table.test.js @@ -1,11 +1,13 @@ 'use strict' const assert = require('assert') +const BigNumber = require('bignumber.js') const RoutingTable = require('../src/lib/routing-table') const Route = require('../src/lib/route') const ledgerA = 'ledgerA.' const ledgerB = 'ledgerB.' +const ledgerC = 'ledgerB.' const markB = ledgerB + 'mark' const maryB = ledgerB + 'mary' @@ -76,6 +78,16 @@ describe('RoutingTable', function () { const table = new RoutingTable() assert.strictEqual(table.findBestHopForSourceAmount(ledgerB, 10), undefined) }) + + it('prefers short routes', function () { + const table = new RoutingTable() + const routeMark = new Route([[0, 0], [100, 999]], {sourceLedger: ledgerA, nextLedger: ledgerB, destinationLedger: ledgerB}) + const routeMary = new Route([[0, 0], [100, 100]], {sourceLedger: ledgerA, nextLedger: ledgerC, destinationLedger: ledgerB}) + table.addRoute(ledgerB, markB, routeMark) + table.addRoute(ledgerB, ledgerC + 'mary', routeMary) + assert.deepEqual(table.findBestHopForSourceAmount(ledgerB, 50), + { bestHop: markB, bestValue: '499', bestRoute: routeMark }) + }) }) describe('findBestHopForDestinationAmount', function () { @@ -102,4 +114,53 @@ describe('RoutingTable', function () { assert.strictEqual(table.findBestHopForDestinationAmount(ledgerB, 200), undefined) }) }) + + describe('getBetterPath', function () { + const getBetterPath = RoutingTable._getBetterPath + + it('returns otherPath if there is no currentPath', function () { + const otherPath = {} + assert.strictEqual(getBetterPath(null, otherPath), otherPath) + }) + + it('returns the shorter hop', function () { + const path1 = {pathLength: 1, value: new BigNumber(1)} + const path2 = {pathLength: 2, value: new BigNumber(2)} + assert.strictEqual(getBetterPath(path1, path2), path1) + assert.strictEqual(getBetterPath(path2, path1), path1) + }) + + it('returns the hop with the better value', function () { + const path1 = {pathLength: 1, value: new BigNumber(1)} + const path2 = {pathLength: 1, value: new BigNumber(2)} + assert.strictEqual(getBetterPath(path1, path2), path2) + assert.strictEqual(getBetterPath(path2, path1), path2) + }) + + it('returns otherPath when otherPath has a value and currentPath doesn\'t', function () { + const path1 = {pathLength: 1} + const path2 = {pathLength: 1, value: new BigNumber(1)} + assert.strictEqual(getBetterPath(path1, path2), path2) + }) + + it('returns the hop with the better cost', function () { + const path1 = {pathLength: 1, cost: new BigNumber(1)} + const path2 = {pathLength: 1, cost: new BigNumber(2)} + assert.strictEqual(getBetterPath(path1, path2), path1) + assert.strictEqual(getBetterPath(path2, path1), path1) + }) + + it('returns otherPath when otherPath has a cost and currentPath doesn\'t', function () { + const path1 = {pathLength: 1} + const path2 = {pathLength: 1, cost: new BigNumber(1)} + assert.strictEqual(getBetterPath(path1, path2), path2) + }) + + it('returns currentPath if neither hop has a curve', function () { + const path1 = {pathLength: 1} + const path2 = {pathLength: 1} + assert.strictEqual(getBetterPath(path1, path2), path1) + assert.strictEqual(getBetterPath(path2, path1), path2) + }) + }) })