Add Deneb Types and Flow #553
Conversation
avalonche
force-pushed
the
develop-deneb
branch
3 times, most recently
from
9ced871
to
b46d453
Compare
avalonche
requested review from
michaelneuder and
metachris
and removed request for
michaelneuder
Codecov ReportAttention:
β Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #553 +/- ##
===========================================
- Coverage 68.85% 68.24% -0.62%
===========================================
Files 7 7
Lines 1111 1203 +92
===========================================
+ Hits 765 821 +56
- Misses 299 328 +29
- Partials 47 54 +7
Flags with carried forward coverage won't be shown. Click here to find out more. β View full report in Codecov by Sentry. |
avalonche
force-pushed
the
develop-deneb
branch
from
49b6f07
to
7f8ccad
Compare
avalonche
force-pushed
the
develop-deneb
branch
from
81fd59e
to
890dd7e
Compare
Co-authored-by: Justin Traglia <95511699+jtraglia@users.noreply.github.com>
metachris
force-pushed
the
develop-deneb
branch
from
e8be0fd
to
4aba68e
Compare
|
applied suggestions from @jtraglia and rebased |
| @@ -646,6 +651,143 @@ func (m *BoostService) processCapellaPayload(w http.ResponseWriter, req *http.Re | |||
| m.respondOK(w, result) | |||
| } | |||
|
|
|||
| func (m *BoostService) processDenebPayload(w http.ResponseWriter, req *http.Request, log *logrus.Entry, blindedBlock *eth2ApiV1Deneb.SignedBlindedBeaconBlock) { | |||
There was a problem hiding this comment.
would it be worth making a generic processPayload function that works with both deneb and capella blocks? just in terms of reducing code copying for ease of review? maybe not if we plan on stripping out the capella payload processing function immediately after the fork. just a thought as its hard to tell what logic is new with deneb and which is just copied from capella.
There was a problem hiding this comment.
I think we should strip Capella shortly afterwards.
There was a problem hiding this comment.
I've thought about this approach but there are issues as the process payload function accepts different types. We would need to have a custom interface to extract the common fields and process them which leads to more potential bugs.w/ versioning. There is code duplication but less prone to issues with version handling.
There was a problem hiding this comment.
good point @avalonche, agree. this is a space with high potential for critical bugs, better be safe with some duplication
| jsonFile, err := os.Open("../testdata/zhejiang-execution-payload-capella.json") | ||
| require.NoError(t, err) | ||
| defer jsonFile.Close() | ||
| func TestGetPayloadResponseIsEmpty(t *testing.T) { |
There was a problem hiding this comment.
could this be written as a table driven test to improve readability ?
There was a problem hiding this comment.
will do as follow up PR
π Summary
Part of #554
β± Motivation and Context
π References
β I have run these commands
make lintmake test-racego mod tidy