Refactor Pyth Lazer to run one bulk subscription per channel

[hacheigriega]

Motivation

Alternative approach to #165 in order to reduce number of incoming messages (frames). Cleaner approach thanks to @Thomasvdam

Explanation of Changes

We now keep at most one subscription per channel.

• The module sends out subscription requests whenever the desired set (tracked by the subscriptions map) "grows" or gains any new IDs.
• The module tracks the current active subscription per channel with a simple logic: It only updates the current active subscription when it receives an ack for a subscription whose ID is greater than the currently active subscription's.
• Idle clean up only removes the ID from the desired set. For it to be actually removed from the subscription, we wait for a future request that grows the desired set.
• Invalid IDs (not u32) cause a WebSocket Pool error. To prevent invalid symbols from getting stuck in the desired set and causing failures for all future subscriptions, we fail requests containing non-u32 IDs.
• I noticed that the subscribe ack message returns a list of unknown feed IDs. Currently we don't do anything with it, but we could consider keeping a list of unknown IDs and returning 400 to requests containing one of those.

Testing

Added unit tests and tested locally

Related PRs and Issues

Alternative to #165

[Thomasvdam]

I noticed that the subscribe ack message returns a list of invalid feed IDs. Currently we don't do anything with it, but we could consider keeping a list of invalid IDs and returning 400 to requests containing one of those.

Great find! Sounds like a nice improvement over the current behaviour that simply times out after 3 seconds :)

[Thomasvdam]

Obviously I think this is cleaner, but I hope it's more than just personal preference 😂

Just a few small nitpicks, the flow looks great and the docs nicely outline how the design works and the tradeoffs/limitations in the current implementation.

Maybe in a future cleanup it's worth splitting some of the logic into Effect services to make testing a little easier, but lets not get into that now. :)

[hacheigriega]

Great find! Sounds like a nice improvement over the current behaviour that simply times out after 3 seconds :)

I suppose we could add an LRU cache of certain capacity, but now I am no longer sure about returning 400 to requests containing IDs that have been returned as unknown from Pyth. I thought Pyth recommends ignoreInvalidFeedIds precisely because feed IDs can become valid or invalid at any time. And of course, simply removing unknown IDs from the desired set doesn't help because next requests would just add them back.

Maybe in a future cleanup it's worth splitting some of the logic into Effect services to make testing a little easier

Ah good point, well noted!!