Add GET /eth/v2/beacon/pool/attestations endpoint
#14560
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saolyn
changed the title
/eth/v2/beacon/pool/attestations endpoint
saolyn
changed the title
/eth/v2/beacon/pool/attestations endpointGET /eth/v2/beacon/pool/attestations endpoint
saolyn
requested review from
nalepae,
prestonvanloon,
potuz,
rkapka and
james-prysm
saolyn
force-pushed
the
add-getAttestations
branch
from
c43e5ba to
33a2948
Compare
james-prysm
reviewed
Oct 22, 2024
james-prysm
reviewed
Oct 22, 2024
james-prysm
reviewed
Oct 22, 2024
rkapka
reviewed
Oct 23, 2024
rkapka
reviewed
Oct 23, 2024
rkapka
reviewed
Oct 23, 2024
rkapka
reviewed
Oct 23, 2024
| } | ||
| httputil.WriteJson(w, &structs.ListAttestationsResponse{Data: filteredAtts}) | ||
| w.Header().Set(api.VersionHeader, version.String(firstVersion)) |
There was a problem hiding this comment.
Same idea as for slashings: we should use the head state or current slot to determine the version and only return attestations for that version.
rkapka
reviewed
Oct 28, 2024
|
|
||
| for _, att := range attestations { | ||
| var includeAttestation bool | ||
| attOld, ok := att.(*eth.Attestation) |
There was a problem hiding this comment.
maybe rename to att since there is only one type of attestations in v1?
| } | ||
| httputil.WriteJson(w, &structs.ListAttestationsResponse{Data: allAtts}) | ||
| data := attOld.GetData() | ||
| attStruct := structs.AttFromConsensus(attOld) |
There was a problem hiding this comment.
this should ideally be done inside if includeAttestation
| return | ||
| } | ||
| data := attElectra.GetData() | ||
| attStruct := structs.AttElectraFromConsensus(attElectra) |
There was a problem hiding this comment.
this should ideally be done inside if includeAttestation
| return | ||
| } | ||
| data := attOld.GetData() | ||
| attStruct := structs.AttFromConsensus(attOld) |
There was a problem hiding this comment.
this should ideally be done inside if includeAttestation
Comment on lines
173
to
174
| if isEmptyReq { | ||
| return true |
There was a problem hiding this comment.
This part is not really needed. If both are empty, then return committeeIndexMatch && slotMatch will return true. Also you will be able to have one function argument less.
Comment on lines
+167
to
+168
| ElectraForkVersion []byte `yaml:"ELECTRA_FORK_VERSION" spec:"true"` // ElectraForkVersion is used to represent the fork version for electra. | ||
| ElectraForkEpoch primitives.Epoch `yaml:"ELECTRA_FORK_EPOCH" spec:"true"` // ElectraForkEpoch is used to represent the assigned fork epoch for electra. |
rkapka
approved these changes
Oct 28, 2024
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
rkapka
pushed a commit
that referenced
this pull request
Nov 29, 2024
* add ListAttestationsV2 endpoint * fix endpoint * changelog * add endpoint to tests * add trailing comma * add version header + lint fix * all reviews * modify v1 and comments * fix linter * Radek' review
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Other
What does this PR do? Why is it needed?
Beacon API Electra alignment, add missing endpoint for GET
/eth/v2/beacon/pool/attestations.As described in the spec https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/getPoolAttestationsV2
Which issues(s) does this PR fix?
Part of #14476
Other notes for review
Acknowledgements