Simplify error handling for JsonRestHandler (#13369)

* Simplify error handling for `JsonRestHandler`

* POST

* reduce complexity

* review feedback

* uncomment route

* fix rest of tests

testing/assertions/assertions.go

@@ -137,7 +137,8 @@ func StringContains(loggerFn assertionLoggerFn, expected, actual string, flag bo
 
 // NoError asserts that error is nil.
 func NoError(loggerFn assertionLoggerFn, err error, msg ...interface{}) {
-	if err != nil {
+	// reflect.ValueOf is needed for nil instances of custom types implementing Error
+	if err != nil && !reflect.ValueOf(err).IsNil() {
 		errMsg := parseMsg("Unexpected error", msg...)
 		_, file, line, _ := runtime.Caller(2)
 		loggerFn("%s:%d %s: %v", filepath.Base(file), line, errMsg, err)

validator/client/beacon-api/BUILD.bazel

@@ -15,7 +15,6 @@ go_library(
         "domain_data.go",
         "doppelganger.go",
         "duties.go",
-        "errors.go",
         "genesis.go",
         "get_beacon_block.go",
         "index.go",

validator/client/beacon-api/activation_test.go

@@ -123,7 +123,6 @@ func TestActivation_Nominal(t *testing.T) {
 		&stateValidatorsResponseJson,
 	).Return(
 		nil,
-		nil,
 	).SetArg(
 		4,
 		beacon.GetValidatorsResponse{
@@ -248,7 +247,6 @@ func TestActivation_InvalidData(t *testing.T) {
 					gomock.Any(),
 				).Return(
 					nil,
-					nil,
 				).SetArg(
 					4,
 					beacon.GetValidatorsResponse{
@@ -289,7 +287,6 @@ func TestActivation_JsonResponseError(t *testing.T) {
 		gomock.Any(),
 		gomock.Any(),
 	).Return(
-		nil,
 		errors.New("some specific json error"),
 	).Times(1)
 

validator/client/beacon-api/attestation_data.go

@@ -24,12 +24,8 @@ func (c beaconApiValidatorClient) getAttestationData(
 	query := buildURL("/eth/v1/validator/attestation_data", params)
 	produceAttestationDataResponseJson := validator.GetAttestationDataResponse{}
 
-	errJson, err := c.jsonRestHandler.Get(ctx, query, &produceAttestationDataResponseJson)
-	if err != nil {
-		return nil, errors.Wrap(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, query, &produceAttestationDataResponseJson); err != nil {
+		return nil, err
 	}
 
 	if produceAttestationDataResponseJson.Data == nil {

validator/client/beacon-api/attestation_data_test.go

@@ -39,7 +39,6 @@ func TestGetAttestationData_ValidAttestation(t *testing.T) {
 		&produceAttestationDataResponseJson,
 	).Return(
 		nil,
-		nil,
 	).SetArg(
 		2,
 		validator.GetAttestationDataResponse{
@@ -190,7 +189,6 @@ func TestGetAttestationData_InvalidData(t *testing.T) {
 				&produceAttestationDataResponseJson,
 			).Return(
 				nil,
-				nil,
 			).SetArg(
 				2,
 				testCase.generateData(),
@@ -219,7 +217,6 @@ func TestGetAttestationData_JsonResponseError(t *testing.T) {
 		fmt.Sprintf("/eth/v1/validator/attestation_data?committee_index=%d&slot=%d", committeeIndex, slot),
 		&produceAttestationDataResponseJson,
 	).Return(
-		nil,
 		errors.New("some specific json response error"),
 	).Times(1)
 

validator/client/beacon-api/beacon_api_beacon_chain_client.go

@@ -30,12 +30,9 @@ const getValidatorPerformanceEndpoint = "/prysm/validators/performance"
 
 func (c beaconApiBeaconChainClient) getHeadBlockHeaders(ctx context.Context) (*beacon.GetBlockHeaderResponse, error) {
 	blockHeader := beacon.GetBlockHeaderResponse{}
-	errJson, err := c.jsonRestHandler.Get(ctx, "/eth/v1/beacon/headers/head", &blockHeader)
+	err := c.jsonRestHandler.Get(ctx, "/eth/v1/beacon/headers/head", &blockHeader)
 	if err != nil {
-		return nil, errors.Wrap(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+		return nil, err
 	}
 
 	if blockHeader.Data == nil || blockHeader.Data.Header == nil {
@@ -53,12 +50,8 @@ func (c beaconApiBeaconChainClient) GetChainHead(ctx context.Context, _ *empty.E
 	const endpoint = "/eth/v1/beacon/states/head/finality_checkpoints"
 
 	finalityCheckpoints := beacon.GetFinalityCheckpointsResponse{}
-	errJson, err := c.jsonRestHandler.Get(ctx, endpoint, &finalityCheckpoints)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, endpoint, &finalityCheckpoints); err != nil {
+		return nil, err
 	}
 
 	if finalityCheckpoints.Data == nil {
@@ -338,12 +331,8 @@ func (c beaconApiBeaconChainClient) GetValidatorPerformance(ctx context.Context,
 		return nil, errors.Wrap(err, "failed to marshal request")
 	}
 	resp := &validator.PerformanceResponse{}
-	errJson, err := c.jsonRestHandler.Post(ctx, getValidatorPerformanceEndpoint, nil, bytes.NewBuffer(request), resp)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err = c.jsonRestHandler.Post(ctx, getValidatorPerformanceEndpoint, nil, bytes.NewBuffer(request), resp); err != nil {
+		return nil, err
 	}
 
 	return &ethpb.ValidatorPerformanceResponse{

validator/client/beacon-api/beacon_api_beacon_chain_client_test.go

@@ -122,10 +122,7 @@ func TestListValidators(t *testing.T) {
 		)
 
 		jsonRestHandler := mock.NewMockJsonRestHandler(ctrl)
-		jsonRestHandler.EXPECT().Get(ctx, blockHeaderEndpoint, gomock.Any()).Return(
-			nil,
-			errors.New("bar error"),
-		)
+		jsonRestHandler.EXPECT().Get(ctx, blockHeaderEndpoint, gomock.Any()).Return(errors.New("bar error"))
 
 		beaconChainClient := beaconApiBeaconChainClient{
 			stateValidatorsProvider: stateValidatorsProvider,
@@ -200,7 +197,6 @@ func TestListValidators(t *testing.T) {
 				jsonRestHandler := mock.NewMockJsonRestHandler(ctrl)
 				jsonRestHandler.EXPECT().Get(ctx, blockHeaderEndpoint, gomock.Any()).Return(
 					nil,
-					nil,
 				).SetArg(
 					2,
 					testCase.blockHeaderResponse,
@@ -752,7 +748,6 @@ func TestGetChainHead(t *testing.T) {
 				finalityCheckpointsResponse := beacon.GetFinalityCheckpointsResponse{}
 				jsonRestHandler := mock.NewMockJsonRestHandler(ctrl)
 				jsonRestHandler.EXPECT().Get(ctx, finalityCheckpointsEndpoint, &finalityCheckpointsResponse).Return(
-					nil,
 					testCase.finalityCheckpointsError,
 				).SetArg(
 					2,
@@ -853,15 +848,13 @@ func TestGetChainHead(t *testing.T) {
 				finalityCheckpointsResponse := beacon.GetFinalityCheckpointsResponse{}
 				jsonRestHandler.EXPECT().Get(ctx, finalityCheckpointsEndpoint, &finalityCheckpointsResponse).Return(
 					nil,
-					nil,
 				).SetArg(
 					2,
 					generateValidFinalityCheckpointsResponse(),
 				)
 
 				headBlockHeadersResponse := beacon.GetBlockHeaderResponse{}
 				jsonRestHandler.EXPECT().Get(ctx, headBlockHeadersEndpoint, &headBlockHeadersResponse).Return(
-					nil,
 					testCase.headBlockHeadersError,
 				).SetArg(
 					2,
@@ -885,7 +878,6 @@ func TestGetChainHead(t *testing.T) {
 		finalityCheckpointsResponse := beacon.GetFinalityCheckpointsResponse{}
 		jsonRestHandler.EXPECT().Get(ctx, finalityCheckpointsEndpoint, &finalityCheckpointsResponse).Return(
 			nil,
-			nil,
 		).SetArg(
 			2,
 			generateValidFinalityCheckpointsResponse(),
@@ -894,7 +886,6 @@ func TestGetChainHead(t *testing.T) {
 		headBlockHeadersResponse := beacon.GetBlockHeaderResponse{}
 		jsonRestHandler.EXPECT().Get(ctx, headBlockHeadersEndpoint, &headBlockHeadersResponse).Return(
 			nil,
-			nil,
 		).SetArg(
 			2,
 			generateValidBlockHeadersResponse(),
@@ -958,7 +949,6 @@ func Test_beaconApiBeaconChainClient_GetValidatorPerformance(t *testing.T) {
 		wantResponse,
 	).Return(
 		nil,
-		nil,
 	)
 
 	c := beaconApiBeaconChainClient{

validator/client/beacon-api/beacon_api_helpers.go

@@ -55,12 +55,8 @@ func (c *beaconApiValidatorClient) getFork(ctx context.Context) (*beacon.GetStat
 
 	stateForkResponseJson := &beacon.GetStateForkResponse{}
 
-	errJson, err := c.jsonRestHandler.Get(ctx, endpoint, stateForkResponseJson)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, endpoint, stateForkResponseJson); err != nil {
+		return nil, err
 	}
 
 	return stateForkResponseJson, nil
@@ -71,12 +67,8 @@ func (c *beaconApiValidatorClient) getHeaders(ctx context.Context) (*beacon.GetB
 
 	blockHeadersResponseJson := &beacon.GetBlockHeadersResponse{}
 
-	errJson, err := c.jsonRestHandler.Get(ctx, endpoint, blockHeadersResponseJson)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, endpoint, blockHeadersResponseJson); err != nil {
+		return nil, err
 	}
 
 	return blockHeadersResponseJson, nil
@@ -93,12 +85,8 @@ func (c *beaconApiValidatorClient) getLiveness(ctx context.Context, epoch primit
 		return nil, errors.Wrapf(err, "failed to marshal validator indexes")
 	}
 
-	errJson, err := c.jsonRestHandler.Post(ctx, url, nil, bytes.NewBuffer(marshalledJsonValidatorIndexes), livenessResponseJson)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err = c.jsonRestHandler.Post(ctx, url, nil, bytes.NewBuffer(marshalledJsonValidatorIndexes), livenessResponseJson); err != nil {
+		return nil, err
 	}
 
 	return livenessResponseJson, nil
@@ -109,12 +97,8 @@ func (c *beaconApiValidatorClient) getSyncing(ctx context.Context) (*node.SyncSt
 
 	syncingResponseJson := &node.SyncStatusResponse{}
 
-	errJson, err := c.jsonRestHandler.Get(ctx, endpoint, syncingResponseJson)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, endpoint, syncingResponseJson); err != nil {
+		return nil, err
 	}
 
 	return syncingResponseJson, nil

validator/client/beacon-api/beacon_api_helpers_test.go

@@ -117,7 +117,6 @@ func TestGetFork_Nominal(t *testing.T) {
 		&stateForkResponseJson,
 	).Return(
 		nil,
-		nil,
 	).SetArg(
 		2,
 		expected,
@@ -145,7 +144,6 @@ func TestGetFork_Invalid(t *testing.T) {
 		forkEndpoint,
 		gomock.Any(),
 	).Return(
-		nil,
 		errors.New("custom error"),
 	).Times(1)
 
@@ -186,7 +184,6 @@ func TestGetHeaders_Nominal(t *testing.T) {
 		&blockHeadersResponseJson,
 	).Return(
 		nil,
-		nil,
 	).SetArg(
 		2,
 		expected,
@@ -214,7 +211,6 @@ func TestGetHeaders_Invalid(t *testing.T) {
 		headersEndpoint,
 		gomock.Any(),
 	).Return(
-		nil,
 		errors.New("custom error"),
 	).Times(1)
 
@@ -265,7 +261,6 @@ func TestGetLiveness_Nominal(t *testing.T) {
 		expected,
 	).Return(
 		nil,
-		nil,
 	).Times(1)
 
 	validatorClient := &beaconApiValidatorClient{jsonRestHandler: jsonRestHandler}
@@ -289,7 +284,6 @@ func TestGetLiveness_Invalid(t *testing.T) {
 		gomock.Any(),
 		gomock.Any(),
 	).Return(
-		nil,
 		errors.New("custom error"),
 	).Times(1)
 
@@ -338,7 +332,6 @@ func TestGetIsSyncing_Nominal(t *testing.T) {
 				&syncingResponseJson,
 			).Return(
 				nil,
-				nil,
 			).SetArg(
 				2,
 				expected,
@@ -369,7 +362,6 @@ func TestGetIsSyncing_Invalid(t *testing.T) {
 		syncingEnpoint,
 		&syncingResponseJson,
 	).Return(
-		nil,
 		errors.New("custom error"),
 	).Times(1)
 

validator/client/beacon-api/beacon_api_node_client.go

@@ -24,12 +24,8 @@ type beaconApiNodeClient struct {
 
 func (c *beaconApiNodeClient) GetSyncStatus(ctx context.Context, _ *empty.Empty) (*ethpb.SyncStatus, error) {
 	syncingResponse := node.SyncStatusResponse{}
-	errJson, err := c.jsonRestHandler.Get(ctx, "/eth/v1/node/syncing", &syncingResponse)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, "/eth/v1/node/syncing", &syncingResponse); err != nil {
+		return nil, err
 	}
 
 	if syncingResponse.Data == nil {
@@ -42,13 +38,10 @@ func (c *beaconApiNodeClient) GetSyncStatus(ctx context.Context, _ *empty.Empty)
 }
 
 func (c *beaconApiNodeClient) GetGenesis(ctx context.Context, _ *empty.Empty) (*ethpb.Genesis, error) {
-	genesisJson, errJson, err := c.genesisProvider.GetGenesis(ctx)
+	genesisJson, err := c.genesisProvider.GetGenesis(ctx)
 	if err != nil {
 		return nil, errors.Wrap(err, "failed to get genesis")
 	}
-	if errJson != nil {
-		return nil, errJson
-	}
 
 	genesisValidatorRoot, err := hexutil.Decode(genesisJson.GenesisValidatorsRoot)
 	if err != nil {
@@ -61,12 +54,8 @@ func (c *beaconApiNodeClient) GetGenesis(ctx context.Context, _ *empty.Empty) (*
 	}
 
 	depositContractJson := config.GetDepositContractResponse{}
-	errJson, err = c.jsonRestHandler.Get(ctx, "/eth/v1/config/deposit_contract", &depositContractJson)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err = c.jsonRestHandler.Get(ctx, "/eth/v1/config/deposit_contract", &depositContractJson); err != nil {
+		return nil, err
 	}
 
 	if depositContractJson.Data == nil {
@@ -89,12 +78,8 @@ func (c *beaconApiNodeClient) GetGenesis(ctx context.Context, _ *empty.Empty) (*
 
 func (c *beaconApiNodeClient) GetVersion(ctx context.Context, _ *empty.Empty) (*ethpb.Version, error) {
 	var versionResponse node.GetVersionResponse
-	errJson, err := c.jsonRestHandler.Get(ctx, "/eth/v1/node/version", &versionResponse)
-	if err != nil {
-		return nil, errors.Wrapf(err, msgUnexpectedError)
-	}
-	if errJson != nil {
-		return nil, errJson
+	if err := c.jsonRestHandler.Get(ctx, "/eth/v1/node/version", &versionResponse); err != nil {
+		return nil, err
 	}
 
 	if versionResponse.Data == nil || versionResponse.Data.Version == "" {