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

feat!(api/rpc): OpenRPC scaffolding, migrating OpenAPI gateway to api/gateway #1175

Merged
merged 8 commits into from
Oct 26, 2022

Conversation

distractedm1nd
Copy link
Member

@distractedm1nd distractedm1nd commented Sep 29, 2022

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.

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

needs some godoc and format but otherwise looks good already.

api/rpc/server.go Outdated Show resolved Hide resolved
api/rpc/server.go Outdated Show resolved Hide resolved
nodebuilder/config.go Outdated Show resolved Hide resolved
nodebuilder/node.go Outdated Show resolved Hide resolved
nodebuilder/rpc/module.go Outdated Show resolved Hide resolved
api/rpc/handler.go Outdated Show resolved Hide resolved
api/rpc/server.go Outdated Show resolved Hide resolved
api/rpc/server.go Outdated Show resolved Hide resolved
api/rpc/server.go Outdated Show resolved Hide resolved
Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

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.

@distractedm1nd
Copy link
Member Author

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.

@renaynay
Copy link
Member

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-commenter
Copy link

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.

renaynay
renaynay previously approved these changes Oct 25, 2022
Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

  • // rpc components
    RPCServer *rpc.Server // not optional

@walldiss
Copy link
Member

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.

Copy link
Member

@Wondertan Wondertan left a comment

Choose a reason for hiding this comment

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

After plenty of discussions, we(@distractedm1nd, @renaynay, @vgonkivs, and @Wondertan) agreed on this approach.

Copy link
Member

@renaynay renaynay left a comment

Choose a reason for hiding this comment

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

lfg

@distractedm1nd distractedm1nd merged commit cb92051 into celestiaorg:main Oct 26, 2022
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Oct 26, 2022
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Oct 26, 2022
distractedm1nd added a commit to distractedm1nd/celestia-node that referenced this pull request Oct 28, 2022
@renaynay renaynay changed the title feat(api/rpc): OpenRPC scaffolding, migrating OpenAPI gateway to api/gateway feat!(api/rpc): OpenRPC scaffolding, migrating OpenAPI gateway to api/gateway Oct 31, 2022
@renaynay renaynay added kind:break! Attached to breaking PRs and removed kind:feat Attached to feature PRs labels Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rpc kind:break! Attached to breaking PRs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Scaffolding for new RPC server/client
6 participants