-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement ListPoolAttesterSlashings in the beacon API #8492
Conversation
beacon-chain/rpc/beaconv1/pool.go
Outdated
|
||
func mapAttestation(attestation *eth.IndexedAttestation) *ethpb.IndexedAttestation { | ||
a := ðpb.IndexedAttestation{ | ||
AttestingIndices: attestation.AttestingIndices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance there could be a bad attestation that makes it into this and causes the code to panic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
if attestation == nil {
return nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is possible. A bad attestation would never result in a slashing in the first place.
beacon-chain/rpc/beaconv1/pool.go
Outdated
@@ -56,3 +78,24 @@ func (bs *Server) ListPoolVoluntaryExits(ctx context.Context, req *ptypes.Empty) | |||
func (bs *Server) SubmitVoluntaryExit(ctx context.Context, req *ethpb.SignedVoluntaryExit) (*ptypes.Empty, error) { | |||
return nil, errors.New("unimplemented") | |||
} | |||
|
|||
func mapAttestation(attestation *eth.IndexedAttestation) *ethpb.IndexedAttestation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does mapAttestation
mean? Do you just mean copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are different types - *eth
and *ethpb
. It's a mapping from old proto to new proto. I will change the name to make it more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry i missed the prefix. This also seems useful, perhaps a better file location in pool
or even shared
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a temporary translation between the v1alpha1 and v1 objects. A comment would be helpful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #7525 I made a migration package for utils like this, maybe that would be useful here?
Codecov Report
@@ Coverage Diff @@
## develop #8492 +/- ##
==========================================
Coverage ? 58.12%
==========================================
Files ? 457
Lines ? 32782
Branches ? 0
==========================================
Hits ? 19055
Misses ? 10835
Partials ? 2892 |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR implements beacon API's
ListPoolAttesterSlashings
method according to the spec: https://ethereum.github.io/eth2.0-APIs/#/Beacon/getPoolAttesterSlashingsWhich issues(s) does this PR fix?
Part of #7510
Other notes for review
N/A