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

Canonicalization rfc8949 and rfc7049 implementation for structs #143

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

hoxxep
Copy link
Contributor

@hoxxep hoxxep commented Nov 26, 2024

This is PR a WIP/example regarding rfc8949 4.2.1 canonicalisation. Related issue is: #144

The aim is to ensure all structs or collection types output with rfc8949 canonicalisation. It would also be fairly easy for me to add rfc7049 canonicalisation too, and change the canonical argument to an enum.

I believe the existing implementation does not output canonical CBOR for structs, while this implementation introduces this as an option.

I'm happy to clean up and finish off this MR, if you're open to including this work?

@hoxxep hoxxep requested a review from a team as a code owner November 26, 2024 18:30
@hoxxep hoxxep requested a review from dpal November 26, 2024 18:30
ciborium/src/ser/mod.rs Outdated Show resolved Hide resolved
ciborium/src/ser/mod.rs Outdated Show resolved Hide resolved
ciborium/src/ser/mod.rs Outdated Show resolved Hide resolved
@hoxxep hoxxep changed the title Example bodged rfc8949 implementation Canonicalization rfc8949 and rfc7049 implementation support for structs Nov 26, 2024
@hoxxep hoxxep changed the title Canonicalization rfc8949 and rfc7049 implementation support for structs Canonicalization rfc8949 and rfc7049 implementation for structs Nov 26, 2024
ciborium/src/ser/mod.rs Outdated Show resolved Hide resolved
@hoxxep hoxxep force-pushed the rfc8949 branch 2 times, most recently from 00913bd to c5485b2 Compare November 27, 2024 14:30
Signed-off-by: Liam Gray <hoxxep@gmail.com>
Signed-off-by: Liam Gray <hoxxep@gmail.com>
@hoxxep
Copy link
Contributor Author

hoxxep commented Nov 27, 2024

This PR includes what I believe is working support for both forms of CBOR canonicalisation (RFC 7049 and RFC 8949). It should be implemented in a fairly clean way, and hides the modifications behind a canonical feature flag. I'll take a look at using generics instead, so that we can have the compiler optimise the overhead away and remove the need for the feature flag entirely.

Let me know if you would be interested in this work, before I look to add a number of test cases and clean up any code nits? Thanks!

This also renames into_vec as to_vec and deprecates into_vec, while adding a sane default buffer capacity (benchmarked in my own use-case with various message sizes). This aligns better with both rust naming convention (into consumes the object, to takes a reference) and the serde naming guidelines (which suggest to_type and from_type naming).

Signed-off-by: Liam Gray <hoxxep@gmail.com>
@hoxxep
Copy link
Contributor Author

hoxxep commented Nov 28, 2024

Update: I have replaced the canonical feature flag in favour of type parameters (see the src/canonical.rs). Was equally clean to implement, matches the original ciborium performance, and improves the canonicalized serialization performance too.

I felt it was best to leave the commits for each change visible instead of rebasing against main in case you would like to compare the changes. Happy to take any feedback!

@hoxxep
Copy link
Contributor Author

hoxxep commented Dec 4, 2024

@dpal @rjzak I'm curious if this change is likely to be something that interests ciborium? If you would prefer I implement this as a separate crate outside of ciborium I'd fully understand, since it might be a niche use case. Happy to hear what you think!

@rjzak
Copy link
Member

rjzak commented Dec 7, 2024

@npmccallum thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants