-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: correctly combine address and contract address list in event filter param construction #3618
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
fix: correctly combine address and contract address list in event filter param construction #3618
Conversation
|
Put a lot of details in the issue #3617 happy to expand on it or add more to this contribution. This just fixed my issue for what I'm doing so thought I'd get the ball rolling |
|
I made a change to make sure the final output works if you have address and/or contract_address as a list or string. Also de-duplicated but just using address if they're the same. Also had to do some gymnastics to get the linter to like the type hints, not sure if that resulted in the best solution. |
|
The linting is still unhappy, would love some direction on how you want the type hinting to work in this section? as there is no type assertion in if blocks it seems overly strict |
ab3e8c4 to
91a53c2
Compare
|
@Aerilym Thanks for the contribution here and apologies for being slow to respond here! I figured out a way to make types cooperate and also make the validation simpler. Have a look and let us know if there's anything further you'd like to add. |
kclowes
left a comment
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.
LGTM, just left one question!
…ter param construction
… contract address in event filter param construct
…ss and/or contract_address params
…es for single item lists
cf48683 to
85fecdf
Compare
Thanks for picking up this PR, we've been using these changes in production had no issues. Can't think of any other related changes needed. |
fselmo
left a comment
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.
@reedsa nice elegant solution with the set check, lgtm 👍🏼
What was wrong?
Related to Issue #3617
Closes #3617
How was it fixed?
See description of #3617
Todo:
Cute Animal Picture