Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

"maker only" mode for buysell #34

Closed
jimdanz opened this issue Oct 23, 2018 · 12 comments · Fixed by #101
Closed

"maker only" mode for buysell #34

jimdanz opened this issue Oct 23, 2018 · 12 comments · Fixed by #101
Assignees
Labels
feature request New feature or request
Projects
Milestone

Comments

@jimdanz
Copy link

jimdanz commented Oct 23, 2018

Desired Behavior

The bot should have a mode where it tries to only make orders that do not cross any existing orders (maker only orders).

Impact

This can help you avoid unnecessary wash trades if someone else is also market-making but your price feeds or preferences are causing you to place slightly different orders.

Feature Suggestion

Without the DEX protocol itself supporting maker-only orders, anything done in the bot will be a best effort. However, I believe that the basic flow is simply: each time re-evaluating orders to place:

  • first load all orders for the asset pair from the DEX
  • check if the order you'd be placing will cross with an existing order. If it would, do not place it.

References

Bots aimed at centralized exchanges will generally have this feature because centralized exchanges usually price maker vs. taker orders quite differently.

Example: https://github.com/DeviaVir/zenbot#strategies

Additional context

I believe this should be a flag/parameter for buysell. There's an argument that it could be its own strategy, but I think it's better as a parameter because it might apply to various different strategies (could apply to sell as well, maybe even to balanced).

Specification

(updated by @nikhilsaraf: flag should be optional and in the strategy config file but execution by the framework, augment the strategy interface to support this. This should be handled by either the framework or the exchange implementation but the framework can do the threading of the flag. For the SDEX implementation we can filter out orders by querying the orderbook to find and delete the intersection between existing orderbook and orders we want to create)

@Reidmcc
Copy link
Contributor

Reidmcc commented Oct 23, 2018

The behavior you describe of not matching any existing orders is already accomplished to the extent possible by the staticSpreadLevelProvider; the bot's order levels are offset from the center price based on the config settings, up and down for sell and buy respectively. By definition, there are no buy offers higher than the center price and no sell offers lower than the center price, hence checking the individual offers is not necessary.

What it does not do is prevent your order from filling another order that arrives between when the bot checks the center price and when the bot's orders are placed. Another check of the orderbook would not help; another trader's order could still arrive between your book check and your order's arrival. Which is the main purpose of a maker-only order on a centralized exchange. This feature is not supported by the Stellar protocol itself, see the market guide.

@nikhilsaraf
Copy link
Contributor

@jimdanz this is a good feature request. We can do this at the framework level to support all strategies and take this input as a command line param to the bot's trade command.

Before placing any orders (submitting to the DEX) we can "delete" any orders (manageOffer operations, really) that will cross the existing orderbook. This can be done by querying the orderbook to find the intersection between the existing orders and the orders you intend on placing.

Is the proposed approach a viable solution for you?


@Reidmcc I think the center price in the staticSpreadLevelProvider would be taken from a priceFeed, not from the DEX itself, so it is likely to differ from the current state of the DEX's orderbook.

@Reidmcc
Copy link
Contributor

Reidmcc commented Oct 23, 2018

@nikhilsaraf Right, and priceFeed calculates center price off of two GetPrice() calls in GetCenterPrice(). Are the GetPrice() calls not pulling fresh data from the DEX during GetCenterPrice()? Or is the priceFeed generated long enough before staticSpreadLevelProvider runs that it's out of date?

@nikhilsaraf
Copy link
Contributor

@Reidmcc GetPrice() does not pull from the DEX :)
it pulls from data sources outside the DEX (such as coinmarketcap, kraken, binance, etc.)

@Reidmcc
Copy link
Contributor

Reidmcc commented Oct 23, 2018

@nikhilsaraf Ah, misunderstanding on my part. Thanks.

@jimdanz
Copy link
Author

jimdanz commented Oct 23, 2018

@nikhilsaraf thanks for taking a look here. Your proposed approach sounds great, and I'm very bullish on it being at the framework level.

As a small matter of personal preference, I think it'd be easier to manage if the flag was set in strategy.cfg rather than as a command-line argument. One concrete reason I say that is that down the line, "maker only" may want to evolve into a parameter that you can tune rather than something you have to be absolute about (eg you might have a tolerance where you're willing to place orders that are up to 10% taker, rather than pure maker only). And also because for me ergonomically it's easier to keep track of changes in the version-controlled cfg file.

@nikhilsaraf
Copy link
Contributor

nikhilsaraf commented Oct 23, 2018

@jimdanz yes, I think we can make it work by keeping it at the strategy config level -- will have to augment the interface for the strategy a little but I think it should be possible. Updated your original comment to include this in the specification section.

this is something that I'll try to include in the December release (possibly November if things go quick) since I have a couple of pressing changes I'm actively working on. Is that something that would work for you?

@nikhilsaraf nikhilsaraf added this to the v2.0.0 milestone Oct 23, 2018
@nikhilsaraf nikhilsaraf self-assigned this Oct 23, 2018
@jimdanz
Copy link
Author

jimdanz commented Oct 23, 2018

@nikhilsaraf sounds great -- thanks! I'll be eager to try it out once it's available.

@nikhilsaraf nikhilsaraf added the feature request New feature or request label Oct 27, 2018
@Reidmcc
Copy link
Contributor

Reidmcc commented Oct 29, 2018

@nikhilsaraf With the orderbook checking code I added for my new strategy in hand, I think I could put this request together pretty easily (especially at a config file level). I don't want to get in the way of your plan if you already have the details in mind though.

@nikhilsaraf nikhilsaraf modified the milestones: v2.0.0, v1.4.0 Dec 5, 2018
@nikhilsaraf
Copy link
Contributor

blocked on #97 which introduces the GetOrderBook API method on sdex.go

@nikhilsaraf
Copy link
Contributor

@jimdanz fyi, you can use this feature now once you check out the master branch.
just add SUBMIT_MODE="maker_only" in the trader.cfg file.

See the sample_trader.cfg file as an example. let me know if you run into any issues.

@jimdanz
Copy link
Author

jimdanz commented Feb 5, 2019

Awesome!!! 💯
Very stoked to give this a try. Thanks @nikhilsaraf !

@nikhilsaraf nikhilsaraf added this to Done in Kelp Feb 6, 2019
@nikhilsaraf nikhilsaraf moved this from Done to Done (earlier sprints) in Kelp Feb 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature request New feature or request
Projects
Kelp
  
Done (earlier sprints)
Development

Successfully merging a pull request may close this issue.

3 participants