feat: enable same HTTP methods on websocket#5217
feat: enable same HTTP methods on websocket#5217bootcodes wants to merge 36 commits intohiero-ledger:mainfrom
Conversation
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| BestPractice | 19 medium |
| CodeStyle | 2 minor |
| Complexity | 51 medium |
🟢 Metrics 96 complexity · -2 duplication
Metric Results Complexity 96 Duplication -2
TIP This summary will be updated as you push new changes. Give us feedback
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
BartoszSolkaBD
left a comment
There was a problem hiding this comment.
Looks nice 😄
I like the idea of creating RpcProtocolClient for unified test cases.
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Ferparishuertas
left a comment
There was a problem hiding this comment.
Good work! Some nits
- you mention that there could be some duplicated acceptance tests. Shall we open a follow up issue?
- I am not seeing rate limiting test for WebSocket methods. As we have added all, Can you implement some tests?
@Ferparishuertas - I opened an issue for unified acceptance tests here |
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
quiet-node
left a comment
There was a problem hiding this comment.
Nice work overall. Left some items and concerns. Also looks like codacy is complaining about many files https://github.com/hiero-ledger/hiero-json-rpc-relay/pull/5217/checks?check_run_id=70629267661.
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
quiet-node
left a comment
There was a problem hiding this comment.
Looks great! Only two items left
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Signed-off-by: Thomas Boot <thomas.boot@swirldslabs.com>
Codecov Report❌ Patch coverage is
❌ Your project check has failed because the head coverage (70.53%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.
@@ Coverage Diff @@
## main #5217 +/- ##
===========================================
- Coverage 95.93% 70.53% -25.40%
===========================================
Files 146 147 +1
Lines 25140 25215 +75
Branches 2044 724 -1320
===========================================
- Hits 24117 17786 -6331
- Misses 1001 7406 +6405
- Partials 22 23 +1
... and 86 files with indirect coverage changes 🚀 New features to boost your workflow:
|
quiet-node
left a comment
There was a problem hiding this comment.
Looking great but looks like Codacy and codecov are complaining quite hard. can you please take a look?
Description
The WebSocket server previously gated method dispatch in
WS_CONSTANTS.METHODS. Any method not in that list (i.e. including eth_feeHistory), returned-32601 Method not foundeven though the relay implemented them.Related issue(s)
Fixes #4991
Testing Guide
Changes from original design (optional)
The bug
In
mainrun :npm run start:wsRun this script :
Fix the bug
webSocketServer.tswas not setting app.proxy = true on its Koa instance.Two features silently broke as a result:
The HTTP server (server.ts) already had app.proxy = true and the RFC 7239 Forwarded→X-Forwarded-For translation middleware.
The fix
Extracted the proxy setup into
src/server/utils/proxyUtils.tsand called it from both servers. The extracted utility:Verification
tests/ws-server/unit/proxyIpIsolation.spec.tscontains two tests that directly prove the behavior:Additional work needed (optional)
Migrate duplicate
eth_*acceptance tests totests/protocol/. Methods tested independently in bothtests/server/acceptance/andtests/ws-server/acceptance/with identical assertions are candidates:eth_blockNumber, eth_gasPrice, eth_getLogs, eth_getBalance, eth_call, eth_estimateGas, etc.When migrating, delete both originals and replace with a single parameterized test in
tests/protocol/Related issue : #5223
Checklist