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

multi: Arbitrage Bot #2005

Closed
wants to merge 3 commits into from
Closed

multi: Arbitrage Bot #2005

wants to merge 3 commits into from

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Dec 20, 2022

This PR adds a new Arbitrage Bot in addition to the existing market maker bot.

libxc: A new lilbxc package is created which contains a CEX interface which allows a user to interact with a centralized exchanges API. Binance is the first exchange implemented.

core:

  • The makerBot is refactored to take a botEngine as a field which contains the logic of when to place and cancel orders. The old maker bot logic is placed into the gapEngine type, and an arbitrage bot is added. The arbitrage bot compares the DEX and the CEX order books, and if there is an arbitrage opportunity, executes a trade on both. In order to easily test the botEngine, the constructors for the botEngines take an "inputs" interface, which is implemented by the makerBot.
  • Functionality to register API keys for a CEX and to later update those API keys are added through the RegisterCEXCreds and UpdateCEXCreds functions. The credentials are stored in the db.
  • Functionality to connect and disconnect from a CEX is added. There should never be any calls made to a CEX API unless the user explicitly connects to a CEX, or restarts a previously created bot that uses that CEX.

app: The maker maker screen is updated to allow both Gap Bots and Arbitrage Bots to be created. Screens to manage the CEX API keys, view CEX balances, and view the markets available on the CEX are also added.

// 2 x L commitment. If low balance restricts the maintenance of L lots on
// one side, allow the difference in lots to be added to the opposite side.
// 6. Place orders, cancels first, then buys and sells.
// The strategy the makerBot follows depends on the engine.
type makerBot struct {
*Core
Copy link
Member

Choose a reason for hiding this comment

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

Will you remind me why bot creation and management is part of Core if (as I would expect) the bot itself wraps/manages a Core instance as here? I'm not 100% against it, but I want to establish boundaries and guidelines for just how integrated with Core internals bots are supposed to get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess the reason is that the makerBot calls many of Core's functions. @buck54321 can probably explain more. I think the two options are doing it this way or doing what I've done with the botEngine input types.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, OK fair enough. I guess I need to audit that to see. Looks like it accesses unexported stuff like Core.db, Core.cancelOrder, Core.notify, Core.syncBook, Core.ctx, etc.
I do wonder what it would take to have a bot function with Core's exported API. It certainly saves a lot of effort to be in the client/core package and expand Core rather than consume it as a black box.

Thanks for refactoring the bot and adding arb. Good stuff!


bnc.markets.Store(make(map[string]symbol))

return (CEX)(bnc)
Copy link
Member

Choose a reason for hiding this comment

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

The mantra in Go that we flaunt often is "Accept Interfaces Return Structs".
I think this should return *binance and the driver-like helper NewCEX that you've defined in interface.go can coerce it into an interface. Although I'm not sure yet that it's the right place to force it into an interface.

@chappjc chappjc added the bot label Dec 29, 2022
@chappjc chappjc added this to the 1.0 milestone Jan 30, 2023
Adds the ability to send raw byte strings over a websocket connection.
A new CEX interface is introduced which allows interaction with a CEX
API. One implementation of this interface is added for Binance.

A new field is added to the  `Core` type to store connections to CEXes.
The DB is extended to store API credentials for CEXes. New webserver
routes are added to register and update CEX credentials, and to connect
and disconnect from a CEX.
The `makerBot` is refactored to take a `botEngine` as a field
which contains the logic of when to place and cancel orders.
The old maker bot logic is placed into the `gapEngine` type,
and an arbitrage bot is added. The arbitrage bot compares the
DEX and the CEX order books, and if there is an arbitrage
opportunity, executes a trade on both.

The UI is also updated to be able to create both gap and arbitrage
bots.
var ok bool
assetID, ok := dex.BipSymbolID(symbol)
if !ok {
return nil, fmt.Errorf("not id found for %q", symbol)
Copy link
Member

Choose a reason for hiding this comment

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

not -> no

@@ -0,0 +1,527 @@
package core
Copy link
Member

Choose a reason for hiding this comment

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

Some files missing the license, if necessary.

}

func (cfg *ArbEngineCfg) validate() error {
if cfg.ProfitTrigger <= 0 || cfg.ProfitTrigger > 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Could this not be above 1 and still function correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will function correctly, but expecting 100% on an arb trade is a bit unrealistic I think.

startEpoch uint64
}

// arbEngineInputs are the input functions required an ArbEngine to function.
Copy link
Member

Choose a reason for hiding this comment

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

required for

cex libxc.CEX
cexTradeUpdatesID int
inputs arbEngineInputs
cfgV atomic.Value
Copy link
Member

Choose a reason for hiding this comment

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

Maybe comment stored type?

Comment on lines +714 to +718
reconnect = time.After(time.Second * 30)
} else {
reconnect = time.After(time.Hour * 12)
}
case <-keepAlive.C:
Copy link
Member

Choose a reason for hiding this comment

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

Probably doesn't happen often, but could continue early in keepAlive ticks while reconnecting, if the reconnect failed and we are waiting the thirty seconds.

}{
Method: method,
Params: []string{
slug + "@depth20",
Copy link
Member

Choose a reason for hiding this comment

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

depth20 is best 20 orders on both sides?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Comment on lines +55 to +58
// SubscribeTradeUpdates returns a channel that the caller can use to
// listen for updates to a trade's status. If integer returned from
// this function must be passed as the updaterID argument to Trade,
// updates to the trade will be sent on the channel.
Copy link
Member

Choose a reason for hiding this comment

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

If integer returned from this function must be passed as the updaterID argument to Trade If -> The?

return weightedSum / qty, extrema, true, nil
}

type binanceNetworkInfo struct {
Copy link
Member

Choose a reason for hiding this comment

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

Can you comment a url where the api specifics exist in case something ever changes?

return MessageRateAlt(conventionalRate, baseInfo.Conventional.ConversionFactor, quoteInfo.Conventional.ConversionFactor)
}

// MessageRate converts an exchange rate in conventional encoding to one
Copy link
Member

Choose a reason for hiding this comment

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

MessageRate -> MessageRateAlt

@martonp
Copy link
Contributor Author

martonp commented May 29, 2023

@JoeGruffins thanks for the review. I'm going to kind of redo this PR to fit with the new design of the market maker, but I'll take your comments into consideration in the other PR.

@martonp martonp marked this pull request as draft May 31, 2023 07:44
@buck54321
Copy link
Member

Might have a fun rebase now. I'd love to start reviewing this soon.

@martonp
Copy link
Contributor Author

martonp commented Aug 11, 2023

@buck54321 I have something on another branch already.. I'll have this updated soon.

@martonp
Copy link
Contributor Author

martonp commented Aug 16, 2023

Closing in favor of #2480.

@martonp martonp closed this Aug 16, 2023
@martonp martonp deleted the arbbot branch January 20, 2024 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants