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: Add last_modified_time to account responses #2528

Merged
merged 3 commits into from
Apr 30, 2020

Conversation

2opremio
Copy link
Contributor

@2opremio 2opremio commented Apr 29, 2020

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 last_modified_time to account responses (making it consistent with offers).

last_modified_time is the closing time of the most recent ledger in which the account was modified.

Also, get rid of the account query consistency check, comparing Horizon's DB and Core's DB.

Why

Fixes #1945

Known limitations

It adds an additional DB query per account which can negatively affect performance.

@2opremio 2opremio requested review from abuiles and bartekn April 29, 2020 12:53
@cla-bot cla-bot bot added the cla: yes label Apr 29, 2020
@2opremio 2opremio changed the title services/horizon: Add last_modified_time to accounts services/horizon: Add last_modified_time to account responses Apr 29, 2020
@2opremio 2opremio force-pushed the 1945_accounts_last_modified_time branch from 8ed254b to 14a12b7 Compare April 29, 2020 13:27
@2opremio 2opremio requested review from bartekn and tamirms April 29, 2020 13:27
@2opremio
Copy link
Contributor Author

@bartekn PTAL

Copy link
Contributor

@tamirms tamirms left a comment

Choose a reason for hiding this comment

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

looks good! I think we should update the changelog with a note about the new last_modified_time field in the account response

@2opremio
Copy link
Contributor Author

I think we should update the changelog with a note about the new last_modified_time field in the account response

@tamirms I added it, but I wasn't sure what format to follow for unreleased changes in master. PTAL

@2opremio 2opremio force-pushed the 1945_accounts_last_modified_time branch from 356107c to 83d9687 Compare April 30, 2020 16:00
Also, get rid of the account query consistency check, comparing Horizon's DB and
Core's DB.
@2opremio 2opremio force-pushed the 1945_accounts_last_modified_time branch from 83d9687 to bd175ed Compare April 30, 2020 20:15
@2opremio 2opremio merged commit 3aecc7e into master Apr 30, 2020
@2opremio 2opremio deleted the 1945_accounts_last_modified_time branch April 30, 2020 21:31
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.

Feature Request: Add last_modified_time to account response
3 participants