[Merged by Bors] - Beacon state validator id filter #1803
Conversation
realbigsean
requested review from
michaelsproul and
paulhauner
and removed request for
michaelsproul
paulhauner
left a comment
paulhauner
left a comment
There was a problem hiding this comment.
Looking good so far, but I think we also need:
- Updates to the
BeaconHttpClientfunction. - Updates to the tests.
beacon_node/http_api/src/lib.rs
Outdated
| .and_then( | ||
| |state_id: StateId, chain: Arc<BeaconChain<T>>, query: api_types::ValidatorsQuery| { | ||
| blocking_json_task(move || { | ||
| let id_predicate = |index: &u64, validator: &Validator| match query.id.as_ref() |
There was a problem hiding this comment.
With the use of map_or you might be able to avoid these predicates.
There was a problem hiding this comment.
Let me know if I got what you were thinking
beacon_node/http_api/src/lib.rs
Outdated
| far_future_epoch, | ||
| ); | ||
|
|
||
| if id_predicate(&(index as u64), validator) |
There was a problem hiding this comment.
I would place these predicates (perhaps as map_ors) above the filter_map on L444; this will stop us from allocating a ValidatorStatus for validators we're not going to return.
There was a problem hiding this comment.
I moved the filters ahead of the map, but because the second filter tests against ValidatorStatus, am I now allocating a ValidatorStatus twice whenever we return a validator that passes a status filter ?
paulhauner
added
waiting-on-author
|
Addressed all the comments so far (had one question), here is the diff. |
realbigsean
added
ready-for-review
beacon_node/http_api/src/lib.rs
Outdated
| .filter(|(_, (validator, _))| { | ||
| query.status.as_ref().map_or(true, |statuses| { | ||
| statuses.0.contains( | ||
| &api_types::ValidatorStatus::from_validator( |
There was a problem hiding this comment.
You can remove the double-instantiation by combining the filter on L439 and map on L451 with a filter_map. E.g.,:
.filter_map(|(index, (validator, balance))| {
let status = api_types::ValidatorStatus::from_validator(
Some(validator),
epoch,
finalized_epoch,
far_future_epoch,
);
if query
.status
.as_ref()
.map_or(true, |statuses| statuses.0.contains(&status))
{
Some(api_types::ValidatorData {
index: index as u64,
balance: *balance,
status: api_types::ValidatorStatus::from_validator(
Some(validator),
epoch,
finalized_epoch,
far_future_epoch,
),
validator: validator.clone(),
})
} else {
None
}
})
paulhauner
added
waiting-on-author
realbigsean
added
ready-for-review
| &self, | ||
| state_id: StateId, | ||
| ids: Option<&[ValidatorId]>, | ||
| statuses: Option<&[ValidatorStatus]>, |
There was a problem hiding this comment.
I see that giving Some(&[]) would yield no results. This seemed odd at first, but after giving it some thought I think it's my preference.
No change requested :)
paulhauner
added
ready-for-merge
## Issue Addressed Michael's comment here: #1434 (comment) Resolves #1808 ## Proposed Changes - Add query param `id` and `status` to the `validators` endpoint - Add string serialization and deserialization for `ValidatorStatus` - Drop `Epoch` from `ValidatorStatus` variants ## Additional Info Please provide any additional information. For example, future considerations or information useful for reviewers.
|
Pull request successfully merged into master. Build succeeded: |
bors
bot
changed the title
2 participants
Issue Addressed
Michael's comment here: #1434 (comment)
Resolves #1808
Proposed Changes
idandstatusto thevalidatorsendpointValidatorStatusEpochfromValidatorStatusvariantsAdditional Info
Please provide any additional information. For example, future considerations
or information useful for reviewers.