feat(starknet): firehose trigger filter #5322
Conversation
chain/starknet/src/adapter.rs
Outdated
| Chain, | ||
| }; | ||
|
|
||
| type TopicWithRanges = HashMap<Felt, Vec<BlockRange>>; |
There was a problem hiding this comment.
This type is quite opaque in how it's supposed to work, what does Felt represent here? Elsewhere it seems to be an address, here seems to be a "topic".
Can you please make sure the Felt type comment covers what is represents?
How many topics do we expect to represent in these? HashMap is probably overkill if it's only handling a handful of values.
There was a problem hiding this comment.
Oh sorry for the confusion. Felt is the universal primitive type in Starknet. Everything is a Felt: from addresses to contract call params to event logs, literally everything. I will make sure to cover this fact in the Felt type comment itself.
As for "topic", probably bad naming but what it means is the first element in any event keys, similar to Ethereum event signatures (which are the first element of any log "topics"). I should probably name this signature instead?
There was a problem hiding this comment.
maybe something like EventPrefix or EventFirstKey something along those lines, signature could make sense too if that's how it's treated, I think adding rust comments to the types will make that clearer regardless of the name :)
ps: I'm terrible at naming things eheh
chain/starknet/src/adapter.rs
Outdated
|
|
||
| #[derive(Default, Clone)] | ||
| pub struct StarknetEventFilter { | ||
| pub contract_addresses: HashMap<Felt, TopicWithRanges>, |
There was a problem hiding this comment.
Could you please make this field private and keep the matching logic with the filter? eg: matches_addresses or matches_event in order to avoid leaking the implementation into a few like trigger_in_block
There was a problem hiding this comment.
Also somewhat related would be great to have test coverage here
| } | ||
|
|
||
| fn to_firehose_filter(self) -> Vec<prost_types::Any> { | ||
| todo!() | ||
| // An empty event filter list means that the subgraph is not interested in events at all. |
There was a problem hiding this comment.
can you please add some unit tests here
chain/starknet/src/chain.rs
Outdated
| block: shared_block.clone(), | ||
| transaction: transaction.clone(), | ||
| }) | ||
| .filter_map(|event| { |
There was a problem hiding this comment.
As mentioned in a different comment, would be great if you could move the matching logic into the filter itself leave this implementation focused on extract the triggers from the block rather than dealing with internals from the filter
There was a problem hiding this comment.
Would also be great if we can have some tests, near has some examples you can follow.
|
Pushed changes:
Notably, since the trigger filter type is now completely encapsulated, it's now rather difficult to construct a |
Supersedes #5201.
This PR adds 2 trigger filters to Starknet:
When used without turning on the
filtersfeature on the Firehose provider, these filters only work on thegraph-nodeside - meaning that full blocks are still always streamed tograph-node. Filters are applied only to avoid excessive trigger parsing. This part was what #5201 enabled.What's new in this new PR is the additional implementation of Firhose triggers (i.e.
to_firehose_filter), allowinggraph-nodeto send atransformsfield in its Firehose stream subscription requests when thefiltersfeature is enabled for the provider. Note that all blocks are still always streamed - the filters only affects the list of transactions that are included (which dominates the size of blocks streamed as block headers are thin).This new feature should (haven't benchmarked though) significantly improve Starknet subgraph indexing performance, as the actual data sent over the wire should now be much smaller for most real-world subgraphs. (Notably, if a subgraph tracks ETH transfers for example, it won't see much difference as ETH is transferred to sequencer in most transactions)
firehose-starknethas already implemented support for filters. This PR has been tested against that implementation and it works as expected - blocks without matching events have empty transactions, and those with matching ones only come with relevant transactions.