Skip to content
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

mm: Individually Start Bots / Live Updates #2738

Merged
merged 4 commits into from
May 16, 2024

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Apr 17, 2024

This diff adds the ability to start/stop bots individually and also to perform live updates to bot settings and the amount of funds controlled by bots.

All balance fields are removed from the BotConfig. These are now specified when a bot is started.

Bots and exchange adaptors are now dex.Connectors. To perform an update, the bot is first paused, the updates are made, and then the bot is resumed again. A botConnectionMaster that wraps a dex.ConnectionMaster is added to facilitate pausing and resuming of bots.

To perform a balance update, we need to know the exact amount that is available in the wallets and on the CEX that is not currently reserved by a running bot. To do this, we first check the available amounts according to the wallet/cex, then we sync the state of all pending trades, deposits, and withdrawals, and then we recheck the available amounts. If the first check is the same as the last, we know nothing has changed and we have the correct amounts, so we can proceed. In order for this to work properly, the WalletTransaction function of wallets must return Confirmed == true if and only if the any incoming funds from that transaction are part of the available balance.

The priceOracle is also refactored. Instead of having a “synced” priceOracle used for markets on which a bot is running, and an “unsynced” one for any other markets, there is now only one priceOracle. startAutoSyncingMarket and stopAutoSyncingMarket are called whenever bots are started / stopped.

A testing program is added that tracks the available balances in wallets/cexes that are unused by any bots. This amount should not change unless the user start, stops, or updates a bot. If there are any unexpected changes, this means there is a bug in the balance tracking.

t.Helper()
var err error
for i := 0; i < 20; i++ {
time.Sleep(100 * time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's not new, but what are we synchronizing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storeEvent is async.

// placementIndex/counterTradeRate are used by MultiTrade to know
// which orders to place/cancel.
placementIndex uint64
counterTradeRate uint64
}

// updateState should only be called with txMtx write locked.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is your mutex comment outdated?

}

func (u *unifiedExchangeAdaptor) updateConfig(cfg *BotConfig, balanceDiffs *BotBalanceDiffs) {
cfgUpdated := !reflect.DeepEqual(cfg, u.botCfg.Load().(*BotConfig))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No DeepEqual in production please. Why are we grouping config and inventory updates. They seem like separate events.

Copy link
Contributor Author

@martonp martonp May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are you thinking for the UI? I was thinking that when you update the config, you would also have the option to update balance in the same operation. Then there would also be a separate screen for just updating balances. I could add a separate function only for balance updates.

client/mm/mm.go Outdated
Comment on lines 674 to 820
ctx, die := context.WithCancel(m.ctx)
adaptorUpdateManager, botUpdateManager := cfgUpdateManagerPair()
mktID := dexMarketID(botCfg.Host, botCfg.BaseID, botCfg.QuoteID)
logger := m.botSubLogger(botCfg)
exchangeAdaptor := unifiedExchangeAdaptorForBot(&exchangeAdaptorCfg{
botID: mktID,
market: mkt,
baseDexBalances: allocation.DEX,
baseCexBalances: allocation.CEX,
core: m.core,
cex: cex,
log: logger,
botCfg: botCfg,
eventLogDB: m.eventLogDB,
cfgUpdateManager: adaptorUpdateManager,
})
if err := exchangeAdaptor.run(ctx); err != nil {
die()
return err
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This context pattern is what we have dex.ConnectionMaster for.

Comment on lines 500 to 503
pauseEpochs <- struct{}{}
pauseCEXUpdates <- struct{}{}
pauseDEXUpdates <- struct{}{}
updateManager.botPaused <- struct{}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems racy. These are zero-capacity channels. If the context is canceled after the updateBot message but before one of these structs{}{} can be received, e.g. while processDEXOrderUpdate is running, this will deadlock.

Copy link
Contributor Author

@martonp martonp May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's purposely zero-capacity because I wanted it to block, but I see your point about the context getting cancelled.

Comment on lines 288 to 293
func coinpapSlug(symbol, name string) string {
slug := fmt.Sprintf("%s-%s", symbol, name)
// Special handling for asset names with multiple space, e.g Bitcoin Cash.
return strings.ToLower(strings.ReplaceAll(slug, " ", "-"))
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused

@martonp martonp force-pushed the mmUpdateConfig branch 2 times, most recently from 7204b9d to 787758e Compare May 9, 2024 06:27
Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martonp please accept these changes to create a transitional API and UI. The front end still looks the same an all bots are still started and stopped together. I'll have lots of followup to adapt the UI and other stuff.

buck54321/dcrdex@787758e...buck54321:dcrdex:bot-transitional

martonp and others added 4 commits May 16, 2024 11:58
This diff adds the ability to start/stop bots individually and also to
perform live updates to bot settings and the amount of funds controlled
by bots.

All balance fields are removed from the BotConfig. These are now specified
when a bot is started.

Bots and exchange adaptors are now dex.Connectors. To perform an update,
the bot is first paused, the updates are made, and then the bot is resumed
again. A botConnectionMaster that wraps a dex.ConnectionMaster is added to
facilitate pausing and resuming of bots.

To perform a balance update, we need to know the exact amount that is
available in the wallets and on the CEX that is not currently reserved
by a running bot. To do this, we first check the available amounts
according to the wallet/cex, then we sync the state of all pending trades,
deposits, and withdrawals, and then we recheck the available amounts. If
the first check is the same as the last, we know nothing has changed and
we have the correct amounts, so we can proceed. In order for this to work
properly, the `WalletTransaction` function of wallets must return
`Confirmed == true` if and only if the any incoming funds from that
transaction are part of the available balance.

The priceOracle is also refactored. Instead of having a “synced”
priceOracle used for markets on which a bot is running, and an “unsynced”
one for any other markets, there is now only one priceOracle.
`startAutoSyncingMarket` and `stopAutoSyncingMarket` are called whenever
bots are started / stopped.

A testing program is added that tracks the available balances in
wallets/cexes that are unused by any bots. This amount should not change
unless the user start, stops, or updates a bot. If there are any unexpected
changes, this means there is a bug in the balance tracking.
@martonp martonp marked this pull request as ready for review May 16, 2024 03:58
@buck54321 buck54321 merged commit d0c76e9 into decred:master May 16, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants