refactor: cleaner routing using gorilla mux regexps #185
Conversation
samlaf
force-pushed
the
refactor--better-routing-using-gorilla-mux-patterns
branch
from
aaad46a
to
cf11d90
Compare
server/routing.go
Outdated
| subrouterGET := r.Methods("GET").PathPrefix("/get").Subrouter() | ||
| // simple commitments (for nitro) | ||
| subrouterGET.HandleFunc("/"+ | ||
| "{optional_prefix:(?:0x)?}"+ // commitments can be prefixed with 0x | ||
| "{version_byte_hex:[0-9a-fA-F]{2}}"+ // should always be 0x00 for now but we let others through to return a 404 | ||
| "{raw_commitment_hex:[0-9a-fA-F]*}", | ||
| withLogging(withMetrics(svr.handleGetSimpleCommitment, svr.m, commitments.SimpleCommitmentMode), svr.log), |
There was a problem hiding this comment.
what benefits do we reap by using the gorilla library? I.e, why use a heavy framework vs. what we had before. Generally agree with the structure of the refactoring but still a bit curious if we need to introduce a heavy framework with more developer complexity.
There was a problem hiding this comment.
also from a philosophical standpoint I've never seen ethereum projects use third party frameworks like this in RPC server implementations
There was a problem hiding this comment.
Oh I've got many mouthfuls to say about this haha:
- re ethereum projects not using web frameworks: not quite sure I haven't done an extensive survey. A lot of them are simple json-rpc, so they use a json-rpc library instead of a rest api framework, but the altda decided to go with rest api. Also if I had to guess I'd say if other projects that are (proxy) servers like ours (like flashbots builder say) are not using web framework either out of ignorance, or because their routing is super simple. But our routing is a bit more involved, so basic stdlib http router (even after go1.22 improvements) isn't enough.
- gorilla is not a heavy framework. it's actually extremely lightweight. If I had my way I would actually use a full fledged framework with middlewares, like gin or echo (note that even these are "lightweight" frameworks). But gorilla mux is purely a routing library, not a framework. See limitations section at the bottom of this for a comparison.
- I already tried to explain the benefits of using gorilla's router instead of stdlib's router. I think this article summarizes my thoughts regarding the problem with our current implementation better than I ever could. Basically our routing ifs/switches are sprinkled everywhere in the proxy. This makes for very difficult reading. There's really only 3 types of commitments, so we should make the choice one of which "row" we're in, at the top routing layer, and then the rest of the code should be aware of which route it's in and be able to specialize. See for eg this switch statement; don't think we should have this that far down code paths.
server/handlers.go
Outdated
|
|
||
| // handlePostSimpleCommitment handles the POST request for simple commitments. | ||
| func (svr *Server) handlePostSimpleCommitment(w http.ResponseWriter, r *http.Request) error { | ||
| svr.log.Info("Processing simple commitment") |
There was a problem hiding this comment.
shouldn't we also put the method type for the simple commitment strings as well?
There was a problem hiding this comment.
465fb84
moved all the logs to the shared handlers (adds a redundant reencoding to hex, but at least its simpler this way)
server/handlers.go
Outdated
|
|
||
| // handlePostOPGenericCommitment handles the POST request for optimism generic commitments. | ||
| func (svr *Server) handlePostOPGenericCommitment(w http.ResponseWriter, r *http.Request) error { | ||
| svr.log.Info("Processing simple commitment") |
There was a problem hiding this comment.
this log stmt doesn't reflect the function
There was a problem hiding this comment.
465fb84
moved all the logs to the shared handlers (adds a redundant reencoding to hex, but at least its simpler this way)
…ared handlers only
samlaf
deleted the
refactor--better-routing-using-gorilla-mux-patterns
branch
This is still WIP, only refactored the GET methods so far, and still having trouble adapting the tests. But curious for opinions/criticisms at this point.
Fixes Issue
Fixes #
Changes proposed
Our routing is a bit convoluted and hard to follow. I'm working on an openapi spec in #181, which we could use with tests to enforce our actual golang router implements exactly. But our code is hard to read because the logic for the 3 types of commitments (simple, op generic, op keccak) is sprinkled all throughout different helper functions. Typically it's better to push ifs up and fors down.
go1.22's revamped the stdlib router, but still doesn't have prefix or regexp abilities, which is why I opted to using gorilla mux, which is a minimal (in the sense that it's not an entire web framework we are taking on), but still very extensive routing library. In my opinion routing paths should be easy to read like openapi specs, not be sprinkled throughout if statements left and write inside helper functions. This PR makes routes like:
Screenshots (Optional)
Note to reviewers