feat: implement eip4788#12570
Merged
terencechain merged 10 commits intodeneb-integrationfrom Aug 8, 2023
Merged
Conversation
terencechain
force-pushed
the
eip4788
branch
3 times, most recently
from
4275a62 to
6bcb81f
Compare
terencechain
requested review from
kasey,
potuz and
rauljordan
and removed request for
a team
rauljordan
reviewed
Jun 28, 2023
| return nil, consensus_types.ErrNotSupported("PayloadAttributePbV3", a.version) | ||
| } | ||
| if a.timeStamp == 0 && len(a.prevRandao) == 0 { | ||
| return nil, nil |
Contributor
There was a problem hiding this comment.
Given we are validating fields here, should we check that parent beacon block root and other fields are non-nil here?
Collaborator
Author
There was a problem hiding this comment.
I dont think this check is necessary so ill remove it
terencechain
force-pushed
the
deneb-integration
branch
from
aaeda1b to
dd3e166
Compare
terencechain
force-pushed
the
eip4788
branch
2 times, most recently
from
96d50be to
e9c0e11
Compare
saolyn
reviewed
Jul 21, 2023
Contributor
saolyn
left a comment
saolyn
left a comment
There was a problem hiding this comment.
You seem to have a couple messed up imports, probably from another branch that was merged in
rkapka
reviewed
Jul 21, 2023
| return nil, errNilPayloadAttribute | ||
| } | ||
| if a.version != version.Deneb { | ||
| return nil, consensus_types.ErrNotSupported("PayloadAttributePbV3", a.version) |
Contributor
There was a problem hiding this comment.
Suggested change
| return nil, consensus_types.ErrNotSupported("PayloadAttributePbV3", a.version) | |
| return nil, consensus_types.ErrNotSupported("PbV3", a.version) |
Previous versions are also mismatched.
| a, err := New(&enginev1.PayloadAttributesV2{}) | ||
| require.NoError(t, err) | ||
| _, err = a.PbV3() | ||
| require.ErrorContains(t, "PayloadAttributePbV3 is not supported for capella: unsupported getter", err) |
Contributor
There was a problem hiding this comment.
This is not a suggestion to change the code, but my personal opinion is that tests are less fragile when we check for a small meaningful part of the error message that is unlikely to change. In this example I would only check for not supported. Having the whole error message in the assertion means that you will need to update the test if the name of the function changes, or the unsupported getter part.
Collaborator
Author
There was a problem hiding this comment.
I agree, but it's also good to check against PbV3 vs V2 or V1. In this case, I am inclined to leave it as it is
terencechain
force-pushed
the
eip4788
branch
4 times, most recently
from
739ca9e to
245b2dc
Compare
Collaborator
Author
|
ready for another look @rkapka @rauljordan ! |
Collaborator
Author
|
e2e failed, we'll need ethereum/go-ethereum#27289 to merge |
terencechain
force-pushed
the
eip4788
branch
2 times, most recently
from
102ff8f to
f8bc11f
Compare
Comment on lines
-1103
to
+1110
| vh0 := [32]byte{1, 207, 35, 21, 201, 118, 88, 167, 237, 84, 173, 161, 129, 118, 94, 35, 179, 250, 219, 81, 80, 250, 179, 149, 9, 246, 49, 192, 185, 175, 69, 102} | ||
| vh1 := [32]byte{1, 226, 124, 226, 142, 82, 126, 176, 113, 150, 179, 26, 240, 245, 250, 24, 130, 172, 231, 1, 166, 130, 2, 42, 183, 121, 248, 22, 172, 57, 212, 126} | ||
| vh0 := "0x01cf2315c97658a7ed54ada181765e23b3fadb5150fab39509f631c0b9af4566" | ||
| vh1 := "0x01e27ce28e527eb07196b31af0f5fa1882ace701a682022ab779f816ac39d47e" |
Member
There was a problem hiding this comment.
yeah this is a nice improvement
terencechain
added a commit
that referenced
this pull request
Aug 16, 2023
james-prysm
pushed a commit
that referenced
this pull request
Aug 16, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 21, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 22, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 22, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 22, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 23, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 23, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 23, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 24, 2023
kasey
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 24, 2023
james-prysm
pushed a commit
that referenced
this pull request
Aug 25, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 30, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 30, 2023
prestonvanloon
pushed a commit
that referenced
this pull request
Aug 31, 2023
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
This PR implements EIP4788 on Prysm. Majority of the changes are on the execution API