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

Add support for .default #42

Closed
SebastienGllmt opened this issue Jul 20, 2022 · 2 comments · Fixed by #111
Closed

Add support for .default #42

SebastienGllmt opened this issue Jul 20, 2022 · 2 comments · Fixed by #111
Labels
CDDL feature Feature that is required for proper parsing of CDDL files

Comments

@SebastienGllmt
Copy link
Collaborator

SebastienGllmt commented Jul 20, 2022

Currently, when a required field is missing we generate code that looks like this

let key_0 = match key_0 {
    Some(x) => x,
    None => return Err(DeserializeFailure::MandatoryFieldMissing(Key::Uint(0)).into()),
};

I believe for .default, all the code is generated exactly as it is now except we change this error to instead return the default value.

Additionally, on serialization, if the field is equal to the default value we just omit it from the cbor notation

This is used in one place in the byron CDDL spec (although byron.cddl does not mention it)

@SebastienGllmt SebastienGllmt added the CDDL feature Feature that is required for proper parsing of CDDL files label Sep 7, 2022
@SebastienGllmt
Copy link
Collaborator Author

@rooooooooob
Copy link
Collaborator

@SebastienGllmt Note that according to the CDDL RFC:

This control is
only meaningful when the control type is used in an optional context;
otherwise, there would be no way to make use of the default value.

I interpreted this as meaning that the code at the top of this issue should stay the same as that's only for mandatory fields. For optional fields we would use the default value though. The governance cip uses it on an optional field. I can't tell for the byron one since I only have the one that doesn't mention it.

rooooooooob added a commit that referenced this issue Nov 1, 2022
Adds .default support to typenames (`foo = bar .default baz`) and to map
fields (`foo = { ? 1 => bar .default baz }`).

This is not applied into nested contexts e.g.:
```
foo = uint .default 2
bar = { * str => foo }
```
as how this should be handled is extremely ambiguous.
Should this result in an extra encoding variable which is a map e.g.
`default_included: BTreeSet<K>` and having the behavior be to omit it if
it's not included in this encoding var and it's equal to the default ?

As per the CDDL RFC we only apply it to optional fields:
```
This control is
only meaningful when the control type is used in an optional context;
otherwise, there would be no way to make use of the default value.
```

We support encoding preservation for round-tripping i.e. if a default
value is explicitly present it will remain explicitly present. With
`preserve-encodings=false` it will be omitted if the value present is
equal to the default value.

No changes were done with canonical encodings as the RFC doesn't seem to
mention how that should be handled as canonical encodings are specified
in the CBOR RFC not the CDDL one, and `.default` is only present in the
CDDL one. It's that possible we could omit fields equal to the default
if needed but we need to confirm this first.

Optional fields with `.default` are no longer encoded as `Option<T>` but
instead just `T` now which should help with usability. The field will
default to the default via `new()` and deserialization.

Fixes #42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDDL feature Feature that is required for proper parsing of CDDL files
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants