Skip to content
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

Merged
merged 8 commits into from
Feb 23, 2021

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Feb 22, 2021

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/getPoolAttesterSlashings

Which issues(s) does this PR fix?

Part of #7510

Other notes for review

N/A

@rkapka rkapka added the API Api related tasks label Feb 22, 2021
@rkapka rkapka requested a review from a team as a code owner February 22, 2021 12:39

func mapAttestation(attestation *eth.IndexedAttestation) *ethpb.IndexedAttestation {
a := &ethpb.IndexedAttestation{
AttestingIndices: attestation.AttestingIndices,
Copy link
Contributor

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?

Copy link
Member

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
}

Copy link
Contributor Author

@rkapka rkapka Feb 23, 2021

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.

@@ -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 {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

Copy link
Member

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.

Copy link
Contributor

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
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

❗ No coverage uploaded for pull request base (develop@6b82873). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #8492   +/-   ##
==========================================
  Coverage           ?   58.12%           
==========================================
  Files              ?      457           
  Lines              ?    32782           
  Branches           ?        0           
==========================================
  Hits               ?    19055           
  Misses             ?    10835           
  Partials           ?     2892           

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants