Deduplicate triggers #4055
Deduplicate triggers #4055
Conversation
| @@ -173,9 +173,22 @@ where | |||
| } | |||
|
|
|||
| impl<C: Blockchain> BlockWithTriggers<C> { | |||
| pub fn new(block: C::Block, mut trigger_data: Vec<C::TriggerData>) -> Self { | |||
| pub fn new(block: C::Block, mut trigger_data: Vec<C::TriggerData>, logger: &Logger) -> Self { | |||
There was a problem hiding this comment.
This to me sounds like a lot of logic for a new function. I'd rather change this to something from_trigger_data or something with a comment saying it will dedup etc.
There was a problem hiding this comment.
I just added a doc comment, what do you think?
There was a problem hiding this comment.
I don't feel strongly about it but I feel like reading the code you would not expect new to do the sort + dedup etc. The doc helps though I'll leave the rest up to you.
evaporei
force-pushed
the
eva/dedup-triggers
branch
2 times, most recently
from
64d615a
to
c8f629a
Compare
evaporei
force-pushed
the
eva/dedup-triggers
branch
from
3a580cc
to
50f7f63
Compare
|
|
||
| - This release includes a **determinism fix** that should affect very few subgraphs on the network (currently only two). There was an issue that if a subgraph manifest had one data source with no contract address, listening to the same events or calls of another data source that has a specified address, the handlers for those would be called twice. With the fix, this will happen no more, the handler will be called just once like it should. | ||
| - The two affected deployments are: `Qmccst5mbV5a6vT6VvJMLPKMAA1VRgT6NGbxkLL8eDRsE7` and `Qmd9nZKCH8UZU1pBzk7G8ECJr3jX3a2vAf3vowuTwFvrQg`; | ||
| - Here's an example [manifest](https://ipfs.io/ipfs/Qmd9nZKCH8UZU1pBzk7G8ECJr3jX3a2vAf3vowuTwFvrQg), taking a look at the data sources of name `ERC721` and `CryptoKitties`, both listen to the `Transfer(...)` event. Considering a block where there's only one occurence of this event, `graph-node` would duplicate it and call `handleTransfer` twice. Now this is fixed and it will be called only once per event/call that happened on chain. |
There was a problem hiding this comment.
Since this is mentioning the network impact it's important to note any impact to indexers and any actions they should take.
There was a problem hiding this comment.
Awesome, thanks for the feedback I'll address it 🙂
There was a problem hiding this comment.
I just pushed what they should do, what do you think? @leoyvens
evaporei
force-pushed
the
eva/dedup-triggers
branch
from
50f7f63
to
b22ea0b
Compare
If multiple data sources fetch the same event/call/etc, they could exist twice inside the trigger_data. This commit removes this duplication by running a .dedup() after the sort.
evaporei
force-pushed
the
eva/dedup-triggers
branch
from
b22ea0b
to
187ef86
Compare
No description provided.