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

protocols/horizon/effects: Add UnmarshalJSON to SequenceBumped. #1909

Merged

Conversation

abuiles
Copy link
Contributor

@abuiles abuiles commented Nov 11, 2019

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    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

Add UnmarshalJSON to SequenceBumped, this method supports new_seq as a string or a int64 in the JSON payload. This will allow us to cut a new release for the horizon client which won't break once we change the data type on the JSON payload.

Why

Using Int64 in JSON payloads produces inconsistent results in JS (see #1363).

We are in the process of updating all the int64 JSON fields to be of type string. The idea with this change is to extend the Unmarshal function to take either int64 or string, that way we can release a version of the horizon client which won't break once the payload changes in the API.

See #1609

Known limitations

[TODO or N/A]

Copy link
Member

@ire-and-curses ire-and-curses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just small comments

protocols/horizon/effects/main.go Outdated Show resolved Hide resolved
clients/horizonclient/effect_request_test.go Outdated Show resolved Hide resolved
@tamirms
Copy link
Contributor

tamirms commented Nov 12, 2019

you can use json.Number instead of an interface map:

https://play.golang.org/p/epDKjGKeg7d

@abuiles
Copy link
Contributor Author

abuiles commented Nov 12, 2019

@ire-and-curses I split the test so it's clear what is being tested.

@tamirms I updated the code to use json.Number - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants