feat: allow filtering of validators by top-level validator status #7143

vladmarusyk · 2024-10-09T15:58:14Z

Add support for filtering validators by top-level status

This pull request implements support for filtering validators by top-level statuses such as active, pending, exited, and withdrawal as specified in the beacon-api spec.

Changes:

Added mapToGeneralStatus function to map detailed statuses to top-level statuses (active, pending, exited, and withdrawal).
Updated filterStateValidatorsByStatus to support filtering validators by both detailed and top-level statuses.

Related issue:

Closes #7141

CLAassistant · 2024-10-09T15:58:22Z

All committers have signed the CLA.

nflaig

Thanks for picking this up, some initial feedback

nflaig · 2024-10-09T18:58:28Z

packages/beacon-node/src/api/impl/beacon/state/utils.ts

@@ -83,6 +83,51 @@ export async function getStateResponseWithRegen(
  return res;
 }

+function mapToGeneralStatus(subStatus: string): string {


switch-case would be nicer here, we can also reuse ValidatorStatus type, and potentially a extended type which includes top-level / general statuses as well

nflaig · 2024-10-09T18:58:45Z

packages/beacon-node/src/api/impl/beacon/state/utils.ts

+ * Get the status of the validator
+ * based on conditions outlined in https://hackmd.io/ofFJ5gOmQpu1jjHilHbdQQ
+ */
+export function getValidatorStatus(validator: phase0.Validator, currentEpoch: Epoch): routes.beacon.ValidatorStatus {


why is this added here?

nflaig · 2024-10-09T19:04:36Z

also note that ValidatorStatus type was just moved to types packages (see #7140), make sure your branch is up-to-date

vladmarusyk · 2024-10-10T07:13:14Z

I've updated the code based on the feedback:

Replaced if-else with switch-case.
Used the ValidatorStatus type and created an extended type for general statuses.

nflaig

Looks good, can you please remove unrelated code updates, see #7143 (comment)

packages/beacon-node/src/api/impl/beacon/state/utils.ts

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

nflaig · 2024-10-10T09:55:34Z

e07a82b just removed the actual code, the unrelated code changes are still here

vladmarusyk · 2024-10-10T09:56:37Z

Sorry for the inconvenience, solving the first issue. Thank you for the opportunity.

vladmarusyk · 2024-10-10T10:28:13Z

Removed unrelated code

nflaig

LGTM - thanks for the cleanup

packages/beacon-node/src/api/impl/beacon/state/utils.ts

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

codecov · 2024-10-10T13:12:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.19%. Comparing base (068fbae) to head (59febed).
Report is 5 commits behind head on unstable.

Additional details and impacted files

@@            Coverage Diff            @@
##           unstable    #7143   +/-   ##
=========================================
  Coverage     49.19%   49.19%           
=========================================
  Files           597      597           
  Lines         39725    39725           
  Branches       2073     2083   +10     
=========================================
+ Hits          19542    19544    +2     
+ Misses        20142    20140    -2     
  Partials         41       41

wemeetagain

LGTM

unrelated, commenting for posterity
ValidatorStatus shouldn't contain "active"

nflaig · 2024-10-10T17:11:32Z

unrelated, commenting for posterity ValidatorStatus shouldn't contain "active"

yeah, let's get rid of this #7146

) * Fix ValidatorStatus type issue * refactor: use switch-case and typed general validator status mapping * Update packages/beacon-node/src/api/impl/beacon/state/utils.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com> * refactor: deleted unrelated code * refactor: deleted unrelated code * Update packages/beacon-node/src/api/impl/beacon/state/utils.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com> --------- Co-authored-by: Nico Flaig <nflaig@protonmail.com>

wemeetagain · 2024-11-11T08:49:02Z

🎉 This PR is included in v1.23.0 🎉

Fix ValidatorStatus type issue

46b8c63

vladmarusyk requested a review from a team as a code owner October 9, 2024 15:58

vladmarusyk changed the title ~~Closes #7141 | Allow filtering of validators by top-level validator status~~ feat: allow filtering of validators by top-level validator status Oct 9, 2024

nflaig reviewed Oct 9, 2024

View reviewed changes

refactor: use switch-case and typed general validator status mapping

7302aaa

nflaig reviewed Oct 10, 2024

View reviewed changes

packages/beacon-node/src/api/impl/beacon/state/utils.ts Outdated Show resolved Hide resolved

vladmarusyk and others added 2 commits October 10, 2024 12:41

Update packages/beacon-node/src/api/impl/beacon/state/utils.ts

f702f91

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

refactor: deleted unrelated code

e07a82b

refactor: deleted unrelated code

361ad8d

nflaig reviewed Oct 10, 2024

View reviewed changes

packages/beacon-node/src/api/impl/beacon/state/utils.ts Outdated Show resolved Hide resolved

Update packages/beacon-node/src/api/impl/beacon/state/utils.ts

59febed

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

nflaig approved these changes Oct 10, 2024

View reviewed changes

wemeetagain approved these changes Oct 10, 2024

View reviewed changes

wemeetagain merged commit 38f54ab into ChainSafe:unstable Oct 10, 2024
20 checks passed

nflaig mentioned this pull request Oct 10, 2024

chore: remove 'active' from validator status type #7146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: allow filtering of validators by top-level validator status #7143

feat: allow filtering of validators by top-level validator status #7143

vladmarusyk commented Oct 9, 2024

CLAassistant commented Oct 9, 2024 •

edited

Loading

nflaig left a comment

nflaig Oct 9, 2024

nflaig Oct 9, 2024

nflaig commented Oct 9, 2024

vladmarusyk commented Oct 10, 2024

nflaig left a comment •

edited

Loading

nflaig commented Oct 10, 2024

vladmarusyk commented Oct 10, 2024 •

edited

Loading

vladmarusyk commented Oct 10, 2024

nflaig left a comment

codecov bot commented Oct 10, 2024 •

edited

Loading

wemeetagain left a comment

nflaig commented Oct 10, 2024 •

edited

Loading

wemeetagain commented Nov 11, 2024

feat: allow filtering of validators by top-level validator status #7143

feat: allow filtering of validators by top-level validator status #7143

Conversation

vladmarusyk commented Oct 9, 2024

Add support for filtering validators by top-level status

Changes:

Related issue:

CLAassistant commented Oct 9, 2024 • edited Loading

nflaig left a comment

Choose a reason for hiding this comment

nflaig Oct 9, 2024

Choose a reason for hiding this comment

nflaig Oct 9, 2024

Choose a reason for hiding this comment

nflaig commented Oct 9, 2024

vladmarusyk commented Oct 10, 2024

nflaig left a comment • edited Loading

Choose a reason for hiding this comment

nflaig commented Oct 10, 2024

vladmarusyk commented Oct 10, 2024 • edited Loading

vladmarusyk commented Oct 10, 2024

nflaig left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 10, 2024 • edited Loading

Codecov Report

wemeetagain left a comment

Choose a reason for hiding this comment

nflaig commented Oct 10, 2024 • edited Loading

wemeetagain commented Nov 11, 2024

CLAassistant commented Oct 9, 2024 •

edited

Loading

nflaig left a comment •

edited

Loading

vladmarusyk commented Oct 10, 2024 •

edited

Loading

codecov bot commented Oct 10, 2024 •

edited

Loading

nflaig commented Oct 10, 2024 •

edited

Loading