-
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
exp/orderbook: Expand path payment algorithm to search liquidity pools. #3921
Conversation
Optimize original version to not need BigFloats Drop the custom Uint128 math/big version entirely
This is the complement to the other equation, letting you say "I want x paid out to me, how much do I need to give you?"
Since LPs can trade for either asset, specifying the name here is actually misleading; the order doesn't matter.
9b8773a
to
acad403
Compare
acad403
to
a968a58
Compare
6b8cd12
to
95d9a13
Compare
95d9a13
to
0378123
Compare
6bc72e7
to
1c7e42b
Compare
exp/orderbook/batch.go
Outdated
case addLiquidityPoolOperationType: | ||
tx.orderbook.liquidityPools[operation.liquidityPoolAssets] = *operation.liquidityPool | ||
ob := tx.orderbook |
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.
to be consistent with the other operations we should either encapsulate the liquidity pool operations into functions on the orderbook graph, or, we should inline orderbook.add()
and orderbook.remove()
into orderBookBatchedUpdates .apply()
. I think it would be better to encapsulate the liquidity pool operations into separate functions
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.
Yep, agreed. Now that these are stable (:sweat_smile:) it makes sense to abstract them like for offers.
exp/orderbook/dfs.go
Outdated
|
||
// chooseVenue determines whether the offer or pool amount should be chosen, | ||
// returning the amount and whether or not the offer was chosen. | ||
chooseVenue(offerAmount, poolAmount xdr.Int64) (xdr.Int64, bool) |
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 can do a slight refactoring to make the interface easier to understand. Instead of having chooseVenue()
, consumeOffers()
, and consumePool()
we could replace it with a single function:
trade(currentAsset xdr.Asset, currentAssetAmount xdr.Int64, venues Venues) (xdr.Asset, xdr.Int64, error)
The trade()
function will essentially do the logic to select the best venue and then execute the trade
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.
Nice idea, I'll give it a whirl.
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.
So after trying this out, I realized why I broke it up into parts like this. If you look at the processVenues
helper (which is what DFS uses):
The only points of divergence are at state.consumePool
, state.consumeOffers
, and state.chooseVenue
, all necessary because buys and sells interact with the pool/offers differently and the results need to be evaluated differently (<
vs. >
).
If we combine these into a single state.trade(...)
, then we have ~45 lines of duplicated code across the sellingGraphSearchState
and buyingGraphSearchState
, with divergences exactly as they are above, except I guess they wouldn't need to be attached to the searchState
.
I agree that this can be improved, but I'm not sure this suggestion would lead to cleaner code overall. A cleaner interface, maybe, but uglier interface implementations. I did do some interface cleanup in the latest push and dropped the awkward chooseVenue()
method. Now it's just consumePool
and consumeOffers
, which I think is pretty straightforward to understand.
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.
See cd60906 for the delta: I essentially extended consumeOffers
to take a "current best" parameter that makes its return values cleaner to process (and opens up an opportunity for a processing optimization, but this is TBD).
1c7e42b
to
08d7d02
Compare
@Shaptic the code looks good to me. I just left a couple of suggestions about some minor issues. if you can fix the tests and resolve any outsanding todos / fixmes then I think we can merge this |
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
Allows bidirectional path-finding through both liquidity pools and offers.
Major changes:
makeTrade
and its corresponding calculation functionsedgeSet
to represent all trade opportunities: both offers (as before) and now liquidity pools.searchState
interface to process offers and LPs ("venues") properly:venues(asset)
retrieves all pools and LPs for a given assetchooseVenue(a, b)
determines whether the path should choose offers (a
) or pools (b
), since the choice depends on if we're going forwards (FindFixedPath
) or backwards (FindPath
) through the graph's edges.consumePool(pool, asset, amount)
evaluates an exchange with a pool, which is also direction-dependentprocessVenues()
is a catch-all helper to evaluate pools and offers (in that order) fairlyOrderBookGraph
to handle batch add-update-removal of liquidity poolsWhy
We need to enable users to find paths through both the existing DEX and through the newly-introduced liquidity pools.
See #3836 for more.
Known limitations
I'd like to incorporate Jon's tests to evaluate the actual liquidity pool exchange math, and I'd also like to ensure that the test cases sufficiently cover the new functionality I've introduced here.