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

Post aggregate and proofs request #8523

Merged
merged 42 commits into from
Aug 28, 2024

Conversation

mehdi-aouadi
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi commented Aug 21, 2024

PR Description

Update the SendAggregateAndProofsRequest to use the new PostAggregateAndProofsV2 API post Electra

Fixed Issue(s)

#8439

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.

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

This PR updates the SendAggregateAndProofsRequest to support the new PostAggregateAndProofsV2 API for the Electra milestone, implementing EIP-7549. Key changes include:

  • Added support for different SpecMilestones in SendAggregateAndProofsRequest and SendSignedAttestationsRequest
  • Introduced separate methods for pre-Electra and post-Electra submissions in both request handlers
  • Added consensus version header for post-Electra requests
  • Updated ValidatorApiMethod enum with new V2 endpoints for signed attestations and aggregate proofs
  • Modified OkHttpValidatorTypeDefClient to include 'spec' parameter in relevant methods
  • Updated AbstractTypeDefRequest to support additional headers in postJson method
  • Added new integration tests for Electra milestone in SendSignedAttestationsRequestElectraTest
  • Updated existing tests to accommodate new changes and improve specificity

These changes ensure compatibility with both pre-Electra and Electra milestones, aligning with EIP-7549 implementation requirements.

13 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

Comment on lines 50 to 51
@TestTemplate
public void getAggregateAttestation_makesExpectedRequest() throws Exception {
Copy link

Choose a reason for hiding this comment

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

logic: Method name doesn't match its functionality

@mehdi-aouadi mehdi-aouadi changed the title 8439 request Post aggregate and proofs request Aug 21, 2024
@mehdi-aouadi mehdi-aouadi self-assigned this Aug 22, 2024
@mehdi-aouadi mehdi-aouadi enabled auto-merge (squash) August 22, 2024 12:50
rolfyone and others added 15 commits August 27, 2024 16:23
…estationsRequest class

 - removed an endpoint that had 1 usage in the AbstractTypeDefRequest class- if we're adding a parameter to a fn thats used in one place i'd prefer we dont make a new fn.
 - new test class for electra attestations (renamed the post test class)

Signed-off-by: Paul Harris <[email protected]>
…estationsRequest class

 - removed an endpoint that had 1 usage in the AbstractTypeDefRequest class- if we're adding a parameter to a fn thats used in one place i'd prefer we dont make a new fn.
 - new test class for electra attestations (renamed the post test class)

Signed-off-by: Paul Harris <[email protected]>
@mehdi-aouadi mehdi-aouadi requested a review from tbenr August 27, 2024 14:33
@mehdi-aouadi mehdi-aouadi disabled auto-merge August 27, 2024 14:33
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 with one cleanup

@mehdi-aouadi mehdi-aouadi enabled auto-merge (squash) August 28, 2024 07:36
@mehdi-aouadi mehdi-aouadi merged commit 4953d5e into Consensys:master Aug 28, 2024
17 checks passed
@mehdi-aouadi mehdi-aouadi deleted the 8439-request branch August 28, 2024 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants