go
go copied to clipboard
stellar
Make sure integer types in protocols/horizon are consistent with XDR
Originally posted by @leighmcculloch in https://github.com/stellar/go/pull/3845#discussion_r697659878:
I understand the motivation is because whilst the XDR is int32, Core will only use positive values.
I think this inconsistency in types could come back to bite us at some point. If the values are inconsistent it is always possible for the protocol to change in a way that breaks the Horizon API. And it's very common in applications to know that an int value will only be positive, so I don't see the problem this solves.
Another way to think about this is that we're trying to constrain the possible values the field will have to only positive values, however by using an uint32 all we've done is shifted the possible values into another range that also won't be used, positive values greater than what a int32 will hold.
If the goal is to capture the space of usable values, the positive side of int32 is uint16.
However, inconsistencies in Horizon's API and the XDR are inconvenient and confusing to developers using both, so I'd vote for just matching the XDR.
The positive side of int32 is uint31, not uint16. The sign bit only causes the representation to be divided by 2.
@bartekn @leighmcculloch to be clear, the scope of the current issue is only the new AMM code?
I was thinking we should probably do the same for all the code, although it will entail a breaking change.
I think we should do it for all fields in the API but I'm not sure when. Looks like Protocol 18 upgrade could be a good moment because everyone will need to upgrade and update their clients anyway. Let's discuss in Horizon meeting.
We discussed this today in the Horizon team meeting. I'd rather not proceed with this for now.
The change is breaking for clients but doesn't solve a problem anybody has. It does reduce surprise for readers of the XDR, but that is more of a nice to have. It is possible the protocol might someday change in a way that affects this, but if we haven't fixed it by then, that would seem to be a forcing function.
Even after these changes, there are places where we will still continue to map types (most notably, strings for large numbers).
This topic is arising now because we are adding more fields. However, as long as we continue to be consistent with existing type approaches there is no problem changing these (and all the others) in the future. An ideal opportunity for this refactor would be when we implement API versioning.
For future context: Core originally switched to signed types in order to avoid issues with their internal SQL data store (https://github.com/stellar/stellar-core/issues/1960). The relevant code change was https://github.com/stellar/stellar-core/commit/d9e80975d5b8edcca54e9eb0143fd0b4796f05fc.
Makes sense.
as long as we continue to be consistent with existing type approaches there is no problem changing these (and all the others) in the future
However, does this have the knock-on affect that Horizon will use different types for new fields? I don't think there's much value in continuing this approach of using uint32 instead of int32 for new fields even if there's no plan to address past fields.
I'm proposing we make the new fields uint32 as well, for consistency. The value is that they are all the same, no surprises. Changing to new types going forward adds complexity for the user.
Since we want to refactor the others anyway eventually, fixing these at that time seems fine.
@ire-and-curses do we want to close it or keep it open in the Backlog? Currently it's open but in Done section.
We can leave it open for now. @paulbellamy and I started discussing how to clean up the backlog, we'll adjust status more then if needed.