[jhonabreul]

Description

Python users reach for the Order property names on order events and fees and fail with attribute errors:

'OrderEvent' object has no attribute 'tag'
'OrderEvent' object has no attribute 'time'
'OrderFee' object has no attribute 'amount'
'ComboLimitOrder' object has no attribute 'group_order_manager_id'

The change:

• OrderEvent.Tag is the order's tag, distinct from Message, the brokerage message. The OrderEvent(Order, ...) constructor copies it and BrokerageTransactionHandler.HandleOrderEvents stamps it, so events after a tag update carry the new tag. It is JsonIgnore: the tag lives on the order, and deserialized events read it back as null.
• OrderEvent.Time is a read/write synonym of UtcTime, marked JsonIgnore so serialized events are unchanged.
• OrderEvent.OrderType exposes the order's type. The OrderEvent(Order, ...) constructor copies it from the order and DefaultExerciseModel sets it on exercise events.
• Order.GroupId and OrderEvent.GroupId expose the combo group id, null for non-combo orders.
• BrokerageTransactionHandler.HandleOrderEvents stamps the type and group id from the order when attaching the ticket, covering brokerages that build events from the order id alone.
• SerializedOrderEvent carries both as orderType and groupId, omitted when null, and OrderEvent.FromSerialized restores them.
• OrderType is nullable, unlike Order.Type: payloads written before this change have no type and read back as null instead of silently as Market.
• GroupOrderExtensions.GetGroupId and IsFilled on IEnumerable<OrderTicket> track a combo order from the leg tickets the combo order methods return.
• OrderFee.Amount and OrderFee.Currency are read-only synonyms of Value.Amount and Value.Currency, marked JsonIgnore.

The StubsIgnore and EditorBrowsable attributes for the stubs generator moved to #9988, which ships with QuantConnect/quantconnect-stubs-generator#104.

Related Issue

N/A

Motivation and Context

Order events, fees and combo legs should expose the same names as orders so Python code written against Order works on them too.

Requires Documentation Change

Yes: document OrderEvent.Tag, Time, OrderType and GroupId, Order.GroupId, OrderFee.Amount and Currency, and the combo ticket helpers.

How Has This Been Tested?

• OrderEventTests.TagIsTheOrderTagAndNotTheMessage: an order tagged entry with message Filled at stale price reads back as both; an event built from the order id alone has a null tag.
• OrderEventTests.TimeIsASynonymOfUtcTime: reads and writes through the synonym.
• OrderEventTests.TagAndTimeAreAccessibleFromPython: order_event.tag, order_event.message and order_event.time from a Python module.
• OrderEventTests.OrderTypeIsTakenFromTheOrder and GroupIdIsTakenFromTheOrder: copied from a stop limit order and a combo order, null otherwise.
• OrderEventTests.RoundTripSerialization: orderType and groupId are serialized and restored.
• OrderEventTests.BackwardsCompatibleDeserialization: a payload without them reads back as null.
• OrderEventTests.NullOrderTypeAndGroupIdAreNotSerialized: null values emit no key through the converter or plain serialization.
• OrderEventTests.JsonIgnores and RoundTripSerialization: the tag and the Time synonym never appear in serialized output, a deserialized event's tag is null.
• BrokerageTransactionHandlerTests.BrokerageOrderEventsCarryTheOrderTypeAndTag and ...TheGroupId: brokerage events built from the order id reach the algorithm with the order's type, tag and group id, keeping their own message; after UpdateOrderFields.Tag the fill event carries the new tag.
• OrderTests.GroupIdComesFromTheGroupOrderManager: null for a market order, the manager id for a combo order.
• GroupOrderExtensionsTests.GetGroupIdOfTheLegTickets and IsFilledOnlyWhenEveryLegTicketIsFilled.
• OrderFeesTests.AmountAndCurrencyAreSynonymsOfTheValueAmountAndCurrency and AmountIsAccessibleFromPython: values and serialization unchanged, order_fee.amount from Python.
• OptionExerciseAssignRegressionAlgorithm and ComboMarketOrderAlgorithm: unchanged statistics.
• End to end: a Python algorithm tags a market, a limit and a bracket order, retags and cancels the limit order, and checks order_event.tag against get_order_by_id(...).tag on every event. Log in the comments.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• Refactor (non-breaking change which improves implementation)
• Performance (non-breaking change which improves performance. Please add associated performance test and results)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Non-functional change (xml comments/documentation/etc)

Checklist:

• My code follows the code style of this project.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• My branch follows the naming convention bug-<issue#>-<description> or feature-<issue#>-<description>

[arhancanli]

Read at head d0a1432 (suite not run). Things that check out:

• ProtoMember(20) and (21) on OrderEvent are free: the file uses 1-19 and nothing else.
• Clone() is MemberwiseClone, so the two new properties carry over.
• Time on OrderEvent now shadows the QuantConnect.Time class inside that file; the one static use I found (FromSerialized, line 387) is fixed to QuantConnect.Time., and a search of the file shows no other Time. call.

What I could not find in the test diff is a round trip for the two new fields through SerializedOrderEvent. The tests cover the null case (NullOrderTypeAndGroupIdAreNotSerialized) and the constructor path (OrderTypeIsTakenFromTheOrder, GroupIdIsTakenFromTheOrder), but nothing builds new SerializedOrderEvent(orderEvent, id) with OrderType = Limit and GroupId = 7, runs FromSerialized, and asserts both come back. FromSerialized and the SerializedOrderEvent constructor are the two places that were edited by hand, so a dropped assignment there would pass the current tests. A protobuf round trip would also confirm that OrderType? (an enum) works as a nullable ProtoMember here.

Minor: events built with the long OrderEvent(orderId, symbol, utcTime, ...) constructor (brokerage code that doesn't go through an Order) leave both properties null. The doc comment says null means "predates this property", so a consumer can't tell that from "created without an Order"; worth a sentence in the summary if downstream code will rely on OrderType.

[quantconnect-kronos-reviewer]

Requesting changes: OrderEvent.Tag gives the wrong value, and the branch conflicts with master.

Blocking

1. OrderEvent.Tag is not the order's tag. It is a synonym of Message, the brokerage/exchange message, while Order.Tag is the user's tag. The PR's motivation is that Python code written against Order should also work on events. Code that reads order_event.tag expecting the tag it set gets something else, with no error. I tested it end to end in a Python backtest on this branch merged with master, calling self.market_order("GOOG", 1, tag="equity-tag"). In on_order_event, order_event.tag was '' for both the submitted and filled events, while self.transactions.get_order_by_id(e.order_id).tag was 'equity-tag'. The setter also overwrites Message, so order_event.tag = ... replaces the brokerage message in logs and results. TagAndTimeAreAccessibleFromPython doesn't catch this because it passes "This is a message" as the message and never sets an order tag. Please either drop Tag or have it return the order's tag, copying it from the order in the constructor and stamping it in HandleOrderEvents the way OrderType and GroupId are, and add a test where the order tag and the message differ.
2. Merge conflict with master in Tests/Common/Orders/Fees/OrderFeesTests.cs. Master's pythonnet 2.0.67 update (#9964) added OrderFeeIsNumericFromPython to the same place, so this needs a rebase. The current CI ran on the old base with pythonnet 2.0.66. On that base the EditorBrowsable markings do nothing: Python.NET binds extension methods to instances only from 2.0.67. So CI hasn't tested that part. Resolving the conflict by keeping both sides is straightforward; I did it locally (results below).

Non-blocking

3. GetGroupId / IsFilled from Python. combo_market_order returns a Python list to algorithms (I logged type(tickets): <class 'list'>). So tickets.is_filled() and tickets.get_group_id() fail with 'list' object has no attribute 'is_filled'. Only the static form works: GroupOrderExtensions.is_filled(tickets) returned True and GroupOrderExtensions.get_group_id(tickets) returned the group id. Since this is a Python-motivated change, please add a Python test for the form that works, and make sure the docs show that form.
4. Scope. Marking about 25 engine helpers EditorBrowsable(Never) undoes something master's pythonnet update introduced: those helpers being bound to instances. That has nothing to do with order-event synonyms. It is backwards compatible (the static forms still work, and I verified Extensions.requires_mapping(symbol)), but it would be easier to review, and to revert, as its own PR together with a Python test that the helpers are not bound. The same goes for the StubsIgnore on OrderTicket's implicit operator int.
5. Doc comment on OrderType (follow-up to the earlier comment): events built with the long constructor, as brokerages do, are also null until HandleOrderEvents stamps them. The comment only mentions old payloads.

What checks out

• Round-trip serialization of orderType/groupId is now covered (RoundTripSerialization), which addresses the earlier review comment. I also checked a protobuf round trip locally: OrderType? and int? with ProtoMember(20/21) come back as ComboLimit/7, and a market order's event comes back as Market/null.
• On this branch merged with master: the 361 tests in OrderEventTests, OrderFeesTests, GroupOrderExtensionsTests, OrderTests and BrokerageTransactionHandlerTests pass.
• From Python, order_event.order_type, group_id, time, order_fee.amount/currency and order.group_id return the right values. hasattr(symbol, 'requires_mapping') and hasattr(order, 'try_get_group_orders') are False.
• In the backtest, every event, for both the equity and the combo legs, had order_type/group_id equal to the order's (MARKET/None, COMBO_MARKET/1).
• The added work per order event is two property assignments, which doesn't matter for performance. The Benchmarks failure is not from this change: master fails the same way (run 40723665017, StatelessCoarseUniverseSelectionBenchmark and others), and none of the benchmark algorithms that failed place orders.

Description: it lists unit and regression tests but no end-to-end run. Please add one after the fix, with a Python algorithm that sets an order tag and checks order_event.tag against it, and give its outcome.

Other repositories: the Documentation repository needs the new members (listed under Requires Documentation Change). QuantConnect/quantconnect-stubs-generator#104 depends on this PR's EditorBrowsable/StubsIgnore markings, so it should go together with whatever PR ends up carrying them.

Risk: risk:high. This adds to Lean's user-facing algorithm API (OrderEvent, Order, OrderFee and new extension methods) and to the serialized order-event payload.

[jhonabreul]

The PR branch has been rebased from master. The review can be re-done since it has a newer version of Pythonnet

[arhancanli]

Re-read at head a0de170 (suite not run). My earlier gap is closed: RoundTripSerialization now sets OrderType = StopLimit and GroupId = 7, asserts both appear in the serialized JSON ("orderType":"stopLimit", "groupId":7) and that they come back after DeserializeObject, and BackwardsCompatibleDeserialization asserts both stay null for old payloads. ProtoMember(20) and (21) are still unused elsewhere in OrderEvent, and the QuantConnect.Time qualification in FromSerialized is still there.

One thing still untested: the protobuf path. ProtoMember(20)/(21) on OrderType? are only exercised through JSON here, so a protobuf serialize/deserialize of an OrderEvent with OrderType = Limit and GroupId = 7 would be the check that the nullable enum round-trips there too. Is OrderEvent protobuf-serialized anywhere in the live or backtest path, or are the attributes only for consistency with the other members?

[quantconnect-kronos-reviewer]

Re-reviewed at head a0de170. The only change since my last review (at 58f16ff) is the rebase onto master. The range-diff shows the four feature commits otherwise unchanged, and the pythonnet 2.0.68 commits and their reverts are gone. Still requesting changes: the OrderEvent.Tag issue has not been addressed.

Resolved since last review

• The merge conflict in OrderFeesTests.cs is resolved by keeping both sides. OrderFeeIsNumericFromPython is still there next to the new tests. GitHub now reports the PR as mergeable.
• CI is green on the new base, which uses pythonnet 2.0.67. That means the EditorBrowsable markings now run against the Python.NET version where they matter.

Still blocking

1. OrderEvent.Tag is not the order's tag (Common/Orders/OrderEvent.cs:150, unchanged). It still gets and sets Message, the brokerage/exchange message. A Python algorithm that places market_order(..., tag="x") and reads order_event.tag gets '' back, with no error. Setting it overwrites the brokerage message. This contradicts the PR's own motivation, which is that names written against Order should work on events. Please either drop Tag, or make it the order's tag: copy it in the OrderEvent(Order, ...) constructor and stamp it in HandleOrderEvents the same way OrderType/GroupId are. Add a test where the order tag and the message differ.
2. No end-to-end run in the description. After the fix, please add a run with a Python algorithm that sets an order tag and checks order_event.tag against it, and give the outcome.

Still open, non-blocking (details in my previous review)

3. GetGroupId/IsFilled: from Python, combo_market_order returns a list, so only GroupOrderExtensions.is_filled(tickets) works. Please add a Python test for that form.
4. Scope: the EditorBrowsable(Never) markings on about 25 engine helpers and the StubsIgnore on OrderTicket's implicit operator int are unrelated to the order-event synonyms. They would be easier to review and revert as a separate PR, with a Python test that the helpers are not bound to instances. No such test exists today, so CI being green doesn't show the markings work.
5. The OrderType doc comment should mention that events built with the long constructor are null until HandleOrderEvents stamps them.

Other repositories: the Documentation repository still needs the new members. QuantConnect/quantconnect-stubs-generator#104 depends on the EditorBrowsable/StubsIgnore markings and should ship together with whichever PR carries them.

Risk: I kept risk:high. The PR adds to Lean's user-facing algorithm API (OrderEvent, Order, OrderFee, new extension methods) and to the serialized order-event payload.

[jhonabreul]

Pushed ec58c00.

1. OrderEvent.Tag is now the order's tag. The OrderEvent(Order, ...) constructor copies order.Tag, and HandleOrderEvents stamps it next to OrderType and GroupId, so brokerage events built from the order id alone get it too and an UpdateOrderFields.Tag update shows on later events. The setter is internal and Message is untouched. It stays JsonIgnore: the tag lives on the order, and deserialized events read it back as null, as the doc comment says.

   Tests: OrderEventTests.TagIsTheOrderTagAndNotTheMessage (tag entry, message Filled at stale price, both read back; null tag on an event built from the id), TagAndTimeAreAccessibleFromPython, JsonIgnores and RoundTripSerialization (tag never serialized), and BrokerageTransactionHandlerTests.BrokerageOrderEventsCarryTheOrderTypeAndTag (brokerage event keeps its own message and gets the tag; after a tag update the fill carries the new one).

2. End-to-end run. The algorithm below ran as a backtest on this branch with pythonnet 2.0.67 on SPY minute data. It tags a market order entry, a limit order exit that it retags to exit-updated and cancels, and a bracket order bracket, and raises if any event's order_event.tag differs from get_order_by_id(...).tag or equals the message.

   Algorithm log

   2013-10-07 09:31:00 order 1 SUBMITTED: tag='entry' order.tag='entry' message=''
   2013-10-07 09:31:00 order 1 FILLED: tag='entry' order.tag='entry' message=''
   2013-10-07 09:31:00 order 2 SUBMITTED: tag='exit' order.tag='exit' message=''
   2013-10-07 09:31:00 order 2 CANCEL_PENDING: tag='exit' order.tag='exit' message=''
   2013-10-07 09:31:00 order 2 UPDATE_SUBMITTED: tag='exit-updated' order.tag='exit-updated' message=''
   2013-10-07 09:31:00 order 2 CANCELED: tag='exit-updated' order.tag='exit-updated' message=''
   2013-10-07 09:31:00 order 3 SUBMITTED: tag='bracket' order.tag='bracket' message=''
   2013-10-07 09:31:00 order 4 SUBMITTED: tag='bracket' order.tag='bracket' message=''
   2013-10-07 09:31:00 order 5 SUBMITTED: tag='bracket' order.tag='bracket' message=''
   2013-10-07 09:31:00 order 3 FILLED: tag='bracket' order.tag='bracket' message=''
   2013-10-08 16:00:00 ALL CHECKS PASSED
   

   OrderEventTagAlgorithm.py

   from AlgorithmImports import *
   
   
   class OrderEventTagAlgorithm(QCAlgorithm):
       """Sets tags on orders and checks that order_event.tag is the order's tag, not the brokerage message."""
   
       def initialize(self) -> None:
           self.set_start_date(2013, 10, 7)
           self.set_end_date(2013, 10, 8)
           self.set_cash(100000)
           self._spy = self.add_equity("SPY", Resolution.MINUTE).symbol
           self._exit_ticket: Optional[OrderTicket] = None
           self._events: List[str] = []
   
       def on_data(self, slice: Slice) -> None:
           if self._exit_ticket is not None:
               return
   
           self.market_order(self._spy, 100, tag="entry")
           price = self.securities[self._spy].price
           self._exit_ticket = self.limit_order(self._spy, -100, price * 1.1, tag="exit")
           self._exit_ticket.update_tag("exit-updated")
           self._exit_ticket.cancel()
           self.bracket_order(self._spy, 10, take_profit_price=price * 1.05, stop_loss_price=price * 0.95, tag="bracket")
   
       def on_order_event(self, order_event: OrderEvent) -> None:
           order = self.transactions.get_order_by_id(order_event.order_id)
           self.log(f"order {order_event.order_id} {order_event.status}: tag={order_event.tag!r} order.tag={order.tag!r} message={order_event.message!r}")
           if order_event.tag != order.tag:
               raise ValueError(f"order_event.tag {order_event.tag!r} != order.tag {order.tag!r}")
           if order_event.tag == order_event.message:
               raise ValueError(f"order_event.tag should not be the message: {order_event.message!r}")
           self._events.append(f"{order_event.status}:{order_event.tag}")
   
       def on_end_of_algorithm(self) -> None:
           expected = [f"{status}:{tag}" for status, tag in [
               (OrderStatus.SUBMITTED, "entry"), (OrderStatus.FILLED, "entry"),
               (OrderStatus.SUBMITTED, "exit"), (OrderStatus.CANCEL_PENDING, "exit"), (OrderStatus.UPDATE_SUBMITTED, "exit-updated"),
               (OrderStatus.CANCELED, "exit-updated"),
           ]]
           if self._events[:len(expected)] != expected:
               raise ValueError(f"Unexpected events: {self._events}")
           if len([event for event in self._events if event.endswith(":bracket")]) < 3:
               raise ValueError(f"Expected the bracket legs to carry the bracket tag: {self._events}")
           self.log("ALL CHECKS PASSED")

3. GetGroupId/IsFilled from Python. We are fine with the static form being the only one on Python lists; no Python test added for it.

4. Scope. The EditorBrowsable(Never) and StubsIgnore markings moved to Hide engine-only extension methods and the OrderTicket int conversion from Python #9988, with a Python test that the hidden helpers are not bound to instances. This PR no longer carries them.

5. OrderType doc comment now says it is null for events built from the order id alone until the transaction handler stamps them, and for older payloads.

The description is updated accordingly.

[quantconnect-kronos-reviewer]

Re-reviewed at head ec58c00. Since my last review (at a0de170) the EditorBrowsable/StubsIgnore commits were reverted and moved to #9988, and OrderEvent.Tag is now the order's tag. Approving. Both blocking items from my last review are resolved.

Resolved since last review

1. OrderEvent.Tag is now the order's tag (Common/Orders/OrderEvent.cs:152). The OrderEvent(Order, ...) constructor copies order.Tag, and HandleOrderEvents stamps it next to OrderType/GroupId. The setter is internal, it is JsonIgnore, and Message is left alone. I checked every engine path that raises an event: all of them either use the Order constructor or go through HandleOrderEvents (BrokerageTransactionHandler.cs lines 393, 556, 1097, 1147, 1236, 1253, 1723, 2167, plus brokerage events). Option exercise events built from the order id go through HandleOrderEvents too. So every event the algorithm receives carries the tag. HandleOrderEvents reads order.Tag at handling time, so an event that arrives after an UpdateOrderFields.Tag update shows the new tag. That matches the "order's tag" meaning, and BrokerageOrderEventsCarryTheOrderTypeAndTag tests it. Both writes happen under _lockHandleOrderEvent, and the value is a reference assignment, so a tag update from the algorithm thread at the same moment is safe.
2. End-to-end run: the author's comment has a Python backtest that tags market, limit and bracket orders, retags and cancels one, and checks order_event.tag against get_order_by_id(...).tag and against the message on every event. Its outcome: every check passed. This run would have failed with the old Message synonym, so it covers the concern.
3. Scope: the EditorBrowsable(Never)/StubsIgnore markings are gone from this PR. The diff against master now holds only the order-event, order, fee and combo-ticket changes.
4. OrderType doc comment now says the value is null for events built from the order id alone until the handler stamps them.

Verified locally at ec58c00

• The 362 tests in OrderEventTests, OrderFeesTests, GroupOrderExtensionsTests, OrderTests and BrokerageTransactionHandlerTests pass.

CI: everything passes except Benchmarks, and that failure doesn't come from this change:

• The failing benchmark is EmptySingleSecuritySecondEquityBenchmark (Python), at 250k dps against a 274.5k threshold. That algorithm places no orders, and the code this PR touches runs only on order events.
• Benchmarks passed at 38c45f3. The only commit since then adds the Tag assignment on the order-event path.
• I ran that benchmark locally on master and on this branch, alternating runs. Master gave 190k, 201k and 181k. This branch gave 197k, 189k and 183k. Both fall in the same range.

Non-blocking

• Common/Orders/OrderEvent.cs now starts with a UTF-8 BOM that master doesn't have, and no other file in Common/Orders has one. It's harmless, but it adds a one-character diff on line 1. Consider dropping it.
• GetGroupId/IsFilled from Python work only in the static form (GroupOrderExtensions.is_filled(tickets)), because combo_market_order returns a Python list. The author accepted that. The docs should show the static form.

Other repositories: the Documentation repository needs the new members: OrderEvent.Tag, Time, OrderType, GroupId, Order.GroupId, OrderFee.Amount/Currency, and the combo ticket helpers. QuantConnect/quantconnect-stubs-generator#104 now goes with #9988, not this PR.

Risk: risk:high. The PR adds to Lean's user-facing algorithm API (OrderEvent, Order, OrderFee, new extension methods) and to the serialized order-event payload (orderType, groupId). Those are high-risk traits however good the evidence is. The additions are backwards compatible: no existing member changes, and older payloads deserialize with null.