Skip to content
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

BTP Spec: Typo for ILQP subprotocol? #303

Closed
sappenin opened this issue Sep 25, 2017 · 6 comments
Closed

BTP Spec: Typo for ILQP subprotocol? #303

sappenin opened this issue Sep 25, 2017 · 6 comments
Labels

Comments

@sappenin
Copy link
Contributor

The BTP spec under the Message request currently says:

ILQP packets are attached under the protocol name ilp with content-type application/octet-stream.

But shouldn't the protocol for ILPQ be ilqp?

@sappenin sappenin added the bug label Sep 25, 2017
@michielbdejong
Copy link
Contributor

michielbdejong commented Sep 26, 2017

You could argue 'ilqp' would have been a better choice there, but it's not a typo. The reasoning is that ILQP and InterledgerProtocolPayment ('IPP'), with the ILP Error format they share, together form 'ilp', as they are exactly the two things that each connector needs to implement. Since then we discussed if it would be possible to have a connector that implements IPP but not ILQP, but in BTP, ilp = ipp + ilqp + ilp errors.

@sappenin
Copy link
Contributor Author

sappenin commented Sep 26, 2017

Ok, I can see that rationale, and thanks for the clarification -- in other words, the ILP payload for a message is ILQP, and the ILP payload for a PrepareRequest is IPP, and so on - so we're just calling all of this the ILP subprotocol.

I'll close this issue then.

@adrianhopebailie
Copy link
Collaborator

in BTP, ilp = ipp + ilqp + ilp errors.

Whoa! Where is this documented?

ILP and ILQP are different protocols that happen to share a common packet envelope. I have no intentions of implementing anything related to ILQP until we decide:

  1. It's required
  2. It's not going to change anymore

@sappenin
Copy link
Contributor Author

So, I was thinking more about this, and think that the usage of ilp as the sub-protocol here is actually incorrect. Here are two reasons.

First, it breaks the boundary between BTP and a Subprotocol carried by BTP. Ordinarily, a Subprotocol should exist independent of its envelope, but by using the sub-protocol ilp in the Message to actually mean "ilqp packets", and then using that same subprotocol (ilp) to mean "ILP payment packets" in other BTP envelope (e.g., PrepareTransferRequest) means that I can't actually discern the meaning of the subprotocol called ilp without knowing the envelope it was delivered in -- i.e., was the ilp payload delivered in a Message or a Prepare envelope?

Second, this problem becomes more acute when creating software to actually read a payload of data found in a particular subprotocol. Ordinarily, I would have something like ILQPPayloadReader, and it would be invoked by passing it some bits. However, with the above naming error, my ILQPPayloadReader now has to know which BTP envelope delivered it. Again, this is illustration of protocol boundary leakage.

I would argue that the subprotocol naming mechanism should be able to stand on its own, regardless of BTP envelope. This would mean the Message should have a subprotocol object of type ilqp for quote request/response, but it could also carry an ilp payment packet (I know we don't have a use-case for that currently, but in the future we might).

@sharafian
Copy link

sharafian commented Sep 27, 2017

ILP and ILQP use the same registry of packet types, as described in the ASN.1. You can always look at the first byte of the message to determine exactly what kind of ILP or ILQP message it is, without looking at the surrounding BTP message. Usually we expect ILP and ILQP to be implemented in the same module, so that's why the ilp subprotocol can contain either; it's being handled by a parser that understands both.

@sappenin
Copy link
Contributor Author

Ah, yes, great points - from that perspective, I think it does make sense to use ilp as the "subprotocol".

As you can probably infer from my comments in this issue, I can't decide if I should think of ILQP and ILP as being the same thing, or two different things. I suppose Michiel's equation is the most accurate thing: ilp = ipp + ilqp + ilp errors

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants