core: proto implementation for builder proposer #837
Conversation
| @@ -54,6 +54,12 @@ func ParSignedDataFromProto(typ DutyType, data *pbv1.ParSignedData) (ParSignedDa | |||
| return ParSignedData{}, errors.Wrap(err, "unmarshal block") | |||
| } | |||
| signedData = b | |||
There was a problem hiding this comment.
i looked for this ParSignedDataFromProto function in the repo and this is it's only declaration so not sure if this needs to be implemented here
There was a problem hiding this comment.
ParSignedDataFromProto is only called from ParSignedData<Set>FromProto which is called from parsigex to unmarshal protos received over the wire. But yeah, adding it here is correct.
core/proto_test.go
Outdated
| @@ -110,6 +124,22 @@ func TestSetBlockSig(t *testing.T) { | |||
| require.NotEqual(t, clone.Signature(), block.Signature()) | |||
| } | |||
|
|
|||
| func TestSetBlindedBlockSig(t *testing.T) { | |||
There was a problem hiding this comment.
may be wrong but this isn't testing anything in the proto.go file as far as i can see
should it be included? it's copied from the TestSetBlockSig which is also in this file
There was a problem hiding this comment.
yeah, these tests should move to signeddata_test.go since it tests whether SignedData implementation is immutable and that SetSignature returns a new instance. This can also easily be converted into a table test that tests each implementation. Maybe add a //TODO(ciaran): extract common SigSignature table test for all implementations
Codecov Report
@@ Coverage Diff @@
## main #837 +/- ##
==========================================
+ Coverage 54.84% 55.20% +0.36%
==========================================
Files 111 111
Lines 11509 11515 +6
==========================================
+ Hits 6312 6357 +45
+ Misses 4283 4236 -47
- Partials 914 922 +8
Continue to review full report at Codecov.
|
core/proto_test.go
Outdated
| @@ -110,6 +124,22 @@ func TestSetBlockSig(t *testing.T) { | |||
| require.NotEqual(t, clone.Signature(), block.Signature()) | |||
| } | |||
|
|
|||
| func TestSetBlindedBlockSig(t *testing.T) { | |||
There was a problem hiding this comment.
yeah, these tests should move to signeddata_test.go since it tests whether SignedData implementation is immutable and that SetSignature returns a new instance. This can also easily be converted into a table test that tests each implementation. Maybe add a //TODO(ciaran): extract common SigSignature table test for all implementations
core/proto_test.go
Outdated
| VersionedSignedBlindedBeaconBlock: eth2api.VersionedSignedBlindedBeaconBlock{ | ||
| Version: spec.DataVersionBellatrix, | ||
| Bellatrix: ð2v1.SignedBlindedBeaconBlock{ | ||
| Message: testutil.RandomBellatrixBlindedBeaconBlock(t), | ||
| Signature: testutil.RandomEth2Signature(), | ||
| }, |
There was a problem hiding this comment.
can extract this to testutil.RandomCoreVersionSignedBlindedBeaconBlock is you want to decrease creating these from scratch every time.
| @@ -54,6 +54,12 @@ func ParSignedDataFromProto(typ DutyType, data *pbv1.ParSignedData) (ParSignedDa | |||
| return ParSignedData{}, errors.Wrap(err, "unmarshal block") | |||
| } | |||
| signedData = b | |||
There was a problem hiding this comment.
ParSignedDataFromProto is only called from ParSignedData<Set>FromProto which is called from parsigex to unmarshal protos received over the wire. But yeah, adding it here is correct.
ciaranmcveigh5
added
the
merge when ready
category: feature
ticket: #809