Add multiple event filter to subscribe #175
Conversation
| impl<M, N: Network> EventScanner<M, N> { | ||
| #[must_use] | ||
| pub fn subscribe(&mut self, filter: EventFilter) -> ReceiverStream<Message> { | ||
| pub fn subscribe(&mut self, filters: impl Into<Vec<EventFilter>>) -> ReceiverStream<Message> { |
There was a problem hiding this comment.
impl IntoVec is quick and dirty. I think we could make it more comprehensive to accept more 'array' types (iters, slices etc...) but I thought this is good enough lmk
There was a problem hiding this comment.
We should use IntoIterator here
| let filters: Vec<EventFilter> = filters.into(); | ||
|
|
||
| for filter in filters { | ||
| self.listeners.push(EventListener { filter, sender: sender.clone() }); | ||
| } |
There was a problem hiding this comment.
With the current setup, every EventListener will run in its own tokio task, but still send to the appropriate stream, which is good.
The potential problem is that event listeners will process block ranges independently and will likely stream their respective logs out of order.
Example showing what I'd consider intuitive:
- event listeners:
AandB - block ranges being processed:
1-10, 11-20, 21-30 - expected: streaming events
AandBin chronological order first in block range1-10, then in11-20, then in21-30... - actual: streaming first all events
Ain ranges1-10, 11-20, 21-30, then all eventsBin the same ranges
Does this make sense?
There was a problem hiding this comment.
Yes I realised however I thought this was a necessary by product, so you can process them in parallel. Otherwise you would need to merge the event filters into one event listener (but miss out on some concurrency "gains"?)
Maybe I'm missing something with my logic here.
I think if the user wanted them in order they could do that on their end by sorting by timestamp
There was a problem hiding this comment.
I think if the user wanted them in order they could do that on their end by sorting by timestamp
They wouldn't need a single stream for different event types then, they could just start multiple streams.
The point of this feature is to do the "sorting by timestamps" on the scanner's end. Will update the issue description to make that clearer.
There was a problem hiding this comment.
They wouldn't need a single stream for different event types then, they could just start multiple streams.
But then they need to run multiple streams so starting multiple connections to same provider, iterating of the same blocks etc.. here the same block numbers are handled in parallel.
But yes im just playing devils advocate i see your point closing the PR for later - just thought it was worth mentioning
Resolves #169
Our scanner internally naturally allows for multiple event listeners so it was more updating the api to allow for this.
Question: What should our scanner do if we ask for count of two events? Return first one with that count? (I forgot).