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

Deneb review common/eth2 #4698

Merged

Conversation

jimmygchen
Copy link
Member

@jimmygchen jimmygchen commented Sep 4, 2023

Issue Addressed

Related to #4676.

Proposed Changes

  • Deserialize deneb fork versioned SsePayloadAttributes into SsePayloadAttributesV3
  • Convert SignedBlockContents::try_into_full_block_and_blobs to return Result instead of Option so an error message can be included during unexpected / error scenarios. Refactored the function a bit to reduce amount of nested maps for readability, and improved error handling.
  • Update SignedBlockContents::blobs_cloned to include blobs for the blinded variant. The original intention to return None was to an attempt to indicate an unexpected scenario, but I've realized this pattern is quite misleading, and can lead to some hard to find bugs and misuse of the function. We're better off doing the assertions / checks outside of these functions.
  • Some other minor cleanups: code comments, remove unnecessary code

…nedBlockContents::blobs_cloned` to return blobs for `BlindedBlockAndBlobSidecars`.
@jimmygchen jimmygchen added work-in-progress PR is a work-in-progress deneb labels Sep 4, 2023
@jimmygchen jimmygchen self-assigned this Sep 4, 2023
@jimmygchen jimmygchen added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Sep 6, 2023
Copy link
Member

@michaelsproul michaelsproul left a comment

Choose a reason for hiding this comment

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

Looks good, thanks

@michaelsproul michaelsproul merged commit f9bea3c into sigp:deneb-free-blobs Sep 6, 2023
@jimmygchen jimmygchen mentioned this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deneb ready-for-review The code is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants