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

Fix/use option string as domain type #636

Merged
merged 1 commit into from
Dec 16, 2024
Merged

Conversation

Ryanmtate
Copy link
Contributor

@Ryanmtate Ryanmtate commented Dec 15, 2024

Description

Changes domains to use a Option<String> type instead of Vec<String>. This was causing an error with vc playground verification.

Other considerations

I've left the domains property as-is, but we should consider changing this value to domain. WDYT @timothee-haudebourg ?

Tested

Tested in sprucekit mobile.

@Ryanmtate Ryanmtate force-pushed the fix/use-string-as-domain-type branch 2 times, most recently from 270d184 to a5d6ad3 Compare December 15, 2024 21:17
@Ryanmtate Ryanmtate changed the title Fix/use string as domain type Fix/use option string as domain type Dec 15, 2024
Copy link
Contributor

@timothee-haudebourg timothee-haudebourg left a comment

Choose a reason for hiding this comment

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

There can be more than one value for the domain property, see the specification here:

If specified, the associated value MUST be either a string, or an unordered set of strings.

So Vec<String> is correct, but the serialization when there is only one value is wrong. You can fix it using the value_or_array serializer defined in the syntax module of ssi-vc crate like this:

#[serde(
    with = "value_or_array",
    skip_serializing_if = "Vec::is_empty"
)]
pub domains: Vec<String> // or &'a [String]

This way if the array contains only one item, this item will be serialized directly.

Base automatically changed from fix/update-credentials-v2-context to main December 16, 2024 15:16
@Ryanmtate Ryanmtate force-pushed the fix/use-string-as-domain-type branch from a5d6ad3 to e083a31 Compare December 16, 2024 17:13
@Ryanmtate
Copy link
Contributor Author

There can be more than one value for the domain property, see the specification here:

If specified, the associated value MUST be either a string, or an unordered set of strings.

So Vec<String> is correct, but the serialization when there is only one value is wrong. You can fix it using the value_or_array serializer defined in the syntax module of ssi-vc crate like this:

#[serde(
    with = "value_or_array",
    skip_serializing_if = "Vec::is_empty"
)]
pub domains: Vec<String> // or &'a [String]

This way if the array contains only one item, this item will be serialized directly.

Nice, this worked well!

since ssi-vc depends on data-integrity, I copied the implementation and re-used it within the data-integrity crate.

This branch is a bit of a misnomer, but the implementation is working in sprucekit mobile.

@Ryanmtate Ryanmtate force-pushed the fix/use-string-as-domain-type branch from e083a31 to ae78231 Compare December 16, 2024 17:29
Copy link
Contributor

@timothee-haudebourg timothee-haudebourg left a comment

Choose a reason for hiding this comment

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

🚀

crates/claims/crates/vc/src/syntax/mod.rs Outdated Show resolved Hide resolved
@timothee-haudebourg
Copy link
Contributor

I approved it, but please wait for the end of CI to be sure 😅

Signed-off-by: Ryan Tate <ryan.tate@spruceid.com>
@Ryanmtate Ryanmtate force-pushed the fix/use-string-as-domain-type branch from ae78231 to d00a3a1 Compare December 16, 2024 17:36
@Ryanmtate Ryanmtate merged commit 565707b into main Dec 16, 2024
3 checks passed
@Ryanmtate Ryanmtate deleted the fix/use-string-as-domain-type branch December 16, 2024 18:03
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