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

Ciborium and Cleanups #2

Merged
merged 9 commits into from
Mar 16, 2023
Merged

Ciborium and Cleanups #2

merged 9 commits into from
Mar 16, 2023

Conversation

jcape
Copy link
Contributor

@jcape jcape commented Mar 11, 2023

  • Port this crate from cert_cbor to ciborium.
  • Move Jsonu64 module to its own file

Motivation

serde_cbor has been unmaintained forever now, we've avoided switching to ciborium because it would not support deserialization to a borrowed string, which serde_cbor did. However, I'd rather pass cargo deny advisories than continue with this.

We can fix any issues arising from this when we make mobilecoin.git use this crate.

@jcape jcape requested a review from cbeck88 as a code owner March 11, 2023 01:07
@jcape
Copy link
Contributor Author

jcape commented Mar 11, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions github-actions bot added the size/L Large PRs label Mar 11, 2023
@meowblecoinbot meowblecoinbot requested review from a team, nick-mobilecoin and varsha888 and removed request for a team March 11, 2023 01:08
@github-actions github-actions bot added the rust Pull requests that update rust code or dependencies label Mar 11, 2023
@github-actions
Copy link

github-actions bot commented Mar 11, 2023

❌ Unreviewed dependencies found

Crate Version Reviews (N/2) LoC Left-Pad Index Geiger Flags

@codecov
Copy link

codecov bot commented Mar 11, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@9203c21). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main       #2   +/-   ##
=======================================
  Coverage        ?   83.08%           
=======================================
  Files           ?        1           
  Lines           ?      136           
  Branches        ?        0           
=======================================
  Hits            ?      113           
  Misses          ?       23           
  Partials        ?        0           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jcape jcape enabled auto-merge (squash) March 11, 2023 01:32
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Chris Beck <[email protected]>
@cbeck88
Copy link

cbeck88 commented Mar 12, 2023

this issue slightly worries me: enarx/ciborium#51

@cbeck88
Copy link

cbeck88 commented Mar 13, 2023

Can we try adding some more tests here, like nested structs, and enums? The test coverage on this seems pretty light compared to the variety of ways in which we use it

@nick-mobilecoin
Copy link

nick-mobilecoin commented Mar 13, 2023

this issue slightly worries me: enarx/ciborium#51

Trying to use the bytes they provide in that issue results in Err(Semantic(None, "missing field pubkey"))

    #[derive(PartialEq, Serialize, Deserialize, Debug, Clone)]
    struct S {
        pubkey: [u8; 32]
    }

    #[test]
    fn try_51() {
        extern crate std;
        let s = S {
            pubkey: [189, 182, 166, 93, 174, 37, 6, 247, 39, 203, 228, 101, 121, 197, 203, 42, 98, 138, 145, 12, 12, 76, 145, 168, 132, 185, 90, 18, 54, 28, 248, 170],
        };
        let serialized = serialize(&s).unwrap();

        // Data from https://github.com/enarx/ciborium/issues/51
        // Uncomment the lines to see the error
        // let data = vec![161, 102, 112, 117, 98, 75, 101, 121, 88, 32, 189, 182, 166, 93, 174, 37, 6, 247, 39, 203, 228, 101, 121, 197, 203, 42, 98, 138, 145, 12, 12, 76, 145, 168, 132, 185, 90, 18, 54, 28, 248, 170];
        // assert_eq!(format!("{r:?}"), "sushi");
        let data = vec![161, 102, 112, 117, 98, 107, 101, 121, 152, 32, 24, 189, 24, 182, 24, 166, 24, 93, 24, 174, 24, 37, 6, 24, 247, 24, 39, 24, 203, 24, 228, 24, 101, 24, 121, 24, 197, 24, 203, 24, 42, 24, 98, 24, 138, 24, 145, 12, 12, 24, 76, 24, 145, 24, 168, 24, 132, 24, 185, 24, 90, 18, 24, 54, 24, 28, 24, 248, 24, 170];
        assert_eq!(serialized, data);
        let r = ciborium::de::from_reader::<S, _>(&data[..]);
        assert_eq!(r.is_ok(), true);
        assert_eq!(r.unwrap(), s);
    }

Copy link

@cbeck88 cbeck88 left a comment

Choose a reason for hiding this comment

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

i'll approve with some more unit test coverage

@jcape jcape requested a review from nick-mobilecoin March 14, 2023 18:52
@jcape
Copy link
Contributor Author

jcape commented Mar 14, 2023

@nick-mobilecoin This is ready to go if you could re-review (for changes).

@jcape jcape merged commit 40d820f into main Mar 16, 2023
@jcape jcape deleted the jcape/ciborium branch March 16, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust Pull requests that update rust code or dependencies size/L Large PRs
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants