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

Tidying up a few core functions #6995

Merged
merged 52 commits into from
Aug 20, 2020
Merged

Tidying up a few core functions #6995

merged 52 commits into from
Aug 20, 2020

Conversation

terencechain
Copy link
Member

@terencechain terencechain commented Aug 13, 2020

What type of PR is this?

Feature

What does this PR do? Why is it needed?
Further clean ups for #6986 and #6983. There's no functionality changes. Just deletion and renaming:

  • Removed ProcessVoluntaryExitsNoVerify. It was only used for tests
  • Renamed ProcessVoluntaryExitsNoVerify to ProcessVoluntaryExitsNoVerifySignature
  • Renamed ProcessAttestationNoVerify to ProcessAttestationNoVerifySignature
  • Renamed VerifyAttestation to VerifyAttestationSignature. It makes the following b2b code more clear

Screen Shot 2020-08-13 at 3 46 41 PM

Which issues(s) does this PR fix?

Fixes #

Other notes for review
CC @nisdas

@terencechain terencechain added Ready For Review A pull request ready for code review Cleanup Code health! labels Aug 13, 2020
@terencechain terencechain requested a review from a team as a code owner August 13, 2020 22:47
@terencechain terencechain self-assigned this Aug 13, 2020
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #6995 into master will increase coverage by 2.34%.
The diff coverage is 72.39%.

@@            Coverage Diff             @@
##           master    #6995      +/-   ##
==========================================
+ Coverage   60.07%   62.41%   +2.34%     
==========================================
  Files         323      402      +79     
  Lines       27422    31895    +4473     
==========================================
+ Hits        16473    19907    +3434     
- Misses       8733     9185     +452     
- Partials     2216     2803     +587     

Copy link
Member

@nisdas nisdas left a comment

Choose a reason for hiding this comment

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

I think we need to keep the voluntary exits(no verify), it is used for fuzzing that method. Otherwise with BLS the fuzzer will constantly fail.

@terencechain
Copy link
Member Author

terencechain commented Aug 14, 2020

I think we need to keep the voluntary exits(no verify), it is used for fuzzing that method. Otherwise with BLS the fuzzer will constantly fail.

I dont think we still use beacon fuzz do we? We switched to cluster fuzz.

cc @prestonvanloon to confirm

nisdas
nisdas previously approved these changes Aug 14, 2020
@prylabs-bulldozer prylabs-bulldozer bot merged commit 55074bc into master Aug 20, 2020
@farazdagi farazdagi deleted the rm-no-verify branch August 21, 2020 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Code health! Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants