juno
juno copied to clipboard
jsonrpc: implement unix handler
closes #1277
Codecov Report
Attention: 40 lines in your changes are missing coverage. Please review.
Comparison is base (
e9e9a57) 73.05% compared to head (0f0778e) 73.20%.
:exclamation: Current head 0f0778e differs from pull request most recent head b7ade80. Consider uploading reports for the commit b7ade80 to get more accurate results
| Files | Patch % | Lines |
|---|---|---|
| jsonrpc/ipc.go | 81.81% | 16 Missing and 6 partials :warning: |
| node/http.go | 74.46% | 9 Missing and 3 partials :warning: |
| node/metrics.go | 75.00% | 2 Missing and 1 partial :warning: |
| node/node.go | 57.14% | 2 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #1362 +/- ##
==========================================
+ Coverage 73.05% 73.20% +0.14%
==========================================
Files 98 98
Lines 10101 10209 +108
==========================================
+ Hits 7379 7473 +94
- Misses 2174 2186 +12
- Partials 548 550 +2
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hey, thanks for the contribution!
Do you have an immediate use case for this? Because I am not sure if the marginal speed up that IPC brings, on its own, warrants a new feature to be added over TCP.
I usually mount the socket instead of using loopback since it saves 2(docker+interface itself) hops for containerized env and is generally easier to manage.
I usually mount the socket instead of using loopback since it saves 2(docker+interface itself) hops for containerized env and is generally easier to manage.
Fair enough
I'll try to review this sometime next week. May take a bit longer depending on prioritization.
Made a first pass. Should Juno be able to communicate over multiple unix sockets, each representing a different IPC connection?
Spawning a socket for each connection is overkill I believe, one is enough. Spawning multiple sockets can be considered for keeping multiple jsonrpc.server to align with current http and ws rpc (jsonrpcServerLegacy and jsonrpcServer) behavior. The problem here is that socket creation can fail in either of those and I didn't know what expected behavior would be in that case so I just spawned the latest one.
Spawning a socket for each connection is overkill I believe, one is enough.
Understood, thanks.
The problem here is that socket creation can fail in either of those and I didn't know what expected behavior would be in that case so I just spawned the latest one.
That's ok, creating TCP sockets can also fail. Let's maintain consistency with websocket and http handlers, and spawn a socket for each server.
Sorry for the two lint commits, got confused by that:
Locally make lint returns directive //nolint:funlen is unused for linter "funlen" (nolintlint)
dev@dev:~/contrib/juno$ golangci-lint --version
golangci-lint has version v1.54.2 built with go1.21.0 from (unknown, mod sum: "h1:oR9zxfWYxt7hFqk6+fw6Enr+E7F0SN2nqHhJYyIb0yo=") on (unknown)
Yet the check fails without that:
[lint: cmd/juno/juno.go#L150](https://github.com/NethermindEth/juno/pull/1362/files#annotation_15384594147)
Function 'NewCmd' has too many statements (51 > 50) (funlen)
No worries. It's flaky sometimes. Thanks for fixing.