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

Enable presentation issuance/verification through vc-api endpoints #2710

Merged
merged 35 commits into from
Jan 29, 2024

Conversation

PatStLouis
Copy link
Contributor

This PR enables presentation issuance and verification using the vc_ld manager and addresses #2693.

There is also 4 new routes included matching the vc-api specification. (minor changes from the /vc/ldp endpoints, mostly key names and defaulting to Ed25519Signature2018 proofType for issuance).
@dbluhm could you help me by reviewing the changes I've made to the vc_ld manager and models? You will see I added an if condition in the verify_presentation section to enable assertionMethod purpose if no challenge is provided, while minimizing the impact on the existing code/workflows. I also improved the presentation models, based on the existing credential ones.

I have a test instance of these changes running here.

@swcurran swcurran marked this pull request as draft January 15, 2024 17:55
@PatStLouis
Copy link
Contributor Author

The last commit added a method to store an issued VC

@swcurran
Copy link
Contributor

I'm trying to re-run the failed test, as it doesn't look like it is related to this PR.

You had flagged this is a [WIP], @PatStLouis , so I marked it as a "Draft" PR. Have you completed it at this point?

Thanks!

@swcurran
Copy link
Contributor

Ah...also there are conflicts with this PR to be resolved. Could you also look at that @PatStLouis ? Thanks!

@PatStLouis
Copy link
Contributor Author

@swcurran yeah I noticed the tests were failing because it wasn't able to write a schema on the ledger which seemed odd, looks like it passed now. Let me fix the conflicts then I would consider this ready.

Signed-off-by: pstlouis <[email protected]>
Signed-off-by: pstlouis <[email protected]>
@swcurran swcurran requested review from dbluhm and ianco January 17, 2024 15:02
@swcurran swcurran marked this pull request as ready for review January 17, 2024 19:43
Signed-off-by: pstlouis <[email protected]>
dbluhm
dbluhm previously approved these changes Jan 24, 2024
@swcurran
Copy link
Contributor

@PatStLouis — looks like this is near ready. Can you pleae update with the base branch so we can merge it?

@PatStLouis
Copy link
Contributor Author

@swcurran @dbluhm @ianco I'm happy with the state of this PR. I will make a subsequent PR with a postman collection in the demo folder, which will include documentation about these endpoints as well as simple test cases.

@swcurran
Copy link
Contributor

Sounds good. The documentation is missing and that is a concern, so that needs to be added.

@swcurran swcurran enabled auto-merge January 24, 2024 16:28
@ianco
Copy link
Contributor

ianco commented Jan 24, 2024

Sounds good. The documentation is missing and that is a concern, so that needs to be added.

Agree we need the documentation and we can deal with the rest in a subsequent PR.

@PatStLouis
Copy link
Contributor Author

Sounds good, I will add this tonight, I will put a small abstract on the main readme with a link to a page in the demo folder with more information!

…md and a postman collection in the demo folder

Signed-off-by: pstlouis <[email protected]>
auto-merge was automatically disabled January 24, 2024 20:23

Head branch was pushed to by a user without write access

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@PatStLouis
Copy link
Contributor Author

@dbluhm I added a manger test for the storage function

Copy link
Contributor

@dbluhm dbluhm left a comment

Choose a reason for hiding this comment

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

I'm happy with these changes!

@dbluhm dbluhm requested a review from ianco January 26, 2024 22:04
@swcurran
Copy link
Contributor

@ianco — are your change requests ok now?

@dbluhm dbluhm enabled auto-merge January 29, 2024 21:02
@dbluhm
Copy link
Contributor

dbluhm commented Jan 29, 2024

@PatStLouis if you want to do one final merge of main, I've enabled auto merge so we can get this merged in next.

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@dbluhm dbluhm merged commit 2a17bd1 into openwallet-foundation:main Jan 29, 2024
7 of 8 checks passed
@PatStLouis PatStLouis deleted the pstlouis/add-vc-api-routes branch January 29, 2024 22:21
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