feat: prioritize short paths during findBestHop #54
Conversation
* `findBestHopFor{Source,Destination}Amount()` should prioritize routes based on shortest `maxPathLength()`.
* Fix `maxPathLength()`: if `nextLedger` is not equal to `destinationLedger`, increment the path length.
* Possible fix for interledgerjs/ilp-connector#382. When two paths score the same, return the first in the Map.
Codecov Report
@@ Coverage Diff @@
## master #54 +/- ##
==========================================
+ Coverage 96.26% 96.28% +0.02%
==========================================
Files 5 5
Lines 535 539 +4
Branches 111 120 +9
==========================================
+ Hits 515 519 +4
Misses 20 20
Continue to review full report at Codecov.
|
src/lib/routing-table.js
Outdated
| if (currentHop.pathLength < otherHop.pathLength) return currentHop | ||
| if (otherHop.pathLength < currentHop.pathLength) return otherHop | ||
| if (otherHop.value !== undefined) { | ||
| return otherHop.value.gt(currentHop.value) ? otherHop : currentHop |
There was a problem hiding this comment.
this will throw an error if currentHop.value is undefined
src/lib/routing-table.js
Outdated
| return otherHop.value.gt(currentHop.value) ? otherHop : currentHop | ||
| } | ||
| if (otherHop.cost !== undefined) { | ||
| return otherHop.cost.lt(currentHop.cost) ? otherHop : currentHop |
There was a problem hiding this comment.
will throw an error if currentHop.cost is undefined
| @@ -76,6 +77,16 @@ describe('RoutingTable', function () { | |||
| const table = new RoutingTable() | |||
| assert.strictEqual(table.findBestHopForSourceAmount(ledgerB, 10), undefined) | |||
| }) | |||
|
|
|||
| it('prefers short routes', function () { | |||
There was a problem hiding this comment.
can you have a test case for each of the comparisons in getBetterHop?
| @@ -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 | |||
There was a problem hiding this comment.
This means we count a route trustLine -> localLedger as length 2, so counting the number of ledgers involved. I originally thought of it as the number of connections, so this.nextLedger === this.destinationLedger ? max + 1 : max + 2 but I guess your way of counting makes (more) sense.
src/lib/routing-table.js
Outdated
| bestRoute = route | ||
| } | ||
| bestHop = getBetterHop(bestHop, { | ||
| nextHop, |
There was a problem hiding this comment.
getBetterHop takes two arguments, function getBetterHop (currentHop, otherHop).
Here, you pass bestHop as the currentHop, and { nextHop, ... } as the otherHop.
Does that mean (if both arguments of getBetterHop are of the same type) that bestHop also has a field nextHop? nextHop comes from routes.forEach((route, nextHop) => {, so it's a route.
Now I no longer remember what data type a 'hop' is. Is it a ledger name string? Or a Route object?
There was a problem hiding this comment.
Do all of currentHop, otherHop, nextHop and bestHop have the same data type? If not, then that's confusing.
src/lib/routing-table.js
Outdated
| }) | ||
|
|
||
| if (bestHop) { | ||
| debug('findBestHopForSourceAmount to ' + destination + ' for ' + sourceAmount + ' found route through ' + bestHop) | ||
| debug('findBestHopForSourceAmount to ' + destination + ' for ' + sourceAmount + ' found route through ' + bestHop.nextHop) |
There was a problem hiding this comment.
See above, bestHop.nextHop - I don't know what that means, the next Hop of the best Hop. Which one is the Route object here and which one is the peer ledger name String?
There was a problem hiding this comment.
With these changes, there is now confusion about what a Hop is. Maybe the best example is this line where
the return value of
findBestHopForDestinationAmount gets a field bestHop (in itself that's ok, that function just returns a bit of extra info apart from the bestHop that was asked for), but the value that's used for it is not actually the local bestHop variable; it is set to bestHop.nextHop. What's the difference between a Hop and the nextHop of that Hop?
* Rename "bestHop" to "bestPath", since "hop" generally refers to a connector address.
* Add tests for `getBetterPath()`.
* Don't throw when `currentHop.{value,cost}` is `undefined`.
findBestHopFor{Source,Destination}Amount()should prioritize routes based on shortestmaxPathLength().maxPathLength(): ifnextLedgeris not equal todestinationLedger, increment the path length.