Credentials crate & PresentationDefinitionV2 struct definition #18
Conversation
amika-sq
requested review from
andresuribe87,
mistermoe,
nitro-neal,
decentralgabe,
phoebe-lew,
jiyoonie9 and
ethan-tbd
| assert!(serialized_pd.contains("input_descriptors")); | ||
| assert!(serialized_pd.contains("123")); |
There was a problem hiding this comment.
I'm not really convinced that this test fully tests serialization, but it's currently what we're doing in the web5-kt test suite.
We could really use some test vectors here to have a more robust test of this functionality.
There was a problem hiding this comment.
I agree. I think most is already tested in the idempotency test.
| [package] | ||
| name = "credentials" | ||
| version = "0.1.0" | ||
| edition = "2021" |
There was a problem hiding this comment.
It's editions. More info here:
#17 (comment)
| assert!(serialized_pd.contains("input_descriptors")); | ||
| assert!(serialized_pd.contains("123")); |
There was a problem hiding this comment.
I agree. I think most is already tested in the idempotency test.
| let pd_string = load_json("tests/resources/pd_sanctions.json"); | ||
| let deserialized_pd: PresentationDefinitionV2 = serde_json::from_str(&pd_string).unwrap(); | ||
|
|
||
| assert_eq!(deserialized_pd.id, expected_id); | ||
| assert_eq!(deserialized_pd.format, Some(expected_format)); | ||
| assert_eq!( | ||
| deserialized_pd.input_descriptors, | ||
| expected_input_descriptors | ||
| ); |
There was a problem hiding this comment.
I think this is the most powerful test. In kt, it checks that JSON is equal (see serialize / deserialize idempotency). Would it make sense to compare the the actual JSON outputs from the pre-serialization JSON vs the serialized and then deserialized JSON?
There was a problem hiding this comment.
I couldn't find a canonicalizer written in Rust that behaves the same way as the Kotlin test. Whitespace differences caused issues when comparing the json strings.
That said, I did vastly improve the deserialize test to ensure that the JSON is parsed into the expected format here, whereas Kotlin is just testing that it doesn't throw when deserializing. It should be testing the same thing.
There was a problem hiding this comment.
Curious if you took a look into https://docs.rs/serde_jcs/latest/serde_jcs/ ?
There was a problem hiding this comment.
Yes. Multiple times. Works well, but I couldn't get it to match white space between json of just the string vs deserialized jcs. I don't know why yet. In all honesty, I don't think it's worth fretting over yet. I'll make a task in the backlog to figure it out.
There was a problem hiding this comment.
Sounds good! I'd love to take a look at that as a first stab at rust!
| /// See [Presentation Definition](https://identity.foundation/presentation-exchange/#presentation-definition) | ||
| /// for more information. | ||
| #[skip_serializing_none] | ||
| #[derive(Debug, Default, Deserialize, PartialEq, Serialize)] |
There was a problem hiding this comment.
Curious why the Debug, Default, PartialEq are needed?
There was a problem hiding this comment.
derive is generating additional code at compile time for this struct:
Debugso I can print out the values and get a debug description of the struct & all of its fieldsDefaultso I can create this struct with pre-populated default values (used in tests)PartialEqso I can use the==operator in tests
| /// can use to articulate proof requirements, and a Presentation Submission data format Holders can | ||
| /// use to describe proofs submitted in accordance with them. | ||
| /// | ||
| /// See [Presentation Definition](https://identity.foundation/presentation-exchange/#presentation-definition) |
There was a problem hiding this comment.
I appreciate the links to spec docs 👍
There was a problem hiding this comment.
| /// See [Presentation Definition](https://identity.foundation/presentation-exchange/#presentation-definition) | |
| /// See [Presentation Definition](https://identity.foundation/presentation-exchange/spec/v2.0.0/#presentation-definition) |
| /// | ||
| /// See [Presentation Definition](https://identity.foundation/presentation-exchange/#presentation-definition) | ||
| /// for more information. | ||
| #[skip_serializing_none] |
There was a problem hiding this comment.
So what's the intent of skip_serialization_none?
Is it such that serialization/deserialization doesn't throw exceptions in the event of optional fields missing?
There was a problem hiding this comment.
I believe it makes it so that when serializing a struct to JSON, fields who's value is set to Option::None will be skipped. If this isn't set, then the fields woulb be serialized as null.
A more explicit example below.
Without the skip_serializing_none macro
{
"foo": 1
"bar": null
}With the macro
{
"foo": 1
}There was a problem hiding this comment.
Yup that makes sense, thanks!
| .as_ref() | ||
| .map(|json| { | ||
| JSONSchema::options() | ||
| .with_draft(Draft::Draft7) |
There was a problem hiding this comment.
What is the concept of a "draft" here?
There was a problem hiding this comment.
it's a revision for json schema itself...unfortunately they're still using quite an old draft
PresentationDefinitionV2is needed for a tbDEX Offering.This PR adds a
PresentationDefinitionV2definition to a newcredentialscrate in this repository, which will be consumed in thetbdex-rsrepository to define an Offering.I primarily followed what is currently in the Kotlin repository here as an example, and re-used the comments / tests from there.