-
Notifications
You must be signed in to change notification settings - Fork 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
Add union type and null to SSZ #893
Conversation
Co-Authored-By: dankrad <[email protected]>
Hi there! I’m just commenting to make a point that in state channels we need to have a single data type to represent the state in the channel and we need it to be of dynamic length. Also we need this to be in solidity early as possible |
This would solve some of the issues in the networking spec, but not all of them. While we can now represent nullable values (which would clean up some of the data structures in the networking spec), it doesn't solve the problem of needing to know an upfront schema to serialize/deserialize SSZ-encoded data. For things like the RPC wrapper, we need a serialization scheme that can model something similar to generics in object-oriented languages. Right now, the easiest way to do this is to not use any serialization scheme for the RPC wrapper and instead just prepend bytes to whatever SSZ-encoded object is being sent. Since the introduction of a |
Would it not be possible to do this using the Union type? If the call can take typs A, B, C or D, you can do this bye defining a union type of Union(A, B, C, D) and defining your RPC call on that. The type index also makes it easy to add more types to the union later without breaking compatibility with the old type. |
This is possible, but unwieldy - there are something like 7 different types that the RPC wrapper can include in the request/response body field. Defining a union type on all of them seems error-prone and difficult to maintain. |
@JustinDrake @vbuterin I am not sure if you think this is a strong enough endorsement to merge this change. I would be in favour of it as it is fairly simple and will not change anything in the consensus layer, but I don't feel very strongly either way. |
Should we discuss this on tomorrow's call? |
Sounds like a good idea |
Don't we need to adjust 'fixed/variable-size' definition? |
I think it is already correct?
Yes.
Hmm, I can see that, but not sure if it would be hugely beneficial to add this special case. I can see it would simplify things if we say that a union of several fixed-size object has the (fixed) size of the max size of those objects. |
|
Ah, yes, I overlooked that sentence. Fixed now. |
Can we alias |
You mean the empty container? That sounds like an interesting idea. |
Yes |
# Conflicts: # specs/simple-serialize.md
Done. |
@@ -97,6 +113,16 @@ fixed_parts = [part if part != None else variable_offsets[i] for i, part in enum | |||
return b"".join(fixed_parts + variable_parts) | |||
``` | |||
|
|||
If `value` is an union type: | |||
|
|||
Define value as an object that has properties `value.value` with the contained value, and `value.type_index` which indexes the type. |
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.
IMO maybe too many "value"s in these sentences and code. Perhaps using value.contained_value
would be more readable?
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.
Shall we use object
instead? (That's what these things are after all, objects.)
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.
You mean value.object
or object.value
@JustinDrake ?
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 was thinking object.value
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'm happy to go with that
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.
Oh but 'object' is a type in python :(
Co-Authored-By: dankrad <[email protected]>
Co-Authored-By: dankrad <[email protected]>
Co-Authored-By: dankrad <[email protected]>
# Conflicts: # specs/simple-serialize.md
This is a suggestion to add an option type and a null type to the SSZ spec. While we will not use it in the spec at the moment, this could be a very simple addition that would make SSZ much more powerful.
In a discussion with @mslipper it transpired that this was one of the main stumbling blocks for making more extensive use of SSZ in the networking. I wonder if this would solve the main problems?