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.

Sign up for GitHub

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

[Exchange] fix open position check and set_symbol_position_mode #807

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+12 −6
Open

[Exchange] fix open position check and set_symbol_position_mode #807

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion octobot_trading/exchanges/connectors/ccxt/ccxt_connector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,7 @@ async def set_symbol_margin_type(self, symbol: str, isolated: bool):
marginType=self.CCXT_ISOLATED if isolated else self.CCXT_CROSSED)

async def set_symbol_position_mode(self, symbol: str, one_way: bool):
return await self.client.set_position_mode(self, hedged=not one_way, symbol=symbol)
return await self.client.set_position_mode(hedged=not one_way, symbol=symbol)
Copy link
Member

Choose a reason for hiding this comment

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

😮


async def set_symbol_partial_take_profit_stop_loss(self, symbol: str, inverse: bool,
tp_sl_mode: enums.TakeProfitStopLossMode):
Expand Down
3 changes: 2 additions & 1 deletion octobot_trading/exchanges/traders/trader.pxd
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,5 @@ cdef class Trader(util.Initializable):
cpdef object set_risk(self, object risk)
cpdef object convert_order_to_trade(self, object order)

cdef bint _has_open_position(self, str symbol)
Copy link
Member

Choose a reason for hiding this comment

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

I don't remember if cpdef works with any()

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 doenst work, I tried both cdef and cpdef

Copy link
Member

@GuillaumeDSM GuillaumeDSM Jan 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

generators don't work with cython. We gotta consume the generator into a list and use this list into any() for it to be cythonizable. here it would mean doing this:

return any([
                position.size
                for position in self.exchange_manager.exchange_personal_data.positions_manager.get_symbol_positions(
                    symbol=symbol
                )])

Not sure it's worth it though. Usually doing this is faster and cythonizable:

for position in self.exchange_manager.exchange_personal_data.positions_manager.get_symbol_positions(
            symbol=symbol
    ):
    if position.size:
        return True
return False

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 changed it to your second suggestion

Copy link
Member

@GuillaumeDSM GuillaumeDSM Feb 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you want to apply those changes or should we merge like this ?

# any() cant be cythonized
# cdef bool _has_open_position(self, str symbol)
7 changes: 5 additions & 2 deletions octobot_trading/exchanges/traders/trader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,5 +677,8 @@ def _has_open_position(self, symbol):
:param symbol: the position symbol
:return: True if open position for :symbol: exists
"""
return len(self.exchange_manager.exchange_personal_data.positions_manager.get_symbol_positions(
symbol=symbol)) != 0
return any(
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this change break the set_position_mode if self._has_open_position(symbol): condition and make it impossible to set a position mode on a empty position ? If yes then I think we should adapt set_position_mode to work on empty positions as well (I believe we should be able to change the position mode before creating the position)

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'm not sure if I get that right. As far as it looks to me, we can change the mode on an 0 size position, but we cant when size != 0.
I just tried it and it raises TooManyOpenPositionError when open position on the exchange and it doesnt when size is 0

Copy link
Member

Choose a reason for hiding this comment

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

Ok then let's forget what i said, your change is right 💯

position.size
for position in self.exchange_manager.exchange_personal_data.positions_manager.get_symbol_positions(
symbol=symbol
))
6 changes: 4 additions & 2 deletions tests/exchanges/traders/test_trader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1049,7 +1049,8 @@ async def test_set_position_mode(future_trader_simulator_with_default_linear):
await position_inst.initialize()
position_inst.update_from_raw(
{
ExchangeConstantsPositionColumns.SYMBOL.value: DEFAULT_FUTURE_SYMBOL
ExchangeConstantsPositionColumns.SYMBOL.value: DEFAULT_FUTURE_SYMBOL,
ExchangeConstantsPositionColumns.SIZE.value: 1
}
)
exchange_manager_inst.exchange_personal_data.positions_manager.upsert_position_instance(position_inst)
Expand All @@ -1071,7 +1072,8 @@ async def test__has_open_position(future_trader_simulator_with_default_linear):
await position_inst.initialize()
position_inst.update_from_raw(
{
ExchangeConstantsPositionColumns.SYMBOL.value: DEFAULT_FUTURE_SYMBOL
ExchangeConstantsPositionColumns.SYMBOL.value: DEFAULT_FUTURE_SYMBOL,
ExchangeConstantsPositionColumns.SIZE.value: 1
}
)
exchange_manager_inst.exchange_personal_data.positions_manager.upsert_position_instance(position_inst)
Expand Down