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

Correct revocation API in credential revocation documentation #612

Merged

Conversation

baegjae
Copy link
Contributor

@baegjae baegjae commented Jul 17, 2020

I found that changes of the revocation API (#428) have not yet been applied to the credential revocation documentation.
Changed to the correct revocation API.

Signed-off-by: Ethan Sung [email protected]

@codecov-commenter
Copy link

Codecov Report

Merging #612 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #612   +/-   ##
=======================================
  Coverage   96.60%   96.60%           
=======================================
  Files         248      248           
  Lines       13206    13206           
=======================================
  Hits        12758    12758           
  Misses        448      448           

```
POST /issue-credential/records/{cred_ex_id}/revoke
POST /issue-credential/revoke
Copy link

@anhem115 anhem115 Jul 17, 2020

Choose a reason for hiding this comment

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

Could we add an example body data for this POST request? I think this POST will need query params as

issue-credential/revoke?rev_reg_id=<revocation_registry_id>&cred_rev_id=<credential_definition_id>&publish=<true|false>

Choose a reason for hiding this comment

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

image

@swcurran
Please Check, the URL once, could not revoke credential in that URL. Image attaches along with it. Thanks!!

@swcurran
Copy link
Contributor

@nrempel is in the midst of revising the revocation API to make it a lot easier for the controller and controller developer. He's almost finished the changes. I'm going to leave this PR for him to decide on approving or closing because of his changes.

Thanks @baegjae - and sorry if you are caught in the middle of this upgrade.

@nrempel
Copy link
Contributor

nrempel commented Jul 17, 2020

Thanks for this! This portion of the API isn't changing so this is good.

I agree with @anhem115's comments. If you could make a slight adjustment to include their recommendation, I'll merge this.

Thanks!

@baegjae
Copy link
Contributor Author

baegjae commented Jul 20, 2020

@anhem115 @nrempel Thanks for the suggestion. I changed the PR according to your suggestion. I hope it fits what you expected.

@swcurran I am impressed by the community's friendly feedback. Thanks !

Copy link

@anhem115 anhem115 left a comment

Choose a reason for hiding this comment

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

LGTM

@swcurran swcurran merged commit 2ab736d into openwallet-foundation:master Jul 20, 2020
@swcurran
Copy link
Contributor

Thanks for the PR!

nrempel pushed a commit to nrempel/aries-cloudagent-python that referenced this pull request Jul 21, 2020
…ocation-document

Correct revocation API in credential revocation documentation
Signed-off-by: Nicholas Rempel <[email protected]>
@baegjae baegjae deleted the correct-revocation-document branch July 28, 2020 04:08
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.

6 participants