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

all_or_nothing transactions #590

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

jsphon
Copy link
Contributor

@jsphon jsphon commented May 25, 2022

This is to address issue:

#542

flumine/execution/transaction.py Outdated Show resolved Hide resolved
flumine/execution/transaction.py Outdated Show resolved Hide resolved
@@ -224,5 +251,15 @@ def __enter__(self):
return self

def __exit__(self, exc_type, exc_val, exc_tb):
if self.all_or_nothing:
Copy link
Member

Choose a reason for hiding this comment

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

Move to below ln264 to keep things quick

flumine/execution/transaction.py Outdated Show resolved Hide resolved
renaming all_or_nothing to atomic
clearing the transaction if there's a failure.
@jsphon
Copy link
Contributor Author

jsphon commented Jun 6, 2022

Thanks for the comments. I have updated the PR accordingly.

@liampauling
Copy link
Member

liampauling commented Jun 8, 2022

Next problem is the runner_context in the transaction we update this but we would need to either delay or unwind when atomic.

@jsphon jsphon requested a review from liampauling June 9, 2022 19:54
@@ -146,6 +168,9 @@ def replace_order(
def execute(self) -> int:
packages = []
if self._pending_place:
for order, market_version in self._pending_place:
Copy link
Member

Choose a reason for hiding this comment

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

We can't delay like this as it will result in the controls that act on live/open/count orders/trades incorrect

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 see that RunnerContext.place updates these fields:

  • invested
  • datetime_last_placed
  • trades
  • live_trades

I've updated the transaction to remove failed trades from live_trades, but am unsure whether the other 3 fields need to be reverted as well.

If a transaction contains multiple orders on the same runner, working out which datetime_last_placed to set it to could be tricky.

Similarly for RunnerContext.trades, if a trade contains multiple orders, but only one of them fails, which element from RunnerContext.trades should we remove?

@jsphon jsphon requested a review from liampauling June 14, 2022 22:05
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