client/mm: Move shared logic to unifiedExchangeAdaptor #2678

martonp · 2024-01-20T13:26:54Z

This diff moves as much shared logic from bot implementations to the unifiedExchangeAdaptor as possible.

All fiat rate and order fee tracking is moved to the unifiedExchangeAdaptor.
The MultiTrade function of the unifiedExchangeAdaptor now takes as a parameter all of the placements that a bot would make, if they had not placed any trades yet, and if they had unlimited balance. All of the logic that used to be in the basic market maker and mm+arb strategies involving determining which trades need to be cancelled/replaced an how many trades the bot has balance for is now handled in the MultiTrade function.
The rebalancing functionalities in simple arb and arb+mm are now handled by the PrepareRebalance and FreeUpFunds functions.
An OrderFeesInUnits function is added that calculates the total order fees in either the base or quote units. This greatly simplifies the basic market maker’s logic that determines the break even half-spread. The simple arb and arb+mm bots also use this function to take into account order fees, which they did not do previously.
The base/quote wallet settings were repeated in each of the bot strategy settings, and the auto-rebalance config was repeated in both the simple arb and arb+mm. These are moved to the top level config / the bot CEX config, respectively.

This diff moves as much shared logic from bot implementations to the unifiedExchangeAdaptor as possible. - All fiat rate and order fee tracking is moved to the unifiedExchangeAdaptor. - The MultiTrade function of the unifiedExchangeAdaptor now takes all of the placements that a bot would make, if they had not placed any trades yet, and if they had unlimited balance. All of the logic that used to be in the basic market maker and mm+arb strategies involving determining which trades need to be cancelled/replaced and how many trades the bot has balance for is now handled in the MultiTrade function. - The rebalancing functionalities in simple arb and arb+mm are now handled by the PrepareRebalance and FreeUpFunds functions. - An OrderFeesInUnits function is added that calculates the total order fees in either the base or quote units. This greatly simplifies the basic market maker’s logic that determines the break even half-spread. The simple arb and arb+mm bots also use this function to take into account order fees, which they did not do previously. - The base/quote wallet settings were repeated in each of the bot strategy settings, and the auto-rebalance config was repeated in both the simple arb and arb+mm. These are moved to the top level config / the bot CEX config, respectively.

JoeGruffins

The stop button here doesnt seem to work

buck54321 · 2024-02-25T18:19:03Z

client/mm/exchange_adaptor.go

 type botCoreAdaptor interface {
-	NotificationFeed() *core.NoteFeed
 	SyncBook(host string, base, quote uint32) (*orderbook.OrderBook, core.BookFeed, error)
-	SingleLotFees(form *core.SingleLotFeesForm) (uint64, uint64, uint64, error)
 	Cancel(oidB dex.Bytes) error
-	MaxBuy(host string, base, quote uint32, rate uint64) (*core.MaxOrderEstimate, error)
-	MaxSell(host string, base, quote uint32) (*core.MaxOrderEstimate, error)
-	DEXBalance(assetID uint32) (*botBalance, error)
-	MultiTrade(pw []byte, form *core.MultiTradeForm) ([]*core.Order, error)
-	MaxFundingFees(fromAsset uint32, host string, numTrades uint32, fromSettings map[string]string) (uint64, error)
-	FiatConversionRates() map[uint32]float64
+	DEXTrade(rate, qty uint64, sell bool) (*core.Order, error)
 	ExchangeMarket(host string, base, quote uint32) (*core.Market, error)
+	MultiTrade(placements []*multiTradePlacement, sell bool, driftTolerance float64, currEpoch uint64, dexReserves, cexReserves map[uint32]uint64) []*order.OrderID
+	CancelAllOrders() bool
+	ExchangeRateFromFiatSources() uint64
+	OrderFeesInUnits(sell, base bool, rate uint64) (uint64, error)
+	SubscribeOrderUpdates() (updates <-chan *core.Order)
+	SufficientBalanceForDEXTrade(rate, qty uint64, sell bool) (bool, error)
 }

-// botCexAdaptor is an interface used by bots to access functionality
-// related to a CEX. Some of the functions are implemented by libxc.CEX, but
-// have some differences, since this interface is meant to be used by only
-// one caller. For example, Trade does not take a subscriptionID, and
-// SubscribeTradeUpdates does not return one. Deposit is not available
-// on libxc.CEX as it involves sending funds from the DEX wallet, but it is
-// exposed here.
+// botCexAdaptor is an interface used by bots to access CEX related
+// functions. Common functionality used by multiple market making
+// strategies is implemented here. The functions in this interface
+// take assetID parameters, unlike botCoreAdaptor, to support a
+// multi-hop strategy.
 type botCexAdaptor interface {
-	CEXBalance(assetID uint32) (*botBalance, error)
 	CancelTrade(ctx context.Context, baseID, quoteID uint32, tradeID string) error
 	SubscribeMarket(ctx context.Context, baseID, quoteID uint32) error
 	SubscribeTradeUpdates() (updates <-chan *libxc.Trade, unsubscribe func())
-	Trade(ctx context.Context, baseID, quoteID uint32, sell bool, rate, qty uint64) (*libxc.Trade, error)
+	CEXTrade(ctx context.Context, baseID, quoteID uint32, sell bool, rate, qty uint64) (*libxc.Trade, error)
+	SufficientBalanceForCEXTrade(baseID, quoteID uint32, sell bool, rate, qty uint64) (bool, error)
 	VWAP(baseID, quoteID uint32, sell bool, qty uint64) (vwap, extrema uint64, filled bool, err error)
-	Deposit(ctx context.Context, assetID uint32, amount uint64, onConfirm func()) error
-	Withdraw(ctx context.Context, assetID uint32, amount uint64, onConfirm func()) error
+	PrepareRebalance(ctx context.Context, assetID uint32) (rebalance int64, dexReserves, cexReserves uint64)
+	FreeUpFunds(assetID uint32, cex bool, amt uint64, currEpoch uint64)
+	Deposit(ctx context.Context, assetID uint32, amount uint64) error
+	Withdraw(ctx context.Context, assetID uint32, amount uint64) error
 }


These intermediate interfaces seem superfluous to me. Examining the way that the test structures (tCore, tBotCoreAdaptor, tCEX, and tBotCexAdaptor) are designed and used seems to confirm my feeling here. So I took a swing at eliminating these intermediate interfaces and just using unifiedExchangeAdaptor. We already have libxc.CEX and clientCore for testing stubs anyway. I think it's an improvement.

buck54321/dcrdex@5f9422e...buck54321:dcrdex:nomiddleinterfaces

I didn't rework all the tests there, but I did do the test structures in mm_test.go and one test (TestBasisPrice) just as part of the proof-of-concept.

I understand that what I'm asking here is challenging work to deeply reconfigure the tests. My view is that your refactor is so righteous, that we can literally eliminate an entire layer of abstraction. IDE navigation will be so much better. Now seems like the right time to adapt at this level and scope.

I'll look into it, but testing the bot logic is so much simpler with these interfaces. Take for example OrderFeesInUnits, the rebalancing functions, and even MultiTrade. These have pretty complicated logic. By having the interfaces, it's way easier to directly test the bots. TestBasisPrice may have been easy to update, but the arb+mm and arb rebalancing tests will need to have so much more additional logic that does not involve directly testing the bots. Why is this better?

buck54321 · 2024-03-06T18:00:17Z

client/mm/exchange_adaptor.go

+	market, err := u.ExchangeMarket(u.market.Host, u.market.BaseID, u.market.QuoteID)
+	if err != nil {
+		u.log.Errorf("Error getting market: %v", err)
+		return 0
+	}
+
+	return market.ConventionalRateToMsg(baseRate / quoteRate)


Calling ExchangeMarket just for the one method is a bit of overkill, but this refactor tightens things up a bit.

Changes look good, thanks.

You haven't applied the changes here in ExchangeRateFromFiatSources.

buck54321 · 2024-03-06T18:59:08Z

client/mm/mm_basic.go

+	 * lot of the base asset plus the total fees in base units.
+	 *
+	 * Solving for g, you get:
+	 * g = q / (f + 2l)


I'm not getting this same result. What am I doing wrong?

(b + g) * l / (b - g) = l + f (b + g) * l = (l + f) * (b - g) lb + lg = lb - lg + fb + fg lg + lg - fg = lb + fb - lb g * (2l - f) = fb = q g = q / (2l - f)

Nevermind. I see where I went wrong. +fg -> -fg

buck54321 · 2024-03-06T19:00:20Z

client/mm/mm_arb_market_maker.go

+func (a *arbMarketMaker) dexPlacementRate(cexRate uint64, sell bool) (uint64, error) {
+	sellFeesInQuoteUnits, err := a.core.OrderFeesInUnits(true, false, cexRate)
 	if err != nil {
-		log.Errorf("error getting quote CEX balance: %v", err)
-		return
+		return 0, fmt.Errorf("error getting sell fees in quote units: %w", err)
 	}

-	processSide := func(sell bool) []*rateLots {
-		var cfgPlacements []*ArbMarketMakingPlacement
-		var existingOrders map[int][]*groupedOrder
-		var remainingDEXBalance, remainingCEXBalance, fundingFees uint64
-		if sell {
-			cfgPlacements = cfg.SellPlacements
-			existingOrders = existingSells
-			remainingDEXBalance = baseDEXBalance.Available
-			remainingCEXBalance = quoteCEXBalance.Available
-			fundingFees = sellFees.funding
-		} else {
-			cfgPlacements = cfg.BuyPlacements
-			existingOrders = existingBuys
-			remainingDEXBalance = quoteDEXBalance.Available
-			remainingCEXBalance = baseCEXBalance.Available
-			fundingFees = buyFees.funding
-		}
-
-		cexReserves := reserves.get(!sell, true)
-		if cexReserves > remainingCEXBalance {
-			log.Debugf("rebalance: not enough CEX balance to cover reserves")
-			return nil
-		}
-		remainingCEXBalance -= cexReserves
+	buyFeesInQuoteUnits, err := a.core.OrderFeesInUnits(false, false, cexRate)
+	if err != nil {
+		return 0, fmt.Errorf("error getting buy fees in quote units: %w", err)
+	}

-		dexReserves := reserves.get(sell, false)
-		if dexReserves > remainingDEXBalance {
-			log.Debugf("rebalance: not enough DEX balance to cover reserves")
-			return nil
-		}
-		remainingDEXBalance -= dexReserves
-
-		// Enough balance on the CEX needs to be maintained for counter-trades
-		// for each existing trade on the DEX. Here, we reduce the available
-		// balance on the CEX by the amount required for each order on the
-		// DEX books.
-		for _, ordersForPlacement := range existingOrders {
-			for _, o := range ordersForPlacement {
-				var requiredOnCEX uint64
-				if sell {
-					rate := uint64(float64(o.rate) / (1 + cfg.Profit))
-					requiredOnCEX = calc.BaseToQuote(rate, o.lots*mkt.LotSize)
-				} else {
-					requiredOnCEX = o.lots * mkt.LotSize
-				}
-				if requiredOnCEX <= remainingCEXBalance {
-					remainingCEXBalance -= requiredOnCEX
-				} else {
-					log.Warnf("rebalance: not enough CEX balance to cover existing order. cancelling.")
-					addCancel(o)
-					remainingCEXBalance = 0
-				}
-			}
-		}
-		if remainingCEXBalance == 0 {
-			log.Debug("rebalance: not enough CEX balance to place new orders")
-			return nil
-		}
+	return dexPlacementRate(cexRate, sell, a.cfg.Profit, a.mkt, sellFeesInQuoteUnits+buyFeesInQuoteUnits)
+}


Don't we just need to add or subtract the halfSpread?

What I had is wrong. It should only be taking into account the fees for either selling or buying, not both. Using the half-spread would be the same as this if initiating and redeeming cost the same amount. It's probably a negligible difference though.

The only value that really matters is the spread between our best buy and best sell. Using halfSpread would keep the mid-gap the same as the cex. Treating the sides separately effectively shifts the gap if fees are assymetric.

Ah I see your point. My logic was that whenever an arb is done, it should cover the fees that are being incurred at that point. I'll just reuse the halfSpread code then.

Edit: Acutally, what's wrong with shifting the gap? Shouldn't each arb just cover the fees that are actually being incurred at that point?

My logic was that whenever an arb is done, it should cover the fees that are being incurred at that point.

Actually, that kinda sounds right too. Leave it your way.

buck54321 · 2024-03-06T19:49:32Z

client/mm/exchange_adaptor_test.go

+				redemption: 4e4,
+			},
+			sellFees: &orderFees{
+				swap:       5e4,


Noting that when converting from MATIC to WBTC, the converted contribution is 0.

5e4 matic / 1e9 * 0.8 / 42500 * 1e8 = 0.09 Sats < 1 Sat

martonp force-pushed the mmSharedLogicToAdaptor branch from 8542eeb to 1ab48c0 Compare January 20, 2024 13:52

martonp marked this pull request as draft January 22, 2024 19:12

martonp force-pushed the mmSharedLogicToAdaptor branch from 1ab48c0 to 5eadaf5 Compare January 30, 2024 22:49

martonp force-pushed the mmSharedLogicToAdaptor branch 3 times, most recently from dea188d to 2a107f8 Compare February 20, 2024 19:08

martonp force-pushed the mmSharedLogicToAdaptor branch from 2a107f8 to 5f9422e Compare February 20, 2024 19:27

martonp marked this pull request as ready for review February 20, 2024 19:30

JoeGruffins approved these changes Feb 23, 2024

View reviewed changes

buck54321 reviewed Feb 25, 2024

View reviewed changes

martonp mentioned this pull request Mar 4, 2024

mm: arbitrage bots need a profit "plus" option #2695

Open

buck54321 reviewed Mar 6, 2024

View reviewed changes

Buck review fixes

e82cca1

martonp force-pushed the mmSharedLogicToAdaptor branch from 01a40f3 to e82cca1 Compare March 7, 2024 22:57

buck54321 approved these changes Mar 8, 2024

View reviewed changes

buck54321 merged commit 52d0682 into decred:master Mar 8, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client/mm: Move shared logic to unifiedExchangeAdaptor #2678

client/mm: Move shared logic to unifiedExchangeAdaptor #2678

martonp commented Jan 20, 2024 •

edited

Loading

JoeGruffins left a comment

buck54321 Feb 25, 2024

buck54321 Feb 26, 2024

martonp Feb 26, 2024

buck54321 Mar 6, 2024

martonp Mar 7, 2024

buck54321 Mar 7, 2024 •

edited

Loading

martonp Mar 7, 2024

buck54321 Mar 6, 2024

buck54321 Mar 7, 2024 •

edited

Loading

buck54321 Mar 6, 2024

martonp Mar 7, 2024

buck54321 Mar 7, 2024

martonp Mar 7, 2024 •

edited

Loading

buck54321 Mar 7, 2024

buck54321 Mar 6, 2024

client/mm: Move shared logic to unifiedExchangeAdaptor #2678

client/mm: Move shared logic to unifiedExchangeAdaptor #2678

Conversation

martonp commented Jan 20, 2024 • edited Loading

JoeGruffins left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buck54321 Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Jan 20, 2024 •

edited

Loading

buck54321 Mar 7, 2024 •

edited

Loading

buck54321 Mar 7, 2024 •

edited

Loading

martonp Mar 7, 2024 •

edited

Loading