From cc2b4db582082d6986d5e1bce6e635a51f425800 Mon Sep 17 00:00:00 2001 From: hyunchel <3271191+hyunchel@users.noreply.github.com> Date: Fri, 13 Oct 2023 13:21:38 -0400 Subject: [PATCH] Add state not found test case (#13034) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * beacon-chain:rpc/eth/shared: prevent mutiple error messages This commit prevents the error writing function from writing multiple JSON objects. An error message with more than one JSON object will not unmarshal into the default error response. * beacon-chain/rpc/eth/beacon: add a test case for missing state This commit adds a test on beacon states finality checkpoints endpoint to cover a case when state is not found. * beacon-chain/rpc/eth: update error response to meet the spec This commit updates error message on beacon states finality checkpoints endpoint to ensure that the response complies to Ethereum Beacon-API specification. * beacon-chain/rpc/eth/shared: add build dependency * beacon-chain/rpc/eth/shared: update test on state not found --------- Co-authored-by: Radosław Kapka --- beacon-chain/rpc/eth/beacon/handlers_test.go | 24 ++++++++++- beacon-chain/rpc/eth/shared/BUILD.bazel | 2 + beacon-chain/rpc/eth/shared/errors.go | 6 ++- beacon-chain/rpc/eth/shared/errors_test.go | 43 ++++++++++++++++++++ 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/beacon-chain/rpc/eth/beacon/handlers_test.go b/beacon-chain/rpc/eth/beacon/handlers_test.go index 55b65fe58..e82c9004b 100644 --- a/beacon-chain/rpc/eth/beacon/handlers_test.go +++ b/beacon-chain/rpc/eth/beacon/handlers_test.go @@ -23,6 +23,7 @@ import ( doublylinkedtree "github.com/prysmaticlabs/prysm/v4/beacon-chain/forkchoice/doubly-linked-tree" "github.com/prysmaticlabs/prysm/v4/beacon-chain/rpc/eth/shared" rpctesting "github.com/prysmaticlabs/prysm/v4/beacon-chain/rpc/eth/shared/testing" + "github.com/prysmaticlabs/prysm/v4/beacon-chain/rpc/lookup" "github.com/prysmaticlabs/prysm/v4/beacon-chain/rpc/testutil" "github.com/prysmaticlabs/prysm/v4/beacon-chain/state" mockSync "github.com/prysmaticlabs/prysm/v4/beacon-chain/sync/initial-sync/testing" @@ -1999,10 +2000,18 @@ func TestGetFinalityCheckpoints(t *testing.T) { fakeState, err := util.NewBeaconState(fillCheckpoints) require.NoError(t, err) + stateProvider := func(ctx context.Context, stateId []byte) (state.BeaconState, error) { + if bytes.Equal(stateId, []byte("foobar")) { + return nil, &lookup.StateNotFoundError{} + } + return fakeState, nil + } + chainService := &chainMock.ChainService{} s := &Server{ Stater: &testutil.MockStater{ - BeaconState: fakeState, + BeaconState: fakeState, + StateProviderFunc: stateProvider, }, HeadFetcher: chainService, OptimisticModeFetcher: chainService, @@ -2039,6 +2048,19 @@ func TestGetFinalityCheckpoints(t *testing.T) { assert.Equal(t, http.StatusBadRequest, e.Code) assert.StringContains(t, "state_id is required in URL params", e.Message) }) + t.Run("state not found", func(t *testing.T) { + request := httptest.NewRequest(http.MethodGet, "/eth/v1/beacon/states/{state_id}/finality_checkpoints", nil) + request = mux.SetURLVars(request, map[string]string{"state_id": "foobar"}) + writer := httptest.NewRecorder() + writer.Body = &bytes.Buffer{} + + s.GetFinalityCheckpoints(writer, request) + assert.Equal(t, http.StatusNotFound, writer.Code) + e := &http2.DefaultErrorJson{} + require.NoError(t, json.Unmarshal(writer.Body.Bytes(), e)) + assert.Equal(t, http.StatusNotFound, e.Code) + assert.StringContains(t, "State not found", e.Message) + }) t.Run("execution optimistic", func(t *testing.T) { chainService := &chainMock.ChainService{Optimistic: true} s := &Server{ diff --git a/beacon-chain/rpc/eth/shared/BUILD.bazel b/beacon-chain/rpc/eth/shared/BUILD.bazel index 1d876427f..6ec854f27 100644 --- a/beacon-chain/rpc/eth/shared/BUILD.bazel +++ b/beacon-chain/rpc/eth/shared/BUILD.bazel @@ -36,6 +36,8 @@ go_test( srcs = ["errors_test.go"], embed = [":go_default_library"], deps = [ + "//beacon-chain/rpc/lookup:go_default_library", + "//network/http:go_default_library", "//testing/assert:go_default_library", "@com_github_pkg_errors//:go_default_library", ], diff --git a/beacon-chain/rpc/eth/shared/errors.go b/beacon-chain/rpc/eth/shared/errors.go index b2c9b9e7a..4e3af713f 100644 --- a/beacon-chain/rpc/eth/shared/errors.go +++ b/beacon-chain/rpc/eth/shared/errors.go @@ -53,11 +53,13 @@ type IndexedVerificationFailure struct { // WriteStateFetchError writes an appropriate error based on the supplied argument. // The argument error should be a result of fetching state. func WriteStateFetchError(w http.ResponseWriter, err error) { - if stateNotFoundErr, ok := err.(*lookup.StateNotFoundError); ok { - http2.HandleError(w, "Could not get state: "+stateNotFoundErr.Error(), http.StatusNotFound) + if _, ok := err.(*lookup.StateNotFoundError); ok { + http2.HandleError(w, "State not found", http.StatusNotFound) + return } if parseErr, ok := err.(*lookup.StateIdParseError); ok { http2.HandleError(w, "Invalid state ID: "+parseErr.Error(), http.StatusBadRequest) + return } http2.HandleError(w, "Could not get state: "+err.Error(), http.StatusInternalServerError) } diff --git a/beacon-chain/rpc/eth/shared/errors_test.go b/beacon-chain/rpc/eth/shared/errors_test.go index 19e7248b8..31b82ae68 100644 --- a/beacon-chain/rpc/eth/shared/errors_test.go +++ b/beacon-chain/rpc/eth/shared/errors_test.go @@ -1,9 +1,14 @@ package shared import ( + "encoding/json" + "net/http" + "net/http/httptest" "testing" "github.com/pkg/errors" + "github.com/prysmaticlabs/prysm/v4/beacon-chain/rpc/lookup" + http2 "github.com/prysmaticlabs/prysm/v4/network/http" "github.com/prysmaticlabs/prysm/v4/testing/assert" ) @@ -14,3 +19,41 @@ func TestDecodeError(t *testing.T) { de = NewDecodeError(de, "X") assert.Equal(t, "could not decode X.Y.Z: not a number", de.Error()) } + +// TestWriteStateFetchError tests the WriteStateFetchError function +// to ensure that the correct error message and code are written to the response +// as an expected JSON format. +func TestWriteStateFetchError(t *testing.T) { + cases := []struct { + err error + expectedMessage string + expectedCode int + }{ + { + err: &lookup.StateNotFoundError{}, + expectedMessage: "State not found", + expectedCode: http.StatusNotFound, + }, + { + err: &lookup.StateIdParseError{}, + expectedMessage: "Invalid state ID", + expectedCode: http.StatusBadRequest, + }, + { + err: errors.New("state not found"), + expectedMessage: "Could not get state", + expectedCode: http.StatusInternalServerError, + }, + } + + for _, c := range cases { + writer := httptest.NewRecorder() + WriteStateFetchError(writer, c.err) + + assert.Equal(t, c.expectedCode, writer.Code, "incorrect status code") + assert.StringContains(t, c.expectedMessage, writer.Body.String(), "incorrect error message") + + e := &http2.DefaultErrorJson{} + assert.NoError(t, json.Unmarshal(writer.Body.Bytes(), e), "failed to unmarshal response") + } +}