-
Notifications
You must be signed in to change notification settings - Fork 22
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
Root and SignedRoot #58
Conversation
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.
looks good! we should update the name to signing_root
and the rest of my comments were minor formatting...
seems like the root
is a good compromise to avoid the circularity of calling hash_tree_root
on itself
ssz/sedes/signed_serializable.py
Outdated
class SignedSerializable(BaseSerializable, metaclass=MetaSignedSerializable): | ||
|
||
@property | ||
def signed_root(self): |
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.
this is now called signing_root
:)
ssz/sedes/serializable.py
Outdated
field_attrs = None | ||
container_sedes = None | ||
class Meta(NamedTuple): | ||
|
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.
minor but i feel like we usually have no whitespace b/t class decl and class attrs
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 don't like how it looks, but I do like conventions, so I removed the new lines for all classes I could find.
|
||
|
||
class SignedMeta(NamedTuple): | ||
has_fields: bool |
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.
example about formatting here
ssz/sedes/signed_serializable.py
Outdated
|
||
|
||
class MetaSignedSerializable(MetaSerializable): | ||
|
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.
whitespace? i'll let you decide if you want to patch these up, not blocking merging imo
What was wrong?
#44
How was it fixed?
root
toBaseSerializable
SignedSerializable
that provides the propertysigned_root
(builds on top of #57)
Cute Animal Picture