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

Create Verifiable Credentials #5

Merged
merged 47 commits into from
Jul 12, 2022
Merged

Create Verifiable Credentials #5

merged 47 commits into from
Jul 12, 2022

Conversation

sKudryashov
Copy link
Contributor

@sKudryashov sKudryashov commented Jun 17, 2022

@sKudryashov sKudryashov marked this pull request as draft June 17, 2022 17:24
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from 9ef5f3e to 8abb8fa Compare June 17, 2022 17:29
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from 8abb8fa to 158a920 Compare June 17, 2022 20:02
Sergey Kudryashov added 2 commits June 21, 2022 14:35
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch 4 times, most recently from 43ef7b3 to ccd843c Compare June 21, 2022 23:52
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from ccd843c to faf7ea1 Compare June 22, 2022 00:04
Sergey Kudryashov added 2 commits June 23, 2022 09:29
@sKudryashov sKudryashov marked this pull request as ready for review June 24, 2022 16:21
@sKudryashov sKudryashov requested review from LuisOsta and fkim7 June 24, 2022 16:39
@sKudryashov sKudryashov marked this pull request as draft June 24, 2022 16:42
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from 66a2141 to 1c73669 Compare June 24, 2022 16:47
@sKudryashov sKudryashov marked this pull request as ready for review June 24, 2022 16:52
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from 1c73669 to cc4dcb1 Compare June 24, 2022 17:05
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch 2 times, most recently from c5edcc1 to a922440 Compare July 12, 2022 00:31
@sKudryashov sKudryashov requested a review from LuisOsta July 12, 2022 00:33
@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from c1b8a8a to b15ce62 Compare July 12, 2022 00:35

#[derive(Debug, Serialize, Deserialize, Clone)]
#[serde(bound(deserialize = "'de: 'static"))]
pub struct VerifiableCredential {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a #[serde(rename_all = "camelCase")] header for the struct whilst keeping explicit fields with their own renames (such as id). Here's an example

}

#[derive(Debug, Serialize, Deserialize, Clone)]
#[serde(bound(deserialize = "'de: 'static"))]
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we can't suppress this warning yet:
rust-lang/rust#96956

Once that's merged into main we can add #[allow(needless_lifetimes)] here
For now, let's add a comment linking to this post:
https://users.rust-lang.org/t/restrict-generic-type-to-static-lifetime/16915/4

The borrow checker currently isn't smart enough to reason around 'de: 'static bounds.

@LuisOsta @sKudryashov LMK if you have any questions about the two links above

#[serde(rename = "@id")]
id: String,
#[serde(rename = "type")]
cred_type: String,
Copy link
Contributor

@mkatychev mkatychev Jul 12, 2022

Choose a reason for hiding this comment

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

if you wanted to keep a field name of type, you could call it r#type: String, it will look weird but this way you can explicitly have the field named self.r#type if you'd like

EDIT: not saying you should, it all depends on how explicit you want the struct to json analogue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, this is a great trick, since the type word is reserved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll keep it as is so far but thank you for introducing it, it is definitely good for the similar situations in the future

Copy link
Contributor

@mkatychev mkatychev left a comment

Choose a reason for hiding this comment

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

LGTM
@sKudryashov we can follow up with suggested changes in a separate PR. The comment section shouldn't hit triple digits 😓

@LuisOsta any concerns about merging as is?

Copy link
Contributor

@LuisOsta LuisOsta left a comment

Choose a reason for hiding this comment

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

@mkatychev @sKudryashov There's a couple of issues but apart from them I'm happy to approve this PR

@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from 5c5e4a6 to df2bada Compare July 12, 2022 19:18
Copy link
Contributor

@LuisOsta LuisOsta left a comment

Choose a reason for hiding this comment

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

One minor nit remaining but looks good to me overall

#[serde(bound(deserialize = "'de: 'static"))]
pub struct VerifiableCredential {
#[serde(flatten)]
verifiable_credential:Credential,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be credential

@sKudryashov sKudryashov force-pushed the verifiable_credentials branch from df2bada to 866f7b3 Compare July 12, 2022 19:28
@sKudryashov sKudryashov merged commit a612f33 into main Jul 12, 2022
@sKudryashov sKudryashov deleted the verifiable_credentials branch July 12, 2022 19:38
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.

Implement create_verifiable_credential in SSI library
6 participants