Open shazbert opened 1 month ago
Attention: Patch coverage is 57.55167%
with 267 lines
in your changes missing coverage. Please review.
Project coverage is 38.41%. Comparing base (
cb6b342
) to head (06acaac
).
This first comment from me covers the overall architecture.
Thanks for the suggestions. I appreciate the thought you've put into the overall architecture and the proposed changes you outlined should help provide a more logical structured framework.
I'd like to suggest for now, we proceed with the current implementation @thrasher- . Just so we can merge the PR and make it usable right away and can get Sam to start rolling that across the exchanges straight away. In the meantime we can start planning iterations to gradually evolve the architecture towards the model you've outlined?
So plan is, after this is merged I want to:
I'd like to defer localizing the interface for the subscription package, just for now. I don't want to start blowing out complexity for this PR, there's already a bit of moving parts. Perhaps you could open a supplementary PR to address this when you have the time?
Thanks again for your input!
Sounds good buddy 👍
This function is very similar to WsConnect, so it might be a good idea to treat WsAuth as a single connection and use the SetupConnection method call to initialize it. Additionally, you could consider moving the dialer and context information inside the function.
Just something to consider for the next round of improvements: we can eliminate these methods by treating each connection as a single instance. This instance can be appended to the slice of ConnectionWrapper, thereby reducing the complexities created by Conn and AuthConn
This function is very similar to WsConnect, so it might be a good idea to treat WsAuth as a single connection and use the SetupConnection method call to initialize it. Additionally, you could consider moving the dialer and context information inside the function.
Good suggestion, I think that would be the direction to go when implementing. I have started a proof of concept to grow connections based off the max subscriptions per connection and I think I have already done what you have suggested. Will send you a link to it here when it's ready for your look over.
Just something to consider for the next round of improvements: we can eliminate these methods by treating each connection as a single instance. This instance can be appended to the slice of ConnectionWrapper, thereby reducing the complexities created by Conn and AuthConn
Is this specifically pointing to the change in SetWebsocketURL?
WebSocket
Multiple Connections for Asset Endpoints
Connection-Coupled Functionality
Reader Routine in Streaming Package
Context Awareness
DialContext
method toWebsocketConnection
to leverage the supported context.Gate.io Enhancements
WebSocket Support
Bug Fixes
generateDeliveryFuturesPayload
where different settlements were handled incorrectly.🐛 fix:
Type of change
Please delete options that are not relevant and add an
x
in[]
as item is complete.How has this been tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration and also consider improving test coverage whilst working on a certain feature or package.
Checklist