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

feat/improve-presentation-exchange-support-in-oid4vp-rs #9

Conversation

Ryanmtate
Copy link
Contributor

add implementation methods for Presentation Definition.

WIP: Need to continue work for Presentation Submission and the rest of the structs used in the presentation exchange flow.

Many thanks to @todd-spruceid for pair programming on this.

Ryanmtate and others added 3 commits August 9, 2024 13:33
add implementation methods for Presentation Definition.

WIP: Need to continue work for Presentation Submission and the rest
of the structs used in the presentation exchange flow.

Signed-off-by: Ryan Tate <[email protected]>

Co-authored-by: Todd Showalter <[email protected]>
Cargo.toml Show resolved Hide resolved
tests/e2e.rs Outdated Show resolved Hide resolved
@Ryanmtate Ryanmtate marked this pull request as ready for review August 11, 2024 19:31
src/json_schema_validation.rs Outdated Show resolved Hide resolved
src/presentation_exchange.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cobward cobward left a comment

Choose a reason for hiding this comment

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

Looking really good, a couple more points of feedback. Once addressed and the CI is passing, then we can look to merge!

src/json_schema_validation.rs Outdated Show resolved Hide resolved
src/json_schema_validation.rs Outdated Show resolved Hide resolved
src/presentation_exchange.rs Outdated Show resolved Hide resolved
tests/e2e.rs Show resolved Hide resolved
src/presentation_exchange.rs Show resolved Hide resolved
@Ryanmtate Ryanmtate force-pushed the feat/improve-presentation-exchange-support-in-oid4vp-rs branch from 2bd0d00 to 26dda42 Compare August 14, 2024 16:40
@Ryanmtate
Copy link
Contributor Author

Todd helped point out that I need to add the submodules for the presentation-exchange. Will re-run tests now that I have the tests locally.

Copy link
Collaborator

@cobward cobward left a comment

Choose a reason for hiding this comment

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

The CI is failing due to the presentation exchange tests.

@Ryanmtate
Copy link
Contributor Author

d18f56d should fix the CI test failures.

#[serde(skip_serializing_if = "Option::is_none")]
filter: Option<SchemaValidator>,
filter: Option<serde_json::Value>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing SchemaValidator to use a generic Value type and using jsonschema to perform the validation.

SchemaValidator provides helpful methods to construct a filter object and will perform some validation, but it is not as feature rich as jsonschema.

Consider removing SchemaValidator from the code, unless there is a reason to keep it.

Copy link
Collaborator

@cobward cobward Aug 16, 2024

Choose a reason for hiding this comment

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

I would be in favour of removing it, I think we eventually need a more featureful builder than what it currently is anyway.

Copy link
Contributor Author

@Ryanmtate Ryanmtate Aug 16, 2024

Choose a reason for hiding this comment

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

I'll wire up the schema validation (using jsonschema) in #8 and remove the schema validator from this PR.

@cobward cobward merged commit a3a9209 into spruceid:main Aug 16, 2024
1 check passed
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