-
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
Use Block Interface Across Prysm #8918
Merged
Merged
Changes from all commits
Commits
Show all changes
53 commits
Select commit
Hold shift + click to select a range
b4e8c2b
commit initial work
nisdas ee2e4af
checkpoint current work
nisdas 4d82c1a
gaz
nisdas 6872b20
checkpoint
nisdas 2fd5e66
req/resp changes
nisdas e0b54c7
initial-sync
nisdas 1ff779a
finally works
nisdas b32d16e
fix error
nisdas 96204c7
fix bugs
nisdas 7b8aecc
fix issue
nisdas 588774b
fix issues
nisdas 46be72e
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas 38a237d
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas ee33eea
fix refs
nisdas 4e60370
tests
nisdas f890977
more text fixes
nisdas 0d12504
more text fixes
nisdas 237b785
more text fixes
nisdas db29b95
fix tests
nisdas 5e234a1
fix tests
nisdas f058514
tests
nisdas 78cd401
finally fix builds
nisdas bd6dcce
finally
nisdas cf97730
comments
nisdas b021a29
Merge branch 'develop' into blockInterface
nisdas c2919f4
fix fuzz
nisdas 6d9bee3
share common library
nisdas 28cd19c
fix
nisdas 19ee4d4
Merge branch 'develop' of https://github.com/prysmaticlabs/geth-shard…
nisdas 27a91a1
fix
nisdas 0c6ad93
add in more defensive nil checks
nisdas fc3e41b
add in more defensive nil checks
nisdas 951192c
imports
nisdas 5a706f8
Apply suggestions from code review
nisdas c550d68
Apply suggestions from code review
nisdas 88b693d
Update shared/interfaces/block_interface.go
nisdas eec67cc
Update shared/interfaces/block_wrapper.go
nisdas cb6dd0b
Update shared/interfaces/block_interface.go
nisdas 07ac83e
imports
nisdas 19fb5b0
fix bad changes
nisdas c267677
fix
nisdas cda2757
Merge branch 'blockInterface' of https://github.com/prysmaticlabs/get…
nisdas eb05917
terence's review
nisdas 2792b56
terence's review
nisdas 778c6ca
Merge branch 'develop' into blockInterface
nisdas a4a684f
fmt
nisdas a259b9d
Merge branch 'blockInterface' of https://github.com/prysmaticlabs/get…
nisdas 8c54db1
Update beacon-chain/rpc/beacon/blocks.go
nisdas 86a3b0e
fix tests
nisdas 33d3033
Merge branch 'blockInterface' of https://github.com/prysmaticlabs/get…
nisdas b380223
fix
nisdas 0798bfe
fix all tests
nisdas ff3b8a2
Merge branch 'develop' into blockInterface
nisdas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
How about this change?
It allows to write a single
IsNil()
check, and I am afraid most of the time the second check will be forgotten. You also never verify ifw.Block() == nil
in this PR (I don't either in my suggestions because this was an afterthought) and you already missed severalw.Block().IsNil()
checks. I'm sure this will happen more often. The downside is that we couple both checks together. Would we ever not care if the underlying beacon block is nil or not though?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.
The goal was to not add in additional nil block checks unless that particular function requires it. I have tried to strictly mimick existing behaviour and minimize any additional logic changes added into an already large PR. I just wanted to replace what were existing nil checks for a block struct with an interface. Adding in an inner block check would differ from what is the current behaviour (
if signedBlock == nil
) . We can add it in a follow up PR in that case, it would only be a one line change for both theSignedBeaconBlock
andBeaconBlock
.