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

fix: added newFilter to paramRearrangementMap for WS server #3117

Merged
merged 2 commits into from
Oct 17, 2024

Conversation

quiet-node
Copy link
Member

Description:
There's a regression in the WS server that breaks the eth_newFilter WS endpoint. Essentially, before the data is submitted to the Relay, there’s a step to rearrange the parameters for certain special endpoints that have a different parameter order than others. This step is handled by the paramRearrangementMap, but it missed the newFilter endpoint, so the parameters weren't ordered correctly, causing the eth_newFilter endpoint to fail.

This PR simply adds the newFilter method to the paramRearrangementMap, fixing the issue.

Related issue(s):

Fixes #3114

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@quiet-node quiet-node linked an issue Oct 17, 2024 that may be closed by this pull request
@quiet-node quiet-node self-assigned this Oct 17, 2024
@quiet-node quiet-node added the bug Something isn't working label Oct 17, 2024
@quiet-node quiet-node added this to the 0.58.0 milestone Oct 17, 2024
Copy link

github-actions bot commented Oct 17, 2024

🚨 Memory Leak Detected 🚨

A potential memory leak has been detected in the test titled validates enforcement of request id. This may impact the application's performance and stability.

Details

📊 Memory Leak Detection Report 📊

GC Type: MarkSweepCompact
Cost: 30,509.8 ms

Heap Statistics (before vs after executing the test):

  • Total Heap Size: increased with 1.46 MB
  • Total Heap Size Executable: no changes
  • Total Physical Size: decreased with 348.16 KB
  • Total Available Size: decreased with 9.33 MB
  • Total Global Handles Size: no changes
  • Used Global Handles Size: decreased with 64.00 bytes
  • Used Heap Size: decreased with 3.45 MB
  • Heap Size Limit: no changes
  • Malloced Memory: no changes
  • External Memory: no changes
  • Peak Malloced Memory: no changes

Heap Space Statistics (before vs after executing the test):

  • Old Space:

    • Space Size: increased with 1.84 MB
    • Space Used Size: increased with 2.07 MB
    • Space Available Size: decreased with 12.90 MB
    • Physical Space Size: increased with 1.84 MB
  • Large Object Space:

    • Space Size: increased with 835.58 KB
    • Space Used Size: increased with 813.50 KB
    • Space Available Size: no changes
    • Physical Space Size: increased with 835.58 KB

Recommendations

Please investigate the memory allocations in this test, focusing on objects that are not being properly deallocated.

Copy link

github-actions bot commented Oct 17, 2024

Tests

       3 files     399 suites   18s ⏱️
1 433 tests 1 432 ✔️ 1 💤 0
1 442 runs  1 441 ✔️ 1 💤 0

Results for commit 2bd1602.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@Nana-EC Nana-EC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but needs tests

packages/ws-server/src/controllers/index.ts Outdated Show resolved Hide resolved
Signed-off-by: Logan Nguyen <[email protected]>
Copy link

sonarcloud bot commented Oct 17, 2024

Copy link

Acceptance Tests

  19 files  290 suites   30m 6s ⏱️
599 tests 591 ✔️ 4 💤 4
879 runs  869 ✔️ 4 💤 6

Results for commit 2bd1602.

Copy link
Collaborator

@ebadiere ebadiere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG.

@ebadiere ebadiere merged commit daf2aef into main Oct 17, 2024
43 checks passed
@ebadiere ebadiere deleted the 3114-ws-regression-eth_newfilter-is-failing branch October 17, 2024 15:06
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.46%. Comparing base (9e73aae) to head (2bd1602).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3117      +/-   ##
==========================================
- Coverage   85.53%   83.46%   -2.08%     
==========================================
  Files          45       66      +21     
  Lines        3387     4402    +1015     
  Branches      673      868     +195     
==========================================
+ Hits         2897     3674     +777     
- Misses        282      459     +177     
- Partials      208      269      +61     
Flag Coverage Δ
relay 85.59% <ø> (+0.05%) ⬆️
server 83.48% <ø> (?)
ws-server 36.15% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/ws-server/src/utils/utils.ts 98.07% <100.00%> (ø)

... and 28 files with indirect coverage changes

ebadiere pushed a commit that referenced this pull request Oct 17, 2024
* fix: added newFilter to paramRearrangementMap for WS server

Signed-off-by: Logan Nguyen <[email protected]>

* test: added unit test

Signed-off-by: Logan Nguyen <[email protected]>

---------

Signed-off-by: Logan Nguyen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[WS Regression] eth_newFilter is failing
3 participants