-
Notifications
You must be signed in to change notification settings - Fork 500
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/internal/ingest/processors: Refactor liquidity pools, trades, and claimable balances processors to support new ingestion data flow #5025
Conversation
|
||
// Insert updates the wrapped AccountLoader so that the given account | ||
// address is mapped to the provided history account id | ||
func (a AccountLoaderStub) Insert(address string, id int64) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to understand what's the reason for using a stub wrapper vs. adding Insert(address, id)
on to existing AccountLoader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the history ids should be determined by calling the Exec()
method which queries the DB. However, I don't want the unit tests to interact with the db so I added an Insert()
method to allow tests to manually specify history ids for accounts. The reason Insert()
is not defined on the existing AccountLoader is that it's only supposed to be used by unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, which means AccountLoaderStub is only intended for test scope as well? can it be defined in account_loader_test.go instead to drive the test aspect from the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AccountLoaderStub is used by the trades processor test which is in a different package. If AccountLoaderStub were defined in account_loader_test.go it would not be visible to other packages
@@ -47,80 +54,63 @@ func (stats *TradeStats) Map() map[string]interface{} { | |||
} | |||
} | |||
|
|||
func assetToKey(asset xdr.Asset) history.AssetKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about history.AssetKey.FromAsset(asset xdr.Asset)
for general re-use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
home stretch, looks great!
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
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
Similar to #5024 , this PR refactors the LiquidityPoolsTransactionProcessor, the TradesProcessor, and the ClaimableBalancesTransactionProcessor so it conforms to the following interface:
This PR also makes use of the loaders implemented in #5019.
Why
#4909
Known limitations
Several tests are failing because the ingestion framework still needs to be updated to support the new interface. All those tests will be fixed after all the processors have been migrated to the new data flow.