-
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
Conversation
9e4f829
to
d79a30f
Compare
@sentientwaffle or @justmoon is there a document for this feature? |
57c238e
to
41860af
Compare
41860af
to
3b4fc0b
Compare
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
Well, eventually routes will arrive on a transfer's additional_info
(as opposed to the dedicated /routes
endpoint), so the sending connector's identity would be implicitly verified when they authorize the debit.
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.
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 comment
The 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.
Not fully understanding everything, but I didn't notice any real issues. I'm not sure if it is necessary to use classes here, it may be simpler to use functions since the classes don't seem to own any state, but not a blocker. LGTM after tests pass. |
|
||
// 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 comment
The 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 const DEFAULT_MIN_MESSAGE_WINDOW
or something like that?
* @param {Object} config | ||
* @returns {Quote[]} | ||
* @returns {Transfer} |
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.
If I request a quote with a fixed source amount, how can I tell what the destination amount is?
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.
quote.credits[0].memo.destination_transfer
will have the destination amount.
Ok, finally made it through the whole thing. The routing changes look good to me overall (there are some details I think could be changed/improved). The main issue I have with this PR is the change to the connector quote API. I think we might want to have a dedicated quote object that includes details like source and final (recipient) amounts, the expiry time or window, and the transfer (in whatever format the source ledger expects). Especially when we start dealing with different transfer object types it seems like it would be useful to return a consistent object that gives you some details, along with the (opaque) transfer object that you should submit to your ledger to kick the process off. (The only problem I could see with this is you might want to verify that the transfer isn't asking for more money than you wanted to send, or something like that) I may be missing something but:
In general, we need to think through how to make sure the recipient of a payment can validate everything they need to about it to avoid having connectors modifying the payment instructions and getting away with it. In theory it doesn't matter if a connector modifies the payment in transit because the sender can check the transfer they're sending to the first connector and the recipient should check the incoming transfer details before signing the receipt. In order to make sure no one gets screwed with this though, we need to be very clear about what the recipient must check. |
I agree with @emschwartz. Right now the connector returns the transfer object for convenience, but this is probably not a good idea. First of all, the transfer object is going to look different for each type of ledger. It's an object from the ledger layer, whereas the quoting happens on the Interledger layer. So we're mixing concerns here. Secondly, the sender should carefully check every field in the transfer object. By returning a pre-filled object, we risk that the sender might not check anything or - if they do - they still might forget to check for any unexpected additional fields. What we should return then in the quote request is the minimum required information using ILP standard formats. In addition to that, there should be a field for ledger-specific extensibility. |
d84184e
to
dc67b8c
Compare
After discussing with @sentientwaffle we decided that since #150 is a blocker for other things and changing the quote response would take until tomorrow, we should merge this now and do the quote response change in a separate PR. LGTM after tests pass. |
Requires interledger-deprecated/five-bells-shared#134
Requires interledger-deprecated/ilp-routing#2
Corresponding sender changes: interledger-deprecated/five-bells-sender#65