Support for GoodTillDate with Interactive Brokers #1201
Conversation
| /// Good till date(time) - The date and time until the order will be active. The order | ||
| /// will be automatically canceled by the exchange after this date/time. | ||
| /// </summary> | ||
| public DateTime GoodTillDate { get; set; } |
There was a problem hiding this comment.
Given the logic on line 1493 of InteractiveBrokersBrokerage.cs I tihnk this will need to be nullable -- DateTime? -- otherwise the null check there will always return true
There was a problem hiding this comment.
Upon further inspection, perhaps we should be using this field on Order.cs:
/// <summary>
/// Order Expiry on a specific UTC time.
/// </summary>
public DateTime DurationValue;There was a problem hiding this comment.
Agreed. Sorry I am a Java developer and am still learning the differences between Java and C#. In Java there is no such thing as not-nullable objects. Only primitive values are not nullable.
We could use DurationValue. Only thing is, conceptually duration and end time are different (e.g. a duration of 5 days vs. 2017-10-16 12:54:53.123) - although DurationValue in here is a DateTime and not really a duration.
Another thing is, the document says "UTC time". This is good but only if QC's servers' time is set to UTC. Can you confirm that?
Thanks for your review.
There was a problem hiding this comment.
Also, please consider that GoodTillDate is a known terminology in trading world. Duration is not.
There was a problem hiding this comment.
I believe the most used term for order duration/validity is Time In Force and common values for this attribute are: GTC (Good Till Cancelled), GTD (Good Till Date), DAY (valid to end of day).
Naming issues aside, I think for a GTD order the enum value (currently named OrderDuration) should also be set by the caller in addition to the expiry/validity date.
There was a problem hiding this comment.
RE: other brokerages -- we can update the brokerage models for the other brokerages to return false for CanSubmitOrder if we don't support certain order features.
There was a problem hiding this comment.
RE: utc time -- as much as possible we try to keep things in utc for simplicity. All orders are timestamped explicitly in UTC, independent of the server's configuration. I think the name duration was chosen for alignment with OrderDuration -- I'm historically not the best namer of things, so I'll let @jaredbroad chime in on that one -- if we want to rename it, we can come up with a shim to prevent old algorithms from breaking if they used it.
There was a problem hiding this comment.
@mchandschuh thanks for the comments. We can always mark it as deprecated and add a new attribute that mirrors the old/deprecated attribute. This way we will have backward compatibility and some few months later get rid of it.
There was a problem hiding this comment.
:D Its relatively easy as an individual @patrickstar1; but it is harder to migrate a community of 40k. We prefer to put in the design work up front. If you can wait 3-7 days we'll do that design work and get back to you.
There was a problem hiding this comment.
That'd be awesome. Thanks Jared. I totally agree with a well designed system rather than a patch even if it delays the change.
|
I think a few things need to happen to implement the desired behavior (assuming starting from
|
|
Closing for now @patrickstar1 but this is close! Please submit a new PR with Mike's suggestions above leveraging Stefano's work. |
No description provided.