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.

Sign up for GitHub

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

Jump to bottom

services/horizon: Issue 3305, Claimable Balance API Extension #3483

Merged
merged 49 commits into from
Mar 24, 2021
Merged

services/horizon: Issue 3305, Claimable Balance API Extension #3483

merged 49 commits into from
Mar 24, 2021

Conversation

paulbellamy
Copy link
Contributor

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

Track transactions and operations for claimable balances, even after claimable balances are claimed.

Why

#3305

Known limitations

[TODO or N/A]

@paulbellamy paulbellamy requested a review from a team March 22, 2021 20:52
@2opremio
Copy link
Contributor

2opremio commented Mar 22, 2021 •
edited
Loading

We still need to look into (and further extend) the integration tests.

bartekn
bartekn reviewed Mar 23, 2021
View reviewed changes
Copy link
Contributor

@bartekn bartekn 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! Before 👍 I think we need to change two things:

  1. Store IDs as hex instead of base64 in a DB. It will allow removing most of the marshaling/unmarshaling code and will make it much easier to read.
  2. Check the comments connected to validation. I think we can simplify the code in actions.

services/horizon/internal/httpx/router.go Outdated Show resolved Hide resolved
services/horizon/internal/httpx/router.go Show resolved Hide resolved
services/horizon/internal/actions/operation.go Outdated Show resolved Hide resolved
services/horizon/internal/actions/transaction.go Outdated Show resolved Hide resolved
services/horizon/internal/actions/operation.go Outdated Show resolved Hide resolved
services/horizon/internal/db2/history/operation.go Outdated Show resolved Hide resolved
services/horizon/internal/ingest/processors/claimable_balances_transaction_processor.go Show resolved Hide resolved
services/horizon/internal/ingest/processors/claimable_balances_transaction_processor.go Outdated Show resolved Hide resolved
services/horizon/internal/ingest/processors/operations_processor.go Outdated Show resolved Hide resolved
services/horizon/internal/integration/protocol14_test.go Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the 3305-claimable-balance-api-extension branch from 5ac7420 to 80d5a4e Compare March 23, 2021 11:04
@2opremio 2opremio changed the title services/horizon: WIP Issue 3305, Claimable Balance API Extension services/horizon: Issue 3305, Claimable Balance API Extension Mar 23, 2021
@2opremio 2opremio force-pushed the 3305-claimable-balance-api-extension branch from 15cc1fa to 3450fa7 Compare March 23, 2021 17:00
@paulbellamy paulbellamy mentioned this pull request Mar 23, 2021
Closed
7 tasks
@2opremio 2opremio mentioned this pull request Mar 23, 2021
Closed
@bartekn bartekn mentioned this pull request Mar 24, 2021
Closed
@paulbellamy paulbellamy force-pushed the 3305-claimable-balance-api-extension branch from 9b89b19 to fc26bd0 Compare March 24, 2021 14:09
@bartekn bartekn linked an issue Mar 24, 2021 that may be closed by this pull request
Feature Request: Expose the transaction and operation(s) that created the claimable balance #3305
Closed
@2opremio
Copy link
Contributor

Good to go?

tamirms
tamirms approved these changes Mar 24, 2021
View reviewed changes
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.

🎉

@2opremio 2opremio merged commit 4469029 into master Mar 24, 2021
@2opremio 2opremio deleted the 3305-claimable-balance-api-extension branch March 24, 2021 19:51
paulbellamy pushed a commit to paulbellamy/new-docs that referenced this pull request Apr 1, 2021
update docs for stellar/go#3483
cc2add3
paulbellamy pushed a commit to paulbellamy/new-docs that referenced this pull request Apr 7, 2021
update docs for stellar/go#3483
f826587
@paulbellamy paulbellamy mentioned this pull request Apr 7, 2021
Open
This was referenced Jun 2, 2021
Closed
Open
Closed
Closed
Closed
Open
Closed
Closed
Closed
Open
Open
Closed
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@2opremio 2opremio 2opremio left review comments

@Shaptic Shaptic Shaptic left review comments

@bartekn bartekn bartekn approved these changes

@tamirms tamirms tamirms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature Request: Expose the transaction and operation(s) that created the claimable balance
5 participants
@paulbellamy @2opremio @bartekn @Shaptic @tamirms