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

Revo cleanup to controller #441

Merged

Conversation

sklump
Copy link
Contributor

@sklump sklump commented Mar 31, 2020

Revoke by rev reg id + cred rev id. Leave rev regs out of credential exchange records until issue. Unit test coverage for indy-issuer.

@andrewwhitehead
Copy link
Contributor

@swcurran This gets rid of the revocation by credential exchange ID so the only way to revoke is by rev reg ID + cred rev ID. Is that the intention? I think it's probably best to only have one way to do it.

@codecov-io
Copy link

Codecov Report

Merging #441 into master will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   88.80%   89.05%   +0.25%     
==========================================
  Files         248      248              
  Lines       12206    12200       -6     
==========================================
+ Hits        10839    10865      +26     
+ Misses       1367     1335      -32     

@sklump
Copy link
Contributor Author

sklump commented Mar 31, 2020

Via cred ex record is janky, as we knew from the start - it was a way to get the thing running, nothing more. It injects revocation into the process way before it needs to be there. I took out the former process on purpose.

I've also taken out the revocation registry cache from the indy Revocation context, since it was indexed by cred def id and there can be multiple active rev regs per cred def id (relying on only the most recent will orphan revocation capacity). Since it's going to the wallet and not the ledger, I don't think it will present a significant loss of amenity. If need be I can put it back in.

@swcurran
Copy link
Contributor

I agree - this should be the only way. The cred_exchange_id approach was a a short cut but this is the right/long term way to do it.

@andrewwhitehead andrewwhitehead merged commit cb5c287 into openwallet-foundation:master Mar 31, 2020
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