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 ListPoolProposerSlashings and ListPoolVoluntaryExits in the beacon API #8508

Merged
merged 6 commits into from
Feb 24, 2021

Conversation

rkapka
Copy link
Contributor

@rkapka rkapka commented Feb 24, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR implements beacon API's ListPoolProposerSlashings and ListPoolVoluntaryExits methods according to the spec: https://ethereum.github.io/eth2.0-APIs/#/Beacon

Which issues(s) does this PR fix?

Part of #7510

Other notes for review

Very similar to #8492

@rkapka rkapka requested a review from a team as a code owner February 24, 2021 09:40
@codecov
Copy link

codecov bot commented Feb 24, 2021

Codecov Report

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

@@            Coverage Diff             @@
##             develop    #8508   +/-   ##
==========================================
  Coverage           ?   58.14%           
==========================================
  Files              ?      458           
  Lines              ?    32813           
  Branches           ?        0           
==========================================
  Hits               ?    19080           
  Misses             ?    10839           
  Partials           ?     2894           

if err != nil {
return nil, status.Errorf(codes.Internal, "Could not get head state: %v", err)
}
sourceSlashings := bs.SlashingsPool.PendingProposerSlashings(ctx, headState, true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sourceSlashings := bs.SlashingsPool.PendingProposerSlashings(ctx, headState, true)
sourceSlashings := bs.SlashingsPool.PendingProposerSlashings(ctx, headState, true /* unlimited return slashings */)

return nil, status.Errorf(codes.Internal, "Could not get head state: %v", err)
}

sourceExits := bs.VoluntaryExitsPool.PendingExits(headState, headState.Slot(), true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. A comment after true will be nice

@@ -93,3 +93,45 @@ func V1Alpha1AttSlashingToV1(v1alpha1Slashing *ethpb_alpha.AttesterSlashing) *et
Attestation_2: V1Alpha1IndexedAttToV1(v1alpha1Slashing.Attestation_2),
}
}

Copy link
Member

@terencechain terencechain Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some tests for the new functions below this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the migration package doesn't have any tests... Would it be OK if I create a separate PR with tests for the whole package?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 👍

@prylabs-bulldozer prylabs-bulldozer bot merged commit 4da7a77 into develop Feb 24, 2021
@delete-merged-branch delete-merged-branch bot deleted the api-get-pool-data branch February 24, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants