fix(node-api): Implement validators endpoint filtering properly #2467

shotes · 2025-02-03T18:17:30Z

Tested these two endpoints -
https://ethereum.github.io/beacon-APIs/#/Beacon/getStateValidators
https://ethereum.github.io/beacon-APIs/#/Beacon/postStateValidators

Refactored code for reusability as part of this change-set.
Increased go-eth2 client version.

Testing other validator endpoints, will open separate PR as don't want to increase diff.

codecov · 2025-02-03T18:19:56Z

Codecov Report

Attention: Patch coverage is 26.01881% with 236 lines in your changes missing coverage. Please review.

Project coverage is 32.77%. Comparing base (d90756b) to head (66c2f31).

Files with missing lines	Patch %	Lines
testing/e2e/suite/types/beacon_client.go	0.00%	71 Missing ⚠️
node-api/backend/validator.go	63.10%	32 Missing and 6 partials ⚠️
consensus-types/types/validator.go	0.00%	33 Missing ⚠️
testing/e2e/suite/types/consensus_client.go	0.00%	23 Missing ⚠️
node-api/handlers/beacon/types/conversions.go	46.15%	14 Missing and 7 partials ⚠️
node-api/handlers/beacon/validators.go	0.00%	15 Missing ⚠️
node-api/handlers/beacon/header.go	0.00%	14 Missing ⚠️
node-api/engines/echo/validator.go	0.00%	9 Missing ⚠️
node-api/handlers/beacon/types/response.go	0.00%	8 Missing ⚠️
node-api/handlers/beacon/historical.go	0.00%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2467      +/-   ##
==========================================
+ Coverage   32.30%   32.77%   +0.46%     
==========================================
  Files         350      351       +1     
  Lines       15674    15861     +187     
  Branches       20       20              
==========================================
+ Hits         5064     5198     +134     
- Misses      10247    10279      +32     
- Partials      363      384      +21

Files with missing lines	Coverage Δ
node-api/handlers/beacon/block.go	`0.00% <0.00%> (ø)`
node-api/handlers/beacon/randao.go	`0.00% <0.00%> (ø)`
node-api/handlers/beacon/historical.go	`0.00% <0.00%> (ø)`
node-api/handlers/beacon/types/response.go	`0.00% <0.00%> (ø)`
node-api/engines/echo/validator.go	`0.00% <0.00%> (ø)`
node-api/handlers/beacon/header.go	`0.00% <0.00%> (ø)`
node-api/handlers/beacon/validators.go	`0.00% <0.00%> (ø)`
node-api/handlers/beacon/types/conversions.go	`38.15% <46.15%> (+38.15%)`	⬆️
testing/e2e/suite/types/consensus_client.go	`0.00% <0.00%> (ø)`
consensus-types/types/validator.go	`67.08% <0.00%> (-17.72%)`	⬇️
... and 2 more

... and 3 files with indirect coverage changes

node-api/handlers/beacon/backend.go

calbera

Changes look mostly good. Can we write 2 E2E tests querying validators endpoint. 1 with empty IDs & another with empty Statuses filters and show both calls return all validators?

node-api/backend/utils/validator.go

node-api/handlers/beacon/validators.go

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

addressed

calbera · 2025-02-18T12:27:42Z

node-api/backend/validator.go

+	}
+
+	if !matchesStatusFilter(status, statuses) {
+		//nolint:nilnil // no data to return


Would agree that rather than returning nil, nil, we should return a defined error like ErrNoValidatorData and then explicitly catch this error case in the calling function.

Will remove the nolinting of nil,nil as well

makes sense, returned an error and then handled it in filterAndBuildValidatorData so that when no status matches, error is not returned (as per spec).

node-api/backend/validator.go

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

consensus-types/types/validator.go

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

consensus-types/types/validator.go

nit for readability Signed-off-by: Cal Bera <calbera@berachain.com>

node-api/backend/validator.go

calbera

couple nits then LGTM

shotes

nits and questions. Really liking how this is turning out and all of the additional test cases. Also like the change of the NewResponse(data any) GenericResponse return value in the node-api. Fits well.

shotes · 2025-02-18T20:59:25Z

testing/e2e/e2e_api_test.go

+		s.Ctx(),
+		&beaconapi.ValidatorsOpts{
+			State:           utils.StateIDHead,
+			ValidatorStates: []apiv1.ValidatorState{}, // empty statuses


Is there any difference between querying with empty-but-initialized ValidatorStates vs not populating the ValidatorStates field at all? This test feels redundant with TestValidatorsEmptyIndices

shotes · 2025-02-18T21:03:03Z

testing/e2e/e2e_staking_test.go

@@ -115,14 +115,15 @@ func (s *BeaconKitE2ESuite) TestDepositRobustness() {

 	// Grab genesis validators to get withdrawal creds.
 	s.Require().NoError(apiClient.Connect(s.Ctx()))
-	s.Require().NotNil(apiClient.BeaconKitNodeClient)
-	response, err := apiClient.BeaconKitNodeClient.Validators(s.Ctx(), &api.ValidatorsOpts{
+	response, err := apiClient.Validators(s.Ctx(), &api.ValidatorsOpts{
 		State:   "genesis",
 		Indices: []phase0.ValidatorIndex{0, 1, 2, 3, 4},


Can this be changed to empty Indices so that we know we are querying for all validators, instead of the hardcoded indices.

Wanted to keep a test for querying multiple specific indices. We already have a test with empty Indices TestValidatorsEmptyIndices in that case we get all the validators.

Edit : TestValidatorsEmptyIndicesAndStatuses is the test as we have merged two tests now.

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

Implement validators endpoint filtering properly

1ac13b4

shotes had a problem deploying to test-e2e February 3, 2025 18:17 — with GitHub Actions Error

Reorganize and add length check

58bdc83

shotes had a problem deploying to test-e2e February 3, 2025 18:26 — with GitHub Actions Error

start adding status type

642bf9a

shotes had a problem deploying to test-e2e February 3, 2025 19:36 — with GitHub Actions Error

Add validator statuses

900f949

shotes had a problem deploying to test-e2e February 3, 2025 20:52 — with GitHub Actions Error

error on bad status

f34d3c2

shotes had a problem deploying to test-e2e February 3, 2025 21:26 — with GitHub Actions Error

shotes marked this pull request as ready for review February 3, 2025 21:26

shotes requested a review from a team as a code owner February 3, 2025 21:26

Fix comments

8546aee

shotes temporarily deployed to test-e2e February 3, 2025 21:36 — with GitHub Actions Inactive

calbera reviewed Feb 3, 2025

View reviewed changes

node-api/handlers/beacon/backend.go Outdated Show resolved Hide resolved

calbera reviewed Feb 3, 2025

View reviewed changes

calbera reviewed Feb 4, 2025

View reviewed changes

node-api/backend/utils/validator.go Outdated Show resolved Hide resolved

calbera reviewed Feb 4, 2025

View reviewed changes

node-api/handlers/beacon/validators.go Show resolved Hide resolved

calbera assigned nidhi-singh02 Feb 4, 2025

add methods to consensus clients and test added

93efc2a

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 temporarily deployed to test-e2e February 4, 2025 08:33 — with GitHub Actions Inactive

go-eth2-client latest version

8f31ecb

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 had a problem deploying to test-e2e February 4, 2025 11:16 — with GitHub Actions Error

add test for empty status - need to fix

a357bc6

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 had a problem deploying to test-e2e February 4, 2025 11:26 — with GitHub Actions Error

nilaway fix

08064f7

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 had a problem deploying to test-e2e February 4, 2025 11:37 — with GitHub Actions Failure

Merge branch 'main' into node-api-validators

fcb400f

calbera had a problem deploying to test-e2e February 4, 2025 17:00 — with GitHub Actions Error

nidhi-singh02 temporarily deployed to test-e2e February 18, 2025 11:14 — with GitHub Actions Inactive

nidhi-singh02 requested a review from abi87 February 18, 2025 11:25

calbera reviewed Feb 18, 2025

View reviewed changes

abi87 reviewed Feb 18, 2025

View reviewed changes

node-api/backend/validator.go Show resolved Hide resolved

abi87 reviewed Feb 18, 2025

View reviewed changes

node-api/backend/validator.go Show resolved Hide resolved

error in case of status

1bdea70

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 had a problem deploying to test-e2e February 18, 2025 15:38 — with GitHub Actions Error

abi87 reviewed Feb 18, 2025

View reviewed changes

consensus-types/types/validator.go Outdated Show resolved Hide resolved

Error msg in caller

63526f8

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 had a problem deploying to test-e2e February 18, 2025 15:55 — with GitHub Actions Error

return error and no response

e1ccf4c

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 had a problem deploying to test-e2e February 18, 2025 16:15 — with GitHub Actions Error

linter

8c66561

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 temporarily deployed to test-e2e February 18, 2025 16:20 — with GitHub Actions Inactive

nidhi-singh02 requested review from abi87 and calbera February 18, 2025 16:24

Merge branch 'main' into node-api-validators

a7bcc6e

calbera had a problem deploying to test-e2e February 18, 2025 18:23 — with GitHub Actions Error

calbera reviewed Feb 18, 2025

View reviewed changes

consensus-types/types/validator.go Show resolved Hide resolved

Update consensus-types/types/validator.go

1d41546

nit for readability Signed-off-by: Cal Bera <calbera@berachain.com>

calbera temporarily deployed to test-e2e February 18, 2025 18:26 — with GitHub Actions Inactive

calbera reviewed Feb 18, 2025

View reviewed changes

node-api/backend/validator.go Outdated Show resolved Hide resolved

calbera reviewed Feb 18, 2025

View reviewed changes

node-api/backend/validator.go Outdated Show resolved Hide resolved

calbera approved these changes Feb 18, 2025

View reviewed changes

shotes commented Feb 18, 2025

View reviewed changes

Merge branch 'main' into node-api-validators

7871a3a

nidhi-singh02 had a problem deploying to test-e2e February 19, 2025 06:35 — with GitHub Actions Error

nit error messages

66c2f31

Signed-off-by: nidhi-singh02 <trippin@berachain.com>

nidhi-singh02 deployed to test-e2e February 19, 2025 06:53 — with GitHub Actions Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(node-api): Implement validators endpoint filtering properly #2467

fix(node-api): Implement validators endpoint filtering properly #2467

shotes commented Feb 3, 2025 •

edited by nidhi-singh02

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

calbera left a comment •

edited

Loading

calbera Feb 18, 2025

nidhi-singh02 Feb 18, 2025

calbera left a comment

shotes left a comment

shotes Feb 18, 2025

shotes Feb 18, 2025

nidhi-singh02 Feb 19, 2025 •

edited

Loading

fix(node-api): Implement validators endpoint filtering properly #2467

Are you sure you want to change the base?

fix(node-api): Implement validators endpoint filtering properly #2467

Conversation

shotes commented Feb 3, 2025 • edited by nidhi-singh02 Loading

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

calbera left a comment • edited Loading

Choose a reason for hiding this comment

calbera Feb 18, 2025

Choose a reason for hiding this comment

nidhi-singh02 Feb 18, 2025

Choose a reason for hiding this comment

calbera left a comment

Choose a reason for hiding this comment

shotes left a comment

Choose a reason for hiding this comment

shotes Feb 18, 2025

Choose a reason for hiding this comment

shotes Feb 18, 2025

Choose a reason for hiding this comment

nidhi-singh02 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

shotes commented Feb 3, 2025 •

edited by nidhi-singh02

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

calbera left a comment •

edited

Loading

nidhi-singh02 Feb 19, 2025 •

edited

Loading