interledger / rfcs

Specifications for Interledger and related protocols
https://interledger.org
Other
453 stars 111 forks source link

Remove .to, .ledger and .outgoing from Transfer and Request objects #234

Closed michielbdejong closed 7 years ago

michielbdejong commented 7 years ago

The .ledger field is always equal to the 'prefix' query parameter The .to field is always ledger + other peer's public key The .direction field in a Transfer is always "outgoing"

michielbdejong commented 7 years ago

This is about the send_transfer and send_request rpc calls.

michielbdejong commented 7 years ago

We could probably also remove source_account and source_ledger from route broadcasts.

michielbdejong commented 7 years ago

With these changes, the binary version of send_request would look like [ { "ilp": "ARwAAAAAB1TUwA5nLnVzLm5leHVzLmJvYgMEEEEA" } ] so that even could be simplified to just: ARwAAAAAB1TUwA5nLnVzLm5leHVzLmJvYgMEEEEA or maybe [ "ARwAAAAAB1TUwA5nLnVzLm5leHVzLmJvYgMEEEEA" ] to match how fulfill_condition looks.

sharafian commented 7 years ago

I think we can start with removing to, ledger, direction, and from. That way it's only a plugin level change. Removing the source_account from route broadcasts sounds like a separate issue

michielbdejong commented 7 years ago

I guess for send_request it's pretty obvious that there's a one-to-one mapping between the ILQP requester / responder and the http request / responder, and that's also how we currently use send_transfer, but maybe the question is, would Alice ever communicate the preparation of a transfer to Bob, where Alice is not herself the from and/or Bob is not himself the to?

sharafian commented 7 years ago

Not on a bilateral ledger like plugin-virtual or a payment channel

michielbdejong commented 7 years ago

What about pull payments?

sharafian commented 7 years ago

pull payments should be implemented as a application level protocol, not as a ledger-layer feature.

The one thing I can think of is if there's several client plugins they might want to be notified of each other's activity. It would complicate the API a lot though, and we don't have any real use-case for that feature.

michielbdejong commented 7 years ago

ok, i'll create a PR :)

justmoon commented 7 years ago

What is the recommended migration path for plugins that are not trustlines? (Gatehub, five-bells-ledger)

sharafian commented 7 years ago

The to/from/ledger fields will stay in the LPI, they can just be removed from the RPC requests that bilateral plugins use. I'm not sure how it would work for the routing protocol; the source_account might have to stay in to support many-to-many ledgers.

michielbdejong commented 7 years ago

I think the only role of the source_account is that a route announcement will be discarded if the sender filled that field with the wrong value (it has to be equal to the message sender, so in no case does it carry any information).

justmoon commented 7 years ago

@sharafian

After thinking about this some more: I recommend keeping the to and from, even in the RPC.

The reason we switched from account to from/to to begin with was to make the object less context-dependent. It's more difficult to understand data the more its meaning changes based on context. If I'm looking at the log of an RPC call, it's very useful to be able to see who the sender thought they were and who they thought they were talking to. (I understand that the latter may be visible in the URL, but iiuc that that isn't guaranteed.)

I don't think simplicity counts as an argument to remove to and from, since you are removing them from the physical representation of the object only. To understand the message, you still need to know the values for to and from, it's just that now you have the added work of having to get them from somewhere else.

Performance optimization would be a valid argument to remove them, but I think we can agree that that's a premature optimization until we switch to a binary protocol. If forward compatibility is a concern, I would say we should make the fields optional, have the RPC receiver complain if they are present-but-wrong and have the RPC sender include them by default.

As for ledger, I think that that can be removed (both from LPI and from RPC) due to Interledger enlightenment. If all balances are independent, it's a bit arbitrary what you call a "ledger" and in many cases there may be no such thing. So it arguably doesn't add much value to have a field for this concept.

sharafian commented 7 years ago

(I understand that the latter may be visible in the URL, but iiuc that that isn't guaranteed.)

I'm not sure what you mean here. Any time there's an RPC call, the "to" account is the RPC URI. The "from" account is the other party. We're already relying on the URI for required information like method and prefix. So in the context of the over-the-wire RPC call I believe we can always tell who it's for. In the LPI version of the object where there's no URI included, I think it's worth keeping those fields in.

The worry about performance is less important to me than useless configuration. Every trustline has to negotiate two account names in order to be used. These account names add no value. Every RPC call you make is for your peer, and every RPC call you receive is from your peer.

michielbdejong commented 7 years ago

I agree with Ben's point about negotiation. In WebFinger-based peering, from/to are set to the public keys, but in ilp_secret-based peering this is not possible. To remedy this, you could always set from to 'peer.ledger.client' and always set to to 'peer.ledger.server', so using relative instead of absolute names (relative to the client/server roles in the http request) but that would mean those roles reverse depending on who is sending the payment, so that's also not ideal. Or you could each generate a uuid as your account name, so basically a random string, but that's also silly.

When I'm looking at my server logs I have to know anyway whether a debug statement refers to an incoming request to my server, or to an outgoing request made by my server. And Ben makes a valid point, if you want the post body to be self-contained then we should also move the method from the query parameter into the post body, which would also seem an arbitrary protocol change. I think it's not too weird if the post body is not self-contained; when logging it, it's probably reasonable to log not only the post body, but also the post query, and the fact that this is a request that your server received (not a http request in which your server acts as the sender).

michielbdejong commented 7 years ago

Given the amount of discussion about how we can improve the protocol, maybe we should start versioning it. As discussed at lunch, an ilp-secret that starts with the string ilp_secret:v1: would define an rpc endpoint + credentials for the current RPC protocol.

In the interest of interoperability, I think we should probably focus on getting things working with the current protocol (v1), and save all proposed improvements for v2. I would also propose that (as a community) we commit to v1 for a full six months. So we can talk all we want about what v2 will look like, but we're not going to implement it until January 2018. That way we aggregate incremental changes into a semesterly update cycle (that's basically the same as what Ubuntu did for Linux).

So maybe we can tag this issue as a v2 discussion?

michielbdejong commented 7 years ago

These fields were removed with the switch from RPC-JSON-HTTP to BTP-OER-WebSockets.