-
Notifications
You must be signed in to change notification settings - Fork 9
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
Allow extra fields in tuple consensus deserialization #307
Comments
I will have a try at this, it looks fun. |
Since this task is taking a lot of time, I will do a write-up about it. This task is way bigger than adding just a "extra fields", because we also need to add those capabilities at the same time:
To fix this, I went with the deserialization needing a bitset so that we know that we have all fields and they don't repeat each other. For the fields in any order, we just have locals to handle all the values needed for the all the tuples fields, and there is a "switch-case-like" structure to know which ones to fill depending on the parsed clarity-name key. This was the easy part. Now the problem is the extra fields. Those are not available in the expression type, so we have to be ready to parse for anything:
Validating the name is easy, the original regex that validates the names is easy to translate to Wat. For the second part, I went with a simple recursive algorithm that goes through the bytes and depending on the value of the first byte, skip the adequate number of following bytes. It's recursive due to the fact that we have composite types such as lists or tuples. Here is now a new issue: let's imagine that we have an extra field whose type is a list of tuples. Since it is an extra field, we do not know its type. Also, due to clarity restrictions, the list must be homogeneous. However, the serialization allows extra fields for the tuples. How should we make sure that the tuples all are of the same type if we are allowed to not consider all the fields? |
Ah, I didn't realize this was going to be so complex. We do have the option of just changing this behavior beginning in Clarity 3, though that would prevent us from using the clarity-wasm runtime to boot from genesis.
I didn't realize that this was allowed 😮
This should have already been the case, no? I'm not sure that I follow exactly what you are asking. In the serialized buffer, we do have the type. It is encoded as part of the serialization. Could you maybe expand and provide an example? |
Me neither at first, and this is so annoying! But this is solved.
Yup, but to achieve this, we relied on the fact that the serialized fields would arrive in the alphabetical order of the keys. But anyway, this is solved.
Yeah sorry, I should have put an example directly. When you use
List being homegenous, we should check that each elements are of the same type that we don't know. This is a bit difficult, but doable.
But now, what about lists of tuples? But now, let's imagine we have Is this behavior documented anywhere? What should we do in this case? Should we ignore |
No, I don't think it is. In this case, I believe the type is decided by the first element in the list. This is true even outside of [de]serialization. For example, Again, we can also choose to disallow these extra fields in Clarity 3. |
I do not have issues with extra fields myself, and the feature should be done in a few days. But please keep me posted if it's removed later. UPDATE: no actually I lie, I have an issue: I think this feature is extremely difficult to test. |
To match interpreter behavior,
from-consensus-buff?
needs to allow unused fields in tuples. For example, the consensus serialization of{n: 42, extra: u32}
is0x0c000000020565787472610100000000000000000000000000000020016e000000000000000000000000000000002a
.We would expect the following expression to return
none
, since it contains extra fields, but it needs to instead return(some {n: 42})
.The text was updated successfully, but these errors were encountered: