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

services/horizon: Change NewSeq attribute in SequenceBumped to be a string. #1604

Merged

Conversation

abuiles
Copy link
Contributor

@abuiles abuiles commented Aug 17, 2019

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • [ X] 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

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.

Summary

Change data type for field NewSeq in SequenceBumped effect from int64 to string. Fixes #1363.

Goal and scope

Update horizon's JSON response for field new_seq in NewSequenceBumped effect to return a string instead of an int64. Using an int64 produces inconsistent results for clients using JavaScript since some int64 numbers cannot be represented as JavaScript numbers.

Summary of changes

  1. Add a new struct to represent SequenceBumped at the database (history) level.
  2. Update the effects resource adapter to use the new struct when parsing the jsonb in effects and then pass the value as a string to the horizon representation of effect.

Known limitations & issues

This will likely break clients who are expecting an int64 instead of a string.

What shouldn't be reviewed

N/A

@abuiles abuiles changed the title services/horizon: Change NewSeq attribute in SequenceBumped to be of type string. services/horizon: Change NewSeq attribute in SequenceBumped to be a string. Aug 17, 2019
@abuiles abuiles changed the base branch from master to release-horizonclient-v2.0.0 August 17, 2019 23:49
@abuiles abuiles force-pushed the fix-data-type-for-new-seq-sequence-bump branch from ec0e31a to 3d41bde Compare August 17, 2019 23:52
@abuiles abuiles changed the base branch from release-horizonclient-v2.0.0 to release-horizon-v0.20.0 August 17, 2019 23:53
@abuiles abuiles requested review from ire-and-curses, tamirms and bartekn and removed request for ire-and-curses August 17, 2019 23:53
@abuiles abuiles force-pushed the fix-data-type-for-new-seq-sequence-bump branch from 3d41bde to 6d635c3 Compare August 17, 2019 23:57
services/horizon/CHANGELOG.md Show resolved Hide resolved
protocols/horizon/effects/main.go Outdated Show resolved Hide resolved
services/horizon/internal/db2/history/main.go Show resolved Hide resolved
@abuiles abuiles force-pushed the fix-data-type-for-new-seq-sequence-bump branch from 0fa9e63 to b070904 Compare August 19, 2019 16:37
@abuiles abuiles requested a review from bartekn August 19, 2019 16:39
@abuiles
Copy link
Contributor Author

abuiles commented Aug 19, 2019

@bartekn I addressed your comments, 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.

3 participants