-
Notifications
You must be signed in to change notification settings - Fork 997
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
Make lightclient
patch pass the linter
#2133
Merged
Merged
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
a4c6c47
Make `lightclient` an executable patch fork
hwwhww da0e33c
Merge branch 'vbuterin-patch-2' into executable-lightclient-spec
hwwhww a33f08f
fix conflicts
hwwhww 59bc3b2
Fix ToC
hwwhww a7e6904
Lightclient -> Light client
hwwhww 94832ea
Try protolambda/remerkleable#8
hwwhww 13edfd7
Merge branch 'vbuterin-patch-2' into executable-lightclient-spec
hwwhww 04969cf
Fix sync-protocol.md ToC
hwwhww 030f611
Build lightclient/sync-protocol
hwwhww 1fe28f2
Fix typo
hwwhww 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
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.
class BeaconBlockBody(phase0.BeaconBlockBody):
is so much cleaner than repeating all the properties!@protolambda: Can we do some Python magic here?
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.
With python we can do a lot, but the question is if we should. Being implicit would mean we need to spec appending of fields. And then sometimes appending is not the approach we want. Or functionality is not sequential, and we remove a field.
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 reason why I did the inheriting from phase0 in the original PR instead of repeating properties is not just to cut down the number of lines of code, it's also to make the patch to the spec truly not sequential (ie. if it's implemented after sharding it could still be included as is). This would make it easier for us to work on light client stuff, the merge and sharding in parallel. So it would be really nice if we can make the inheritance work!
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.
That sounds great, but then do what we want some diamond structure? I.e. phase 0 type inherited by various phase 1 types, and then recombined in the complete phase 1 type? This becomes "the diamond problem" once we start overriding things in different types, which we then need to reconsolidate.
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.
Well, I don't support the concept of "phases" anymore, so... 😄
Realistically, once some patch gets implemented, we could just quickly rewrite the other patches to point to the previous patch as a base (eg.
s/phase0./lightclient_fork./g
), and when writing each individual patch make sure that it is valid under any combination of the other patches.This does of course assume that the absolute number of patches that are treated this way (as opposed to just being PRs to existing spec as all the beacon chain changes pre-launch were) needs to be very small.
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.
Ok, implemented it and opened a PR to my SSZ library that we use here in the spec, with support for all this (inheritance, mixins, diamonds): protolambda/remerkleable#8
Please give it a review, and let's discuss on next spec call. It seems small, but it's quite a step away from bare struct definitions.