Luke/dpe 3219 candle api trades integration #187
Conversation
# Conflicts: # protocol
…scription succeeded
…logic. Fixed bug when cleaning up subscriptions. Added more unit tests.
| } | ||
|
|
||
| constructor( | ||
| marketSymbol: MarketSymbol, |
There was a problem hiding this comment.
in what situation will the initialSubscriber have a different marketSymbol than this argument passed in? if there isn't, will be good to get the marketSymbol from the initialSubscriber instead
|
|
||
| this.closeConnection(1000, 'Connection reset'); | ||
|
|
||
| this.subscribe(); |
There was a problem hiding this comment.
should this be awaited? otherwise the resetConnectionMutex doesn't have any value I feel
| trades: JsonTrade[]; | ||
| }; | ||
|
|
||
| export class DataApiWsClient { |
There was a problem hiding this comment.
can this client make use of the MultiplexWebSocket class?
| @@ -30,6 +30,7 @@ | |||
| "watch": "yarn clean && tsc --watch", | |||
| "clean": "rm -rf lib", | |||
| "test": "npx mocha -r ts-node/register tests/**/*.ts", | |||
| "test:": "npx mocha -r ts-node/register", | |||
| } | ||
| } | ||
|
|
||
| this.subscribeToApi(); |
There was a problem hiding this comment.
i feel sus about this implementation...
we are subscribing to the API on object creation, but we don't have any information about the subscription from the consumer's POV.
e.g. we do const marketDataApiSubscription = new ApiSubscription(...)
but this code don't know anything about the underlying subscription, whereas we'd usually have something like const subscription = await marketDataApiSubscription.subscribe()
| return getCompatibleTradeSubscriptionLookupKey(this.config); | ||
| } | ||
|
|
||
| get candleSubscriptionsLookupKey() { |
There was a problem hiding this comment.
good to document these as "Any candle subscription key that matches this key can use this ApiSubscription"
same for trade
| newSubscription.tradeSubscriptionsLookupKey; | ||
|
|
||
| const existingTradeSubscription = | ||
| this.compatibleTradeSubscriptionLookup.get(tradeSubscriptionLookupKey); |
There was a problem hiding this comment.
not obvious to me yet, but why is the first ApiSubscription (because compatibleTradeSubscriptionLookup.get returns the first in the list) expected to be the existing trade subscription? should we not check for all in the list?
| constructor( | ||
| marketSymbol: MarketSymbol, | ||
| resolution: CandleResolution, | ||
| env: UIEnv, |
There was a problem hiding this comment.
same for this - can get from initialSubscriber
| - * We could more optimally close Subscription A and make Subscriber C use Subscription B | ||
| - * We're not doing this for the same of simplicity. In the future we could add this optimisation if it becomes necessary. | ||
| */ | ||
|
|
There was a problem hiding this comment.
while this overall documentation helps a lot, I'd really wished that individual classes and methods are documented too
| } | ||
| } | ||
|
|
||
| get(subscriptionLookupKey: TradeSubscriptionLookupKey): ApiSubscription { |
There was a problem hiding this comment.
return type should be ApiSubscription | null
See associated UI changes in this PR :: https://github.com/drift-labs/protocol-v2-mono/pull/3593