-
Notifications
You must be signed in to change notification settings - Fork 499
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: Liquidity pool happy path integration test #3868
Conversation
close(a.done) | ||
a.doneOnce.Do(func() { | ||
close(a.done) | ||
}) |
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.
For visibility: it fixes a close of closed channel
bug when trying to interrupt an integration test by pressing CMD+C. The annoying thing was that in such situation docker containers had to be stopped manually. This is now fixed.
if err == errLiquidtyPoolChangeNotFound { | ||
return nil, nil | ||
} |
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 think we should also check that the operation failed (and add a comment of why it's acceptable to not add details in that case)
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.
Also, I guess we will need the same for effects. Although we may not be generating effects for failed transactions.
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 think we should also check that the operation failed (and add a comment of why it's acceptable to not add details in that case)
Actually, I think we should simply check if the transaction failed and , in that case avoid trying to add more details
} | ||
} else { | ||
addAssetDetails(details, op.Line.ToAsset(), "") | ||
for _, change := range changes { |
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.
A quick explanation of changes in this method. It was incorrectly ingesting effects for change_trust when trusting a liquidity pool because such operation's meta contains two or three trust line related changes: for all shares trust line (assetA and/or assetB - depends if any is native) and liquidity_pool trust line.
@@ -604,7 +604,7 @@ type LiquidityPoolRevoked struct { | |||
Base | |||
LiquidityPool LiquidityPool `json:"liquidity_pool"` | |||
ReservesRevoked []LiquidityPoolClaimableAssetAmount `json:"reserves_revoked"` | |||
SharesRevoked uint64 `json:"shares_revoked,string"` |
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.
Why is this?
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.
(and the other, similar changes)
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.
Ah, because we are going to treat it as an amount ... ok, that was a bit unintuitive.
@@ -323,23 +323,23 @@ type SetTrustLineFlags struct { | |||
// LiquidityPoolDeposit. | |||
type LiquidityPoolDeposit struct { | |||
Base | |||
LiquidityPoolID string `json:"liquidty_pool_id"` | |||
LiquidityPoolID string `json:"liquidity_pool_id"` |
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.
Good catch!!
Revoke case will be added when stellar/stellar-core#3162 is merged. |
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.
This commit adds a happy path integration test for liquidity pool. It includes LP creation, deposit, withdrawal and deletion. Then it checks if operations, effects and trades are ingested correctly. Finally, all bugs found by the test has been fixed.
This is still in progress but I think it's ready for an initial review.
Close #3812.
Close #3842.