-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(node-api): added ValidateValidatorStatus #2090
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2090 +/- ##
==========================================
- Coverage 23.47% 23.47% -0.01%
==========================================
Files 357 357
Lines 16055 16056 +1
Branches 12 12
==========================================
Hits 3769 3769
- Misses 12116 12117 +1
Partials 170 170
|
WalkthroughThe changes introduce a new validation function, Changes
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Outside diff range comments (1)
mod/node-api/engines/echo/vaildator.go (1)
Line range hint
163-180
: LGTM! Consider using a constant for allowed statuses.The implementation of
ValidateValidatorStatus
is correct and aligns with the PR objective. It efficiently validates the validator status against the allowed values from the Ethereum Beacon Node API specifications.For improved maintainability, consider defining the allowed statuses as a package-level constant. This would make it easier to update the list if needed and potentially reuse it elsewhere. For example:
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 { return validateAllowedStrings(fl.Field().String(), allowedValidatorStatuses) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- mod/node-api/engines/echo/vaildator.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
mod/node-api/engines/echo/vaildator.go (1)
Line range hint
1-228
: Overall, the changes look good and achieve the PR objective.The implementation of
ValidateValidatorStatus
and its integration into theConstructValidator
function successfully reintroduce validation for validator status fields. The code is consistent with the existing structure and follows good practices.Consider the minor suggestions for improved clarity and maintainability:
- Adding a comment for the new validator in
ConstructValidator
.- Defining the allowed validator statuses as a package-level constant.
These changes will enhance the code's readability and make it easier to maintain in the future.
This change is part of https://github.com/berachain/beacon-kit/pull/1983/files#diff-a80e7b873ee5fd2392e59400c2fcf215804b06ec615e73b55a0e936d4ae77b52R69 which is available for review , @itsdevbear Wanted to know your thoughts on merging it so it gets addressed automatically. |
The
ValidateValidatorStatus
function is unused, leaving validator_status fields from API requests unvalidated. This PR adds back the validationSummary by CodeRabbit