Protocol 6.1 new

[NichUK]

Here's the new 6.1 protocol branch.

I've pulled all the relevant stuff from the previous work and updated it for your new structure (without generics) and your implementation of Dynamic Fields (except MarketSummaries, which are handled similarly but not exactly the same).

I believe it ticks all the 6.1 boxes, and it tests OK (except I haven't been able to run option chains as they're broken in the version of IQFeed I'm currently running - they always return the new 6.2 dialect), and Level 2 as I don't currently have it.

Let me know if you think anything needs discussion, but it should all be pretty self-explanatory, but happy to discuss/deal with anything that doesn't gel for you.

Then we can look at 6.2, which on a brief look, doesn't seem to have anything particularly scary in there.

Nich

[mathpaquette]

@NichUK could you please check your build.. its failing.

[NichUK]

@mathpaquette I've made some updates to sort the test and the TestData (I didn't initially understand how the test data system worked), but it's still failing on IQFeed.CSharpApiClient.Extensions.Tests.Integration.Lookup.Historical.Resample.TickMessageExtensionsTests.Should_Resample_Bars_From_Ticks It's failing because the interval.TotalVolume != bar.TotalVolume on a bar 2422 bars in. I've had a look at the "ToHistoricalBars" extension, but I can't see why that would have suddenly stopped working, as none of the protocol updates have had anything to do with any of that stuff, and so I haven't touched it. I have updated the TestData, and had to change the date that is taken from in order to get it into the last 6 months to be able to download it from IQ, but again, I don't see why that would affect the conversion of bars. Do you have any insights into what might be failing there?

Nich

[NichUK]

Still getting an error on Should_Resample_Bars_From_Ticks, even with the protocol 6.0 file compatibility added back. I honestly don't think it's anything to do with that. For some reason on this one bar, the TotalVolume calculated in .ToHistoricalBars is not equal to the TotalVolume parsed in from the file, and I cannot see why. I haven't touched that stuff at all, or changed any of the volume handling, so I'm a little confused.

[NichUK]

@mathpaquette I've tried to dig further into the remaining problem with the bar conversion. It looks like it's actually the data/conversion. I had to generate new test data as the protocol format had changed, and the new data just doesn't seem to convert and match up with the interval file.

The problem seems to be with these three ticks. ▶ | [11015] | {Timestamp: 02/03/2021 04:11:33, Last: 1.58, LastSize: 10, TotalVolume: 10, Bid: 1.57, Ask: 1.59, TickId: 1, BasisForLast: O, TradeMarketCenter: 11, TradeConditions: 873D17, TradeAggressor: 0, DayCode: 2, RequestId: } | IQFeed.CSharpApiClient.Lookup.Historical.Messages.TickMessage ▶ | [11016] | {Timestamp: 02/03/2021 04:12:40, Last: 1.57, LastSize: 10, TotalVolume: 20, Bid: 1.57, Ask: 1.58, TickId: 2, BasisForLast: O, TradeMarketCenter: 11, TradeConditions: 873D17, TradeAggressor: 0, DayCode: 2, RequestId: } | IQFeed.CSharpApiClient.Lookup.Historical.Messages.TickMessage ▶ | [11017] | {Timestamp: 02/03/2021 04:12:40, Last: 1.57, LastSize: 340, TotalVolume: 360, Bid: 1.57, Ask: 1.58, TickId: 3, BasisForLast: E, TradeMarketCenter: 11, TradeConditions: 173D, TradeAggressor: 0, DayCode: 2, RequestId: } | IQFeed.CSharpApiClient.Lookup.Historical.Messages.TickMessage

The total volume on the tick is correct, but the ToHistoricalBarsAscending method ignores that, and creates it's own somehow and ends up with:

▶ | [2422] | {Timestamp: 02/03/2021 04:12:40, High: 1.57, Low: 1.57, Open: 1.57, Close: 1.57, TotalVolume: 340, PeriodVolume: 340, TotalTrade: 1, PeriodTrade: 1, VWAP: 1.57} | IQFeed.CSharpApiClient.Extensions.Lookup.Historical.HistoricalBar

where you can see the TotalVolume is 340, not 360. The Interval file also has the total volume as 360.

I believe that it is something to do with the first two ticks of the day being 'O' ticks which are ignored, and thus the bar is incorrectly written.

I think the new data has actually revealed a failing edge case in ToHistoricalBarsAscending. Can you confirm?

[mathpaquette]

@mathpaquette I've tried to dig further into the remaining problem with the bar conversion. It looks like it's actually the data/conversion. I had to generate new test data as the protocol format had changed, and the new data just doesn't seem to convert and match up with the interval file.

The problem seems to be with these three ticks. ▶ | [11015] | {Timestamp: 02/03/2021 04:11:33, Last: 1.58, LastSize: 10, TotalVolume: 10, Bid: 1.57, Ask: 1.59, TickId: 1, BasisForLast: O, TradeMarketCenter: 11, TradeConditions: 873D17, TradeAggressor: 0, DayCode: 2, RequestId: } | IQFeed.CSharpApiClient.Lookup.Historical.Messages.TickMessage ▶ | [11016] | {Timestamp: 02/03/2021 04:12:40, Last: 1.57, LastSize: 10, TotalVolume: 20, Bid: 1.57, Ask: 1.58, TickId: 2, BasisForLast: O, TradeMarketCenter: 11, TradeConditions: 873D17, TradeAggressor: 0, DayCode: 2, RequestId: } | IQFeed.CSharpApiClient.Lookup.Historical.Messages.TickMessage ▶ | [11017] | {Timestamp: 02/03/2021 04:12:40, Last: 1.57, LastSize: 340, TotalVolume: 360, Bid: 1.57, Ask: 1.58, TickId: 3, BasisForLast: E, TradeMarketCenter: 11, TradeConditions: 173D, TradeAggressor: 0, DayCode: 2, RequestId: } | IQFeed.CSharpApiClient.Lookup.Historical.Messages.TickMessage

The total volume on the tick is correct, but the ToHistoricalBarsAscending method ignores that, and creates it's own somehow and ends up with:

▶ | [2422] | {Timestamp: 02/03/2021 04:12:40, High: 1.57, Low: 1.57, Open: 1.57, Close: 1.57, TotalVolume: 340, PeriodVolume: 340, TotalTrade: 1, PeriodTrade: 1, VWAP: 1.57} | IQFeed.CSharpApiClient.Extensions.Lookup.Historical.HistoricalBar

where you can see the TotalVolume is 340, not 360. The Interval file also has the total volume as 360.

I believe that it is something to do with the first two ticks of the day being 'O' ticks which are ignored, and thus the bar is incorrectly written.

I think the new data has actually revealed a failing edge case in ToHistoricalBarsAscending. Can you confirm?

Sure, Ill check this weekend. Was a bit busy this week. Thank you for following up!

[NichUK]

@mathpaquette I got bored, so dug in and found/fixed the bar conversion stuff. See my new PR #111

[NichUK]

@mathpaquette I rebased this branch on the new master with the bar calculation PR included, and it now seems to build perfectly. So if you're happy with the review fixes I did, then it should be good to go!

[mathpaquette]

@mathpaquette I rebased this branch on the new master with the bar calculation PR included, and it now seems to build perfectly. So if you're happy with the review fixes I did, then it should be good to go!

thats funny, I was about to rebase it. Let me do a last final review. lets merge it today.

[NichUK]

Then, I've put a set of proposals in the Issue tracking 6.2, and started the base work on that. But there's a couple of decisions that should be made/agreed before I do the rest of it, to save doing it twice. :) So interested in your comments when you have some time.

I'm dipping in and out this weekend, but frankly, it shouldn't take too long to get it done once I we've agreed the approach.

Also, the current beta of IQFeed (6.2.0.15) has some bugs, which I've reported, so we won't be able to actually launch it for a bit, but we could give users access to a beta version to test with the 6.2 beta.

[mathpaquette]

@NichUK I was about to merge but I realized that some integration tests are not passing. Could you please fix before ? Thanks

[NichUK]

What version of IQFeed did you test against?

[mathpaquette]

What version of IQFeed did you test against?

IQFeed Client	6.1.0.20	September 4th, 2019

[mathpaquette]

What version of IQFeed did you test against?

IQFeed Client | 6.1.0.20 | September 4th, 2019

-- | -- | --

@NichUK is it supposed to work ?

[mathpaquette]

@NichUK I was about to merge but I realized that some integration tests are not passing. Could you please fix before ? Thanks

Did you try ?

[NichUK]

with regards to https://github.com/mathpaquette/IQFeed.CSharpApiClient/pull/100#discussion_r606701586 (@NichUK to be honest, we should add in GetHistoryTickDaysAsync something like. "days = days > int16.MaxDouble ? int16.MaxDouble" days instead...)

I'm not sure that's the right way to go. If someone requests a number of days that's larger than Int16.MaxValue, then I believe that it should cause an exception, so they know that the request is wrong, rather than silently just handling it down to the correct value, and then returning less data than the user was expecting. Obviously it's a lot of days, so it probably won't be an issue, but my instinct is that we should push the Int16.MaxValue limitation up the whole stack through the RequestFormatter, and make it obvious, that it's a breaking change from IQ.

I've prepared a commit that does that which will be in the next push.

[mathpaquette]

with regards to https://github.com/mathpaquette/IQFeed.CSharpApiClient/pull/100#discussion_r606701586

(@NichUK to be honest, we should add in GetHistoryTickDaysAsync something like. "days = days > int16.MaxDouble ? int16.MaxDouble" days instead...)

I'm not sure that's the right way to go. If someone requests a number of days that's larger than Int16.MaxValue, then I believe that it should cause an exception, so they know that the request is wrong, rather than silently just handling it down to the correct value, and then returning less data than the user was expecting. Obviously it's a lot of days, so it probably won't be an issue, but my instinct is that we should push the Int16.MaxValue limitation up the whole stack through the RequestFormatter, and make it obvious, that it's a breaking change from IQ.

I've prepared a commit that does that which will be in the next push.

But it would be nice to not mess around int32 and int16 in the codebase to avoid confusion for dev. Not sure it brings enough value to raise an exception up to the user... I'll check what you propose but personally I agree we can adjust that in the formatters.

[NichUK]

When doing the upgrade to Options chains, I didn't realise that as well as adding the "includeNonStandardOptions" parameter, they had also (not explicitly documented) removed the BinaryOptionsFilter parameter. I have fixed it all up, and the tests now seem to run happily again v6.1

[NichUK]

For me, the Int16.MaxValue thing is simply that IQFeed have made a breaking change to their API, and anyone using it should "know", by updating their code appropriately, and thus considering any ramifications. If we just handle it transparently, it might cause problems for someone down the line who had no idea that the change had happened at all.

[NichUK]

@mathpaquette OK, I've pushed all the fixes... I think everything should be good, except the Int16.MaxValue which from your comments I suspect you won't like. I can revert that one if I can't persuade you. :)

[mathpaquette]

@mathpaquette OK, I've pushed all the fixes... I think everything should be good, except the Int16.MaxValue which from your comments I suspect you won't like. I can revert that one if I can't persuade you. :)

haha I definitely dont like it lol. I mean, int16 means 90 years lol. I dont want to deal with different int types and also this breaking change could be avoided from a consumer perspective. That being said, Ill put it in the changelog. Just revert this one and we're ready to go.

I have to say, I'm pretty excited to merge it. thank you so much for your effort in this journey!!

[NichUK]

OK, I reverted that, and put in a limiting check. All yours!