-
Notifications
You must be signed in to change notification settings - Fork 44
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
Binary Protocol + Protocol Organization (step 1) #92
Conversation
@djrtwo I'll check this out and merge it tonight! |
This would be _swell_!
…On Tuesday, October 31, 2017, Nate Rush ***@***.***> wrote:
@djrtwo <https://github.com/djrtwo> I'll check this out and merge it
tonight!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#92 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABXf-8jsHUtUETddY62xhGjjlb1770T_ks5sx895gaJpZM4QMKul>
.
|
Starting review now. |
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.
Checked this our briefly. Gotta hop on a train soon and will be able to review more thoroughly.
Looks much cleaner than before. This is great!
casper/binary/binary_view.py
Outdated
class BinaryView(AbstractView): | ||
"""A view class that also keeps track of a last_finalized_block and children""" | ||
def __init__(self, messages=None): | ||
super().__init__() |
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.
Don't add messages here through super, but down in BlockchainView. We should pick one.
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.
I say do super.
You have to define add_messages
in child, but the parent will always do the "if none, and add_message" call
self.validator_set = validator_set | ||
self.global_view = view_class() | ||
self.global_view = protocol.View(set()) |
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.
pretty :)
This looks good. Merging now. |
I think we should get it merged in as is for now. Will allow Vlad to generate some binary gifs if he wants.
Stuff:
--protocol
(defaults to blockchain) and must specify when making an experiment ("protocol")NOTE: This is pointed at master. This includes your binary stuff but I already merged in master to here and handled a bunch of conflicts. I recommend merging this into master when ready and closing the other Binary PR.