Skip to content
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

[EVM] subscribe logs empty params crash #1779

Merged
merged 3 commits into from
Jul 26, 2024

Conversation

jewei1997
Copy link
Contributor

Describe your changes and provide context

Our websocket RPC would crash when given this empty filter where we don't filter on anything so it should return all logs: {"jsonrpc":"2.0","id": 2, "method": "eth_subscribe", "params": ["logs"]}. This PR creates an empty filter if the filter given as input from the rpc is nil.

Testing performed to validate your change

unit tests, manual testing locally.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.51%. Comparing base (aefdcf8) to head (58ed215).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1779      +/-   ##
==========================================
+ Coverage   61.47%   61.51%   +0.04%     
==========================================
  Files         257      257              
  Lines       22284    22287       +3     
==========================================
+ Hits        13698    13710      +12     
+ Misses       7629     7619      -10     
- Partials      957      958       +1     
Files Coverage Δ
evmrpc/subscribe.go 66.83% <100.00%> (+2.10%) ⬆️

... and 2 files with indirect coverage changes

@jewei1997 jewei1997 enabled auto-merge (squash) July 26, 2024 12:50
@jewei1997 jewei1997 merged commit 645b3a9 into main Jul 26, 2024
46 checks passed
@jewei1997 jewei1997 deleted the evm-subscribe-logs-empty-params-crash branch July 26, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants