Kucoin: Add subscription templating and various fixes #1579
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1579 +/- ##
==========================================
- Coverage 38.20% 36.32% -1.89%
==========================================
Files 421 422 +1
Lines 152345 183014 +30669
==========================================
+ Hits 58204 66480 +8276
- Misses 86072 108483 +22411
+ Partials 8069 8051 -18
|
gbjk
force-pushed
the
feature/kucoin_sub_templating
branch
3 times, most recently
from
7754e57
to
c969696
Compare
gloriousCode
added
the
tests require fixing
gbjk
force-pushed
the
feature/kucoin_sub_templating
branch
3 times, most recently
from
f45a386
to
7718f2b
Compare
shazbert
requested changes
Jul 16, 2024
shazbert
added
reconstructing
gbjk
force-pushed
the
feature/kucoin_sub_templating
branch
3 times, most recently
from
52f9f16
to
bd970c0
Compare
|
Tests fixed. Can't and shouldn't cache a compiled template closuring anything on Kucoin itself. |
gbjk
added
review me
shazbert
requested changes
Jul 22, 2024
gbjk
force-pushed
the
feature/kucoin_sub_templating
branch
from
9c56aab
to
d3d2fa5
Compare
Now only the assetPairs relevant to the subscription are in the context
We want to trim before checking for "AssetSeparator vs All" because a template should be allowed to reuse a range template and generate just one trailing AssetSeparator whilst using a specific Asset
Turns out that contary to the documentation, kucoin supports batching of all symbols and currencies
This reduces the complexity of error checking to just be "do we get the correct numbers". Fixes Asset.All with only one asset erroring on xpandPairs, because we trimmed the only asset separator, and then errored that we're not xpanding Assets and the asset on the sub is asset.All This use-case conflicted with commit 6bbd546, which required: ``` Subscriptions: Trim AssetSeparator early We want to trim before checking for "AssetSeparator vs All" because a template should be allowed to reuse a range template and generate just one trailing AssetSeparator whilst using a specific Asset ``` Now we set up the assets earlier, and we remove the check for xpandAssets, since the number of asset lines matching is all that matters. I've removed the asset tests for this, but they were correctly erroring on the number of asset lines instead. Everything hits coverage, as well.
gbjk
force-pushed
the
feature/kucoin_sub_templating
branch
from
01a9f9f
to
82c5a54
Compare
|
Squashed down for final review 😄 |
thrasher-
changed the title
thrasher-
requested changes
Aug 9, 2024
There was a problem hiding this comment.
Just one minor nit on the introduction of the new Batch helper. It makes a pre-existing helper func obsolete.
Tests, functionality for Kucoin and config uprade all worked well. Nice stuff! Appreciate the strings.HasPrefix improvement as well on the websocket handling topic info!
thrasher-
approved these changes
Aug 9, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Kucoin
Subscription Templates
BatchSizesupport$.AssetPairsonly receives relevant assets for the subscriptionCommon
BatchandSortto commonpairs.Removecommon.SortStringspairs.Addandpairs.RemoveStacked Dependencies
Type of change
How has this been tested