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

client/{mm, core}: Simple Arbitrage #2480

Merged
merged 6 commits into from
Sep 18, 2023
Merged

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Aug 16, 2023

This implements the simple arbitrage strategy which only places orders
when there is an arbitrage opportunity.

  • A libxc package is added which contains a CEX interface used to
    interact with a centralized exchange's API. It is implemented for
    Binance.
  • The new strategy is implemented in mm_simple_arb.go and can be run by
    creating a BotConfig with a non-nil ArbCfg.
  • A testbinance command line tool is added which starts a webserver that
    responds to the requests that the Binance testnet does not support.
  • A VWAP function is added to the client orderbook.
  • client/comms/WSConn is updated with a SendRaw function which sends
    arbitrary byte slices over the websocket connection.

client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/interface.go Outdated Show resolved Hide resolved
Comment on lines 329 to 332
// Trade executes a trade on the CEX. updaterID takes an ID returned from
// SubscribeTradeUpdates, and tradeID takes an ID returned from
// GenerateTradeID.
func (bnc *binance) Trade(baseSymbol, quoteSymbol string, sell bool, rate, qty uint64, updaterID int, orderID string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Comment says tradeID but there is no argument by that name.

Should this check that orderID is a valid order ID? Or should the variable be called tradeID? Then should it check it is the correct length? Should it check that it is unique in the bnc.tradeToUpdater map?

client/mm/libxc/interface.go Outdated Show resolved Hide resolved
client/mm/libxc/interface.go Show resolved Hide resolved
return 0, 0, false, nil
}

return weightedSum / lots, extrema, true, nil
Copy link
Member

Choose a reason for hiding this comment

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

I remember seeing this code before and commenting that, if the book was sparse, would that allow an attacker to place some orders near the mid gap and then one order way far back, the bot buys them all up because the average looks good, the attacker then doesn't follow through with the extrema trades and just leaves the one at a bad rate. Was this a valid concern? I think my solution was to prevent orders that are too far from the extrema in these calculations.

Copy link
Contributor Author

@martonp martonp Aug 24, 2023

Choose a reason for hiding this comment

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

The code here makes sure that each lot included in the arbitrage must be profitable, not just the average:

var currLotProfitable bool
if sellOnDEX {
currLotProfitable = dexExtrema > cexExtrema
} else {
currLotProfitable = cexExtrema > dexExtrema
}

This would avoid the attack you are talking about right?

Copy link
Member

Choose a reason for hiding this comment

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

Oh that's right, the extrema is the worst rate for us, I was wording it as best. Unless both markets are sparse, then Yes this should do about the same thing as I suggested. Would this be even too restrictive? Orders will probably be further apart on dex... If I am understanding correctly.

Copy link
Contributor Author

@martonp martonp Aug 25, 2023

Choose a reason for hiding this comment

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

I guess we could do something like buying 1/2 a lots worth on the CEX and a full lots worth on the DEX (since we can't buy any less), but maybe save that for a future iteration. This probably is a good idea, especially for markets with a large lot size, since it will be rare that a full lot can be arbitraged.

Unless it's someone who is running the CEX trying to manipulate someone running the arb bot, I don't think it's possible. If somehow they trick us into taking a really bad trade on the DEX, then they would've had to also trick us into taking a really good trade on the CEX. Unless they are running the CEX, they would not be able to back out of the CEX trade like they could on the DEX.

client/cmd/testbinance/main.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/interface.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/mm_simple_arb.go Outdated Show resolved Hide resolved
client/mm/mm_simple_arb.go Outdated Show resolved Hide resolved
client/mm/libxc/binance_live_test.go Outdated Show resolved Hide resolved
client/mm/libxc/binance_live_test.go Show resolved Hide resolved
client/mm/libxc/binance.go Show resolved Hide resolved
Comment on lines 384 to 387
bnc.tradeToUpdater[tradeID] = updaterID
bnc.tradeUpdaterMtx.Unlock()

return bnc.postAPI(ctx, "/api/v3/order", v, nil, true, true, &struct{}{})
Copy link
Member

Choose a reason for hiding this comment

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

Should we be adding the updaterID to the map if the request fails?

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 should be added to the map beforehand, because an update may arrive before postAPI returns, but we should be removing it if it fails.

client/mm/libxc/binance.go Show resolved Hide resolved
client/mm/libxc/binance.go Show resolved Hide resolved
client/mm/libxc/binance.go Outdated Show resolved Hide resolved
client/mm/libxc/interface.go Outdated Show resolved Hide resolved
}
}

// keep retrying if failed to cancel?
Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I guess so.

client/mm/mm_simple_arb_test.go Outdated Show resolved Hide resolved
dex/bip-id.go Outdated Show resolved Hide resolved
client/mm/libxc/binance.go Show resolved Hide resolved
@martonp martonp force-pushed the simpleArbStrat branch 3 times, most recently from 6a6c0df to 42dce09 Compare September 12, 2023 04:18
@buck54321
Copy link
Member

Looks ready to me. Looking for a rebase and a thumbs up from @JoeGruffins.

This implements the simple arbitrage opportunity which only places orders
when there is an arbitrage opportunity.

- A `libxc` package is added which contains a `CEX` interface used to
  interact with a centralized exchange's API. It is implemented for
  Binance.
- The new strategy is implemented in `mm_simple_arb.go` and can be run by
  creating a `BotConfig` with a non-nil `ArbCfg`.
- A testbinance command line tool is added which starts a webserver that
  responds to the requests that the Binance testnet does not support.
- A `VWAP` function is added to the client orderbook.
- `client/comms/WSConn` is updated with a `SendRaw` function which sends
  arbitrary byte slices over the websocket connection.
@buck54321 buck54321 merged commit 6a04997 into decred:master Sep 18, 2023
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.

3 participants