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 API and support for draft 20 #6

Merged
merged 31 commits into from
Aug 1, 2024

Conversation

cobward
Copy link
Collaborator

@cobward cobward commented Jul 31, 2024

No description provided.

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.

Looks great and should be ready for people to start implementing.

I have a few questions though:

  • This doesn't yet support OID4VP Error Responses. Do we feel comfortable putting that on the sprucekit implementers? I think it's better if that lives here
  • Same for the RequestVerifier trait implementation for client_id_schemes. I think we should have a default trait implementation here, not in the sprucekit wallet.
  • It doesn't look like we're fully ready for PresentationExchange 2.0, with SubmissionRequirementPicks being ignored in the PresentationDefinition parsing.

Should I open issues for these on this repo?

@justAnIdentity
Copy link
Contributor

Maybe we should also provide some support structs for creating JARMs

@@ -0,0 +1,2 @@
[net]
git-fetch-with-cli = true
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is required now that all dependencies are public (or not git at all)

///
/// A trait is used here so to facilitate native HTTP/TLS when compiled for mobile applications.
#[async_trait]
pub trait HttpClient {
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be good to rename this AsyncHttpClient to avoid any confusion

src/wallet.rs Outdated

#[async_trait]
pub trait Wallet: RequestVerifier + Sync {
type HttpClient: HttpClient + Send + Sync;
Copy link
Member

Choose a reason for hiding this comment

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

All these Send + Sync will be a problem if we want to update vp_interop

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Discussed, we will leave it for now and address this later if needed.

@cobward
Copy link
Collaborator Author

cobward commented Aug 1, 2024

This doesn't yet support OID4VP Error Responses. Do we feel comfortable putting that on the sprucekit implementers? I think it's better if that lives here

Good point, will add an issue for this.

Same for the RequestVerifier trait implementation for client_id_schemes. I think we should have a default trait implementation here, not in the sprucekit wallet.

I disagree with this. There's so much custom stuff that goes into handling the client_id_schemes. We do have some "generic" support for did and x509_san_X schemes, but they aren't there by default since it would add a load of new dependencies on the trait for things like "trusted verifiers".

It doesn't look like we're fully ready for PresentationExchange 2.0, with SubmissionRequirementPicks being ignored in the PresentationDefinition parsing.

Yep, I'm adding a issue to generally improve our Presentation Exchange support. We need to have some support for parsing and handling the request, either here or in SpruceKit Mobile.

Maybe we should also provide some support structs for creating JARMs

I will add an issue for this too - but we also need to consider that we might need to use native crypto in order to meet requirements for FIPS 140-X cryptography.

@cobward cobward merged commit 8f5b573 into main Aug 1, 2024
2 checks passed
@cobward cobward deleted the feat/update-api-and-support-for-draft-20 branch August 1, 2024 13:16
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