-
Notifications
You must be signed in to change notification settings - Fork 59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BREAKING] destination routing #150
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
'use strict' | ||
|
||
const requestUtil = require('five-bells-shared/utils/request') | ||
const routingTables = require('../services/routing-tables') | ||
const routeBroadcaster = require('../services/route-broadcaster') | ||
const knownConnectors = {} | ||
|
||
exports.post = function * () { | ||
const routes = yield requestUtil.validateBody(this, 'Routes') | ||
|
||
// TODO verify that POSTer of these routes matches route.connector. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sentientwaffle @justmoon How are connectors going to verify routes are coming from the right connector? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, eventually routes will arrive on a transfer's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, we'll have to talk more about that. I'm not sure it makes sense to add routes to the incoming transfer. Even if we want to have connectors pay one another it might make more sense to pre-pay a bit and draw down against a balance rather than pay for every advertisement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Paying for advertisements should be a standard "paid HTTP" call. So let's make sure we record these requirements (e.g. "how do we know the advertisement is coming from a certain connector") for when we work on the paid-HTTP protocol. It seems like what would happen is that the other connector would pay you (associating a sending identity with a token with a balance) and then use the token to pay for API calls. If that's the case, then you would be able to associate the token back to the sending identity. |
||
for (const route of routes) { | ||
routingTables.addRoute(route) | ||
} | ||
|
||
const connector = routes[0] && routes[0].connector | ||
if (connector && !knownConnectors[connector]) { | ||
yield routeBroadcaster.broadcast() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will only come back from the yield after all the notifications have been sent out to other connectors, shouldn't it respond to the connector that sent it this route update before sending out the other notifications? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. |
||
knownConnectors[connector] = true | ||
} | ||
|
||
this.status = 200 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -171,7 +171,6 @@ function getLocalConfig () { | |
JSON.parse(Config.getEnv(envPrefix, 'PAIRS') || 'false') || generateDefaultPairs(ledgers) | ||
|
||
const features = {} | ||
features.quoteFullPath = Config.castBool(Config.getEnv(envPrefix, 'QUOTE_FULL_PATH')) | ||
features.debugAutoFund = Config.castBool(Config.getEnv(envPrefix, 'DEBUG_AUTOFUND')) | ||
|
||
const adminEnv = parseAdminEnv() | ||
|
@@ -195,12 +194,14 @@ 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') || 1 // seconds | ||
expiry.maxHoldTime = +Config.getEnv(envPrefix, 'MAX_HOLD_TIME') || 10 // seconds | ||
|
||
// The spread is added to every quoted rate | ||
const fxSpread = Number(Config.getEnv(envPrefix, 'FX_SPREAD')) || 0.002 // = .2% | ||
|
||
const slippage = +Config.getEnv(envPrefix, 'SLIPPAGE') || 0.001 // = 0.1% | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you put these hard coded defaults up at the top of the file as |
||
|
||
// BACKEND_URI must be defined for backends that connect to an external | ||
// component to retrieve the rate or amounts (it is therefore required | ||
// when using the ilp-quote backend) | ||
|
@@ -233,6 +234,7 @@ function getLocalConfig () { | |
backend, | ||
ledgerCredentials, | ||
fxSpread, | ||
slippage, | ||
expiry, | ||
features, | ||
admin, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,126 @@ | ||
'use strict' | ||
|
||
const co = require('co') | ||
const defer = require('co-defer') | ||
const _ = require('lodash') | ||
const request = require('co-request') | ||
const BROADCAST_INTERVAL = 30 * 1000 // milliseconds | ||
|
||
class RouteBroadcaster { | ||
/** | ||
* @param {RoutingTables} routingTables | ||
* @param {Backend} backend | ||
* @param {Object} config | ||
* @param {Object} config.ledgerCredentials | ||
* @param {Object} config.tradingPairs | ||
* @param {Number} config.minMessageWindow | ||
*/ | ||
constructor (routingTables, backend, config) { | ||
this.baseURI = routingTables.baseURI | ||
this.routingTables = routingTables | ||
this.backend = backend | ||
this.ledgerCredentials = config.ledgerCredentials | ||
this.tradingPairs = config.tradingPairs | ||
this.minMessageWindow = config.minMessageWindow | ||
this.adjacentConnectors = {} | ||
this.adjacentLedgers = {} | ||
for (const pair of config.tradingPairs) { | ||
const destinationLedger = pair[1].split('@')[1] | ||
this.adjacentLedgers[destinationLedger] = true | ||
} | ||
} | ||
|
||
* start () { | ||
yield this.crawlLedgers() | ||
yield this.reloadLocalRoutes() | ||
yield this.broadcast() | ||
setInterval(() => this.routingTables.removeExpiredRoutes(), 1000) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like this interval should be configurable and/or have a default value defined at the top of the file |
||
defer.setInterval(() => { | ||
return this.reloadLocalRoutes().then(this.broadcast.bind(this)) | ||
}, BROADCAST_INTERVAL) | ||
} | ||
|
||
broadcast () { | ||
const routes = this.routingTables.toJSON() | ||
return Promise.all( | ||
Object.keys(this.adjacentConnectors).map( | ||
(adjacentConnector) => this._broadcastTo(adjacentConnector, routes))) | ||
} | ||
|
||
_broadcastTo (adjacentConnector, routes) { | ||
return request({ | ||
method: 'POST', | ||
uri: adjacentConnector + '/routes', | ||
body: routes, | ||
json: true | ||
}).then((res) => { | ||
if (res.statusCode !== 200) { | ||
throw new Error('Unexpected status code: ' + res.statusCode) | ||
} | ||
}) | ||
} | ||
|
||
crawlLedgers () { | ||
return Object.keys(this.adjacentLedgers).map(this._crawlLedger, this) | ||
} | ||
|
||
* _crawlLedger (ledger) { | ||
const res = yield request({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should use the multiledger abstraction (if you don't want to change it here we can also make that change when everything is switched over to using the ledger plugin framework) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ya, its so far behind I'll just wait until we switch everything over. |
||
method: 'GET', | ||
uri: ledger + '/connectors', | ||
json: true | ||
}) | ||
if (res.statusCode !== 200) { | ||
throw new Error('Unexpected status code: ' + res.statusCode) | ||
} | ||
const connectors = _.map(res.body, 'connector') | ||
for (const connector of connectors) { | ||
// Don't broadcast routes to ourselves. | ||
if (connector === this.baseURI) continue | ||
this.adjacentConnectors[connector] = true | ||
} | ||
} | ||
|
||
/** | ||
* @returns {Promise} | ||
*/ | ||
reloadLocalRoutes () { | ||
return this._getLocalRoutes().then( | ||
(routes) => this.routingTables.addLocalRoutes(routes)) | ||
} | ||
|
||
_getLocalRoutes () { | ||
return Promise.all(this.tradingPairs.map((pair) => { | ||
return this._tradingPairToQuote(pair) | ||
.then((quote) => this._quoteToLocalRoute(quote)) | ||
})) | ||
} | ||
|
||
_tradingPairToQuote (pair) { | ||
const sourceLedger = pair[0].split('@')[1] | ||
const destinationLedger = pair[1].split('@')[1] | ||
// TODO change the backend API to return curves, not points | ||
return co(this.backend.getQuote.bind(this.backend), { | ||
source_ledger: sourceLedger, | ||
destination_ledger: destinationLedger, | ||
source_amount: 100000000 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It creates a curve |
||
}) | ||
} | ||
|
||
_quoteToLocalRoute (quote) { | ||
return { | ||
source_ledger: quote.source_ledger, | ||
destination_ledger: quote.destination_ledger, | ||
connector: this.baseURI, | ||
min_message_window: this.minMessageWindow, | ||
source_account: this.ledgerCredentials[quote.source_ledger].account_uri, | ||
destination_account: this.ledgerCredentials[quote.destination_ledger].account_uri, | ||
points: [ | ||
[0, 0], | ||
[+quote.source_amount, +quote.destination_amount] | ||
] | ||
} | ||
} | ||
} | ||
|
||
module.exports = RouteBroadcaster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since
src/models/quote.js
only exposes one method, it may be clearer to call thisgetQuote
.