-
Notifications
You must be signed in to change notification settings - Fork 511
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: Allow filtering accounts by liquidity pool participation. #3873
Conversation
bd4104b
to
9f0e19d
Compare
9f0e19d
to
3a260bd
Compare
While this is technically less defensive, it's still strictly correct, because it covers all of the possible cases for this code block: only pool shares / asset4 / asset12 can be a part of the switch statement.
@@ -366,6 +366,32 @@ func (q *Q) AccountEntriesForSigner(ctx context.Context, signer string, page db2 | |||
return results, nil | |||
} | |||
|
|||
// AccountEntriesForLiquidityPool returns a list of `AccountEntry` rows that | |||
// have trustlines established with a given liquidity pool ID. | |||
func (q *Q) AccountEntriesForLiquidityPool(ctx context.Context, poolId string, page db2.PageQuery) ([]AccountEntry, error) { |
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.
we already have AccountsForLiquidityPool()
, do we need to add this function?
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.
Fixed in 1b8eeb9.
"native": xdr.AssetTypeAssetTypeNative, | ||
"credit_alphanum4": xdr.AssetTypeAssetTypeCreditAlphanum4, | ||
"credit_alphanum12": xdr.AssetTypeAssetTypeCreditAlphanum12, | ||
"liquidity_pool_shares": xdr.AssetTypeAssetTypePoolShare, |
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.
AssetTypeMap
is used to validate asset query parameters for a bunch of different endpoints in horizon including the trades and path finding endpoints. Not all horizon endpoints should accept pool share asset types. For example pool shares are not transferrable therefore it doesn't make sense to do path payments where the source or destination asset is a pool share.
I think we should treat the endpoints which accept pool shares asset parameters as a special case rather than making all asset parameters validate pool shares as ok
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 removed it in 1b8eeb9 however I think AssetTypeMap
should be a general usage map that's mapping all types. If there are actions that doesn't allow liquidity pools we should probably add an extra validation there. We should probably do it after future net deploy.
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.
LGTM, thanks! Will fix bugs if any in #3868.
thanks @bartekn , looks good! |
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
GET /accounts?liquidity_pool=[...some id...]
).Why
Resolves #3832.
Known limitations
Still needs to be tested, hence why it's in draft.