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

horizon: Ingest Liquidity Pool Ledger Entries #3815

Merged
merged 23 commits into from
Aug 13, 2021
Merged

horizon: Ingest Liquidity Pool Ledger Entries #3815

merged 23 commits into from
Aug 13, 2021

Conversation

2opremio
Copy link
Contributor

Fixes #3805

tamirms
tamirms reviewed Aug 10, 2021
View reviewed changes
services/horizon/internal/db2/schema/migrations/49_liquidity_pools.sql
@@ -0,0 +1,31 @@
-- +migrate Up

CREATE TABLE liquidity_pools (
Copy link
Contributor

Choose a reason for hiding this comment

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

@sydneynotthecity just wanted to let you know about the new tables we're going to add in horizon to support CAP-38. Not all the schema changes will be included in this PR because we've broken down the tasks over a bunch of issues:

https://github.com/stellar/go/issues?q=is%3Aissue+is%3Aopen+label%3Aamm

I will make sure to tag you in each PR which introduces a schema change so you can be up to date

Copy link

Choose a reason for hiding this comment

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

thanks, @tamirms! appreciate the heads up and will review each one. Will these changes be a part of the Nov release?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The schema has been updated to use a single table (and store the reserves in a json blob just like we do for the claimants in the claimable_balances table)

Copy link
Contributor

@tamirms tamirms Aug 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@sydneynotthecity

Will these changes be a part of the Nov release?

yeah, I think November is when the protocol is scheduled to be upgraded. At that point liquidity pools will be enabled in pubnet. But, we aim to finish horizon support for liquidity pools by the end of August so we can have sufficient time to test the functionality ahead of the protocol launch.

@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch from b6ce2c2 to 1427766 Compare August 11, 2021 11:57
sydneynotthecity
sydneynotthecity reviewed Aug 11, 2021
View reviewed changes
Copy link

@sydneynotthecity sydneynotthecity left a comment

Choose a reason for hiding this comment

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

LGTM! Had one nit and a couple questions for my own understanding. Will create the subsequent issues in stellar-etl to account for liquidity pools.

services/horizon/internal/db2/history/liquidity_pools.go Outdated
case db2.OrderAscending:
if r != nil {
sql = sql.
Where(sq.Expr("lp.id > ?", r))
Copy link

Choose a reason for hiding this comment

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

❓ Is this a way to limit the amount of results returned? Or is the where clause used as a way to paginate the query?

services/horizon/internal/db2/history/liquidity_pools.go Outdated Show resolved Hide resolved
services/horizon/internal/db2/history/liquidity_pools.go Outdated Show resolved Hide resolved
services/horizon/internal/db2/schema/migrations/49_liquidity_pools.sql
@@ -0,0 +1,31 @@
-- +migrate Up

CREATE TABLE liquidity_pools (
Copy link

Choose a reason for hiding this comment

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

thanks, @tamirms! appreciate the heads up and will review each one. Will these changes be a part of the Nov release?

services/horizon/internal/db2/schema/migrations/49_liquidity_pools.sql Outdated Show resolved Hide resolved
@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch from 3ed0cfb to aa00199 Compare August 11, 2021 17:58
@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch 2 times, most recently from 4acad65 to 5526fa6 Compare August 11, 2021 18:49
@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch from 5526fa6 to 2752759 Compare August 11, 2021 19:02
@2opremio
Copy link
Contributor Author

@tamirms this is ready for review. Tests are failing at:

=== RUN   TestFuzzOffers
--- FAIL: TestFuzzOffers (0.00s)
panic: logger already overridden [recovered]
	panic: logger already overridden

goroutine 221 [running]:
testing.tRunner.func1.2(0x19f8060, 0x1cdfa80)
	/opt/local/lib/go/src/testing/testing.go:1143 +0x332
testing.tRunner.func1(0xc000443080)
	/opt/local/lib/go/src/testing/testing.go:1146 +0x4b6
panic(0x19f8060, 0x1cdfa80)
	/opt/local/lib/go/src/runtime/panic.go:965 +0x1b9
github.com/stellar/go/services/horizon/internal/test.OverrideLogger(...)
	/Users/fons/stellar-go/services/horizon/internal/test/main.go:68
github.com/stellar/go/services/horizon/internal/test.Start(0xc000443080, 0x22cfca0)
	/Users/fons/stellar-go/services/horizon/internal/test/main.go:99 +0x378
github.com/stellar/go/services/horizon/internal/ingest/processors.TestFuzzOffers(0xc000443080)
	/Users/fons/stellar-go/services/horizon/internal/ingest/processors/offers_processor_test.go:23 +0x45
testing.tRunner(0xc000443080, 0x1b8cf20)
	/opt/local/lib/go/src/testing/testing.go:1193 +0xef
created by testing.(*T).Run
	/opt/local/lib/go/src/testing/testing.go:1238 +0x2b3

There seems to be a clash in the use of test.Start() between TestFuzzOffers and TestLiquidityPools. I will investigate tomorrow.

@2opremio 2opremio marked this pull request as ready for review August 11, 2021 19:11
@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch 2 times, most recently from cdf9026 to e5e7a10 Compare August 12, 2021 17:47
@2opremio 2opremio changed the title [WIP] horizon: Ingest Liquidity Pool Ledger Entries horizon: Ingest Liquidity Pool Ledger Entries Aug 12, 2021
@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch 3 times, most recently from 6c6c2b8 to 32aadda Compare August 12, 2021 17:55
@2opremio 2opremio force-pushed the amm-ingest-liquidity-pool branch from 32aadda to 2d00973 Compare August 12, 2021 18:19
@2opremio 2opremio mentioned this pull request Aug 12, 2021
Closed
7 tasks
@2opremio
Copy link
Contributor Author

@tamirms PTAL

tamirms
tamirms approved these changes Aug 13, 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.

looks great!

@2opremio 2opremio merged commit e769c87 into stellar:amm Aug 13, 2021
@2opremio 2opremio deleted the amm-ingest-liquidity-pool branch August 13, 2021 08:16
bartekn
bartekn reviewed Aug 23, 2021
View reviewed changes
services/horizon/internal/db2/history/liquidity_pools.go

// GetLiquidityPoolsByID finds all liquidity pools by PoolId
func (q *Q) GetLiquidityPoolsByID(ctx context.Context, poolIDs []string) ([]LiquidityPool, error) {
var cBalances []LiquidityPool
Copy link
Contributor

Choose a reason for hiding this comment

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

cBalances -> liquidityPools.

services/horizon/internal/db2/history/liquidity_pools.go
return lp, err
}

// GetLiquidityPools finds all liquidity pools where accountID is one of the claimants
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment needs updating.

services/horizon/internal/ingest/processors/liquidity_pools_processor.go
if err != nil {
return errors.Wrap(err, "Error creating ledger key")
}
rowsAffected, err = p.qLiquidityPools.RemoveLiquidityPool(ctx, hex.EncodeToString(lPool.LiquidityPoolId[:]))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: We added PoolIDToString in https://github.com/stellar/go/pull/3826/files#diff-aaf89e53adf03495063ecc74f1e3725e7f299fa8b5679f6592174ae6d12fa2c3R138.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonjove jonjove jonjove left review comments

@sydneynotthecity sydneynotthecity sydneynotthecity left review comments

@bartekn bartekn bartekn 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.
5 participants
@2opremio @bartekn @tamirms @sydneynotthecity @jonjove