Conversation
Inspector-Butters
force-pushed
the
GetVersionV2
branch
2 times, most recently
from
1469307 to
393a8bc
Compare
Inspector-Butters
force-pushed
the
GetVersionV2
branch
from
393a8bc to
3b0f34a
Compare
we should mark it for deprecation in the comments |
api/server/structs/endpoints_node.go
Outdated
| } | ||
| type VersionV2 struct { | ||
| BeaconNode *ClientVersionV1 `json:"beacon_node"` | ||
| ExecutionClient []*ClientVersionV1 `json:"execution_client"` |
There was a problem hiding this comment.
you need ,omitempty added
There was a problem hiding this comment.
make sure there's a unit test where it's omitted instead of null, null is incorrect to spec
| ) | ||
|
|
||
| if len(result) == 0 { | ||
| return nil, errors.New("execution client returned no result") |
There was a problem hiding this comment.
this will hide the error, you need to handle the rpc error first then do the len check
| Code: "PM", | ||
| Name: "Prysm", | ||
| Version: version.SemanticVersion(), | ||
| Commit: version.GitCommit()[:8], |
There was a problem hiding this comment.
this is a potential panic I think, don't we need some len check
| assert.StringContains(t, arch, resp.Data.Version) | ||
| } | ||
|
|
||
| func TestGetVersionV2(t *testing.T) { |
There was a problem hiding this comment.
we should have some unhappy testcases too
| require.NoError(t, err) | ||
| require.DeepEqual(t, want, resp) | ||
| }) | ||
| t.Run(GetClientVersionV1, func(t *testing.T) { |
There was a problem hiding this comment.
also needs some unhappy cases
api/server/structs/endpoints_node.go
Outdated
| } | ||
| type VersionV2 struct { | ||
| BeaconNode *ClientVersionV1 `json:"beacon_node"` | ||
| ExecutionClient []*ClientVersionV1 `json:"execution_client"` |
There was a problem hiding this comment.
fyi, we've decided to only return the first value again and not an array
Inspector-Butters
force-pushed
the
GetVersionV2
branch
from
5f7060b to
fea9930
Compare
|
|
||
| var elData *structs.ClientVersionV1 | ||
| elDataList, err := s.ExecutionEngineCaller.GetClientVersionV1(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
should this be testing len of elDatalist being at least 1 just in case?
There was a problem hiding this comment.
I can add that, but I made sure that the function being called will only return the list if the length is >0
3 participants
What does this PR do? Why is it needed?
This PR adds the implementation for
GetVersionV2based on ethereum/beacon-APIs#568.Q: is there anything to be done for deprecating
GetVersionV1?