-
Notifications
You must be signed in to change notification settings - Fork 7
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
CCIP-3713 Filtering by the 3rd word in USDC Reader #221
Conversation
pkg/reader/usdc_reader.go
Outdated
for _, id := range eventIDs { | ||
eventFilter = append( | ||
eventFilter, | ||
query.Comparator(consts.EventFilterCCIPMessageSent, |
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.
Name for value filter primitive shouldn't be mixed up with the key name. I would prefer if the comparator was called something like MessageValue. You are essentially filtering for values contained within the EventFilterCCIPMessageSent
entity
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 don't follow. Is it about the name of the const (consts.EventFilterCCIPMessageSent
) or its value? What do you suggest instead?
Name for value filter primitive shouldn't be mixed up with the key name.
I think it's different, isn't it?
Query key is consts.EventNameCCTPMessageSent
, but comparator is consts.EventFilterCCIPMessageSent
, am I missing sth?
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.
Sry I misread that multiple times while looking at the code. Still, wouldn't constants like this: consts.CCIPMessageSentValue
and for the event itself consts.CCIPMessageSent
make more sense?
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.
Trying to follow existing patters in naming, so I've renamed to
consts.CCTPMessageSentValue // for the comparator
consts.EventNameCCTPMessageSent // kept existing for the event
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.
sgtm
|
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.
neat
No description provided.