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

Index #1040

Merged
merged 6 commits into from
Mar 24, 2024
Merged

Index #1040

merged 6 commits into from
Mar 24, 2024

Conversation

GuillaumeDSM
Copy link
Member

Comment on lines +73 to +77
return (
f"Not enough funds to create new orders after {final_note} evaluation: "
f"{self.exchange_manager.exchange_name} exchange minimal order "
f"volume has not been reached."
)
Copy link
Member

Choose a reason for hiding this comment

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

That's the reason why symbol is None?

Copy link
Member Author

Choose a reason for hiding this comment

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

in index trading, we don't send a given symbol to buy, we tell the consumer to "rebalance the index" and the consumer will create orders accordingly (it's not symbol related).

Currently:

  • producer decides if a rebalance should happen
  • consumer executes the rebalance

Copy link
Member

@Herklos Herklos Mar 23, 2024

Choose a reason for hiding this comment

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

(it's not symbol related).

That's my point, I don't understand why we trigger this error here when symbol is None. I don't think it's a good idea to trigger this error due to the expected caller

Copy link
Member Author

Choose a reason for hiding this comment

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

This function's goal is to log an error, if it's called it's that the error already occured and we want inform the user that an error occured. This symbol condition allows to create the error without given symbols, which is a case that can now happen

Comment on lines 234 to 235
assert consumer.get_holdings_ratio("ETH", include_assets_in_open_orders=True) \
== decimal.Decimal('0.01819090909090909090909090909')
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 understand why this value doesn't change due to order_3 addition

Copy link
Member Author

Choose a reason for hiding this comment

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

we are selling XRP for ETH in order3 making more ETH available when it will be filled

Copy link
Member

Choose a reason for hiding this comment

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

So why the ETH ratio is the same as without order_3 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's not the same, before it was 0.01818181818181818181818181818 and after it's 0.01819090909090909090909090909

Copy link
Member

Choose a reason for hiding this comment

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

Oooh right, my bad. It saw the same number 😄

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.

👍

@GuillaumeDSM GuillaumeDSM merged commit c3b797b into master Mar 24, 2024
6 checks passed
@GuillaumeDSM GuillaumeDSM deleted the index branch March 24, 2024 09:49
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.

2 participants