services/horizon: Add an endpoint that allows querying for which liquidity pools an account is participating in #4043
Conversation
|
|
||
| // MakeTestPool is a helper to make liquidity pools for testing purposes. It's | ||
| // public because it's used in other test suites. | ||
| func MakeTestPool(A xdr.Asset, a uint64, B xdr.Asset, b uint64) LiquidityPool { |
There was a problem hiding this comment.
note for reviewers: these are intentionally public as they're used both in these tests and in the actions' tests.
erika-sdf
changed the title
erika-sdf
changed the title
| handler := GetLiquidityPoolsHandler{} | ||
| response, err := handler.GetResourcePage(httptest.NewRecorder(), request) | ||
| assert.NoError(t, err) | ||
| assert.Len(t, response, 1) |
There was a problem hiding this comment.
Worth checking the actual response if the LP is the one we expect.
There was a problem hiding this comment.
This check has been added.
| }, | ||
| LastModifiedLedger: 123, | ||
| } | ||
| lp := MakeTestPool(usdAsset, 450, xlmAsset, 450) |
There was a problem hiding this comment.
I can't see a test checking Account filter. Maybe worth adding one?
There was a problem hiding this comment.
I was curious about this (did most of the test work): the db code path is already checked by using the actions code path. It seems like the tests are very similar to the action ones. Is there a benefit to having them in both places?
There was a problem hiding this comment.
The main idea we had when doing a refactoring was that actions package will not use DB directly but mocks. And because we have integration tests framework, the entire stack should be tested there. Unfortunately previous action tests did not have this rule so quite a lot new tests access DB too.
I think it's fine if we test it in one place. What we could also do instead here is add a very simple (a few lines of code) test sending a new query and ensuring the result is correct in TestLiquidityPoolHappyPath test.
There was a problem hiding this comment.
A test has been added here to test querying by account id. Thanks for @Shaptic for helping debug.
| } | ||
|
|
||
| if len(query.Account) > 0 { | ||
| return q.findLiquidityPoolsByAccountId(ctx, query.Account) |
There was a problem hiding this comment.
I don't think there's a need to extract this to a separate method. First, it's easy to miss stuff, for example currently the code does not apply cursor so pagination probably doesn't work. Second, it actually duplicates code: deleted where clause is added in two methods.
I think we can rewrite selectLiquidityPools to have named table lp. Then in a single if statement we can set the query we want to work on and add filters.
There was a problem hiding this comment.
I have combined this into a single method, and set the filters in an if/else branch
Co-authored-by: Bartek Nowotarski <[email protected]>
The order is expected, actual so the error messages make more sense if we swap the ID order.
|
@erika-sdf please remember to squash commits before merging (this can be done via GH GUI). It's easier to explore diffs when PRs are squashed. 👍 |
PR Checklist
PR Structure
otherwise).
services/friendbot, orallordocif the changes are broad or impact manypackages.
Thoroughness
.mdfiles, etc... affected by this change). Take a look in the
docsfolder 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
GET /liquidity_pools?account=<id>returns a list of pools that have as a participantWhy
Getting this information in the current api requires querying an account for its balances to obtain pool ids, then querying each liquidity pool by id. This simplifies that look up into a single call.
Known limitations
This change does not allow requests that specify both account if and existing reserves parameter in the same request.