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

Include operation information in the ingest.Change struct #5536

Merged
merged 29 commits into from
Dec 3, 2024
Merged

Include operation information in the ingest.Change struct #5536

merged 29 commits into from
Dec 3, 2024

Conversation

karthikiyer56
Copy link
Contributor

@karthikiyer56 karthikiyer56 commented Nov 20, 2024 •
edited
Loading

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 reviewed the changes in this PR and if I consider them worthwhile for being mentioned on release notes then I have updated the relevant CHANGELOG.md within the component folder structure. For example, if I changed horizon, then I updated (services/horizon/CHANGELOG.md. I add a new line item describing the change and reference to this PR. If I don't update a CHANGELOG, I acknowledge this PR's change may not be mentioned in future release notes.
  • 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

This PR deals with this github issue - #5535

Why

Details in the associated github issue - github issue - #5535

Tl;DR:

  • This serves as as the basis for identifying exactly what operation/activity caused the change in ledgreEntry
  • This change serves as a precursor for writing several generic change/transaction processors, primarily offer+trade processor

Known limitations

These extensions to the Change struct do not work when the compacting change reader is used.

[TODO or N/A]

@karthikiyer56 karthikiyer56 force-pushed the karthik/5535/Extend-change-struct-with-operationDetails branch from ba3ad68 to d158205 Compare November 20, 2024 08:40
tamirms
tamirms reviewed Nov 20, 2024
View reviewed changes
ingest/change.go Outdated Show resolved Hide resolved
tamirms
tamirms reviewed Nov 21, 2024
View reviewed changes
ingest/change.go Outdated Show resolved Hide resolved
tamirms
tamirms reviewed Nov 21, 2024
View reviewed changes
ingest/change.go Outdated Show resolved Hide resolved
tamirms
tamirms reviewed Nov 21, 2024
View reviewed changes
ingest/change.go Outdated Show resolved Hide resolved
@tamirms
Copy link
Contributor

tamirms commented Nov 21, 2024

@karthikiyer56 TestLiquidityPoolDepositDetails in master/services/horizon/internal/ingest/processors/transaction_operation_wrapper_test.go is one of the unit tests that is failing on your branch.

I looked into it and the reason it is failing is because the ingest.LedgerTransaction instance in the test is not complete. It is missing the transaction envelope.

The test exercises a code path where GetOperationChanges() is called on the ingest.LedgerTransaction instance. It fails there because you have modified the code to include operation details into the change. The operation info is pulled from the transaction envelope (which is the right thing to do). However, because the test case does not populate the ingest.LedgerTransaction instance with a transaction envelop, the operation is not found and that causes the test not to pass.

@karthikiyer56
Copy link
Contributor Author

karthikiyer56 commented Nov 21, 2024 •
edited
Loading

@karthikiyer56 TestLiquidityPoolDepositDetails in master/services/horizon/internal/ingest/processors/transaction_operation_wrapper_test.go is one of the unit tests that is failing on your branch.

I looked into it and the reason it is failing is because the ingest.LedgerTransaction instance in the test is not complete. It is missing the transaction envelope.

The test exercises a code path where GetOperationChanges() is called on the ingest.LedgerTransaction instance. It fails there because you have modified the code to include operation details into the change. The operation info is pulled from the transaction envelope (which is the right thing to do). However, because the test case does not populate the ingest.LedgerTransaction instance with a transaction envelop, the operation is not found and that causes the test not to pass.

Yup.
Earlier in the day, when the code was fetching operational results, more tests were failing because the ledger operations were missing in more tests.
I tried fixing them, and they still were failing, even when i did add operations to a few test fixtures

So, i decided to simplify the data returned in the change structure 😓
Just as well, since we can derive more details from just envelope and tx result, at the call site itself, when we use the code in trades processing.

At any rate,i thought the latest changes should not cause any unit test failures anymore. I'll look into the one that is failing now.

Good call-out.
Ima just pass a pointer to the ingest.edgerTransaction itself in change,instead of TX envelope and result pair

tamirms
tamirms reviewed Nov 22, 2024
View reviewed changes
ingest/change.go Outdated Show resolved Hide resolved
@karthikiyer56
- Several changes to integration.go to pull out common functions from…
dccf1e2 
… parameters_test.go to configure captive core

- Add flags to capture state for upgrade version in test.
- Add support to skip container deletion for debugging

- Add test for upgrade related changes
@karthikiyer56 karthikiyer56 requested a review from a team December 1, 2024 07:53
tamirms
tamirms approved these changes Dec 3, 2024
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.

looks great! I left a couple suggestions regarding style

@karthikiyer56 karthikiyer56 changed the title Include operation information in the Change struct Include operation information in the ingest.Change struct Dec 3, 2024
@karthikiyer56 karthikiyer56 merged commit 9b89f4b into master Dec 3, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sydneynotthecity sydneynotthecity sydneynotthecity left review comments

@Shaptic Shaptic Shaptic left review comments

@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.
4 participants
@karthikiyer56 @tamirms @Shaptic @sydneynotthecity