Add GET /eth/v2/beacon/pool/attester_slashings
#14479
Conversation
saolyn
changed the title
/eth/v2/beacon/pool/attester_slashings/eth/v2/beacon/pool/attester_slashings
| ss := make([]*eth.AttesterSlashingElectra, len(sourceSlashings)) | ||
| for i, slashing := range sourceSlashings { | ||
| a, ok := slashing.(*eth.AttesterSlashingElectra) | ||
| if ok { |
There was a problem hiding this comment.
you could do the reverse here and avoid the else
| @@ -172,6 +172,11 @@ type GetAttesterSlashingsResponse struct { | |||
| Data []*AttesterSlashing `json:"data"` | |||
| } | |||
|
|
|||
| type GetAttesterSlashingsV2Response struct { | |||
| Version string `json:"version"` | |||
| Data []json.RawMessage `json:"data"` // Accepts both `[]*AttesterSlashing` and `[]*AttesterSlashingElectra` types | |||
There was a problem hiding this comment.
Single json.RawMessage will make the code more compact
| } | ||
| v := version.String(headState.Version()) |
There was a problem hiding this comment.
The version should be based on the version of the slashing, not the state. This is because (1) the only possible slashing versions are Phase0 and Electra, and (2) the state here can be after a fork with the slashing being before the fork.
| @@ -169,7 +169,8 @@ type BLSToExecutionChangesPoolResponse struct { | |||
| } | |||
|
|
|||
| type GetAttesterSlashingsResponse struct { | |||
| Data []*AttesterSlashing `json:"data"` | |||
| Version string `json:"version"` | |||
There was a problem hiding this comment.
In https://github.com/prysmaticlabs/prysm/pull/14481/files#diff-b7de0123b995294eb1e799afb080f62c9a4a08195f9ed1f387a9071922b1ec97 the Version field has the omitifempty tag. I believe this is required for v1 to work correctly?
There was a problem hiding this comment.
It does seem to work correctly but for the sake of accuracy we should definitely add the field
| attStruct := structs.AttesterSlashingFromConsensus(a) | ||
| attStructs = append(attStructs, attStruct) | ||
| } | ||
| resp.Version = version.String(slashing.Version()) |
There was a problem hiding this comment.
It doesn't make sense to rewrite this value in a loop. The effect of this will be that the version will be taken from the last slashing. How to correctly treat different types of slashings post-Electra is still something that I have to think about, but for now let's use the current slot to determine the version.
There was a problem hiding this comment.
Then it's probably fine for now to add the version using the first slashing:
sourceSlashings[0].Version()
| require.DeepEqual(t, slashing1, ss[0]) | ||
| require.DeepEqual(t, slashing2, ss[1]) | ||
| }) | ||
| t.Run("V2", func(t *testing.T) { |
There was a problem hiding this comment.
can you add a pre-Electra v2 test case?
What type of PR is this?
Other
What does this PR do? Why is it needed?
Beacon API Electra alignment, add missing endpoint for
/eth/v2/beacon/pool/attester_slashings.As described in the spec https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Beacon/getPoolAttesterSlashingsV2
Which issues(s) does this PR fix?
Part of #14476
Other notes for review
Acknowledgements