From bd7e2a63263b54d6e2c178f7e08d207a3785399f Mon Sep 17 00:00:00 2001 From: Samuel Laferriere Date: Tue, 15 Oct 2024 18:53:51 +0100 Subject: [PATCH] refactor: use gorilla mux for POST routes --- server/handlers.go | 88 +++++++++++++-------- server/routing.go | 30 +++++-- server/routing_test.go | 105 +++++++++++++++++++++++++ server/server.go | 86 -------------------- server/server_test.go | 174 ++++++----------------------------------- 5 files changed, 211 insertions(+), 272 deletions(-) create mode 100644 server/routing_test.go diff --git a/server/handlers.go b/server/handlers.go index 06a3d5b8..49e301c7 100644 --- a/server/handlers.go +++ b/server/handlers.go @@ -7,7 +7,6 @@ import ( "fmt" "io" "net/http" - "path" "github.com/Layr-Labs/eigenda-proxy/commitments" "github.com/Layr-Labs/eigenda-proxy/store" @@ -19,6 +18,10 @@ func (svr *Server) handleHealth(w http.ResponseWriter, _ *http.Request) error { return nil } +// ================================================================================================= +// GET ROUTES +// ================================================================================================= + func (svr *Server) handleGetSimpleCommitment(w http.ResponseWriter, r *http.Request) error { versionByte, err := parseVersionByte(r) if err != nil { @@ -111,22 +114,56 @@ func (svr *Server) handleGetShared(ctx context.Context, w http.ResponseWriter, c return nil } -// handlePut handles the PUT request for commitments. -// Note: even when an error is returned, the commitment meta is still returned, -// because it is needed for metrics (see the WithMetrics middleware). -// TODO: we should change this behavior and instead use a custom error that contains the commitment meta. -func (svr *Server) handlePut(w http.ResponseWriter, r *http.Request) (commitments.CommitmentMeta, error) { - meta, err := readCommitmentMeta(r) +// ================================================================================================= +// PUT ROUTES +// ================================================================================================= + +func (svr *Server) handlePutSimpleCommitment(w http.ResponseWriter, r *http.Request) error { + svr.log.Info("Processing simple commitment") + commitmentMeta := commitments.CommitmentMeta{ + Mode: commitments.SimpleCommitmentMode, + CertVersion: byte(commitments.CertV0), // TODO: hardcoded for now + } + return svr.handlePutShared(w, r, nil, commitmentMeta) +} + +// handleGetOPKeccakCommitment handles the GET request for optimism keccak commitments. +func (svr *Server) handlePutOPKeccakCommitment(w http.ResponseWriter, r *http.Request) error { + // TODO: do we use a version byte in OPKeccak commitments? README seems to say so, but server_test didn't + // versionByte, err := parseVersionByte(r) + // if err != nil { + // err = fmt.Errorf("error parsing version byte: %w", err) + // http.Error(w, err.Error(), http.StatusBadRequest) + // return err + // } + commitmentMeta := commitments.CommitmentMeta{ + Mode: commitments.OptimismKeccak, + CertVersion: byte(commitments.CertV0), + } + + rawCommitmentHex, ok := mux.Vars(r)["raw_commitment_hex"] + if !ok { + return fmt.Errorf("commitment not found in path: %s", r.URL.Path) + } + commitment, err := hex.DecodeString(rawCommitmentHex) if err != nil { - err = fmt.Errorf("invalid commitment mode: %w", err) - http.Error(w, err.Error(), http.StatusBadRequest) - return commitments.CommitmentMeta{}, err + return fmt.Errorf("failed to decode commitment %s: %w", rawCommitmentHex, err) + } + + svr.log.Info("Processing op keccak commitment", "commitment", rawCommitmentHex, "commitmentMeta", commitmentMeta) + return svr.handlePutShared(w, r, commitment, commitmentMeta) +} + +func (svr *Server) handlePutOPGenericCommitment(w http.ResponseWriter, r *http.Request) error { + svr.log.Info("Processing simple commitment") + commitmentMeta := commitments.CommitmentMeta{ + Mode: commitments.OptimismGeneric, + CertVersion: byte(commitments.CertV0), // TODO: hardcoded for now } - // ReadCommitmentMeta function invoked inside HandlePut will not return a valid certVersion - // Current simple fix is using the hardcoded default value of 0 (also the only supported value) - //TODO: smarter decode needed when there's more than one version - meta.CertVersion = byte(commitments.CertV0) + return svr.handlePutShared(w, r, nil, commitmentMeta) +} +func (svr *Server) handlePutShared(w http.ResponseWriter, r *http.Request, comm []byte, meta commitments.CommitmentMeta) error { input, err := io.ReadAll(r.Body) if err != nil { err = MetaError{ @@ -134,22 +171,7 @@ func (svr *Server) handlePut(w http.ResponseWriter, r *http.Request) (commitment Meta: meta, } http.Error(w, err.Error(), http.StatusBadRequest) - return commitments.CommitmentMeta{}, err - } - - key := path.Base(r.URL.Path) - var comm []byte - - if len(key) > 0 && key != Put { // commitment key already provided (keccak256) - comm, err = commitments.StringToDecodedCommitment(key, meta.Mode) - if err != nil { - err = MetaError{ - Err: fmt.Errorf("failed to decode commitment from key %v (commitment mode %v): %w", key, meta.Mode, err), - Meta: meta, - } - http.Error(w, err.Error(), http.StatusBadRequest) - return commitments.CommitmentMeta{}, err - } + return err } commitment, err := svr.router.Put(r.Context(), meta.Mode, comm, input) @@ -165,7 +187,7 @@ func (svr *Server) handlePut(w http.ResponseWriter, r *http.Request) (commitment } else { http.Error(w, err.Error(), http.StatusInternalServerError) } - return commitments.CommitmentMeta{}, err + return err } responseCommit, err := commitments.EncodeCommitment(commitment, meta.Mode) @@ -175,7 +197,7 @@ func (svr *Server) handlePut(w http.ResponseWriter, r *http.Request) (commitment Meta: meta, } http.Error(w, err.Error(), http.StatusInternalServerError) - return commitments.CommitmentMeta{}, err + return err } svr.log.Info(fmt.Sprintf("response commitment: %x\n", responseCommit)) @@ -183,5 +205,5 @@ func (svr *Server) handlePut(w http.ResponseWriter, r *http.Request) (commitment if meta.Mode != commitments.OptimismKeccak { svr.writeResponse(w, responseCommit) } - return meta, nil + return nil } diff --git a/server/routing.go b/server/routing.go index 86149b7b..129bbd07 100644 --- a/server/routing.go +++ b/server/routing.go @@ -46,10 +46,30 @@ func (svr *Server) registerRoutes(r *mux.Router) { http.Error(w, fmt.Sprintf("unsupported commitment type %s", commitType), http.StatusBadRequest) }, ) - // we need to handle both: see https://github.com/ethereum-optimism/optimism/pull/12081 - // /put is for generic commitments, and /put/ for keccak256 commitments - // TODO: we should probably separate their handlers? - // r.HandleFunc("/put", WithLogging(WithMetrics(svr.handlePut, svr.m), svr.log)).Methods("POST") - // r.HandleFunc("/put/", WithLogging(WithMetrics(svr.handlePut, svr.m), svr.log)).Methods("POST") + + subrouterPOST := r.Methods("POST").PathPrefix("/put").Subrouter() + // simple commitments (for nitro) + subrouterPOST.HandleFunc("", // commitment is calculated by the server using the body data + withLogging(withMetrics(svr.handlePutSimpleCommitment, svr.m, commitments.SimpleCommitmentMode), svr.log), + ).Queries("commitment_mode", "simple") + // op keccak256 commitments (write to S3) + subrouterPOST.HandleFunc("/"+ + "{optional_prefix:(?:0x)?}"+ // commitments can be prefixed with 0x + // TODO: do we need this 0x00 byte? keccak commitments are the only ones that have anything after /put/ + "{commit_type_byte_hex:00}"+ // 00 for keccak256 commitments + // TODO: should these be present..?? README says they should but server_test didn't have them + // "{da_layer_byte:[0-9a-fA-F]{2}}"+ // should always be 0x00 for eigenDA but we let others through to return a 404 + // "{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}", + withLogging(withMetrics(svr.handlePutOPKeccakCommitment, svr.m, commitments.OptimismKeccak), svr.log), + ) + // op generic commitments (write to EigenDA) + subrouterPOST.HandleFunc("", // commitment is calculated by the server using the body data + withLogging(withMetrics(svr.handlePutOPGenericCommitment, svr.m, commitments.OptimismGeneric), svr.log), + ) + subrouterPOST.HandleFunc("/", // commitment is calculated by the server using the body data + withLogging(withMetrics(svr.handlePutOPGenericCommitment, svr.m, commitments.OptimismGeneric), svr.log), + ) + r.HandleFunc("/health", withLogging(svr.handleHealth, svr.log)).Methods("GET") } diff --git a/server/routing_test.go b/server/routing_test.go new file mode 100644 index 00000000..89a5e39a --- /dev/null +++ b/server/routing_test.go @@ -0,0 +1,105 @@ +package server + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/Layr-Labs/eigenda-proxy/metrics" + "github.com/Layr-Labs/eigenda-proxy/mocks" + "github.com/ethereum/go-ethereum/log" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/require" +) + +// TestRouting tests that the routes were properly encoded. +// We should eventually replace this with autogenerated specmatic tests over an openapi spec. +func TestRouting(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + mockRouter := mocks.NewMockIRouter(ctrl) + + m := metrics.NewMetrics("default") + server := NewServer("localhost", 8080, mockRouter, log.New(), m) + err := server.Start() + require.NoError(t, err) + + tests := []struct { + name string + url string + method string + body []byte + expectedCode int + expectedBody string + }{ + { + name: "Not Found - Must have a commitment key", + url: "/get/0x", + method: http.MethodGet, + body: nil, + // originally we returned 400 for these, but now we return 404 because + // not having a commitment is not a valid route. + expectedCode: http.StatusNotFound, + expectedBody: "404 page not found\n", + }, + { + name: "Not Found - Op Mode InvalidCommitmentKey", + url: "/get/0x1", + body: nil, + // originally we returned 400 for these, but now we return 404 because + // not having a commitment is not a valid route. + expectedCode: http.StatusNotFound, + expectedBody: "404 page not found\n", + }, + { + name: "Not Found - Op Mode InvalidCommitmentKey", + url: "/get/0x999", + body: nil, + // originally we returned 400 for these, but now we return 404 because + // not having a commitment is not a valid route. + expectedCode: http.StatusNotFound, + expectedBody: "404 page not found\n", + }, + { + name: "Not Found OP Keccak256 - TooShortCommitmentKey", + url: "/put/0x", + method: http.MethodPut, + body: []byte("some data"), + // originally we returned 400 for these, but now we return 404 because + // not having a commitment is not a valid route. + expectedCode: http.StatusNotFound, + expectedBody: "404 page not found\n", + }, + { + name: "Not Found OP Keccak256 - TooShortCommitmentKey", + url: "/put/0x1", + body: []byte("some data"), + // originally we returned 400 for these, but now we return 404 because + // not having a commitment is not a valid route. + expectedCode: http.StatusNotFound, + expectedBody: "404 page not found\n", + }, + { + name: "Not Found OP Keccak256 - InvalidCommitmentPrefixBytes", + url: fmt.Sprintf("/put/0x999%s", testCommitStr), + body: []byte("some data"), + // originally we returned 400 for these, but now we return 404 because + // not having a commitment is not a valid route. + expectedCode: http.StatusNotFound, + expectedBody: "404 page not found\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest(tt.method, tt.url, nil) + rec := httptest.NewRecorder() + server.httpServer.Handler.ServeHTTP(rec, req) + + require.Equal(t, tt.expectedCode, rec.Code) + require.Equal(t, tt.expectedBody, rec.Body.String()) + + }) + } +} diff --git a/server/server.go b/server/server.go index 34d8487e..c32d0118 100644 --- a/server/server.go +++ b/server/server.go @@ -7,15 +7,12 @@ import ( "fmt" "net" "net/http" - "path" "strconv" - "strings" "time" "github.com/Layr-Labs/eigenda-proxy/commitments" "github.com/Layr-Labs/eigenda-proxy/metrics" "github.com/Layr-Labs/eigenda-proxy/store" - "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/log" "github.com/gorilla/mux" ) @@ -116,89 +113,6 @@ func (svr *Server) Port() int { return port } -// Read both commitment mode and version -func readCommitmentMeta(r *http.Request) (commitments.CommitmentMeta, error) { - // label requests with commitment mode and version - ct, err := readCommitmentMode(r) - if err != nil { - return commitments.CommitmentMeta{}, fmt.Errorf("failed to read commitment mode: %w", err) - } - if ct == "" { - return commitments.CommitmentMeta{}, fmt.Errorf("commitment mode is empty") - } - cv, err := readCommitmentVersion(r, ct) - if err != nil { - // default to version 0 - return commitments.CommitmentMeta{Mode: ct, CertVersion: cv}, err - } - return commitments.CommitmentMeta{Mode: ct, CertVersion: cv}, nil -} - -func readCommitmentMode(r *http.Request) (commitments.CommitmentMode, error) { - query := r.URL.Query() - // if commitment mode is provided in the query params, use it - // eg. /get/0x123..?commitment_mode=simple - // TODO: should we only allow simple commitment to be set in the query params? - key := query.Get(CommitmentModeKey) - if key != "" { - return commitments.StringToCommitmentMode(key) - } - - // else, we need to parse the first byte of the commitment - commit := path.Base(r.URL.Path) - if len(commit) > 0 && commit != Put { // provided commitment in request params (op keccak256) - if !strings.HasPrefix(commit, "0x") { - commit = "0x" + commit - } - - decodedCommit, err := hexutil.Decode(commit) - if err != nil { - return "", err - } - - if len(decodedCommit) < 3 { - return "", fmt.Errorf("commitment is too short") - } - - switch decodedCommit[0] { - case byte(commitments.GenericCommitmentType): - return commitments.OptimismGeneric, nil - - case byte(commitments.Keccak256CommitmentType): - return commitments.OptimismKeccak, nil - - default: - return commitments.SimpleCommitmentMode, fmt.Errorf("unknown commit byte prefix") - } - } - return commitments.OptimismGeneric, nil -} - -func readCommitmentVersion(r *http.Request, mode commitments.CommitmentMode) (byte, error) { - commit := path.Base(r.URL.Path) - if len(commit) > 0 && commit != Put { // provided commitment in request params (op keccak256) - if !strings.HasPrefix(commit, "0x") { - commit = "0x" + commit - } - - decodedCommit, err := hexutil.Decode(commit) - if err != nil { - return 0, err - } - - if len(decodedCommit) < 3 { - return 0, fmt.Errorf("commitment is too short") - } - - if mode == commitments.OptimismGeneric || mode == commitments.SimpleCommitmentMode { - return decodedCommit[2], nil - } - - return decodedCommit[0], nil - } - return 0, nil -} - func (svr *Server) GetEigenDAStats() *store.Stats { return svr.router.GetEigenDAStore().Stats() } diff --git a/server/server_test.go b/server/server_test.go index 00969e56..f6653238 100644 --- a/server/server_test.go +++ b/server/server_test.go @@ -7,7 +7,6 @@ import ( "net/http/httptest" "testing" - "github.com/Layr-Labs/eigenda-proxy/commitments" "github.com/Layr-Labs/eigenda-proxy/metrics" "github.com/Layr-Labs/eigenda-proxy/mocks" "github.com/ethereum/go-ethereum/log" @@ -25,71 +24,6 @@ const ( testCommitStr = "9a7d4f1c3e5b8a09d1c0fa4b3f8e1d7c6b29f1e6d8c4a7b3c2d4e5f6a7b8c9d0" ) -// TestRouting tests that the routes were properly encoded. -// We should eventually replace this with autogenerated specmatic tests over an openapi spec. -func TestRouting(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - mockRouter := mocks.NewMockIRouter(ctrl) - - m := metrics.NewMetrics("default") - server := NewServer("localhost", 8080, mockRouter, log.New(), m) - err := server.Start() - require.NoError(t, err) - - tests := []struct { - name string - url string - method string - mockBehavior func() - expectedCode int - expectedBody string - }{ - { - name: "Not Found - Must have a commitment key", - url: "/get/0x", - method: http.MethodGet, - mockBehavior: func() {}, - // originally we returned 400 for these, but now we return 404 because - // not having a commitment is not a valid route. - expectedCode: http.StatusNotFound, - expectedBody: "404 page not found\n", - }, - { - name: "Failure - Op Mode InvalidCommitmentKey", - url: "/get/0x1", - mockBehavior: func() {}, - // originally we returned 400 for these, but now we return 404 because - // not having a commitment is not a valid route. - expectedCode: http.StatusNotFound, - expectedBody: "404 page not found\n", - }, - { - name: "Failure - Op Mode InvalidCommitmentKey", - url: "/get/0x999", - mockBehavior: func() {}, - // originally we returned 400 for these, but now we return 404 because - // not having a commitment is not a valid route. - expectedCode: http.StatusNotFound, - expectedBody: "404 page not found\n", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tt.mockBehavior() - - req := httptest.NewRequest(tt.method, tt.url, nil) - rec := httptest.NewRecorder() - server.httpServer.Handler.ServeHTTP(rec, req) - - require.Equal(t, tt.expectedCode, rec.Code) - require.Equal(t, tt.expectedBody, rec.Body.String()) - - }) - } -} - func TestHandleOPCommitments(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -174,51 +108,14 @@ func TestHandlerPut(t *testing.T) { server := NewServer("localhost", 8080, mockRouter, log.New(), metrics.NoopMetrics) tests := []struct { - name string - url string - body []byte - mockBehavior func() - expectedCode int - expectedBody string - expectError bool - expectedCommitmentMeta commitments.CommitmentMeta + name string + url string + body []byte + mockBehavior func() + expectedCode int + expectedBody string + expectError bool }{ - { - name: "Failure OP Keccak256 - TooShortCommitmentKey", - url: "/put/0x", - body: []byte("some data"), - mockBehavior: func() { - // Error is triggered before calling the router - }, - expectedCode: http.StatusBadRequest, - expectedBody: "", - expectError: true, - expectedCommitmentMeta: commitments.CommitmentMeta{}, - }, - { - name: "Failure OP Keccak256 - TooShortCommitmentKey", - url: "/put/0x1", - body: []byte("some data"), - mockBehavior: func() { - // Error is triggered before calling the router - }, - expectedCode: http.StatusBadRequest, - expectedBody: "", - expectError: true, - expectedCommitmentMeta: commitments.CommitmentMeta{}, - }, - { - name: "Failure OP Keccak256 - InvalidCommitmentPrefixBytes", - url: fmt.Sprintf("/put/0x999%s", testCommitStr), - body: []byte("some data"), - mockBehavior: func() { - // Error is triggered before calling the router - }, - expectedCode: http.StatusBadRequest, - expectedBody: "", - expectError: true, - expectedCommitmentMeta: commitments.CommitmentMeta{}, - }, { name: "Failure OP Mode Alt-DA - InternalServerError", url: "/put", @@ -226,10 +123,9 @@ func TestHandlerPut(t *testing.T) { mockBehavior: func() { mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, fmt.Errorf("internal error")) }, - expectedCode: http.StatusInternalServerError, - expectedBody: "", - expectError: true, - expectedCommitmentMeta: commitments.CommitmentMeta{}, + expectedCode: http.StatusInternalServerError, + expectedBody: "", + expectError: true, }, { name: "Success OP Mode Alt-DA", @@ -238,10 +134,9 @@ func TestHandlerPut(t *testing.T) { mockBehavior: func() { mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, - expectedCode: http.StatusOK, - expectedBody: opGenericPrefixStr + testCommitStr, - expectError: false, - expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.OptimismGeneric, CertVersion: 0}, + expectedCode: http.StatusOK, + expectedBody: opGenericPrefixStr + testCommitStr, + expectError: false, }, { name: "Success OP Mode Keccak256", @@ -250,10 +145,9 @@ func TestHandlerPut(t *testing.T) { mockBehavior: func() { mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, - expectedCode: http.StatusOK, - expectedBody: "", - expectError: false, - expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.OptimismKeccak, CertVersion: 0}, + expectedCode: http.StatusOK, + expectedBody: "", + expectError: false, }, { name: "Success Simple Commitment Mode", @@ -262,10 +156,9 @@ func TestHandlerPut(t *testing.T) { mockBehavior: func() { mockRouter.EXPECT().Put(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return([]byte(testCommitStr), nil) }, - expectedCode: http.StatusOK, - expectedBody: simpleCommitmentPrefix + testCommitStr, - expectError: false, - expectedCommitmentMeta: commitments.CommitmentMeta{Mode: commitments.SimpleCommitmentMode, CertVersion: 0}, + expectedCode: http.StatusOK, + expectedBody: simpleCommitmentPrefix + testCommitStr, + expectError: false, }, } @@ -273,15 +166,15 @@ func TestHandlerPut(t *testing.T) { t.Run(tt.name, func(t *testing.T) { tt.mockBehavior() - req := httptest.NewRequest(http.MethodPut, tt.url, bytes.NewReader(tt.body)) + req := httptest.NewRequest(http.MethodPost, tt.url, bytes.NewReader(tt.body)) rec := httptest.NewRecorder() - meta, err := server.handlePut(rec, req) - if tt.expectError { - require.Error(t, err) - } else { - require.NoError(t, err) - } + // To add the vars to the context, + // we need to create a router through which we can pass the request. + r := mux.NewRouter() + server.registerRoutes(r) + r.ServeHTTP(rec, req) + require.Equal(t, tt.expectedCode, rec.Code) if !tt.expectError && tt.expectedBody != "" { require.Equal(t, []byte(tt.expectedBody), rec.Body.Bytes()) @@ -290,21 +183,6 @@ func TestHandlerPut(t *testing.T) { if !tt.expectError && tt.expectedBody == "" { require.Equal(t, []byte(nil), rec.Body.Bytes()) } - require.Equal(t, tt.expectedCommitmentMeta, meta) }) } - - // for _, tt := range tests { - // t.Run(tt.name+"/CheckMiddlewaresNoPanic", func(_ *testing.T) { - // tt.mockBehavior() - - // req := httptest.NewRequest(http.MethodGet, tt.url, nil) - // rec := httptest.NewRecorder() - // // we also run the request through the middlewares, to make sure no panic occurs - // // could happen if there's a problem with the metrics. For eg, in the past we saw - // // panic: inconsistent label cardinality: expected 3 label values but got 1 in []string{"GET"} - // handler := withLogging(withMetrics(server.handlePut, server.m, commitments.OptimismKeccak), server.log) - // handler(rec, req) - // }) - // } }