-
Notifications
You must be signed in to change notification settings - Fork 100
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
client/mm: Arb Market Maker #2530
Conversation
... order on the CEX is immediately... ? |
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.
Concept ACK, but looking for some clarification on the multiplier setting. Some general observations for now.
- There's a fair amount of duplication. The code could potentially benefit from some base type that both
basicMarketMaker
andarbMarketMaker
embed. I played with the idea for a minute and I was able dedup maybe 40 lines, so not out-of-the-park improvement, but something to think about. - More inline comments would be helpful.
client/mm/mm_arb_market_maker.go
Outdated
// The first placement will be closest to the mid gap, and each subsequent one | ||
// will be farther from the mid-gap. The multiplier ensures that even if some | ||
// of the orders closest to the mid-gap are filled, there will still be enough | ||
// on the CEX orderbook for a profitable 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.
Kinda seems like the multiplier
is just extra padding that could be accomplished to a large degree by choosing a larger profit
instead. Can you help me understand the multiplier
knob better?
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 point of the multiplier
is to avoid the situation where the trades on the CEX are matched before the bot can get around to making its counter-trade. multiplier
gives the user more control over the size of the buffer than profit
. Depending on how sparse the CEX orderbook is near the mid-gap, changing the profit
could give you much less or much more buffer than you need. The point is to be able to lock in a certain amount of profit and have precise control over the probability you will be able to fill the counter-trade.
974ef4e
to
3a5cad9
Compare
I agree on the duplication.. I'll see if I can think of something that makes sense. I've updated the comments on the Testing this using the Binance Testnet is currently not really possible. There are no updates being sent on the testnet market data stream. Our code is fine though, they are coming through on the mainnet API. However, there will need to be updates made to the Binance code. Only relying on the streams will likely not be sufficient as only the first 20 bids/asks closest to the mid-gap come through. This arb market maker will need more. |
This adds a market making strategy that is kind of a combination between the basic market maker and the simple arbitrage strategies. Based on a CEX order book, it places orders on the DEX order book, and when there is a match on the DEX, the opposite order on the DEX is immediately executed for a profit.
3a5cad9
to
cc8e827
Compare
a.ordMtx.Lock() | ||
o, found := a.ords[oid] | ||
if !found { | ||
a.ordMtx.Unlock() | ||
return | ||
} | ||
a.ords[oid] = note.Order | ||
a.ordMtx.Unlock() |
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.ordMtx.Lock() | |
o, found := a.ords[oid] | |
if !found { | |
a.ordMtx.Unlock() | |
return | |
} | |
a.ords[oid] = note.Order | |
a.ordMtx.Unlock() | |
a.ordMtx.RLock() | |
o, found := a.ords[oid] | |
a.ordMtx.RUnlock() | |
if !found { | |
return | |
} | |
a.ords[oid] = note.Order |
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.
But then a.ords
won't be guarded when doing a.ords[oid] = note.Order
.
for i, ord := range orders { | ||
var oid order.OrderID | ||
copy(oid[:], ord.ID) | ||
a.ords[oid] = ord | ||
a.oidToPlacement[oid] = placements[i].placementIndex | ||
} |
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.
MultiTrade in core can fail one trade and keep going, possibly causing this index to be off if an order is not appended.
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.
MultiTrade
always returns the trades in the same order as they were passed in. If second one fails, then it won't try the third.
if requiredOnDEX > remainingDEXBalance { | ||
log.Debugf("not enough DEX balance to place %d lots", lotsToPlace) | ||
continue | ||
} | ||
if requiredOnCEX > remainingCEXBalance { | ||
log.Debugf("not enough CEX balance to place %d lots", lotsToPlace) | ||
continue | ||
} |
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.
Can also log the balances we have and needed? Is this not a warning? Does the user always to expect to have cfgPlacement.Lots placed?
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 that's OK. If the balance has shifted to one side of the market then this is to be expected.
@martonp Please consider this (completely untested) alternative approach. Instead of requiring the user to define preset "placements", this approach uses a binary search to find the maximum number of lots to place that satisfy the user's profit demands. This avoids complex configuration and prevents the user from shooting themselves in the foot and missing out on profits. You still have an option to include a user-defined quantity multiplier, though I'm still convinced that it's necessary or prudent. func (a *arbMarketMaker) rebalanceV1(epoch uint64) {
if !a.rebalanceRunning.CompareAndSwap(false, true) {
return
}
defer a.rebalanceRunning.Store(false)
type dexBin struct {
ords []*core.Order
rate uint64
lots uint64
}
findBin := func(rate uint64, bins []*dexBin) ([]*dexBin, *dexBin) {
for _, bin := range bins {
if bin.rate == rate {
return bins, bin
}
}
bin := &dexBin{
rate: rate,
}
bins = append(bins, bin)
return bins, bin
}
var dexBuys, dexSells []*dexBin
a.ordMtx.RLock()
for _, ord := range a.ords {
var bin *dexBin
if ord.Sell {
dexSells, bin = findBin(ord.Rate, dexSells)
} else {
dexBuys, bin = findBin(ord.Rate, dexBuys)
}
bin.ords = append(bin.ords, ord)
bin.lots += (ord.Qty - ord.Filled) / a.mkt.LotSize
}
a.ordMtx.RUnlock()
sort.Slice(dexSells, func(i, j int) bool { return dexSells[i].rate < dexSells[j].rate })
sort.Slice(dexBuys, func(i, j int) bool { return dexBuys[i].rate > dexBuys[j].rate })
const qtyBuffer = 1.5 // like ArbMarketMakingPlacement.Multiplier
calcProfit := func(lots, rate uint64, cexSell bool) float64 {
vwap, _, filled, err := a.vwap(cexSell, uint64(float64(a.mkt.LotSize)*qtyBuffer))
if err != nil {
a.log.Errorf("Error calculating VWAP for %s: %w", a.mkt.Name, err)
return -1
}
if !filled {
return -1
}
dexQuoteQty := calc.BaseToQuote(rate, a.mkt.LotSize)
cexQuoteQty := calc.BaseToQuote(vwap, a.mkt.LotSize)
if cexSell {
// Buying on DEX and selling on CEX. We want our CEX quote qty to
// be larger than our DEX quote qty.
return (float64(cexQuoteQty) - float64(dexQuoteQty)) / float64(dexQuoteQty)
}
// if we're buying on CEX and selling on DEX, we want our DEX quote qty
// to be larger than our CEX quote qty.
return (float64(dexQuoteQty) - float64(cexQuoteQty)) / float64(cexQuoteQty)
}
belowProfitThreshold := func(lots, rate uint64, cexSell bool) bool {
return calcProfit(lots, rate, cexSell) < a.cfg.Profit
}
calcProfitableLots := func(rate, maxLots uint64, cexSell bool) uint64 {
// Check for a single lot. No need to search if this doesn't produce
// a profit.
if belowProfitThreshold(1, rate, cexSell) {
return 0
}
return uint64(sort.Search(int(maxLots), func(n int) bool {
return belowProfitThreshold(uint64(n+1), rate, cexSell)
}))
}
var dexSellLots, dexSellRate uint64
for _, buyBin := range dexBuys {
cexSell := false // selling on DEX, buying on CEX
profitableLots := calcProfitableLots(buyBin.rate, buyBin.lots+dexSellLots, cexSell)
if profitableLots == 0 {
break
}
dexSellRate = buyBin.rate
if profitableLots == buyBin.lots {
// All lots are profitable.
dexSellLots += buyBin.lots
continue
}
break
}
if dexSellLots > 0 {
// place sell order on DEX...
return
}
var dexBuyLots, dexBuyRate uint64
for _, sellBin := range dexSells {
cexSell := true // buying on DEX, selling on CEX
profitableLots := calcProfitableLots(sellBin.rate, sellBin.lots+dexBuyLots, cexSell)
if profitableLots == 0 {
break
}
dexBuyRate = sellBin.rate
if profitableLots == sellBin.lots {
// All lots are profitable.
dexBuyLots += sellBin.lots
continue
}
break
}
if dexSellLots > 0 {
// place sell order on DEX...
return
}
} |
@buck54321 I'm having trouble understanding what this is doing. It seems like you're placing orders just based on orders that were already placed by this bot. Why do you want a binary search? |
Oh right. Those should be from the order book. My bad.
Lot sizes for non- BTC, non-ETH markets could be thousands of times smaller smaller than what we're used to working with. This introduces a couple of new challenges. 1) Users can very easily screw up configuration if parameters are based on lot size, and 2) searching for the right number of lots to satisfy our parameters should not be done linearly. (this is also a problem for our current maxOrder implementation,which does a loop with |
a.matchesMtx.Unlock() | ||
return | ||
} | ||
a.matchesSeen[matchID] = true |
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.
Should find a place to clean up matchesSeen
once the match settles.
a.ordMtx.Lock() | ||
delete(a.ords, oid) | ||
delete(a.oidToPlacement, oid) | ||
a.ordMtx.Unlock() |
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.
Maybe this is where you can clean up matches?
mkt, err := c.ExchangeMarket(cfg.Host, cfg.BaseAsset, cfg.QuoteAsset) | ||
if err != nil { | ||
log.Errorf("Failed to get market: %v", err) | ||
return | ||
} | ||
|
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 wonder if we need to somehow account for the ability of the exchange operator to change market parameters, e.g. max fee rate or lot size.
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.
Yeah I think so. Core could send a notification regarding this. I'll make an issue.
|
||
tests := []*test{ | ||
{ | ||
name: "one buy and one cell match notifications", |
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.
cell
6b6a544
to
cc6eeb5
Compare
This adds a market making strategy that is kind of a combination between the basic market maker and the simple arbitrage strategies. Based on a CEX order book, it places orders on the DEX order book, and when there is a match on the DEX, the opposite order on the CEX is immediately executed for a profit.