Skip to content
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

HTTP endpoint for GetValidatorParticipation #14261

Merged
merged 15 commits into from
Aug 2, 2024

Conversation

saolyn
Copy link
Contributor

@saolyn saolyn commented Jul 25, 2024

What type of PR is this?

Other

What does this PR do? Why is it needed?

Adding the HTTP endpoint for GetValidatorParticipation.
I had discussed with Preston about potentially adding more default epoch options, beyond just genesis.
I added a new query param called state_id and added cases for head, finalized and genesis

Which issues(s) does this PR fix?

N/A

Other notes for review

@saolyn saolyn requested a review from rkapka July 25, 2024 16:06
@saolyn saolyn requested a review from a team as a code owner July 25, 2024 16:06
@saolyn saolyn requested review from kasey and rauljordan July 25, 2024 16:06
return nil, &RpcError{Reason: Internal, Err: errors.Wrap(err, "could not pre compute attestations: %v")}
}
} else {
return nil, &RpcError{Reason: Internal, Err: fmt.Errorf("invalid state type retrieved with a version of %d", beaconSt.Version())}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return nil, &RpcError{Reason: Internal, Err: fmt.Errorf("invalid state type retrieved with a version of %d", beaconSt.Version())}
return nil, &RpcError{Reason: Internal, Err: fmt.Errorf("invalid state type retrieved with a version of %s", version.String(beaconSt.Version()))}

Comment on lines 123 to 124
Epoch string `json:"epoch"`
StateID bool `json:"state_id"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you do not ever read the epoch parameter in the handler and rely on state_id instead. This makes sense to me but:

  • we have to remove the epoch param if we ignore it anyway; otherwise it will confuse callers
  • let's handle state_id in the same way as in Beacon API handlers; this means using the Stater to fetch states, which allows us to pass the following into the param:

"head" (canonical head in node's view), "genesis", "finalized", "justified", , <hex encoded stateRoot with 0x prefix>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do use the epoch, I set it to prioritize the state_id if it's set but it isn't required so in the default case where state_id doesn't match anything we set the epoch which is technically required in this case so that else I have doesn't actually do anything and would need to be removed here, or just remove the required:

	default:
		_, e, ok := shared.UintFromQuery(w, r, "epoch", true)
		if !ok {
			currentSlot := s.CoreService.GenesisTimeFetcher.CurrentSlot()
			currentEpoch := slots.ToEpoch(currentSlot)
			epoch = uint64(currentEpoch)
		} else {
			epoch = e
		}
	}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK, I missed it. I don't have that strong of an opinion on this, but having just a state_id and using the Stater similarly to how we do it in Beacon API makes things simpler and more uniform.

template: "/prysm/v1/validators/participation",
name: namespace + ".GetValidatorParticipation",
middleware: []mux.MiddlewareFunc{
middleware.ContentTypeHandler([]string{api.JsonMediaType}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not needed for GET

@saolyn saolyn enabled auto-merge August 2, 2024 13:33
@saolyn saolyn added the API Api related tasks label Aug 2, 2024
@saolyn saolyn added this pull request to the merge queue Aug 2, 2024
Merged via the queue into develop with commit 8366085 Aug 2, 2024
16 of 17 checks passed
@saolyn saolyn deleted the http-validator-participation branch August 2, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants