ASN.1: Add final ILP Payment Packet format (and drafts of ILQP packets) #168

justmoon · 2017-03-14T22:50:04Z

No description provided.

Also includes draft of ILQP formats.

adrianhopebailie · 2017-03-15T09:24:02Z

asn1/InterledgerTypes.asn

+AUTOMATIC TAGS ::=
+BEGIN
+
+IMPORTS


Can drop all but UInt64 from IMPORTS as they are not used

adrianhopebailie · 2017-03-15T09:25:33Z

asn1/InterledgerQuotingProtocol.asn

@@ -2,40 +2,76 @@ InterledgerQuotingProtocol DEFINITIONS AUTOMATIC TAGS ::=
 BEGIN

 IMPORTS


Can drop all but UInt32 and UInt128 from IMPORTS as they are not used

adrianhopebailie · 2017-03-15T09:26:31Z

asn1/InterledgerProtocol.asn

+
+InterledgerProtocolPaymentMessage ::= SEQUENCE {
+    -- Amount which must be received at the destination
+    amount UInt64,


Use Amount type?

adrianhopebailie · 2017-03-15T09:27:16Z

asn1/InterledgerProtocol.asn

+BEGIN
+
+IMPORTS
+    UInt64


Don't import from GenericTypes. Use Amount type instead of UInt64

I was thinking about that. If there is an amount type, we should use it everywhere obviously. But if Amount is just UInt64 I think it's more readable if we just put UInt64 everywhere. For complex types, sure, define it once. But if it's just a primitive type, don't make me go to another file just to find out what it is. What do you think?

adrianhopebailie · 2017-03-15T12:37:36Z

asn1/GenericTypes.asn

+BEGIN
+
+-- Small Integers
+Int8 ::= INTEGER (-128..127)


Do we need all of these types anymore? I appreciate the value of the comments for documenting how the design was finalized but perhaps we can put that somewhere else to keep this clean?

I like having a full suite of generic types, in case we need them in the future. And I like having them in the repo to avoid external dependencies. So my vote is to keep this. It's in a separate file, so I don't see how it would be distracting.

emschwartz · 2017-03-15T12:47:52Z

asn1/InterledgerQuotingProtocol.asn

+  -- amounts) for the quoted route
+    liquidity LiquidityCurve,
+    -- Common prefix of all addresses for which this liquidity curve applies
+    -- SHOULD end with a period "." (but clients MUST NOT enforce this)


I agree but this seems like a bit of an odd thing to say so it might be worth providing more rationale or linking to the ILP addresses spec

emschwartz · 2017-03-15T12:49:35Z

asn1/InterledgerQuotingProtocol.asn

+    destinationPrefix Address,
+    -- How long the sender should put money on hold (in milliseconds)
+    sourceHoldDuration UInt32
+    -- TODO: An exact quote expiry doesn't seem necessary...


Why not? If a connector is giving you a liquidity curve, wouldn't you want them to tell you how long you can use it for? If a sender gets a liquidity curve they might be tempted to cache it for a very long time as they send repeated streaming payments.

Will change to include expiry date.

emschwartz · 2017-03-15T12:53:03Z

asn1/InterledgerQuotingProtocol.asn

-	-- provided in the request.
-	requestedAmount Amount,
-	sourceHoldDuration IlpHoldDuration
+-- Quote with a specific source or destination amount


I think this comment predates switching to having two different messages for quote source and quote destination

emschwartz · 2017-03-15T12:54:12Z

asn1/InterledgerQuotingProtocol.asn

+    destinationAccount Address,
+    sourceAmount Amount,
+    -- How much time the receiver needs to fulfill the payment (in milliseconds)
+    destinationHoldDuration UInt32


What if the sender doesn't know this but does know how long they're willing to put funds on hold for? We might want to suggest some default value in case that isn't known.

Not sure if we should care about that use case. You will always want to hold it for the minimum amount of time. If you don't know how long the recipient needs, you can probably make a reasonable guess or just ask them.

It just doesn't make much sense why you would choose the destination hold based on the sender's willingness to hold. It's like buying paint based on how much they have at the store instead of estimating how much you need.

emschwartz · 2017-03-15T13:00:20Z

asn1/InterledgerTypes.asn

+        | "A".."Z"
+        | underscore
+        | "a".."z"
+        | tilde )


Really not very important, but why are the categories ordered in this way? I would expect alphanumeric characters to come before the symbols

They are ordered by value, i.e. their order in the ASCII table. E.g. 'A' == 0x41, so it comes before '_' == 0x5F, which in turn comes before 'a' == 0x61

emschwartz · 2017-03-15T13:01:32Z

asn1/InterledgerTypes.asn

+
+Amount ::= UInt64
+
+-- Selection of other options considered:


I think this can be taken out

emschwartz · 2017-03-15T13:02:02Z

asn1/InterledgerTypes.asn

+
+-- --------------------------------------------------------------------------
+
+-- Different ledgers may use different rounding parameters


Can this section be taken out?

emschwartz · 2017-03-15T13:03:39Z

asn1/InterledgerTypes.asn

+--
+-- The only amounts
+
+LiquidityCurve ::= SEQUENCE OF SEQUENCE {


Why do we call this a liquidity curve instead of a rate curve?

The rate is the slope of this curve, so the "rate curve" would be the derivative of this curve. Open to another name if anyone has ideas.

emschwartz · 2017-03-15T13:06:37Z

asn1/InterledgerTypes.asn

+-- output amount. Each point must have larger x and y values than the previous
+-- point. All coordinates must be greater than zero.
+--
+-- The only amounts


Oh the suspense! "The only amounts"...what?

emschwartz · 2017-03-15T13:07:32Z

asn1/InterledgerTypes.asn

+-- Liquidity curves describe the relationship between input and output amount
+-- for a given path between a given pair of ledgers.
+--
+-- The curve is expressed as a series of points given as <x, y> coordinates.


If x and y need to be explained in this context, what do you think about just renaming them to something like inputAmount and outputAmount?

mDuo13

I can barely read ASN.1, but what I saw mostly makes sense. Agree w/ @adrianhopebailie and @emschwartz about removing more stuff that's apparently unused. Otherwise, looks fine?

mDuo13 · 2017-03-16T20:54:31Z

asn1/InterledgerTypes.asn

+Address ::= IA5String
+    (FROM
+        ( hyphen
+        | period


Should this definition include the rule that segments cannot be zero-length—that is, you cannot have .. in an ILP address?

I don't think that's possible in ASN.1, but am happy to be proven wrong by someone with better ASN.1-fu.

mDuo13 · 2017-03-16T21:16:45Z

asn1/InterledgerTypes.asn

+-- The curve is expressed as a series of points given as <x, y> coordinates.
+--
+-- Each x coordinate corresponds to an input amount, each y corresponds to an
+-- output amount. Each point must have larger x and y values than the previous


Are you sure both x and y have to be larger? Can you not have a "curve" that plateaus in the middle, e.g.:

y₃ ┨ ╭┄ ┃ ╱ ┃ ╱ y₁,y₂ ┨ ╭╌╌╌╯ ┃ ╱ y₀ ┨╯ ┗━━┳━━━┳━━┳━━ x₀ x₁ x₂ x₃

(in this case y₁=y₂ but x₂>x₁)

In that case, the price is zero/infinity. My initial intuition was that that could result in some mathematical funkiness. But I haven't been able to think of a case. In crypto, you always disallow zero basically everywhere, so it's become my default habit.

The current implementation does allow equal y coordinates, but no equal x coordinates. That means that inverting a curve may give you an invalid curve. Not sure if that matters since we don't really invert curves.

I'd be ok to write what the current implementation does into the spec, but would caution us to think about this point some more.

I think it makes sense that you may have a plateau in the curve. (I haven't thought too deeply into this, but that might be meaningful for assets that have some weird external costs involved in transferring them, or some sort of transition point between transfer methods.) I'd just reword it to say that the x values must always be greater than the previous and the y values must be equal or greater.

justmoon · 2017-03-22T23:50:08Z

@adrianhopebailie @emschwartz @mDuo13 Updated to address your comments.

I used UInt64 as the amount format everywhere rather than giving it an alias. It saves readers from having to look up what Amount is.

That meant taking out the explanation of why we chose UInt64 as the amount because there wasn't an obvious place to put it. (Didn't want to clutter the ILP Packet format with a long-winded, slightly off-topic explanation of the amount data type.) I think that's fine, we can explain it in the ILP RFC instead.

emschwartz · 2017-03-23T08:40:45Z

asn1/InterledgerProtocol.asn

@@ -7,8 +7,7 @@ IMPORTS
    UInt64
    FROM GenericTypes

-    Address,
-    Amount
+    Address
    FROM InterledgerTypes

    MetadataEntry


MetadataEntry should be removed

justmoon added 2 commits March 14, 2017 15:37

chore(ci): make generated docs accessible as artifacts

2291cc0

docs(asn1): add generic type shorthands

6f6bba7

justmoon force-pushed the feat/st-asn1-refresh branch from 5079410 to 05bbe3a Compare March 14, 2017 22:50

justmoon requested review from adrianhopebailie, emschwartz and mDuo13 March 14, 2017 23:51

docs(asn1): update with final ilp payment packet

9716fb7

Also includes draft of ILQP formats.

justmoon force-pushed the feat/st-asn1-refresh branch from 05bbe3a to 9716fb7 Compare March 15, 2017 00:37

adrianhopebailie requested changes Mar 15, 2017

View reviewed changes

adrianhopebailie mentioned this pull request Mar 15, 2017

Discuss ILP Packet Design and Implementation interledger-deprecated/java-ilp-core#17

Closed

emschwartz requested changes Mar 15, 2017

View reviewed changes

sappenin mentioned this pull request Mar 15, 2017

Initial discussion-point commit of proposed InterledgerAddress per IL-RFC 15 interledger-deprecated/java-ilp-core#18

Merged

mDuo13 reviewed Mar 16, 2017

View reviewed changes

docs(asn1): updated asn.1 based on pr feedback

51e2ec2

adrianhopebailie approved these changes Mar 23, 2017

View reviewed changes

emschwartz reviewed Mar 23, 2017

View reviewed changes

emschwartz approved these changes Mar 23, 2017

View reviewed changes

justmoon merged commit 031fc33 into master Mar 23, 2017

justmoon deleted the feat/st-asn1-refresh branch March 23, 2017 17:39

sentientwaffle mentioned this pull request Mar 23, 2017

feat: add LiquidityCurve.{toBuffer,fromBuffer} interledger-deprecated/ilp-routing#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASN.1: Add final ILP Payment Packet format (and drafts of ILQP packets) #168

ASN.1: Add final ILP Payment Packet format (and drafts of ILQP packets) #168

justmoon commented Mar 14, 2017

adrianhopebailie Mar 15, 2017

adrianhopebailie Mar 15, 2017

adrianhopebailie Mar 15, 2017

adrianhopebailie Mar 15, 2017

justmoon Mar 20, 2017

adrianhopebailie Mar 15, 2017

justmoon Mar 20, 2017

emschwartz Mar 15, 2017

emschwartz Mar 15, 2017

justmoon Mar 20, 2017

emschwartz Mar 15, 2017

emschwartz Mar 15, 2017

justmoon Mar 20, 2017

emschwartz Mar 15, 2017

justmoon Mar 15, 2017

emschwartz Mar 15, 2017

emschwartz Mar 15, 2017

emschwartz Mar 15, 2017

justmoon Mar 15, 2017

emschwartz Mar 15, 2017

emschwartz Mar 15, 2017

mDuo13 left a comment

mDuo13 Mar 16, 2017

justmoon Mar 20, 2017

mDuo13 Mar 16, 2017

justmoon Mar 21, 2017

mDuo13 Mar 21, 2017

justmoon commented Mar 22, 2017

emschwartz Mar 23, 2017

		@@ -2,40 +2,76 @@ InterledgerQuotingProtocol DEFINITIONS AUTOMATIC TAGS ::=
		BEGIN

		IMPORTS

+              BEGIN
+              IMPORTS
+                  UInt64


		-- --------------------------------------------------------------------------

		-- Different ledgers may use different rounding parameters

ASN.1: Add final ILP Payment Packet format (and drafts of ILQP packets) #168

ASN.1: Add final ILP Payment Packet format (and drafts of ILQP packets) #168

Conversation

justmoon commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mDuo13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justmoon commented Mar 22, 2017

Choose a reason for hiding this comment