-
Notifications
You must be signed in to change notification settings - Fork 987
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize log filtering in blockstream #5015
Conversation
1b9d5ef
to
b1fa9ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! We should put this through an integration cluster run.
chain/ethereum/src/trigger.rs
Outdated
} | ||
} | ||
|
||
impl PartialEq for LogRef { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this impl being relied on exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
graph-node/chain/ethereum/src/trigger.rs
Line 303 in b1fa9ce
impl PartialEq for EthereumTrigger { |
Its relied in impl PartialEq for EthereumTrigger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find it weird that this compares the receipt
field given that it is optional, seems wrong really. Lets not have this impl, and inline the logic into EthereumTrigger
as it was before. That code is critical for correct ordering of the triggers so I find that less indirection makes it easier to read. Also lets stop comparing the receipt
, because it's optional, and instead compare only on transaction_hash
and log_index
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leoyvens clarifying i understand this right, should we remove the previously existing comparison of the optional receipt right? Or should we just inline the logic and keep the original comparison of the optional receipt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should remove the previously existing comparison.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
I'll run it through the integration cluster
1bd6d11
to
11a4fea
Compare
…r EthereumTrigger
11a4fea
to
0bb45a0
Compare
ee6a439
to
894e61d
Compare
chain/ethereum/src/data_source.rs
Outdated
@@ -399,7 +399,11 @@ impl DataSource { | |||
|
|||
fn handlers_for_log(&self, log: &Log) -> Result<Vec<MappingEventHandler>, Error> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this no longer return Result
?
No description provided.