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

int64 in horizon JSON responses #1363

Closed
TorstenStueber opened this issue May 31, 2019 · 5 comments
Closed

int64 in horizon JSON responses #1363

TorstenStueber opened this issue May 31, 2019 · 5 comments
Assignees
Labels
horizon horizon-api Issues or features related to the Horizon API

Comments

@TorstenStueber
Copy link

Horizon uses int64 for some data types internally that end up in JSON resonses as numbers. One example is the entry new_seq in the SequenceBumped effect (see [1] for an example).

Since some int64 numbers cannot be represented as JavaScript numbers (which are 64 bit floating point) there is a risk of potential information loss.

As an example see the last effect in [1]. It is displayed as a bump to sequence number 90071992547409920 in the horizon response. However, the actual effect is a bump to sequence number 90071992547409923. This becomes obvious when looking at the original transaction that created this sequence bump and decoding the XDR: [2].

Hence, the horizon response is wrong! The reason is that 90071992547409923 is not a valid JavaScript number and will be coerced to 90071992547409920.

[1] https://horizon-testnet.stellar.org/accounts/GB5L6RL523F72DBU6UEPI4VRPHYRISP46OWOYL2M324NNRVUPV76PQ6B/effects

[2] https://horizon-testnet.stellar.org/transactions/a1dfac3f3a7b6aa753c20f03450c4559cdfc5311e71a71111e42db183cd2711a

@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 Aug 5, 2019
@abuiles
Copy link
Contributor

abuiles commented Aug 7, 2019

After auditing the code, I found multiple places where we are using int64 where we might want strings instead.

    182:go/support/render/hal/page.go:46:10:	Limit  uint64 `json:"-"`
    360:go/protocols/horizon/operations/main.go:139:10:	OfferID int64 `json:"offer_id"`
    361:go/protocols/horizon/operations/main.go:146:10:	OfferID int64 `json:"offer_id"`
    364:go/protocols/horizon/effects/main.go:201:9:	NewSeq int64 `json:"new_seq"`
    365:go/protocols/horizon/effects/main.go:260:20:	OfferID           int64  `json:"offer_id"`
    367:go/protocols/horizon/main.go:228:21:	ID                 int64      `json:"id"`
    368:go/protocols/horizon/main.go:376:16:	Timestamp     int64     `json:"timestamp"`
    369:go/protocols/horizon/main.go:377:16:	TradeCount    int64     `json:"trade_count"`
    383:go/services/bridge/internal/db/main.go:41:16:	ID            int64     `db:"id" json:"id"`
    384:go/services/bridge/internal/db/main.go:63:16:	ID            int64                 `db:"id" json:"id"`
    884:go/services/ticker/internal/main.go:10:21:	GeneratedAt        int64         `json:"generated_at"`
    885:go/services/ticker/internal/main.go:21:19:	TradeCount24h    int64   `json:"trade_count"`
    886:go/services/ticker/internal/main.go:28:19:	TradeCount7d     int64   `json:"trade_count_7d"`
    887:go/services/ticker/internal/main.go:48:21:	GeneratedAt        int64   `json:"generated_at"`

To fix this issue, I suggest we represent those fields as strings where it makes sense. I have some questions:

  1. From a semver point of view, doing this fix is a breaking change, however, I noticed in the changelog that we have done similar fixes doing only a patch bump. I'd argue that since int64 could lead to inconsistent behavior in JSON, I think it's fine for us to handle this as a patch, since we are fixing a bug in the data format.
  2. There are places where we can probably keep the Int64, but for consistency I think we should, moving forward, use strings to represent int64.

@bartekn @ire-and-curses @tomquisel thoughts?

@tomquisel
Copy link
Contributor

I think you're right about addressing this issue across the board. And maybe there's even some way to have an automated check that prevents it in the future? I also prefer to handle this as a minor version bump. It'll fix issues for some, and break things for others, so let's do the version bump to protect those who will experience breakage.

Will we also need SDK fixes?

@bartekn
Copy link
Contributor

bartekn commented Aug 8, 2019

I agree that we should fix this as soon as possible (patch, minor whatever is first) and agree that we should render all int64's as strings for JS. Would be great to check if the change breaks SDKs, if so, we should wait 2 release cycles to give developers time to update the code.

@abuiles
Copy link
Contributor

abuiles commented Aug 19, 2019

Closing this since the main issue reported here has been addresses in #1604. I created #1609 to continue the conversation on what to do moving forward with int64.

@abuiles
Copy link
Contributor

abuiles commented Jan 20, 2020

This will be fix on the next release of horizon.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants