-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update WebSocket message parsing and types #116
Conversation
FYI - Added a few more reviewers since this is a pretty large change. Just wanted to make sure it was cool. Also, I'm not really sure who wants to review all this stuff. So, if it's a pain for anyone let me know and I'll stop. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the changes make sense, LMK what you think about using a sealed class instead of enum for Feed and Market.
Also I can't believe how many fields we were missing in our message models 😅
enum class Feed(val url: String) { | ||
Delayed("delayed.polygon.io"), | ||
RealTime("socket.polygon.io"), | ||
Nasdaq("nasdaqfeed.polygon.io"), | ||
PolyFeed("polyfeed.polygon.io"), | ||
PolyFeedPlus("polyfeedplus.polygon.io"), | ||
StarterFeed("starterfeed.polygon.io"), | ||
LaunchpadFeed("launchpad.polygon.io") | ||
} | ||
|
||
/** | ||
* Market is the type of market (e.g. Stocks, Crypto) used to connect to the server. | ||
*/ | ||
enum class Market(val market: String) { | ||
Stocks("stocks"), | ||
Options("options"), | ||
Forex("forex"), | ||
Crypto("crypto"), | ||
Options("options"), | ||
Indices("indices"), | ||
LaunchpadStocks("stocks"), | ||
LaunchpadOptions("options"), | ||
LaunchpadForex("forex"), | ||
LaunchpadCrypto("crypto") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I experimented with using a sealed class here instead of enum to support an "Other" option in this PR: #119
LMK what you think about the trade-offs there. Personally I really like the forward-compatibility that the sealed class supports even with the slight trade-off of how Java usages access these fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment in the other thread #119 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #122
src/main/kotlin/io/polygon/kotlin/sdk/websocket/PolygonWebSocketClient.kt
Outdated
Show resolved
Hide resolved
return collector | ||
} | ||
|
||
private fun parseStockMessage(frame: JsonObject): PolygonWebSocketMessage? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this or any of the other parse...
functions need to return a nullable result, since all of them have else
branches they all return a non-null PolygonWebsocketMessage
As an added bonus if all of the parse functions return a non-null result, we don't need that finalMessage
line that null-coalesces anymore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #122
…etClient.kt Co-authored-by: Michael Moghaddam <6424916+mmoghaddam385@users.noreply.github.com>
Closing this in favour of #122. |
This PR brings several major breaking changes but hopefully the enhancements around routing, message parsing, and improved spec alignment are worth it and make it easier to maintain. There are five areas that have changes but they are very interrelated and here's a breakdown:
WebSocket Connection Refactoring
Added the ability for users to select the Market and Feed type they are interested in. Previously, all WebSocket traffic ran through
socket.polygon.io
, and with this explicit selection, users will know exactly what they're connecting to. I think this also enables folks to use delayed feeds like the go and python clients. But, the major downstream effect is that this feature also enables us to route messages based on market type of easier message parsing.Message Routing / Parsing Overhaul
Added logic for routing and parsing WebSocket messages, enabling parsing of each message into specific message types based on the market selected. The need for this arose due to Stocks, Options, Indices, and LaunchPad message types with
A.*
orAM.*
being parsed incorrectly asStocksMessage.Aggregate
. The fields in the spec for these message types didn't align accurately withStocksMessage.Aggregate
. With the updated logic, we can now correctly parse messages based on theMarket
type, ensuring the correct message type is assigned each time and there are not missing or null fields.Message Type Revision
The message types for Stocks, Options, Indices, Forex, Crypto, and LaunchPad have been reviewed and updated to align with the latest spec. This ensures fields and their data types provide an accurate representation of data. I added some new fields and removed older ones. We should be in 100% compliance now.
Launchpad Message Organization
Launchpad messages are routed into their own message parser, providing future flexibility in case of spec modifications, in that if options, or indices, etc, gets more fields we can easily added them.
Dedicated Examples
Added dedicated examples for each asset class along with Launchpad. Should make onboarding very easy.
Collectively, these changes will hopefully greatly enhance the WebSocket functionality, offering better alignment with the spec and improved maintainability. Also, added in the changed from PR #115 into here.
Here's what an example client looks like now (note the feed and market types are specified):