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

ETH2 Types: Epoch #8373

Merged
merged 42 commits into from
Feb 9, 2021
Merged

ETH2 Types: Epoch #8373

merged 42 commits into from
Feb 9, 2021

Conversation

farazdagi
Copy link
Contributor

@farazdagi farazdagi commented Feb 1, 2021

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Which issues(s) does this PR fix?

Part of #8205

Other notes for review

@farazdagi farazdagi marked this pull request as ready for review February 5, 2021 22:31
@farazdagi farazdagi requested a review from a team as a code owner February 5, 2021 22:31
terencechain
terencechain previously approved these changes Feb 6, 2021
rauljordan
rauljordan previously approved these changes Feb 7, 2021
Copy link
Contributor

@rauljordan rauljordan left a comment

Choose a reason for hiding this comment

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

I went through every single file carefully and this PR is good to go, did not find anything contentious

@rkapka
Copy link
Contributor

rkapka commented Feb 7, 2021

😱

var epoch uint64

@farazdagi
Copy link
Contributor Author

@rkapka

prysm/endtoend/helpers/epochTimer.go

That's a misnomer, what is there is actually a slot number (I've that as type.Slot in my original custom types PR, and will update that file in the next Slot related PR). Basically, we are just ticking slots, but call them epoch ticker.

@@ -115,6 +117,12 @@ func deepValueEqual(v1, v2 reflect.Value, visited map[visit]bool, depth int) boo
}
return true
case reflect.Uint64:
switch v1.Type().Name() {
case "Epoch":
return v1.Interface().(types.Epoch) == v2.Interface().(types.Epoch)
Copy link
Member

Choose a reason for hiding this comment

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

For deep value assertions to work shouldn't uint64 and types.Epoch be equal ? This makes it only equal if they are of types.Epoch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, if we are comparing epochs, both should be epochs, imo. While underlying type of Epoch is uint64, and their integral values are the same, I believe if sth is epoch, it should be typed correctly, or not be considered as equal.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, the deep equal is only used in our pending queue while comparing the same type, so not a big issue. Its fine in that case.

@farazdagi
Copy link
Contributor Author

Note: Per Nishant's request will sync from genesis and run some validators on top of this branch. If everything works, will merge this PR.

@codecov
Copy link

codecov bot commented Feb 9, 2021

Codecov Report

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

@@            Coverage Diff             @@
##             develop    #8373   +/-   ##
==========================================
  Coverage           ?   58.03%           
==========================================
  Files              ?      454           
  Lines              ?    32212           
  Branches           ?        0           
==========================================
  Hits               ?    18695           
  Misses             ?    10657           
  Partials           ?     2860           

@farazdagi farazdagi requested a review from nisdas February 9, 2021 10:04
@prylabs-bulldozer prylabs-bulldozer bot merged commit a8e501b into develop Feb 9, 2021
@delete-merged-branch delete-merged-branch bot deleted the eth2-types-epoch branch February 9, 2021 10:05
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.

5 participants