-
Notifications
You must be signed in to change notification settings - Fork 64
add MinimumOrderCostNotReachedError and minor fixes #803
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
Conversation
octobot_trading/api/exchange.py
Outdated
| import octobot_trading.exchanges.connectors.ccxt.enums | ||
| import octobot_trading.exchanges.connectors.ccxt.enums as ccxt_enums | ||
| import octobot_trading.exchanges as exchanges | ||
| import octobot_trading.exchanges.exchange_builder as exchange_builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import philosophy is to import high level module (exchanges, personal_data) when not importing from within the module. The idea here is to prevent changing all imports when we update internal module architecture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, I removed those changes
octobot_trading/errors.py
Outdated
| """ | ||
|
|
||
|
|
||
| class MinimumOrderCostNotReachedError(Exception): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really understand why we need both MinimumOrderCostNotReachedError and MissingMinimalExchangeTradeVolume. MissingMinimalExchangeTradeVolume is not doing the same thing ? Should we raise it where you are raising MinimumOrderCostNotReachedError or you need another exception ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it so its using MissingMinimalExchangeTradeVolume, didnt notice that one
| enums.ExchangeConstantsMarketPropertyColumns.TAKER.value: | ||
| market_status.get(enums.ExchangeConstantsMarketPropertyColumns.TAKER.value, | ||
| constants.CONFIG_DEFAULT_FEES), | ||
| ) or constants.CONFIG_DEFAULT_FEES, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When market_status[ExchangeConstantsMarketPropertyColumns.TAKER.value], market_status[ExchangeConstantsMarketPropertyColumns.MAKER.value] or market_status[ExchangeConstantsMarketPropertyColumns.FEE.value] is None, it will now properly use the fallback fees. Before my changes it would set them to None
| if not personal_data.check_cost(float(total_order_price), min_cost): | ||
| return [] | ||
| symbol = f"{symbol_market['symbol']} - " if 'symbol' in symbol_market else "" | ||
| raise errors.MinimumOrderCostNotReachedError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it should be catch if necessary everywhere this method is called
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added exceptions, also in the PR in octobot tentacles -> [Order] handle MinimumOrderCostNotReachedError
| return side, parsed_order_type | ||
| except (KeyError, ValueError): | ||
| return None, None | ||
| return side, enums.TraderOrderType.UNKNOWN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was an accident, I removed it
3f15e66 to
f8f66a4
Compare
f8f66a4 to
15b91f8
Compare
15b91f8 to
18f12ce
Compare
18f12ce to
134e5c6
Compare
|
to reopen of relevant |
properly handle MissingMinimalExchangeTradeVolume