chore: refactor builder workflows #61
Conversation
ckartik
left a comment
ckartik
left a comment
There was a problem hiding this comment.
First Iteration of the Review! I really like the idea of breaking out the Searcher logic into a searcher client!
| Name: "rollupkey", | ||
| Usage: "Private key to interact with rollup", | ||
| Value: "", | ||
| EnvVars: []string{"ROLLUP_KEY"}, |
There was a problem hiding this comment.
This should probably be renamed to client-private-key. The name rollup key was a legacy decision. I don't think it's relevant, especially considering that key is going to be used to sign pre-confirmation commitments. We should also update the usage tag with this information. Maybe: `The Signing key used to authenticate the node to the rollup and construct pre-confirmation.
| } | ||
|
|
||
| // BindJSON helper is used to bind the request body to the given type. | ||
| func BindJSON[T any](w http.ResponseWriter, r *http.Request) (T, error) { |
There was a problem hiding this comment.
Nice! I forget Golang introduced Generics!
| server.ChainHandlers( | ||
| "/health", | ||
| apiserver.MethodHandler(http.MethodGet, a.handleHealthCheck), | ||
| a.authSearcher, |
There was a problem hiding this comment.
I think the health check should authenticateBuilder, it was primarily meant for the builder to be able to view connected searcher list to the builder-boost instance.
| server.ChainHandlers( | ||
| "/builder", | ||
| apiserver.MethodHandler(http.MethodGet, a.handleBuilderID), | ||
| a.authenticateBuilder, |
There was a problem hiding this comment.
This should be an open endpoint (e.g no authentication/authorization).
The flow for a searcher involves hitting this endpoint to receive a BuilderID which it uses to construct a flashbots style token: :<Signature(Builder Address)>. So it can subsequently pass the searcherAuth middleware, by passing the token as a X-Primev-Signature header.
Here's a reference to the flashbots documentation: https://docs.flashbots.net/flashbots-auction/searchers/advanced/rpc-endpoint#authentication
| ) | ||
|
|
||
| server.ChainHandlers( | ||
| "/primev/v1/builder/blocks", |
There was a problem hiding this comment.
This should be guarded to an auth'd builder. We don't want to allow anyone to post fake blocks to builder-boost.
|
|
||
| server.ChainHandlers( | ||
| "/ws", | ||
| apiserver.MethodHandler(http.MethodGet, a.connectSearcher), |
There was a problem hiding this comment.
I'm thinking we can pass the authentication step to middleware, and handle authorization inside of this business logic handler. There, we can check if the searcher has sufficent balance in the rollup to connect, but the middleware loads the authenticated ID of the searcher trying to connect. that being said, it's not neccesary, just a nice to have refactor.
| ) | ||
| a.sclient.AddSearcher(searcher) | ||
|
|
||
| logger.Info("searcher attempting connection", |
There was a problem hiding this comment.
Should this be changed from "attempting" to "connected"?
| "bid_tnx", bid.TxnHash, | ||
| "bid_amt", bid.GetBidAmt(), | ||
| ) | ||
|
|
There was a problem hiding this comment.
| /* | |
| Provider can add there on logic here to decide on constructing the bid or returning from this function early. | |
| */ |
|
|
||
| s.logger.Info("block metadata processed", "blockMetadata", blockMetadata) | ||
|
|
||
| if s.inclusionProofActive { |
There was a problem hiding this comment.
Longer term, I was thinking we decouple the inclusionProof section from the hints payload (the section above this conditional block).
Basically defer this processing, since it's pretty heavy duty. compared to the above data processing which only needs to happen once, and can be copied to N searchers directly.
| blockMetadata.SearcherTxns = make(map[string][]string) | ||
| } | ||
|
|
||
| for idx, btxn := range payload.ExecutionPayload.Transactions { |
There was a problem hiding this comment.
We should probably move these computations up a level, to avoid having to repeat them for all searchers.
Restructuring the builder-related packages for better maintainability and testing.
Breaking changes: