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

Update library to implementor's draft 1 (draft 13) #14

Merged

Conversation

theosirian
Copy link
Collaborator

No description provided.

Copy link
Member

@sbihel sbihel left a comment

Choose a reason for hiding this comment

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

Good work 👍

A couple of "meta" pieces of feedback:

  • it would would have been easier to review if the fix of the typo for AuthorizationDetailsProfile had been in a separate commit
  • in general, at least listing the changes in the PR description if not through commits would have been helpful

src/deny_field.rs Show resolved Hide resolved
src/core/profiles/mod.rs Outdated Show resolved Hide resolved
src/core/profiles/mod.rs Outdated Show resolved Hide resolved
src/proof_of_possession.rs Outdated Show resolved Hide resolved
src/token.rs Outdated Show resolved Hide resolved
src/metadata.rs Outdated Show resolved Hide resolved
src/metadata.rs Outdated Show resolved Hide resolved
src/metadata.rs Outdated Show resolved Hide resolved
Signed-off-by: Tiago Nascimento <[email protected]>
src/credential_offer.rs Outdated Show resolved Hide resolved
src/credential_offer.rs Outdated Show resolved Hide resolved
Signed-off-by: Tiago Nascimento <[email protected]>
Copy link

@rschulman rschulman left a comment

Choose a reason for hiding this comment

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

I've left a couple comments in places, but it broadly looks good to me. It's rather difficult to look at the spec and try to figure out if the code is missing anything, but I didn't spot anything!

Copy link
Contributor

@justAnIdentity justAnIdentity left a comment

Choose a reason for hiding this comment

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

  • can we rename IssuerMetadata to CredentialIssuerMetadata to align with the naming in the spec? maybe people disagree with me, but I'd prefer a 1:1 naming mapping
  • update jwt.rs and ldp.rs to use credential_signing_alg_values_supported instead of cryptographic_suites_supported
  • update CredentialMetadata tests to new version
    • add background image in tests
    • replace cryptographic_suites_supported with credential_signing_alg_values_supported
    • can we add an example test for jwtld.rs ?
  • typo: field setters -> “set_proof_types_suuported”

no support for sd-jwt-vc (probably for the future)

Signed-off-by: Tiago Nascimento <[email protected]>
@theosirian
Copy link
Collaborator Author

After some discussion above, I've pushed a "forgiving" version of the server selection with comments outlining which situations could also be interpreted as errors.
Now, I think this just needs a last Rust-specific review on the latest commit.
@sbihel @justAnIdentity

src/metadata.rs Outdated
http_client: HC,
logging_fn: Option<LF>,
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed that in your proposed snippet. tracing is the globally accepted logging crate and we should just use that. But in general, it's only in very specific cases that you would pass around function and in general you would rely on a trait.

Copy link
Collaborator Author

@theosirian theosirian Aug 14, 2024

Choose a reason for hiding this comment

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

I've replaced log_and_continue and added spans on the outer and inner functions. Is there anything else that we should be logging in those functions besides what I added?

Copy link
Member

Choose a reason for hiding this comment

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

maybe but that's something we'll discover organically, we've got the most important things in place

src/metadata.rs Outdated Show resolved Hide resolved
src/metadata.rs Outdated Show resolved Hide resolved
src/metadata.rs Outdated Show resolved Hide resolved
Signed-off-by: Tiago Nascimento <[email protected]>
@theosirian theosirian merged commit bee9a3d into main Aug 14, 2024
1 check passed
@theosirian theosirian deleted the feat/skit-455-update-the-oid4vci-library-to-implementors-draft-1 branch August 14, 2024 16:15
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.

4 participants