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

[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

Conversation

techfreaque
Copy link
Contributor

No description provided.

@techfreaque techfreaque force-pushed the minor_exchange_fixes branch 3 times, most recently from ee7f10f to 31c2468 Compare January 20, 2023 14:18
@@ -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

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

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 ?

Copy link
Member

@Herklos Herklos left a comment

Choose a reason for hiding this comment

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

Can you fix the tests please?

@techfreaque
Copy link
Contributor Author

Can you fix the tests please?

Tests are fixed now

Copy link
Member

@Herklos Herklos left a comment

Choose a reason for hiding this comment

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

Thanks for your fix!

@@ -460,7 +460,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.

😮

@@ -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

@GuillaumeDSM GuillaumeDSM Jan 28, 2023

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

@@ -671,5 +671,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 💯

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.

None yet

3 participants