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

PART 3 - GET/Attestation Pool API - Add API interface #8438

Merged
merged 19 commits into from
Jul 26, 2024

Conversation

mehdi-aouadi
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi commented Jul 10, 2024

PR Description

Add the new GetAttestationV2 /eth/v2/beacon/pool/attestations

Fixed Issue(s)

#8029

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@mehdi-aouadi mehdi-aouadi changed the title 8029 3 api interface PART 3 - GET/Attestation Pool API - Add API interface Jul 10, 2024
@mehdi-aouadi mehdi-aouadi self-assigned this Jul 10, 2024
Copy link

@greptile-apps greptile-apps bot left a 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 and NodeDataProvider 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

Copy link

@greptile-apps greptile-apps bot left a 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

Copy link

@greptile-apps greptile-apps bot left a 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 in GetAttestationsV2.java
  • Implemented getAttestationsAndMetaData method in NodeDataProvider.java
  • Created JSON schema for v2 attestation response in GetPoolAttestationsV2Response.json
  • Added unit tests for GetAttestationsV2 in GetAttestationsV2Test.java
  • Updated AggregatingAttestationPool.java to support different attestation formats

16 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a 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

Copy link
Contributor

@tbenr tbenr left a 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.

@mehdi-aouadi mehdi-aouadi requested a review from tbenr July 24, 2024 13:22
@mehdi-aouadi
Copy link
Contributor Author

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

@mehdi-aouadi mehdi-aouadi force-pushed the 8029-3-api-interface branch 3 times, most recently from c9b2960 to f375c94 Compare July 24, 2024 14:31
@mehdi-aouadi mehdi-aouadi requested a review from tbenr July 25, 2024 16:12
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM

@mehdi-aouadi mehdi-aouadi enabled auto-merge (squash) July 26, 2024 11:47
@mehdi-aouadi mehdi-aouadi merged commit cf6cc06 into Consensys:master Jul 26, 2024
16 checks passed
@mehdi-aouadi mehdi-aouadi deleted the 8029-3-api-interface branch July 26, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants