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

Implement strategy-controlled stake sizes #5189

Merged
merged 2 commits into from Jul 11, 2021

Conversation

rokups
Copy link
Contributor

@rokups rokups commented Jun 27, 2021

Submitting initial draft of strategy-controlled stake sizes so you can get the feel of it.

What should be max stake size? Should it be stake_amount specified in config or entire available balance? Both have their merits i would say.

Otherwise tests pass, but i totally did not test the code so treat it as a rough sketch, a fuel for further discussion.

closes #5076

@coveralls
Copy link

coveralls commented Jun 27, 2021

Coverage Status

Coverage increased (+0.02%) to 98.101% when pulling 7ea0a74 on rokups:rk/custom-stake into f658cfa on freqtrade:develop.

Copy link
Member

@xmatthias xmatthias left a comment

Choose a reason for hiding this comment

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

overall, i'd say it feels pretty great already - and seems to do what it's supposed to.

What should be max stake size? Should it be stake_amount specified in config or entire available balance? Both have their merits i would say.

i'm ... not sure about this either.
I guess there's nothing wrong with "entire available balance".
if you limit it to "100$" because that was set as stake_amount - you'll only allow to scale down - never to scale up.
In certain combinations (static stake + static max_open) this might have some benefits, as we'll risk having one trade block all balance (go all in) - which would cause the bot to have a lot of "failing" log messages thereafter as the bot sees a trade-slot available - but lacks the balance to enter it (obviously that check might need adjusting to check available balance > min_balance ...).

the ones i mean:

2021-06-27 15:26:17,041 - freqtrade.freqtradebot - WARNING - Can't open a new trade for LTC/USDT: stake amount is too small (0.030290051900578874 < 11.05263157894737)

freqtrade/freqtradebot.py Show resolved Hide resolved
freqtrade/strategy/interface.py Show resolved Hide resolved
freqtrade/strategy/interface.py Outdated Show resolved Hide resolved
@xmatthias xmatthias added the Enhancement Enhancements to the bot. Get lower priority than bugs by default. label Jul 4, 2021
@rokups
Copy link
Contributor Author

rokups commented Jul 9, 2021

@xmatthias i propose getting rid of min_stake and max_stake parameters altogether. max_stake can be substituted by self.wallets.get_available_stake_amount() and min_stake should be irrelevant. Also i propose to change bot behavior to clamp stake size to allowed range instead of throwing an error and bailing on a trade like it does now. Reason is same as why we use strategy_safe_wrapper() - users are careless and they will mess up, but there is no need to skip on a trade. Maybe returning 0 stake amount could be an exception to this rule, allowing to discard a trade this way. What do you think?

@rokups rokups force-pushed the rk/custom-stake branch 2 times, most recently from b1d2d15 to 41bdb8d Compare July 9, 2021 13:48
@xmatthias
Copy link
Member

I don't see why min_stake would be irrelevant.
It may for you if you're using big(ish) stakes - but for a new user trying a strategy close to minimum stake (maybe with a 100$ account), it's very much relevant (and can also influence the calculation itself - if you know you can't go below 15$).
Sure, we'll also min-cap it by this - but you can end up with randomly rejected trades because of it - for no apparent reason.

regarding max_stake - i don't see what we gain by not providing it other than increased complexity to the user.
Sure - it can be queried - but i'm pretty sure that most will require that anyway as part of the calculations....

@rokups
Copy link
Contributor Author

rokups commented Jul 11, 2021

I am conflicted about these parameters because it is unclear where we draw the line. Sure min_stake and max_stake can be used in calculations. So can be used wallet_size in stake currency (including tied up stake). So can be used max_open_trades and current_open_trades. It is getting wild fast.

Sure, we'll also min-cap it by this - but you can end up with randomly rejected trades because of it - for no apparent reason.

Part of suggestion is to enforce minimal stake size with a warning, instead of rejecting a trade. I implemented this already so please look at it and think on it whether it should be reverted. Seems to me this is much like errors in user strategy - these conditions can manifest many days after strategy started, so just like not bailing on exception from user strategy we probably should not bail on requested incorrect stake amount and instead clamp it to allowed range.

Copy link
Member

@xmatthias xmatthias left a comment

Choose a reason for hiding this comment

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

Part of suggestion is to enforce minimal stake size with a warning

This is perfectly fine the way you implemented (just not sure on the log-level for these messages).


There's however a difference between providing min_stake and max_stake - and the "open trades" values.

The trade related values are relatively easy to get from the strategy. You can't however get to min_stake without some wild hackary (exchange is NOT exposed to the strategy directly / in a documented way - which is on purpose).

I'd also want to provide max_stake due to #5247 - as i think we will need to have a similar method than max_stake - respecting the exchange's top limit (you might have 10.000$ as your stake amount - but if the exchange only allows 5000$ that'll need to be respected).
Having a documentation part that shows "get your wallet size this way" will fail in that case - as the wallet is potentially not the limiting factor.

While this will be a seperate PR / change for sure - providing max_stake will simplify exposing this information to the strategy.

freqtrade/optimize/backtesting.py Outdated Show resolved Hide resolved
freqtrade/freqtradebot.py Outdated Show resolved Hide resolved
@rokups
Copy link
Contributor Author

rokups commented Jul 11, 2021

Yeah what you say makes sense. Ill get these parameters back 👍🏻 Although i am no longer sure backtesting should throw an error when min/max is not honored. If we want to free user from having to absolutely care for these details - it must be a warning, even though if it is easier to miss.

@xmatthias
Copy link
Member

Although i am no longer sure backtesting should throw an error when min/max is not honored. If we want to free user from having to absolutely care for these details - it must be a warning, even though if it is easier to miss.

I'm not 100% sure on the implications of this - but i think showing warnings will suffice - it's possible to have the calculation "sometimes" below min/max - and if you get no buys/sells you'll (hopefully?) look at the log output ...

@rokups
Copy link
Contributor Author

rokups commented Jul 11, 2021

I am suggesting to clamp requested stake size to min-max range so buys are not ignored like they are now.

@xmatthias
Copy link
Member

I am suggesting to clamp requested stake size to min-max range so buys are not ignored like they are now.

Basically - if the strategy says "buy for 5$" - you'd increase this to 15$ (or whatever the minimum is) ?
works fine for me - just make sure we got it documented 👍

@rokups rokups marked this pull request as ready for review July 11, 2021 09:57
@rokups
Copy link
Contributor Author

rokups commented Jul 11, 2021

I think i am done with machinery. What i do not know is how to make a test for this.

Copy link
Member

@xmatthias xmatthias left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Nice work 💯

@xmatthias xmatthias merged commit 38296e8 into freqtrade:develop Jul 11, 2021
@rokups rokups deleted the rk/custom-stake branch July 17, 2021 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Enhancements to the bot. Get lower priority than bugs by default.
Projects
None yet
4 participants