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

Anoncreds revoke and publish-revocations endorsement #2782

Merged

Conversation

jamshale
Copy link
Contributor

@jamshale jamshale commented Feb 13, 2024

Opening for visibility. I'm still going to manually test more but the Integration tests are passing for endorsement and publishing revocations.

One area of confusion for me is what happens with the local wallet verse the ledger. Write now an endorsement transaction is created when the update_revocation_list is called with changes. Then the local wallet is updated. At the same time the endorsement manager receives the 114 event and notifies the anoncreds RevocationListFinished event handler. It checks if the wallet contains a record for the list via rev_reg_def_id and if it doesn't it creates the record. If it does then it assumes the record has been updated already and does nothing.

I was trying some other stuff like deleting the record and creating a new one with a wait state but couldn't decide if this was necessary, or something we wanted to do.

I deleted the /anoncreds/revoke and /anoncreds/publish-revocation endpoints and instead just have the /revocation/revoke and /revocation/publish-revocation. I thought this was appropriate because the revocation endpoint are all loaded for anoncreds specifically and the rest of the anoncreds endpoints are to do with creating objects. I changed the params for requesting a transaction manually to use the body options and updated the integration tests.

@jamshale jamshale marked this pull request as ready for review February 14, 2024 16:58
@jamshale jamshale requested review from ianco and dbluhm February 14, 2024 17:09
@ianco
Copy link
Contributor

ianco commented Feb 14, 2024

I deleted the /anoncreds/revoke and /anoncreds/publish-revocation endpoints and instead just have the /revocation/revoke and /revocation/publish-revocation. I thought this was appropriate because the revocation endpoint are all loaded for anoncreds specifically and the rest of the anoncreds endpoints are to do with creating objects. I changed the params for requesting a transaction manually to use the body options and updated the integration tests.

I think this happened when I did the original merge of the anoncreds-rs branch. You're correct we should just have the endpoints in one place and it makes sense to be under the /revocation section.

@jamshale jamshale force-pushed the anoncreds-endorsement-revoke branch from 336416b to 6f6ad0d Compare February 14, 2024 18:02
@ianco
Copy link
Contributor

ianco commented Feb 14, 2024

Overall looks good, is it ready to go or are you still working on it?

@jamshale
Copy link
Contributor Author

My manual testing and integration testing has been good, so I think it's ready.

The last integration test run failed but I'm pretty sure it was a random unrelated fail.

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@jamshale jamshale merged commit eaee031 into openwallet-foundation:main Feb 14, 2024
8 checks passed
gvelez17 pushed a commit to Whats-Cookin/aries-cloudagent-python that referenced this pull request Mar 25, 2024
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