-
Notifications
You must be signed in to change notification settings - Fork 303
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
PART 3 - GET/Attestation Pool API - Add API interface #8438
PART 3 - GET/Attestation Pool API - Add API interface #8438
Conversation
...t/resources/tech/pegasys/teku/beaconrestapi/beacon/schema/getPoolAttestationsV2Response.json
Outdated
Show resolved
Hide resolved
da3b0b4
to
9cdb8a5
Compare
.../src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/GetAttestationsV2Test.java
Outdated
Show resolved
Hide resolved
.../src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/GetAttestationsV2Test.java
Outdated
Show resolved
Hide resolved
ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/DataStructureUtil.java
Outdated
Show resolved
Hide resolved
9cdb8a5
to
20a9857
Compare
37b93c8
to
d5924cf
Compare
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.
PR Summary
- Added new API endpoint
/eth/v2/beacon/pool/attestations
(data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/GetAttestationsV2.java
) - Deprecated old endpoint
/eth/v1/beacon/pool/attestations
(data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/beacon/GetAttestations.java
) - Updated
DataProvider
andNodeDataProvider
to support new endpoint (data/provider/src/main/java/tech/pegasys/teku/api/DataProvider.java
,data/provider/src/main/java/tech/pegasys/teku/api/NodeDataProvider.java
) - Added integration and unit tests for new endpoint (
data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/GetAttestationsV2Test.java
) - Updated
AggregatingAttestationPool
to handle new attestation schema (ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPool.java
)
15 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings
data/provider/src/main/java/tech/pegasys/teku/api/NodeDataProvider.java
Outdated
Show resolved
Hide resolved
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.
PR Summary
(updates since last review)
- Deprecated v1 attestation pool API endpoint (
/data/beaconrestapi/src/integration-test/resources/tech/pegasys/teku/beaconrestapi/beacon/paths/_eth_v1_beacon_pool_attestations.json
) - Marked v1 endpoint as deprecated in preparation for v2 API
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
986d73f
to
b69a3aa
Compare
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.
PR Summary
(updates since last review)
- Added new
/eth/v2/beacon/pool/attestations
endpoint inGetAttestationsV2.java
- Implemented
getAttestationsAndMetaData
method inNodeDataProvider.java
- Created JSON schema for v2 attestation response in
GetPoolAttestationsV2Response.json
- Added unit tests for
GetAttestationsV2
inGetAttestationsV2Test.java
- Updated
AggregatingAttestationPool.java
to support different attestation formats
16 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
b69a3aa
to
934c297
Compare
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.
PR Summary
(updates since last review)
The pull request introduces a new API interface for retrieving attestations from the attestation pool, focusing on the /eth/v2/beacon/pool/attestations
endpoint.
- Added
GetAttestationsV2.java
to handle the new/eth/v2/beacon/pool/attestations
endpoint. - Updated
NodeDataProvider.java
to include methods for fetching attestations and their metadata. - Introduced JSON schema for v2 attestation responses in
GetPoolAttestationsV2Response.json
. - Deprecated the old v1 endpoint in
GetAttestations.java
. - Added unit tests for the new endpoint in
GetAttestationsV2Test.java
.
58 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
934c297
to
cbb3f88
Compare
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.
as discussed in call, the current commiteeIndex filter requires an update. We can leverage the committee filtering we already have for the aggregation flow, but requires to be updated to cover the phase0 because it has been design for electra only.
Done, made the filter applied differently based on the milestone. Pre electra we apply the filter to the index at the attestation data level and post Electra we rather apply it to the committee bits |
c9b2960
to
f375c94
Compare
.../src/main/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPool.java
Outdated
Show resolved
Hide resolved
ae2b2d7
to
3c8c0b7
Compare
028fca4
to
c66166c
Compare
c66166c
to
b9604a8
Compare
ef962b1
to
e6b2024
Compare
...rc/main/java/tech/pegasys/teku/statetransition/attestation/MatchingDataAttestationGroup.java
Outdated
Show resolved
Hide resolved
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.
LGTM
PR Description
Add the new
GetAttestationV2
/eth/v2/beacon/pool/attestations
Fixed Issue(s)
#8029
Documentation
doc-change-required
label to this PR if updates are required.Changelog