Basic groups inside arrays support. #120
Conversation
e.g.: ``` foo = (uint, text) bar = [ foos: [* foo], ] ``` which if you had `n` foos would be a `2 * n` long CBOR array as there's no wrapping map/array around the basic group `foo`. This can be done in some CDDL specs as a space optimization.
| @@ -1451,20 +1481,39 @@ impl GenerationScope { | |||
| if CLI_ARGS.preserve_encodings { | |||
| deser_code.content | |||
| .line(&format!("let len = {}.array_sz()?;", deserializer_name)) | |||
There was a problem hiding this comment.
Ignore this added mut. If we need to make any other changes to this PR I''ll remove it in that commit, otherwise we can remove it when we fix the other warnings in cddl-codegen.
| @@ -18,6 +18,11 @@ bar = { | |||
|
|
|||
| plain = (d: #6.23(uint), e: tagged_text) | |||
| outer = [a: uint, b: plain, c: "some text"] | |||
| plain_arrays = [ | |||
| ; this is not supported right now. When single-element arrays are supported remove this. | |||
| ; single: [plain], | |||
There was a problem hiding this comment.
we get around this in babbage.cddl by creating another truct like:
single_plain = [plain]
plain_arrays = [
single: single_plain,
...
]
so it's not super high priority to support this, but it would be good if we did when there are less high priority things. I made an issue here: #121
| let orig_bytes = orig.to_bytes(); | ||
| print_cbor_types("orig", &orig_bytes); | ||
| let mut deserializer = Deserializer::from(std::io::Cursor::new(orig_bytes.clone())); | ||
| let deser = T::deserialize(&mut deserializer).unwrap(); | ||
| print_cbor_types("deser", &deser.to_bytes()); | ||
| assert_eq!(orig.to_bytes(), deser.to_bytes()); |
There was a problem hiding this comment.
This check was never failing in this PR, but I just added it in just to be sure. It can't hurt.
| //let array_wrapper_name = element_type.name_as_wasm_array(); | ||
| //types.create_and_register_array_type(element_type, &array_wrapper_name) | ||
| if let ConceptualRustType::Rust(element_ident) = &element_type.conceptual_type { | ||
| types.set_rep_if_plain_group(parent_visitor, element_ident, Representation::Array); |
There was a problem hiding this comment.
This is what we had been doing previously. Ideally we shouldn't assume anything about the encoding for basic(plain) groups (i.e. foo = (uint, uint)). This was done back at the very start (2 1/2 years ago+) as the only plain groups we had to deal with then were things like:
; plain groups defined here
multisig_pubkey = (0, addr_keyhash)
multisig_all = (1, [ * multisig_script ])
multisig_any = (2, [ * multisig_script ])
multisig_n_of_k = (3, n: uint, [ * multisig_script ])
; then we encountered them embedded in an array-encoded group choice
multisig_script =
[ multisig_pubkey
// multisig_all
// multisig_any
// multisig_n_of_k
]
and this made it easier to implement that and let you serialize individual variants of that enum outside of the enum and still end up with a valid multisig_script (which would make more sense to users of CML) even though strictly according to the CDDL if you serialized just a multisig_pubkey you wouldn't get the outer array (which might confuse users of CML despite being truer to the CDDL spec). If we're being totally true to the CDDL this should only be dealt with there and we would get rid of the DeserializeEmbeddedGroup and SerializeEmbeddedGroup traits entirely and have the regular Serialize and Deserialize traits do that behavior. Either that or keep those traits and only implement those traits for plain groups and not implement the direct Serialize/Deserialize traits for them. Unfortunately with preserve-encodings=true this would mean you would have to store the array encoding variable in the enum so you'd end up with so it's possible we want to just keep this how it is. It does, however, make the code surrounding plain groups fiddlier and annoying.
Basic(plain) groups have no encoding naturally, and are only a thing when it's embedded elsewhere, but that should be a detail there, not within the basic group itself. This technically stops us from supporting a plain group being used in both a map and an array, although we've never seen a CDDL definition that does that and it's probably pretty rare.
I didn't make this change here since it would've been a bigger change (and would make the generated code for group choices like the above uglier and possibly surprising to CML users) and this probably isn't high priority.
There was a problem hiding this comment.
Note: If we ever decide to be totally faithful to the CDDL definition regarding plain groups, we could still always rewrite the CDDL in the above as:
multisig_pubkey = [0, addr_keyhash]
multisig_all = [1, [ * multisig_script ]]
multisig_any = [2, [ * multisig_script ]]
multisig_n_of_k = [3, n: uint, [ * multisig_script ]]
multisig_script =
multisig_pubkey
/ multisig_all
/ multisig_any
/ multisig_n_of_k
to get the current behavior. I'm not sure why they were written in the spec as plain groups to be honest. Maybe to make it more obvious that every mutlisig_script variant is an array-encoded group?
|
For context, this was needed to generate the CIP-36 library for catalyst. |
|
Yes, that was the part that we couldn't generate. |
Specifically tests for support for #121 for plain groups. For multi arrays covered by #120 For single ones covered by #210 We are keeping #121 open for now as the case for single non-plain-groups e.g. `[uint]` is not covered. We've yet to see this used anywhere in Cardano so it's low priority to fix.
Specifically tests for support for #121 for plain groups. For multi arrays covered by #120 For single ones covered by #210 We are keeping #121 open for now as the case for single non-plain-groups e.g. `[uint]` is not covered. We've yet to see this used anywhere in Cardano so it's low priority to fix.
e.g.:
which if you had
nfoos would be a2 * nlong CBOR array as there's no wrapping map/array around the basic groupfoo. This can be done in some CDDL specs as a space optimization.