protocols/horizon: Change Transaction.AccountSequence to int64 #4409
Conversation
bartekn
force-pushed
the
horizon-accountsequence-int64
branch
from
468b91c to
70b9753
Compare
| @@ -14,11 +14,12 @@ import ( | |||
| ) | |||
|
|
|||
| func TestNewOperationAllTypesCovered(t *testing.T) { | |||
| tx := &history.Transaction{TransactionWithoutLedger: history.TransactionWithoutLedger{AccountSequence: "1"}} | |||
There was a problem hiding this comment.
would it help for consistency of AccountSequence type to also have 'TransactionWithoutLedget.AccountSequence' type int64 also?
| @@ -511,7 +511,7 @@ type Transaction struct { | |||
| Account string `json:"source_account"` | |||
| AccountMuxed string `json:"account_muxed,omitempty"` | |||
| AccountMuxedID uint64 `json:"account_muxed_id,omitempty,string"` | |||
| AccountSequence string `json:"source_account_sequence"` | |||
| AccountSequence int64 `json:"source_account_sequence,string"` | |||
There was a problem hiding this comment.
Is this a breaking change to the Go SDK?
Also, we have some other fields like this (that are strings but could be int64s). We should probably do a pass to find them all (this came up briefly during P19).
There was a problem hiding this comment.
Is this a breaking change to the Go SDK?
Yes, this is a breaking change. However this will probably allow Go SDK users to remove some conversion code. Are we ok with merging this into master?
Also, we have some other fields like this (that are strings but could be int64s). We should probably do a pass to find them all (this came up briefly during P19).
Sounds good. I changed this one because it simplifies the code in Starbridge. I did a quick pass and couldn't find anything else but if there are other instances of this we should definitely change them to ints.
There was a problem hiding this comment.
💯 We should do this. It moves validation of the response data into the SDK so if the response contained a non-integer string the failure would occur at response parsing rather than later on in application code, so this is a good move and I doubt anyone would be disappointed by it. Thanks for shipping it! Let's do this in all the integer fields we can.
There was a problem hiding this comment.
Added a line to horizonclient CHANGELOG for visibility.
Change `protocols/horizon/Account.Sequence` to `int64` from `string`. There shouldn't be any extra parsing required in Go since `Account.Sequence` returned by Horizon will be always a valid int64 value. We can make sure the value is rendered as a string to support JS using `string` tag. Similar to: #4409
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
Change
protocols/horizon.Transaction.AccountSequencetoint64fromstring.Why
There shouldn't be any extra parsing required in Go since
AccountSequencereturned by Horizon will be always a valid int64 value. We can make sure the value is rendered as a string to support JS usingstringtag.Known limitations
[TODO or N/A]