Skip to content

Conversation

@maxtropets
Copy link
Collaborator

@maxtropets maxtropets commented Jan 5, 2026

One more step towards #7542

❗ More of an incremental improvement rather than a finalised version, therefore no fancy docstrings/documentation, etc. It will come once we throw QCBOR away and are fully settled on these wrappers' interface.

Changelist

  • Error handling and context business simplified (are they?)
    • look as ugly as before
    • but are more straightforward and avoid duplicating context message, by stacking up getting/parsing flexibly.
  • Integer keys comparison changed
    • we allow equality of signed(42) and unsigned(42) now
    • Why? it seems to be the way CBOR libraries treat keys, and is slightly more flexible
    • UPD. Not true anymore, discussed here: Better CBOR wrappers #7549 (comment)
  • Some APIs renamed to make more sense
  • No \n at the end when to_string()ing
  • SimpleValue enum type for commonly used types
  • Test coverage (lots of LLM work, thoroughly (sort of) read by me, with lots of human editing sprinkled on top)

✅ Long Test

@maxtropets maxtropets self-assigned this Jan 5, 2026
@maxtropets maxtropets force-pushed the f/cbor-better-api branch 2 times, most recently from ec187c0 to 771af29 Compare January 6, 2026 16:25
@maxtropets maxtropets force-pushed the f/cbor-better-api branch 2 times, most recently from f3ab01c to e9a6c40 Compare January 7, 2026 10:58
@maxtropets maxtropets changed the title [Work-in-progress] Better CBOR wrappers Better CBOR wrappers Jan 7, 2026
@maxtropets maxtropets added the run-long-test Run Long Test job label Jan 7, 2026
@maxtropets maxtropets marked this pull request as ready for review January 7, 2026 11:11
@maxtropets maxtropets requested a review from a team as a code owner January 7, 2026 11:11
Copilot AI review requested due to automatic review settings January 7, 2026 11:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the CBOR wrapper API to simplify error handling and improve usability. The changes remove context parameters from accessor methods in favor of a new rethrow_with_context helper function, allow signed/unsigned integer key equivalence in maps, rename functions for clarity (e.g., parse_valueparse, print_valueto_string), and add comprehensive test coverage.

Key Changes

  • Error handling simplified with rethrow_with_context helper that allows flexible context stacking without duplicating messages
  • Integer key comparison now treats signed(42) and unsigned(42) as equal in map lookups
  • API functions renamed for clarity and consistency (parse, to_string instead of parse_value, print_value)
  • Added SimpleValue enum for common CBOR simple values (False, True, Null)
  • Added extensive test suite with 70+ test cases covering various CBOR data types

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/crypto/cbor.h Adds SimpleValue enum, removes context parameters from method signatures, adds rethrow_with_context helper, renames parse_value/print_value to parse/to_string, adds make_bytes helper
src/crypto/cbor.cpp Removes with_context helper, implements signed_equals_unsigned for map key comparison, adds signed/unsigned cross-conversion in as_signed/as_unsigned, updates to_string to remove trailing newline, improves bytes/simple value printing
src/node/uvm_endorsements.cpp Updates to use new CBOR API with rethrow_with_context pattern, removes context parameters from accessor calls, changes to use parse instead of parse_value, uses new PARAM_CONTENT_TYPE_HASH_ENVELOPE constant
src/node/cose_common.h Adds PARAM_CONTENT_TYPE_HASH_ENVELOPE constant (value 259) for COSE hash envelope content type
src/crypto/test/cbor.cpp New comprehensive test file with 70+ test cases covering integers, strings, bytes, simple values, tags, arrays, maps, and error conditions
CMakeLists.txt Adds cbor_test unit test target

@maxtropets maxtropets removed the run-long-test Run Long Test job label Jan 8, 2026
@maxtropets maxtropets merged commit 900a5ce into microsoft:main Jan 9, 2026
16 of 17 checks passed
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.

3 participants