celestia-node
celestia-node copied to clipboard
feat(`api/rpc`): OpenRPC scaffolding, migrating OpenAPI gateway to `api/gateway`
Based on #1056
- Moves the gateway to
api/gateway
. This is no longer bundled with the node as of right now. - Removes
/service
directory - Creates new JSON-RPC server in
api/rpc
- Moves rpc Config definition to
nodebuilder/rpc
(no more cyclical dependency problem)
I would suggest reintroducing the gateway, with an additional config and a default "off" mode, should be done in a different PR.
I disagree here. The RPC is a service that is running for the lifecycle of the node, and accesses the other modules - just like the other services. I do not understand the comparison with commands, config, or flags.
Generally, by letting each module be in charge of registering itself on the RPC server, it will no longer be possible to see at a glance which APIs the RPC is exposing. More importantly, this clearly leads to higher coupling (of the individual modules to the RPC) and lower cohesion (inside the individual nodebuilder modules, since they now have to register on an RPC). I can explain this and the consequences more in depth if you disagree.
RPC is not a module and should not have
nodebuilder
. It's like commands, config, or flags of each module. Each module has an API and each module can be accessed through RPC.If the RPC server is provided to DI, each module should register its handler there, not some high-level module or API importing it and registering handlers manually. This is something that should be done in this or other RPC related PR and not deferred to some later refactorings.
Gateway though could be a module.
@Wondertan I think rpc warrants its own fx.Module -- it is not one of our "Modules" offered by node per-se but I would consider it a module in our software.
I am, however, okay with each module registering its own endpoint on RPC, but I don't really see the added benefit rn.
Codecov Report
Merging #1175 (5dec0ba) into main (f4e582e) will decrease coverage by
0.72%
. The diff coverage is65.51%
.
@@ Coverage Diff @@
## main #1175 +/- ##
==========================================
- Coverage 56.34% 55.62% -0.73%
==========================================
Files 160 162 +2
Lines 9935 9981 +46
==========================================
- Hits 5598 5552 -46
- Misses 3783 3876 +93
+ Partials 554 553 -1
Impacted Files | Coverage Δ | |
---|---|---|
api/gateway/availability.go | 0.00% <ø> (ø) |
|
api/gateway/config.go | 0.00% <0.00%> (ø) |
|
api/gateway/das.go | 0.00% <ø> (ø) |
|
api/gateway/endpoints.go | 0.00% <ø> (ø) |
|
api/gateway/handler.go | 0.00% <ø> (ø) |
|
api/gateway/header.go | 0.00% <ø> (ø) |
|
api/gateway/middleware.go | 29.41% <ø> (ø) |
|
api/gateway/server.go | 68.88% <ø> (ø) |
|
api/gateway/share.go | 0.00% <ø> (ø) |
|
api/gateway/state.go | 0.00% <ø> (ø) |
|
... and 17 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
I think each module should expose / return ServeHTTP
or other handler
- type interface so they can be registered manually in nodebuilder or other higher level caller. It will only expose required handler logic without need to pass transport or middleware level of abstractions inside the components that they don't have any use of. So we can construct server as an independent module and assign all required transport settings and middleware according to our vision of node, not of the pkg itself.