Staked Builder API REST Client - #11026
Conversation
0baf796 to
71a3287
Compare
5c4ae8d to
68b3483
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 68b3483. Configure here.
75a1255 to
f07ae53
Compare
921918d to
1f5f5d0
Compare
6ed7989 to
f48c9d8
Compare
|
|
would be good to add the references like OpenApiIntegrationTest does so that we can easily see the api definitions... |
f48c9d8 to
01d69a4
Compare
|
@tbenr fixed the 5 points, will tackle testing one separately |
They will show eventually anyways when we pipe them through the Beacon API. |
|
just one minor thing: Eth-Consensus-Version presence is not tested in all methods |
bdcdaec to
fa389c9
Compare
Done |
b5ffe69 to
246a35a
Compare
|
claude review comments:
builder/rest/src/main/java/tech/pegasys/teku/builder/rest/handlers/GetExecutionPayloadBidRequest.java The response handler only parses JSON, but no Accept header is sent. A builder that supports multiple content types might respond with SSZ, causing a JsonProcessingException (which becomes UncheckedIOException thrown as a failed future). Other
builder/rest/src/main/java/tech/pegasys/teku/builder/rest/BuilderApiMethod.java:32 If a caller omits a required URL param, the template literal {param} is sent as-is in the URL, resulting in a 404 or a malformed request with no hint of what went wrong. A guard like: if (result.contains("{")) { after the loop would catch misuse early.
builder/rest/src/main/java/tech/pegasys/teku/builder/rest/OkHttpStakedBuilderClient.java:57,67,76 new GetExecutionPayloadBidRequest(spec, baseEndpoint, httpClient) (and the two others) are constructed on every call. These are stateless given spec and baseEndpoint — they could be final fields set in the constructor. No functional bug, but
builder/rest/src/integration-test/.../SubmitSignedBeaconBlockRequestTest.java:84 The test derives the expected header value using spec.atSlot(...).getMilestone().name().toLowerCase(Locale.ROOT), while the implementation uses lowerCaseName(). They produce identical output today, but the test should mirror the implementation
builder/rest/src/main/java/tech/pegasys/teku/builder/rest/ResponseHandler.java:58 withHandler is chained from the constructor to register defaults, then exposed publicly. A caller could replace e.g. the SC_OK handler post-construction, introducing subtle bugs. Making it package-private (or protected) would limit the risk since |
246a35a to
47f4e14
Compare
|
…nt-Type - SendSignedBeaconBlockRequest now takes Spec and sends Eth-Consensus-Version header derived from the block's slot milestone; postOctetStream gains a headers overload to support this - postEmpty no longer sets Content-Type: application/json on a zero-length body; uses null MediaType so no Content-Type is sent (empty body is not valid JSON and would cause builders to return 400)
bcd802f to
f85f884
Compare
1ef46b9
into
Consensys-Incorporated:master

PR Description
Client for the builder rest api as per ethereum/builder-specs#138 . There is a repetition but I found it difficult to create a generic rest client implementation that both the VC client and builder client can use.
Fixed Issue(s)
related to #10822
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Medium Risk
New client for execution-payload bids, builder preferences, and signed beacon block submission; incorrect HTTP/SSZ/auth handling could affect block production once wired in, but this PR is an isolated library with integration tests and no main-node wiring in the diff.
Overview
Adds a new
builder:restGradle module (plus abuilderparent with no jar) and registers both insettings.gradle.Introduces an OkHttp-based Staked Builder API client aligned with builder-specs#138:
StakedBuilderClientwithgetExecutionPayloadBid,submitBuilderPreferences, andsubmitSignedBeaconBlock, exposed viaStakedBuilderClientProvider(one cached client per full URL, sharedOkHttpClientwithBUILDER_CALL_TIMEOUT).Request plumbing includes
BuilderApiMethodpaths,AbstractBuilderRequest(JSON, empty POST, or SSZ octet-stream),ResponseHandlerfor 200/202/204 and error statuses →BuilderClientException, and Gloas-specific headers (e.g.Eth-Consensus-Version, timing headers on payload-bid requests). MockWebServer integration tests cover the three handlers and provider caching behavior.Reviewed by Cursor Bugbot for commit f85f884. Bugbot is set up for automated code reviews on this repo. Configure here.