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: Ingest Liquidity Pool Trades #3857

Merged
merged 9 commits into from
Aug 30, 2021
Merged

services/horizon: Ingest Liquidity Pool Trades #3857

merged 9 commits into from
Aug 30, 2021

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Aug 26, 2021 •
edited
Loading

Fixes #3810

This PR implements ingestion the db functionality required for ingestion. I still need to implement functions and tests to query liquidity pool trades. The remaining functionality will be implemented in #3835

@tamirms tamirms force-pushed the trades branch 2 times, most recently from 49a4d03 to 970c2e7 Compare August 26, 2021 21:55
@tamirms tamirms changed the title [WIP] Ingest Liquidity Pool Trades Ingest Liquidity Pool Trades Aug 28, 2021
@tamirms tamirms marked this pull request as ready for review August 28, 2021 10:37
@tamirms tamirms changed the title Ingest Liquidity Pool Trades services/horizon: Ingest Liquidity Pool Trades Aug 28, 2021
@tamirms tamirms requested a review from a team August 30, 2021 06:09
bartekn
bartekn approved these changes Aug 30, 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.

LGTM! When reading the code I started wondering if it's possible that the trade against LP is successful but the LP doesn't change (ex. <1 stroop trade) so the meta changes are not generated for it. If it's true then there are several places where we rely on this. It's not clear from CAP so we should check with Stellar-Core team.

services/horizon/internal/db2/history/trade.go Outdated Show resolved Hide resolved
services/horizon/internal/ingest/processors/trades_processor.go Outdated Show resolved Hide resolved
services/horizon/internal/ingest/processors/trades_processor.go
}

for _, trade := range p.trades {
row := trade.row
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't row a copy? So all updates below will update a copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's ok because in the end we are calling on batch.Add() on row. trade.row is never used anywhere

services/horizon/internal/ingest/processors/trades_processor.go Outdated Show resolved Hide resolved
services/horizon/internal/ingest/processors/trades_processor.go
if fee, err = p.findPoolFee(transaction, opidx, id); err != nil {
return nil, err
}
row.LiquidityPoolFee = null.IntFrom(int64(fee))
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find setting row.*LiquidityPoolID fields. Should we do it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bartekn that is done in Commit(). In this function we gather all the liqudity pool id strings. Then in commit we map the liquidity pool id strings to their int ids from the history_liquidity_pools table. Once we have the mapping we set the row.*LiquidityPoolID fields with the int id values

@tamirms
Copy link
Contributor Author

tamirms commented Aug 30, 2021

When reading the code I started wondering if it's possible that the trade against LP is successful but the LP doesn't change (ex. <1 stroop trade) so the meta changes are not generated for it. If it's true then there are several places where we rely on this. It's not clear from CAP so we should check with Stellar-Core team.

I asked this to the Stellar-Core team and they said:

[it] should still generate the meta for two reasons:

  • we had to load the entry to modify it, and everything we load to modify will end up in the meta (although it may end up in
  • the meta as unchanged, as you noted in the case of no-op or do-undo)
    fees have accrued, so I don't think it's actually possible for the state to end up exactly the same as it started.

https://github.com/stellar/stellar-protocol/blob/master/core/cap-0017.md if we ever get around to it would change the behavior of the meta with respect to no-ops

@tamirms tamirms merged commit 5ea473b into stellar:amm Aug 30, 2021
@tamirms tamirms deleted the trades branch August 30, 2021 18:33
@bartekn bartekn mentioned this pull request Aug 31, 2021
Closed
@tamirms tamirms mentioned this pull request Sep 2, 2021
Merged
This was referenced Sep 28, 2021
Closed
Merged
bartekn added a commit that referenced this pull request Sep 29, 2021
@bartekn
services/horizon: Ingest base and counter offer id when offer consume…
8c276d1 
…d on creation (#3970)

Add synthetic offer IDs (removed in #3857) to trades in which a maker offer was
not assigned an ID by Stellar-Core. Removing synthetic IDs is a breaking change
but even without it they are used in non-liquidity pool trades.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartekn bartekn bartekn 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.
2 participants
@tamirms @bartekn