search

nautechsystems / nautilus_trader

A high-performance algorithmic trading platform and event-driven backtester
https://nautilustrader.io
GNU Lesser General Public License v3.0
1.71k stars 402 forks source link

Fix matching engine cached filled quantity applying partial fills #1642

Closed davidsblom closed 1 month ago

davidsblom commented 1 month ago

Pull Request

Fixes #1639

Type of change

Delete options that are not relevant.

How has this change been tested?

Currently running a long paper trading sandbox test.

cjdsellers commented 1 month ago

Hey @davidsblom thanks for the fix!

The Cython issue is due to this:

>   leaves_qty = Quantity.from_raw_c(order.quantity._mem.raw - self._cached_filled_qty[order.client_order_id]._mem.raw, last_qty._mem.precision)
E   AttributeError: 'nautilus_trader.model.objects.Quantity' object has no attribute '_mem'

The issue is here:

self._cached_filled_qty[order.client_order_id]._mem.raw

You can't actually do that because what is returned from self._cached_filled_qty[order.client_order_id] is a PyObject unless you declare the C struct type / assign it to some cdef.

e.g. this would be legal Cython:

cdef Quantity cached_filled_qty = self._cached_filled_qty[order.client_order_id]
cached_filled_qty._mem.raw

Also we'd only want to hit the cache once, and with a .get in case its None.

davidsblom commented 1 month ago

Thanks for the feedback! I'll make the changes.

cjdsellers commented 1 month ago

Just on mobile so this may not be relevant. But the quantity on the OrderFilled event should be the size for that individual fill (last_qty), not total cumulative filled for the order.

davidsblom commented 1 month ago

Yeah that's right. I've renamed the variable fill_qty to corrected_last_qty for clarity.

cjdsellers commented 1 month ago

This is looking reasonable to me, good catch moving the fill correction above the commission calculation too.

Does this fix the sandbox issues you were seeing in real time?

davidsblom commented 1 month ago

Thanks! Yeah I'm trying to run a sandbox experiment overnight but it crashes everytime after 1 or 2 hours due to the timer panic. So I haven't been able to test this one thoroughly.

davidsblom commented 1 month ago

From my point of view this PR can be merged. The sandbox run works without any issues for a couple hours now.