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

Dates in Horizon response not ISO8601 compliant. #1381

Closed
Synesso opened this issue Jun 10, 2019 · 4 comments
Closed

Dates in Horizon response not ISO8601 compliant. #1381

Synesso opened this issue Jun 10, 2019 · 4 comments
Assignees
Labels
horizon horizon-api Issues or features related to the Horizon API small fix

Comments

@Synesso
Copy link
Contributor

Synesso commented Jun 10, 2019

I'm assuming here that dates returned by Horizon should meet ISO8601.

This transaction contains a value for the year which does not meet the standard.

An expanded year representation [±YYYYY] must have an agreed-upon number of extra year digits beyond the four-digit minimum, and it must be prefixed with a + or − sign

@Synesso
Copy link
Contributor Author

Synesso commented Jun 10, 2019

@jem that looks like a horizon bug. the tx xdr has a timeBounds.maxTime that makes sense.

@tomerweller
Copy link
Contributor

Specifically, the TX Envelope for this TX contains: maxTime: 1591765704893, which is June 10, 2020 5:08:24.893 AM UTC, not 52411-01-18T04:14:53Z as the valid_before field might suggest.

@bartekn bartekn added this to the Horizon 0.19.0 milestone Jun 13, 2019
@ire-and-curses ire-and-curses added the horizon-api Issues or features related to the Horizon API label Jul 19, 2019
@abuiles abuiles self-assigned this Jul 24, 2019
@bartekn bartekn removed this from the Horizon 0.18.1 milestone Jul 24, 2019
@abuiles
Copy link
Contributor

abuiles commented Aug 5, 2019

After investigating this problem, I don't think this is a bug but expected behavior (up to some point).

There are a couple of mysteries here. Let's start by the first one:

Is the upper limit 1591765704893 in the time bound equal to June 10, 2020 5:08:24.893 AM UTC?

No, Horizon handles time-bounds as epoch values in seconds, however if you paste the value in an online tool like https://www.epochconverter.com/, it will show June 10, 2020. The problem is, those tools take seconds or milliseconds. In this scenario, they are handling the value as if it were milliseconds but in reality they are seconds and the value is a date, far, far, far away in the future. We might not even exist by then (but that's a different conversation).

The culprit here was probably someone submitting a transaction which was built in JS and they forgot to divide by 1000, epoch in JS land is in milliseconds. See the XDR in the Stellar laboratory

The second mystery is:

Why are we showing a year with 5 digit but don't have ± as specified in the expansion session? See https://en.wikipedia.org/wiki/ISO_8601#cite_note-19
An expanded year representation [±YYYYY] must have an agreed-upon number of extra year digits beyond the four-digit minimum, and it must be prefixed with a + or − sign

I think an important distinction to make here is that we are not really returning a ISO8601 string but rather a RFC3339 string, which is an extension of ISO8601, as such, it has some slightly deviations from ISO8601, the most important for this case is that they assume all dates to be in the current era:

All dates and times are assumed to be in the "current era", somewhere between 0000AD and 9999AD.
https://tools.ietf.org/html/rfc3339#section-1

This becomes clear by looking at the ABNF:

date-fullyear   = 4DIGIT
date-month      = 2DIGIT  ; 01-12
date-mday       = 2DIGIT  ; 01-28, 01-29, 01-30, 01-31 based on
                          ; month/year
time-hour       = 2DIGIT  ; 00-23
time-minute     = 2DIGIT  ; 00-59
time-second     = 2DIGIT  ; 00-58, 00-59, 00-60 based on leap second
                          ; rules
time-secfrac    = "." 1*DIGIT
time-numoffset  = ("+" / "-") time-hour ":" time-minute
time-offset     = "Z" / time-numoffset

partial-time    = time-hour ":" time-minute ":" time-second
                  [time-secfrac]
full-date       = date-fullyear "-" date-month "-" date-mday
full-time       = partial-time time-offset

date-time       = full-date "T" full-time

Horizon hasn't agreed to handle years with more than 4 digits, hence we don't use the ± extension. However, that leave us with the last mystery which is:

Why are we seeing a 5 digit year when RFC3339 says that it only handles 4 digits?

I didn't look at the Format implementation in golang, but I tested this in other languages and they all return 52411-01-18T04:14:53Z when converting to RFC3339.

My assumption here is that they are doing it because they can. We are storing the value as an Int64, which let us represent epoch until 2^63 - 1. 1591765704893 is less than 2^63 - 1, so when we convert the value to Unix and then to format, it does the right thing and show the year 52411, ignoring the 4 digits restriction from RFC3339.

We can close this issue, but I'd like to leave the following questions:

@ire-and-curses @tomerweller:

  • Shall we update the docs to say valid_after and valid_before return a RFC3339 date rather than ISO8601 string?
  • Do we want to be strict on the timebounds and only allow dates in the current era?

@abuiles abuiles closed this as completed Aug 5, 2019
@ire-and-curses
Copy link
Member

Ah, the famous Y10K bug!

IMHO: 1) we should clarify RFC3339 and 2) we should be strict, since the provided timebounds were probably not what was intended.

abuiles added a commit to abuiles/go that referenced this issue Aug 7, 2019
Let's make clear that we are returning an RFC3339 date-time and not ISO8601. There are some deviations between both formats, specifically, RFC3339 expects the years to be in the current era.

See stellar#1381 for more info.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
horizon horizon-api Issues or features related to the Horizon API small fix
Projects
None yet
Development

No branches or pull requests

7 participants
@abuiles @Synesso @bartekn @tomerweller @tomquisel @ire-and-curses and others