[WIP] fix validators ids filter behavior & comment status filters #2072
Conversation
WalkthroughThe pull request introduces several enhancements to the validator management system in the codebase. Notably, a new method Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Outside diff range comments (2)
mod/node-api/engines/echo/vaildator.go (1)
Line range hint
151-166: ApproveValidateValidatorStatusimplementation with a suggestionThe
ValidateValidatorStatusfunction is well-implemented, correctly validating validator statuses based on the Eth Beacon Node API specifications. The use of a map for allowed statuses is efficient, and leveraging the existingvalidateAllowedStringshelper function promotes code reuse.To improve maintainability, consider defining the
allowedStatusesmap as a package-level constant. This would make it easier to update the allowed statuses in the future and potentially reuse them in other parts of the code. Here's a suggested implementation:+var allowedValidatorStatuses = map[string]bool{ + "pending_initialized": true, + "pending_queued": true, + "active_ongoing": true, + "active_exiting": true, + "active_slashed": true, + "exited_unslashed": true, + "exited_slashed": true, + "withdrawal_possible": true, + "withdrawal_done": true, +} func ValidateValidatorStatus(fl validator.FieldLevel) bool { // Eth Beacon Node API specs: https://hackmd.io/ofFJ5gOmQpu1jjHilHbdQQ - allowedStatuses := map[string]bool{ - "pending_initialized": true, - "pending_queued": true, - "active_ongoing": true, - "active_exiting": true, - "active_slashed": true, - "exited_unslashed": true, - "exited_slashed": true, - "withdrawal_possible": true, - "withdrawal_done": true, - } - return validateAllowedStrings(fl.Field().String(), allowedStatuses) + return validateAllowedStrings(fl.Field().String(), allowedValidatorStatuses) }mod/storage/pkg/beacondb/registry.go (1)
Line range hint
288-289: Consider creating an issue for the suggested refactoringThe TODO comments indicate awareness of potential improvements:
- Storing total active balances as a value updated on write.
- Moving this method out of KVStore for better separation of concerns.
These changes could significantly improve performance and code organization.
Consider creating a GitHub issue to track these refactoring tasks. This will ensure they're not forgotten and can be prioritized appropriately in future development cycles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (12)
- examples/berad/pkg/storage/registry.go (1 hunks)
- mod/node-api/backend/validator.go (3 hunks)
- mod/node-api/engines/echo/vaildator.go (1 hunks)
- mod/node-api/handlers/beacon/backend.go (1 hunks)
- mod/node-api/handlers/beacon/types/request.go (1 hunks)
- mod/node-api/handlers/beacon/validators.go (1 hunks)
- mod/node-api/handlers/config/handler.go (1 hunks)
- mod/node-api/handlers/config/routes.go (1 hunks)
- mod/node-core/pkg/components/interfaces.go (2 hunks)
- mod/state-transition/pkg/core/interfaces.go (1 hunks)
- mod/state-transition/pkg/core/state/interfaces.go (1 hunks)
- mod/storage/pkg/beacondb/registry.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (13)
mod/node-api/handlers/config/routes.go (3)
43-43: LGTM! Verify theGetSpecmethod implementation.The change from
h.NotImplementedtoh.GetSpecfor the/eth/v1/config/specendpoint is correct and aligns with the PR objectives.Please ensure that the
GetSpecmethod is properly implemented. Run the following script to verify its implementation:#!/bin/bash # Description: Verify the implementation of the GetSpec method # Test: Search for the GetSpec method definition ast-grep --lang go --pattern 'func ($_ *Handler[$_]) GetSpec($_) $_'
48-48: LGTM! Verify theGetDepositContractmethod implementation.The change from
h.NotImplementedtoh.GetDepositContractfor the/eth/v1/config/deposit_contractendpoint is correct and aligns with the PR objectives.Please ensure that the
GetDepositContractmethod is properly implemented. Run the following script to verify its implementation:#!/bin/bash # Description: Verify the implementation of the GetDepositContract method # Test: Search for the GetDepositContract method definition ast-grep --lang go --pattern 'func ($_ *Handler[$_]) GetDepositContract($_) $_'
38-38: LGTM! Verify theGetForkSchedulemethod implementation.The change from
h.NotImplementedtoh.GetForkSchedulefor the/eth/v1/config/fork_scheduleendpoint is correct and aligns with the PR objectives.Please ensure that the
GetForkSchedulemethod is properly implemented. Run the following script to verify its implementation:✅ Verification successful
GetForkSchedulemethod is correctly implemented.The
GetForkSchedulemethod inmod/node-api/handlers/config/handler.gois properly defined and aligns with the PR objectives.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the GetForkSchedule method # Test: Search for the GetForkSchedule method definition ast-grep --lang go --pattern 'func ($_ *Handler[$_]) GetForkSchedule($_) $_'Length of output: 727
Script:
#!/bin/bash # Description: Verify the implementation of the GetForkSchedule method # Test: Search for the GetForkSchedule method definition ast-grep --lang go --pattern 'func ($_ *Handler[$_]) GetForkSchedule($_) $_'Length of output: 846
mod/node-api/handlers/beacon/backend.go (2)
Line range hint
1-84: Overall, the changes look good and align with the PR objectives.The addition of the
ListValidatorsmethod to theValidatorBackendinterface enhances the functionality for querying validators. It provides a more flexible approach to listing validators, which addresses the issue mentioned in the PR objectives.Key points:
- The new method integrates well with existing methods in the interface.
- The change is localized and doesn't disrupt the existing code structure.
- The method signature is consistent with other methods in the interface.
To complete this PR:
- Ensure that all concrete implementations of
ValidatorBackendare updated to include the newListValidatorsmethod.- Consider adding a brief comment to document the new method.
- Implement the
validator_statusvalidation function and associated status filters as mentioned in the PR description.
80-84: Verify implementation in concrete typesThe addition of the
ListValidatorsmethod to theValidatorBackendinterface is a good enhancement. To ensure full compatibility and correct implementation:Please run the following script to check for implementations of the
ValidatorBackendinterface and verify that they include the newListValidatorsmethod:This script will help identify any concrete types that implement
ValidatorBackendand ensure they have been updated to include the newListValidatorsmethod.mod/node-api/handlers/beacon/types/request.go (1)
42-42: 🧹 Nitpick (assertive)Summary of changes and future steps
The changes to both
GetStateValidatorsRequestandPostStateValidatorsRequestare consistent and align with the PR objectives. The temporary removal ofvalidator_statusvalidation is noted.To ensure these changes don't get overlooked in the future:
- Create a new issue to track the implementation of the
validator_statusvalidation function.- Update the PR description to mention this pending task.
- Consider adding a comment in the code or a note in the project documentation about this temporary change.
To verify that these are the only instances where
validator_statusvalidation was removed, run the following script:This will help ensure that all instances have been addressed consistently and that appropriate TODO comments are in place.
Also applies to: 48-48
✅ Verification successful
Validation Successful 🔍
All instances of
validator_statusvalidation have been removed from the codebase, except for a TODO comment inmod/node-api/engines/echo/vaildator.go. This aligns with the review objectives and ensures that the current changes are correctly implemented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of `validator_status` validation in the codebase # Test: Search for `validator_status` in all Go files rg --type go 'validator_status' # Test: Search for TODO comments related to validator status rg --type go 'TODO.*validator.?status'Length of output: 212
mod/node-api/handlers/beacon/validators.go (2)
40-42: Track the implementation of status filters.The commented-out code removes a validation check for
req.Statuses. While this aligns with the current work-in-progress state of the PR, it's important to ensure this doesn't get overlooked.Consider the following actions:
- Create a separate issue to track the implementation of status filters.
- Add a link to the created issue in the TODO comment for better traceability.
- Set a timeline for addressing this TODO to prevent it from being forgotten.
To verify if an issue already exists for this, run:
#!/bin/bash gh issue list --repo berachain/beacon-kit --search "in:title status filter implementation"If no matching issue is found, consider creating one.
48-53: Verify the implementation ofListValidatorsin the backend.The change from
ValidatorsByIDstoListValidatorsaligns with the PR objectives of allowing listing validators without needing their IDs. This is a good improvement in the API's flexibility.To ensure the backend implementation correctly handles this new use case:
- Verify that
ListValidatorsin the backend properly handles cases wherereq.IDsis empty.- Check if there are any performance implications for listing all validators when no IDs are provided.
Run the following command to inspect the
ListValidatorsimplementation:Review the implementation to ensure it meets the new requirements.
mod/node-api/engines/echo/vaildator.go (1)
Line range hint
1-214: Summary of changes and alignment with PR objectivesThe changes in this file contribute significantly to the PR's objective of improving validator management. The new
ValidateValidatorStatusfunction and the planned integration of validator status validation inConstructValidatoralign well with the stated goals.Some points to consider:
- The
validator_statusvalidation is prepared but not yet fully implemented inConstructValidator, which is consistent with the WIP status of the PR.- The implementation of
ValidateValidatorStatusis solid and follows best practices.Next steps:
- Complete the integration of
ValidateValidatorStatusinConstructValidator.- Consider the suggested refactoring for improved maintainability.
- Ensure comprehensive testing of the new validation logic.
Overall, these changes represent good progress towards the PR objectives. Once the TODO items are addressed and the integration is complete, this part of the PR should be ready for final review and merging.
To ensure consistency across the codebase, let's check for any other occurrences of validator status validation:
mod/state-transition/pkg/core/state/interfaces.go (1)
92-93: Verify implementation and usage of the new method.The addition of
GetValidatorIndices()is consistent with the interface's design. However, ensure that:
- All implementations of the
KVStoreinterface are updated to include this new method.- The method is implemented efficiently, especially if the number of validators is large.
- Unit tests are added or updated to cover this new functionality.
- Documentation is updated to reflect this new capability of the
KVStoreinterface.To help verify the implementation, you can run the following script:
This script will help identify areas that may need updates due to the interface change.
mod/node-core/pkg/components/interfaces.go (3)
959-961: LGTM: New methodGetValidatorIndices()addedThe addition of the
GetValidatorIndices()method to theReadOnlyBeaconStateinterface is a good improvement. It aligns with the PR objectives by allowing the listing of validators without needing their IDs. The method signature is consistent with other methods in the interface, returning a slice ofmath.ValidatorIndexand an error.
1165-1169: LGTM: New methodListValidators()addedThe addition of the
ListValidators()method to theValidatorBackendinterface is an excellent improvement. It directly addresses the PR objectives by implementing a new query to list validators. The method signature is consistent with other methods in the interface, and the parameters (slot, ids, statuses) provide flexible filtering options for querying validators.
959-961: Summary: Excellent additions to improve validator queryingThe changes in this file are well-implemented and directly address the PR objectives. The new methods
GetValidatorIndices()inReadOnlyBeaconStateandListValidators()inValidatorBackendsignificantly improve the ability to list and query validators efficiently. These additions will enhance the functionality of the/eth/v1/beacon/states/:state/validatorsendpoint, allowing for more flexible and efficient validator management.Also applies to: 1165-1169
| Handler: h.GetForkSchedule, | ||
| }, | ||
| { | ||
| Method: http.MethodGet, | ||
| Path: "/eth/v1/config/spec", | ||
| Handler: h.NotImplemented, | ||
| Handler: h.GetSpec, | ||
| }, | ||
| { | ||
| Method: http.MethodGet, | ||
| Path: "/eth/v1/config/deposit_contract", | ||
| Handler: h.NotImplemented, | ||
| Handler: h.GetDepositContract, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Summary: Route handlers successfully implemented
The changes in this file successfully implement the new route handlers for the /eth/v1/config/fork_schedule, /eth/v1/config/spec, and /eth/v1/config/deposit_contract endpoints. These changes align well with the PR objectives to enhance the functionality of the API by providing actual implementations for the Ethereum configuration endpoints.
To ensure consistency across the codebase and maintain a clear API structure, consider the following:
- Document the purpose and expected behavior of each new handler method (
GetForkSchedule,GetSpec, andGetDepositContract) in their respective implementations. - Update any relevant API documentation or OpenAPI/Swagger specifications to reflect these new endpoints.
- Implement appropriate error handling and logging within each new handler method to facilitate debugging and maintain system reliability.
| ListValidators( | ||
| slot math.Slot, | ||
| ids []string, | ||
| statuses []string, | ||
| ) ([]*types.ValidatorData[ValidatorT], error) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding a brief comment for the new method.
The new ListValidators method is a valuable addition to the ValidatorBackend interface. It aligns well with the PR objectives and provides flexibility in querying validators.
Consider adding a brief comment above the method to describe its purpose and any important details about its parameters or return values. For example:
// ListValidators retrieves a list of validators for a given slot, optionally filtered by IDs and statuses.
// It returns a slice of ValidatorData pointers and an error if the operation fails.
ListValidators(
slot math.Slot,
ids []string,
statuses []string,
) ([]*types.ValidatorData[ValidatorT], error)This will improve the code's self-documentation and make it easier for other developers to understand and use the method.
| @@ -39,13 +39,13 @@ type GetFinalityCheckpointsRequest struct { | |||
| type GetStateValidatorsRequest struct { | |||
| types.StateIDRequest | |||
| IDs []string `query:"id" validate:"dive,validator_id"` | |||
| Statuses []string `query:"status" validate:"dive,validator_status"` | |||
| Statuses []string `query:"status" validate:"dive"` | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Removal of validator_status validation noted.
The change aligns with the PR objectives, as the validator_status validation function hasn't been fully implemented yet. However, please ensure that this validation is added back in the future to maintain data integrity.
Consider adding a TODO comment to remind about implementing the validator_status validation:
- Statuses []string `query:"status" validate:"dive"`
+ Statuses []string `query:"status" validate:"dive"` // TODO: Add back `validator_status` validation once implemented📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Statuses []string `query:"status" validate:"dive"` | |
| Statuses []string `query:"status" validate:"dive"` // TODO: Add back `validator_status` validation once implemented |
| } | ||
|
|
||
| type PostStateValidatorsRequest struct { | ||
| types.StateIDRequest | ||
| IDs []string `json:"ids" validate:"dive,validator_id"` | ||
| Statuses []string `json:"statuses" validate:"dive,validator_status"` | ||
| Statuses []string `json:"statuses" validate:"dive"` |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consistent removal of validator_status validation.
This change is consistent with the modification made to GetStateValidatorsRequest. As before, ensure that the validator_status validation is implemented in the future.
Consider adding a TODO comment here as well:
- Statuses []string `json:"statuses" validate:"dive"`
+ Statuses []string `json:"statuses" validate:"dive"` // TODO: Add back `validator_status` validation once implemented📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Statuses []string `json:"statuses" validate:"dive"` | |
| Statuses []string `json:"statuses" validate:"dive"` // TODO: Add back `validator_status` validation once implemented |
| h.Logger().Error("ERROR AFTER LIST VALIDATORS") | ||
| return nil, err | ||
| } | ||
| if len(validators) == 0 { | ||
| h.Logger().Error("NOT FOUND") | ||
| return nil, types.ErrNotFound |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Enhance error logging messages and format.
While the additional error logging is beneficial, consider the following improvements:
- Use more descriptive error messages that include context about the operation being performed.
- Follow consistent logging conventions, typically avoiding all caps unless for specific emphasis.
- Include relevant request parameters in the log messages for easier debugging.
Consider updating the log messages as follows:
-h.Logger().Error("ERROR AFTER LIST VALIDATORS")
+h.Logger().Error("Failed to list validators", "slot", slot, "error", err)
-h.Logger().Error("NOT FOUND")
+h.Logger().Error("No validators found for the given criteria", "slot", slot, "ids", req.IDs, "statuses", req.Statuses)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| h.Logger().Error("ERROR AFTER LIST VALIDATORS") | |
| return nil, err | |
| } | |
| if len(validators) == 0 { | |
| h.Logger().Error("NOT FOUND") | |
| return nil, types.ErrNotFound | |
| h.Logger().Error("Failed to list validators", "slot", slot, "error", err) | |
| return nil, err | |
| } | |
| if len(validators) == 0 { | |
| h.Logger().Error("No validators found for the given criteria", "slot", slot, "ids", req.IDs, "statuses", req.Statuses) | |
| return nil, types.ErrNotFound |
| } | ||
| return b.ValidatorByIndex(st, index) | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Implement Status Filtering in ValidatorsByIDs Function
The TODO comment suggests that status filtering needs to be implemented in the ValidatorsByIDs function. Adding this feature will enable users to filter validators based on their statuses, aligning with API requirements.
Do you need assistance in implementing the status filtering logic?
| // TODO: to adhere to the spec, this shouldn't error if the error | ||
| // is not found, but i can't think of a way to do that without coupling | ||
| // db impl to the api impl. |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Handle 'Validator Not Found' Without Returning an Error
The TODO comment indicates that according to the specification, the function should not return an error if the validator is not found. Instead, it should handle the situation gracefully, possibly by returning a nil validator or a specific response indicating that the validator does not exist. This approach avoids coupling the database implementation to the API implementation.
Would you like assistance in modifying the function to comply with the specification?
| // TODO: I guess once there are status filters it makes sense | ||
| // to merge ListValidators and ValidatorsByIds to reuse this filtering | ||
| // logic, but as of right now they are pretty much different functions |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Plan to Merge ListValidators and ValidatorsByIDs After Status Filters Implementation
The TODO comment notes that once status filters are in place, it would be beneficial to merge ListValidators and ValidatorsByIDs to reuse filtering logic. This refactoring will reduce code duplication and enhance maintainability.
| // statusSet := make(map[string]struct{}) | ||
| // for _, status := range statuses { | ||
| // statusSet[status] = struct{}{} | ||
| // } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Complete or Remove Commented-Out Status Filtering Code
The code related to status filtering is currently commented out in both the status set creation and the conditional block. To maintain code cleanliness and readability, consider completing the implementation of status filtering or removing the commented-out code until it's ready to be implemented.
Would you like assistance in finalizing the status filtering feature?
Also applies to: 155-161
| for _, index := range indices { | ||
| validator, err := b.ValidatorByIndex(st, index) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // if len(statuses) > 0 { | ||
| // if _, exists := statusSet[validator.Status]; exists { | ||
| // validators = append(validators, validator) | ||
| // } | ||
| // } else { | ||
| validators = append(validators, validator) | ||
| // } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optimize Retrieval of Validators for Better Performance
In the ListValidators function, when no IDs are provided, the code retrieves all validator indices and fetches each validator individually using ValidatorByIndex. This approach might lead to performance issues with a large number of validators. Consider optimizing the retrieval process by fetching validators in bulk or implementing a more efficient method to handle large datasets.
[performance]
|
never mind just realized this is already implemented here: #2050 |
This PR fixes the behavior of the
/eth/v1/beacon/states/:state/validatorsregarding IDs filters: the current behavior is listing over the IDs filter, but it's optional, leaving no way to list validators without previously knowing their IDs. We fixed this by exposing a new method on the backend and implementing a new query.It's not ready yet because we still have to implement
validator_statusvalidation function and status filters. I would love some pointers to the right direction for implementing this.Also please let me know about any changes to the code I need to make to make sure it matches the codebase's standards. I know I still left some comments and stuff I have to remove before we merge.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores