Skip to content

op-service: RPC server wraps http-util server now, and factor out RPC handler#14491

Merged
protolambda merged 2 commits intodevelopfrom
rpc-server-cleanup
Feb 21, 2025
Merged

op-service: RPC server wraps http-util server now, and factor out RPC handler#14491
protolambda merged 2 commits intodevelopfrom
rpc-server-cleanup

Conversation

@protolambda
Copy link
Copy Markdown
Contributor

@protolambda protolambda commented Feb 21, 2025

Description

This PR:

  • Improves httputil.Server
  • Wraps rpc.Server around httputil.Server
  • Factors out the RPC-serving from rpc.Serveras a http.Handler
  • Adds support for having multiple RPC routes with the same general RPC configuration, like an http mux of RPC handlers.

The rpc.NewServer public API is still mostly the same. Options are passed down to the RPC handler. A new from-config constructor allows for further customization.

The WithAPIs option is no longer there: RPC APIs should be registered to the server with the method, not as option. The server can be started after adding the RPCs to ensure they are available. Or the RPCs can be added later, since they are just new additional routes, not conflicting with the HTTP server lifetime.

Tests

  • Added tests for new RPC handler functionality
  • Extended RPC server tests to cover adding an extra route
  • Added tests for RPC server Auth

Additional context

This make it easy to create a tool that can host a single server endpoint that provides multiple RPCs, each matching a test-actor, chain, etc.

Metadata

@protolambda protolambda added the A-op-service Area: op-service label Feb 21, 2025
@protolambda protolambda requested review from a team as code owners February 21, 2025 21:44
@protolambda protolambda requested a review from ajsutton February 21, 2025 21:44
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 21, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.41%. Comparing base (df743f6) to head (b67312c).
Report is 6 commits behind head on develop.

Additional details and impacted files 
@@             Coverage Diff             @@
##           develop   #14491      +/-   ##
===========================================
+ Coverage    78.11%   85.41%   +7.29%     
===========================================
  Files          178      121      -57     
  Lines        10667     5985    -4682     
===========================================
- Hits          8333     5112    -3221     
+ Misses        2143      861    -1282     
+ Partials       191       12     -179
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests 94.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 57 files with indirect coverage changes

axelKingsley
axelKingsley approved these changes Feb 21, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@axelKingsley axelKingsley left a comment

Choose a reason for hiding this comment

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

This PR is a little difficult to confidently validate, since there are a lot of pieces that are being organized, and there is not one specific Feature-Goal being accomplished here. The additional comments throughout are appreciated!

The tests all pass, and I see you've added new ones, so I am good to approve.

This make it easy to create a tool that can host a single server endpoint that provides multiple RPCs, each matching a test-actor, chain, etc.

I don't think I see a test matching this particular use-case? Can you give an example of what the endpoints would look like in this scenario?

@protolambda
Copy link
Copy Markdown
Contributor Author

protolambda commented Feb 21, 2025

I don't think I see a test matching this particular use-case? Can you give an example of what the endpoints would look like in this scenario?

In the server tests, there's:

	require.NoError(t, server.AddRPC("/extra"))
	require.NoError(t, server.AddAPIToRPC("/extra", rpc.API{
		Namespace: "test2",
		Service:   new(testAPI),
	}))

and similar in the auth-test.
Here, the http://localhost:$PORT/extra endpoint can be used to make an test2_frobnicate RPC call, an RPC namespace not served on the regular root http://localhost:$PORT/ endpoint.
So e.g. we could do http://localhost:$PORT/sequencers/chain-a in tooling. E.g. to make RPCs chain-specific, or purpose-specific, without creating a lot of HTTP servers, and while having nice HTTP routes to identify the RPC endpoint.

@protolambda protolambda added this pull request to the merge queue Feb 21, 2025
Merged via the queue into develop with commit 4ebfca5 Feb 21, 2025
46 checks passed
@protolambda protolambda deleted the rpc-server-cleanup branch February 21, 2025 22:40
Rjected pushed a commit to paradigmxyz/optimism that referenced this pull request Feb 25, 2025
@protolambda @Rjected
op-service: RPC server wraps http-util server now, and factor out RPC…
b9ab2a0 
… handler (ethereum-optimism#14491)

* op-service: RPC server wraps http-util server now, and factor out RPC handler

* op-service: fix sub-test name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@axelKingsley axelKingsley axelKingsley approved these changes

@ajsutton ajsutton Awaiting requested review from ajsutton ajsutton was automatically assigned from ethereum-optimism/consensus

Assignees

No one assigned

Labels

A-op-service Area: op-service

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@protolambda @axelKingsley