-
Notifications
You must be signed in to change notification settings - Fork 106
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
docs(asn1): ilp version 4 I guess #361
Conversation
61af505
to
9c74c7a
Compare
asn1/InterledgerProtocol.asn
Outdated
executionCondition UInt256, | ||
|
||
-- Expiry date | ||
expiresAt Timestamp, |
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.
Can we put expiresAt
before the executionCondition
? It's a minor point but that way the two fields that are modified at every hop come first and connectors leave the rest the same
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.
Can we also add a comment to the expiresAt
to indicate that it changes at each hop (like the comment on local amount)?
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.
executionCondition
is a fixed size field, whereas expiresAt
is a variable-length field with a length prefix. The idea here was that we felt that it was more important to put all the fixed size fields at the beginning than all the modifiable fields.
In case you're surprised that Timestamp is variable length: The CANONICAL-OER spec says to use the DER spec and the DER spec says that you must leave out trailing zeros in the milliseconds and you must leave out the period if the milliseconds are entirely zero. The rest is always required. It also says you must use UTC time (Z
), not local time zones.
So, the spec says:
2017-12-24T16:14:32.279Z
->20171224161432.279Z
2017-12-24T16:14:32.200Z
->20171224161432.2Z
2017-12-24T16:14:32.000Z
->20171224161432Z
2017-12-24T16:14:30.000Z
->20171224161430Z
2017-12-24T16:14:00.000Z
->20171224161400Z
2017-12-24T16:10:00.000Z
->20171224161000Z
2017-12-24T16:00:00.000Z
->20171224160000Z
2017-12-24T10:00:00.000Z
->20171224100000Z
2017-12-24T00:00:00.000Z
->20171224000000Z
2017-12-24T18:14:32.000+0200
->20171224161432Z
There was a bit of a hubbub at ITU at one point because they had defined GeneralizedTime in terms of ISO 8601 and then later realized that ISO 8601 actually allows infinite precision, not just milliseconds. This was resolved by saying that GeneralizedTime was limited to millisecond precision. (Which is what most implementors had used.)
The DER spec says that you must use the period as the decimal separator. (ISO 8601 allows both period and comma.) The BER spec also says that midnight must be 00:00:00
on the following day. In BER and OER the seconds and minutes are optional, but in DER and C-OER they are required. So these are all invalid:
20171224215312.4318Z
-> INVALID, too much precision20171224161432,279Z
-> INVALID, wrong decimal element20171224240000Z
-> INVALID, wrong representation of midnight201712242153Z
-> INVALID, missing seconds2017122421Z
-> INVALID, missing seconds and minutes
The overall conclusion from reviewing GeneralizedTime: Time is very subtle, but I really like what ISO/ITU ended up with. Fixed-length would have been nice, but it makes sense why they didn't go that way. I need to look at our implementation again to make sure it's correct. I also wrote up a first draft of an OER guide for Interledger.
-- Information for sender (transport layer information) | ||
data OCTET STRING (SIZE (0..32767)) | ||
} | ||
|
||
InterledgerProtocolRejectionData ::= SEQUENCE { | ||
InterledgerReject ::= SEQUENCE { |
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.
I'm probably just not understanding the totality of the new flow, but why is the forwardedBy
no longer in this error? Won't the receiver of a reject-message pass it back to any prior nodes in the payment chain?
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.
The flow is the same but we thought the forwardedBy
field was unnecessary. Connectors can pass the error back without needing to modify it to add their address. The feeling was that this field was a bit annoying to deal with, it has the potential to get very large and be abused, and doesn't provide that much value because you mostly care about knowing who threw the error.
(This PR doesn't actually modify IL-RFC 4) |
InterledgerProtocolPaymentData ::= SEQUENCE { | ||
-- Amount which must be received at the destination | ||
InterledgerPrepare ::= SEQUENCE { | ||
-- Local amount (changes at each hop) | ||
amount UInt64, |
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.
Why is the amount back in the ILP packet?
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.
This time it's the transfer amount, not the destination amount
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.
I don't follow why this is required at this layer?
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.
Because of #359
f5f1df3
to
3f2e264
Compare
3f2e264
to
d6fa3f3
Compare
No description provided.