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

Clarify SSZ default values #1346

Merged

Conversation

lightclient
Copy link
Member

I initially set out to clarify the default value of Vector, but felt like this definition would be much clearer in a table format versus a sentence.

specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Looks good, but could use a few minor changes. Also, this PR would be nice to merge into v.0.8.3, so I would change the target base before merging.

specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
@protolambda
Copy link
Collaborator

protolambda commented Aug 7, 2019

@c-o-l-o-r Can you please rebase this PR to branch v08x, and change the PR target to that too? It will get into a release quicker that way, we generally like cosmetic changes to not get delayed to a major release for no reason :).

@protolambda protolambda added general:presentation Presentation (as opposed to content) scope:SSZ Simple Serialize labels Aug 7, 2019
@lightclient lightclient force-pushed the clarify-vector-initialization branch from f37001b to 7159e40 Compare August 7, 2019 20:59
@lightclient lightclient changed the base branch from dev to v08x August 7, 2019 21:00
@lightclient lightclient force-pushed the clarify-vector-initialization branch from 7159e40 to 97347ff Compare August 7, 2019 21:04
@lightclient
Copy link
Member Author

Thanks @protolambda! I just rebased against v08x.

specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
@lightclient lightclient force-pushed the clarify-vector-initialization branch from 9586d19 to b347cf1 Compare August 8, 2019 13:34
@protolambda
Copy link
Collaborator

Also, we generally sign commits in the specs repository @c-o-l-o-r (with the exception for commits signed through github, i.e. using the web-interface for merges).
It would be nice if you can squash the commits (since it's a single table) and then sign it.
I understand that it's all a bit much for this PR, but future PRs will be much easier when that's familiar.

@lightclient
Copy link
Member Author

@protolambda no worries! Sorry for so much back-and-forth -- I will squash into one signed commit!

@lightclient lightclient force-pushed the clarify-vector-initialization branch from b347cf1 to 5523a45 Compare August 8, 2019 13:55
@lightclient lightclient force-pushed the clarify-vector-initialization branch from 5523a45 to 3358d70 Compare August 8, 2019 13:59
@lightclient
Copy link
Member Author

Okay great, I have signed my commit. 😄 Let me know if there is anything else I should do!

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

Looks good. Not entirely sure about the syntax for Union: type_0 or Union(type_0). But that's something to revisit when unions are actually used in practice, the idea is there.

@protolambda protolambda merged commit 3b2b688 into ethereum:v08x Aug 8, 2019
@lightclient lightclient deleted the clarify-vector-initialization branch August 8, 2019 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:presentation Presentation (as opposed to content) scope:SSZ Simple Serialize
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants