celestia-node icon indicating copy to clipboard operation
celestia-node copied to clipboard

feat(`api/rpc`): OpenRPC scaffolding, migrating OpenAPI gateway to `api/gateway`

Open distractedm1nd opened this issue 2 years ago • 2 comments

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.

distractedm1nd avatar Sep 29 '22 11:09 distractedm1nd

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.

distractedm1nd avatar Oct 12 '22 06:10 distractedm1nd

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.

renaynay avatar Oct 12 '22 11:10 renaynay

Codecov Report

Merging #1175 (5dec0ba) into main (f4e582e) will decrease coverage by 0.72%. The diff coverage is 65.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.

codecov-commenter avatar Oct 25 '22 06:10 codecov-commenter

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.

walldiss avatar Oct 25 '22 12:10 walldiss