docs: introduce CommonLedgerProtocol

[michielbdejong]

This PR aims to find a compromise between all our different views. It differs from #263 in that it does not allow for requestId=0, it distinguishes Response from ErrorResponse in the CallSet, and it leaves CallSet id's reserved for non-ILP Prepare/Fulfill/Reject.

With these minor changes, CLP still has only one native protocol, namely Interledger. All other protocols have to use SideProtocolData. However, the CallSet could easily be extended with other native protocols. Additionally, it could easily be extended with conditionally paid custom requests, as well as unconditional ones.

As Ben and I discussed this afternoon, it may be a useful compromise not to define these currently unused features yet, but to accommodate for possibly adding them in the future.

[michielbdejong]

It also fixes a few syntax errors in asn documents it depends on, and renames the GenericPacket document to InterledgerPacket, to match the name of the object it describes.

[adrianhopebailie]

This proposal still doesn't address my concern about separating the logical and messaging layers.

Eg. It is impossible to send an InterledgerReject as a response to an InterledgerPrepare.

[adrianhopebailie]

Where is Response defined?

[michielbdejong]

That may have been missing from this morning's version, it's now here: https://github.com/interledger/rfcs/pull/271/files#diff-2ed9291d851bc56085df61488a4d3b10R44

[adrianhopebailie]

I'd prefer if an Ack was simple Ack ::= NULL.

If there is a need to return a response that carries side protocol data then define a LocalResponse :: = SideProtocolData

[michielbdejong]

There is (now) an Ack type and a Response type, which are both the same (they are also the same as Message), in that they only contain protocolData. The difference between Ack and Response is subtle, and arguably useless, but it's a bit like the difference between return and return undefined in JavaScript. The effect is the same, the semantics it expresses is slightly different.

[adrianhopebailie]

I prefer the name "LocalRequest". The absence of an internetworking packet implies this request is not being relayed.

[michielbdejong]

Ben and I agreed to merge native protocols and named protocols into just numbered protocols. :) So there is now only 'protocolData', and the first entry in that list will usually have a protocolId of 1, meaning it's an InterledgerPacket.

[sharafian]

LGTM, but we should get some input on:

1. Should we use a UInt8, UInt16, ..., or VarUInt for protocol ID? (I'm leaning UInt16 or UInt8)

2. Should there be any ordering for protocol data? (i.e. should ILP always come first). I don't think it should be necessary but there could be tricky bugs that appear due to ordering of protocols

3. Given that an Ack is a response with no protocols, does it need to be separate from Response. I do think that it's worth having Message separate, though.

[michielbdejong]

OK, I'm cool with UInt16 instead of UInt8, and with removing Ack if @emschwartz agrees.

About ordering of protocols - I think only the first protocol packet in a request should affect the response, the subsequent ones should be just piggybacking side protocols that may support responding to the first one, but don't get a response themselves. So putting a getAvailableLiquidity and a QuoteByDestinationAmount together in one requests makes no sense, and when you add something like 'UpdatedPaychanClaim' packet to an 'InterledgerProtocolPayment', the actual request which results in the fulfill should come first, before the side-effect ones.

[sharafian]

cool, I think UInt16 should be a good choice. As for the protocol ordering, I kind of see what you mean where it may be ambiguous which protocols in the request correspond to which protocols in the response, but I think that could be better handled by the definitions of the protocols being used. If incompatible packets are being included (like QuoteByDestinationResponse and QuoteBySourceResponse in the same packet), it should cause an error in any case.

[michielbdejong]

If incompatible packets are being included (like QuoteByDestinationResponse and QuoteBySourceResponse in the same packet), it should cause an error in any case.

Right, I guess we can assume the peers know exactly which protocols the other peer supports, so there's no reason someone would ever mess that up. Cool, i think we're good then, let's wait for review from @emschwartz and @justmoon.

[adrianhopebailie]

This has become a generic (not ILP specific) protocol which:

1. Assumes that a transfer always has a condition
2. Assume a transfer response is always a fulfillment
3. Uses ILP errors
4. Has a logical id for tranfers (transferId) but not for messages

I'm now very confused what the goal is.

[adrianhopebailie]

This doesn't make sense. Why would a generic error contain InterledgerError data?

[michielbdejong]

we said we reuse InterledgerError codes instead of inventing a new system of OER-encoded errors for CLP.

[adrianhopebailie]

See @emschwartz comment below re using ILP packets outside their wrapper.

Also, this is lazy design. Those errors are for ILP. If we need errors for this protocol we should define them.

[emschwartz]

I think it would make sense to use the same error schema here and define a set of CLP-specific errors (maybe with codes that start with L). Most of them are already defined in the LPI so I would use those as a starting point

[sharafian]

do we include the envelope with typeId and length prefix for the error, though?

[michielbdejong]

Oh, but if you use the Message clp-message to ask for a quote (or a Prepare clp-message to request an Interledger Payment), and get back an Error clp-message, then it might contain an error from the connector, or an error from the ledger.

Likewise, if you receive a Reject call, its rejectionReason might come from the connector or (in case of timeout) from the ledger.

[michielbdejong]

Therefore I think it makes sense to just use IlpError wrapped in an InterledgerPacket envelope.

[adrianhopebailie]

I agree with @justmoon here. We shouldn't be passing ILP Errors directly at this level. They should be created at the connector and encapsulate the CLP error if that was the cause.

[dappelt]

So if the ledger triggers L99 - AcmePay Is Not Feeling Like It, the connector should not relay it (as an L99) but rather it should emit its own T01 - Ledger Unreachable. They should feel free to include the ledger error in the freeform debug info if they want, because that doesn't force anyone to understand it.

@justmoon In your example, in what kind of packet would the relayed error T01 - Ledger Unreachable be wrapped? Since T01 is an IlpError, I assume it would go inside a CLP Error, with protocolData carrying the ILP error? Or would it go into protocolData of a CLP Response, since the local CLP link works fine and the error occurred in a different CLP link?

[justmoon]

@dappelt Good question. I wrote down some suggested flows that make sense to me, but interested in feedback.

[michielbdejong]

Assumes that a transfer always has a condition

For now we just use Unpaid and paid-conditional requests, but unconditional-paid requests can be added. In fact, I was in favor of that (for completeness, mainly), I left them out because Ben and Evan didn't want to add things which we currently don't use.

Assume a transfer response is always a fulfillment

It can be a fulfillment, meaning succes, or a rejection, meaning error. Rejections carry an error packet. Fulfillments don't have to carry anything, but are allowed to carry any packet from any protocol you want.

Uses ILP errors

yeah, I agree it's a bit funny but it avoids having to set up a whole extra error registry.

Has a logical id for tranfers (transferId) but not for messages

The reason for that is that messages are responded to in the response, but conditional transfers are not. I tried to convince Evan and Ben that the fulfill/reject that comes back from a Prepare is like a request/response, but they finally convinced me that a fulfill is a request in itself, where as the fulfiller you want something from the other party. That's why there are two flows:

Message flow:

[requestId: 1] -> Message
[requestId: 1] <- Response

or

[requestId: 1] -> Message
[requestId: 1] <- Error

and

Conditional Transfer Flow:

[requestId: 2] -> Prepare
[requestId: 2] <- Ack
[requestId: 3] <- Fulfill
[requestId: 3] -> Ack

or

[requestId: 2] -> Prepare
[requestId: 2] <- Ack
[requestId: 3] <- Reject
[requestId: 3] -> Ack

or
[requestId: 2] -> Prepare
[requestId: 2] <- Ack
[requestId: 3] <- Fulfill
[requestId: 3] -> Error

or

[requestId: 2] -> Prepare
[requestId: 2] <- Ack
[requestId: 3] <- Reject
[requestId: 3] -> Error

or

[requestId: 2] -> Prepare
[requestId: 2] <- Error

[justmoon]

Whoops, Adrian commented "Why can't this be NULL?" - I tried to respond, then wanted to cancel responding and accidentally deleted Adrian's comment. ^^

Anyway, my comment is: I can't say that you would never want to include data with an ack and setting this to ProtocolData costs one byte and enables lots of use cases in the future. Maybe I want to send a balance update with the ack or the local timestamp when the message was received, or performance data like "it took 8.2ms to process this request" etc.

[justmoon]

I much prefer a string here. You're hardly losing any bytes. ilp for example is four bytes vs one byte. And you are so much less likely to have collisions between different vendor-specific extensions and you can see which protocol you're dealing with.

I would also include a mime type, which would be a one byte field with the options 0 = application/octet-string, 1 = text/plain; charset=utf-8, 2 = application/json, with more potentially being added in the future. As I've said in another thread, not including the mime type turned out to be a huge mistake in Ripple Memos, because it made it impossible to print any useful information for debugging.

Maybe one difference in our viewpoints is that I definitely expect more than 256 protocols here. Ripple is going to define its own and I'm sure most vendors will. Most settlement methods will define their own protocols here.

[michielbdejong]

OK, as discussed in the research team meeting, changed protocolId to protocolName, and added the mime types.

[justmoon]

You should give this a type with named values, like so:

ContentType ::= INTEGER {
  applicationOctetString  (0),
  textPlainUtf8           (1),
  applicationJson         (2)
} (0..255)

[emschwartz]

What is the expected behavior if you don't respond to a given request? Should the server keep sending you the message?

[michielbdejong]

Hm, hadn't really thought about that much.
If Alice sends a request to Bob then I guess three things can happen:

1. Bob responds with Ack or Response
2. Bob responds with Error
3. Bob doesn't respond, or responds with a message that Alice doesn't understand.

If Bob responds multiple times, I guess only the first response counts.
If Bob doesn't respond:

• if Alice's request was a request for information, then she can just consider it as failed.
• if Alice's request was a request to update the state of a transfer, then she doesn't know if Bob processed her request, so they're now in limbo on that transfer's state. They should then use either manual reconciliation or a reconciliation protocol, to debug/resolve that.

I guess even with comments this asn can't stand on its own because of questions like this - maybe this is something we should write up in an RFC?

[emschwartz]

That is precisely the purpose behind having the option to set requestId to 0 if you don't care about whether you get a response back.

Protocols that use these data formats should define what to do in these cases. Doesn't need to be in the ASN.1 but we will also need an RFC written up that defines how to use CLP.

[sharafian]

I agree that in an asymmetric trustline, you often want to send messages without caring about response. But Adrian raised a good point that if all notifications have ID 0 it's hard to do any logging with them. Furthermore, in a client/server relationship the server never cares whether the client responds, so it can be agreed at establishment that the client doesn't respond, rather than on a per-message basis

[justmoon]

in a client/server relationship the server never cares whether the client responds

How so? What if the server is configured to re-send after a timeout if the client hasn't responded?

[michielbdejong]

The scenario Ben refers to is where you have 1 or more clients listening at the same time. But you're right, the server may be configured to resend all notifications if none of the clients answered.

[emschwartz]

I don't think this comment is necessary. The ILP packet is the InterledgerProtocolPayment inside the packet wrapper. Interledger packets should never appear without that wrapper with the type and the length. Is there somewhere that isn't clear?

[michielbdejong]

Will merge as once @adrianhopebailie approves.

[adrianhopebailie]

I agree with @emschwartz that we should merge soon and get some implementation experience.

That said, I still have some concerns so feel free to override and merge anyway if you think my concerns are unfounded.

Errors

Errors in the CLP are not ILP Errors. Re-using that packet for CLP specific errors is in-correct and re-use just because it's convenient is lazy.

@justmoon gave a good example of scenario where an error caused at the ledger layer should be conveyed in a response in this protocol but is now hard to do because the ledger must figure out how to package that as an ILP Error.

Further to that, the Error type can carry protocol data so it could carry an ILP Error packet in there anyway.

Logical and Messaging concepts mixed

I keep raising this but clearly I'm not making the point clearly or I'm shouting in the wind. As @sharafian and @justmoon describe above there is a flow we expect for transfers but it seems we expect something different for messages?

Logically, when a node requests a Prepare then it will get either a Fulfillment or Error in response. However, we've prescribed that this MUST be done asynchronously by saying it will be:

Prepare ->
Ack <-
Fulfill <-
Ack ->

Presumably the first two messages have the same requestId to allow them to be matched (what I'd call the message identifier). We also define that the Prepare and Fulfill have a transferId so that even though they are in different request/response message pairs they can be matched (they have a common logical identifier).

This begs the question how a quote request/response can be sent or must it always be synchronous? In the following flow there is no way to match the QuoteRequest/QuoteResponse.

Message(QuoteRequest) ->
Ack <-
Response(QuoteResponse) <-
Ack <-

So, maybe we should have a messageId in the Message, Response and possibly Error types so that there is a logical identifier for non-transfer messages?

[sappenin]

@adrianhopebailie above you wrote:

This begs the question how a quote request/response can be sent or must it always be synchronous? In the following flow there is no way to match the QuoteRequest/QuoteResponse.

Message(QuoteRequest) ->
Ack <-
Response(QuoteResponse) <-
Ack <-

But why wouldn't messages look more like this instead:

Message(QuoteRequest) ->
Response <-
Message(QuoteResponse) <-
Response ->

[michielbdejong]

I think we should merge now, since we've already started implementing this, both for ilp-kit and for testnet-of-testnets. We can't let those project be stalled.

I do agree with Adrian though, and as I said in https://lists.w3.org/Archives/Public/public-interledger/2017Aug/0021.html, I think we should design a better version 2, which we can launch later, for instance on 1 january 2018.

[adrianhopebailie]

@michielbdejong implementing is entirely up to you. If you want to start before the spec is final then you may just have more to change when it does. Also, implementation experience is also helpful to inform the design.

I don't think we should stop working on this because we have run out of energy and want to move onto other things. There are still a number of unanswered questions. This is a simple spec so we should be able to solve them.

That said, I'd be happy to merge this and log issues against the spec as I think this discussion is becoming unwieldy. BUT, I don't think we should take that as license to ignore these issues and turn all of our attention to implementations.

[sappenin]

Can this just be an IA5String so we don't have CLP re-defining Content-Types?

[justmoon]

This field is intended to give applications that don't know/understand the custom protocol a way to at least partially decode the data. For example. we have values to indicate whether it's binary or text data. We intentionally want a small, defined list of supported formats here, NOT support any arbitrary mime type. If we want to add a format to the list, there should be broad agreement in the Interledger community that's it is worth adding this format to a large number of implementations. If the values in this field aren't widely understood the field is not useful.

So I don't think we redefining anything - we're just defining a short list of mime types that are widely understood and since it's a small, standardized list, an enum is the correct type to use. I just realized that there is an ENUMERATED type in ASN.1 which would perhaps be an even better fit than INTEGER.

[sappenin]

This field is intended to give applications that don't know/understand the custom protocol a way to at least partially decode the data.

Ok, perhaps I've been viewing this field a bit differently. I was thinking this content-type would be useful for applications that do understand the side-protocol...i.e., it gives those applications a mechanism to decode the data. For example, if I have a side-protocol named aml_protocol, then perhaps this payload is encoded as JSON, perhaps it's binary. Are you suggesting that if aml_protocol wants different encodings, then one should create separate protocolNames, like aml_protocol_json and aml_protocol_oer?

[justmoon]

I do agree with Adrian though, and as I said in https://lists.w3.org/Archives/Public/public-interledger/2017Aug/0021.html, I think we should design a better version 2, which we can launch later, for instance on 1 january 2018.

I agree that we should freeze v1 asap, but I don't think you can finalize a spec without implementation experience. (Ideally multiple implementations by different people, but given the current size of our community, I'll take a single implementation.)

What that means is that we can use this as a draft, implement it, play around with it for a bit and when we notice that we're making fewer and less urgent changes, let's call it v1 and make it final. If we want to change it after that, it'll be v2 and be separate.

[adrianhopebailie]

It's been almost a week since I raised two issues regarding: 1. errors and 2. the conflation of logical and message semantics and yet most of the discussion has been about whether or not to freeze the spec 😄

If we just resolve those two issues I'd be very supportive of freezing the spec

[dappelt]

Discussing all these issues in the same PR becomes unwieldy. I propose to merge this PR as an initial draft. This draft should not be frozen and we should adapt it based on first implementation experiences. I think we can all agree on that (@adrianhopebailie, please approve so we can merge).

@adrianhopebailie, I agree with you on the issue around errors (see my latest comment, input appreciated). Also, I would like to add a 3. item to the open issue list: Given that we have a message type Response, we don't need Ack (discussion here)

[justmoon]

Quick point which I don't think has been said explicitly: Can a protocol appear more than once in the protocol data? I'd vote no for simplicity, but could be convinced by a strong use case.

[michielbdejong]

Adrian wrote:

I'd be happy to merge this and log issues against the spec as I think this discussion is becoming unwieldy. BUT, I don't think we should take that as license to ignore these issues and turn all of our attention to implementations.

Agreed! I'll merge this so that the discussion can continue. Also, we shouldn't forget to write the actual rfc, so far this just defines the asn.1 format.

Can a protocol appear more than once in the protocol data?

I think just once, that's what I tried to convey in https://github.com/interledger/rfcs/pull/271/files#diff-2ed9291d851bc56085df61488a4d3b10R21 but we should describe things like that in an rfc document.

[michielbdejong]

We can keep discussing changes like removing Ack and changing the way errors are encoded, but I settled on commit 8b65d63 for the testnet of testnets. It is documented here: https://github.com/interledger/interledger/wiki/Interledger-over-CLP and implemented here: interledgerjs/btp-packet#4.

as discussed via bluejeans just now