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(credential-status): rename plugin interfaces and methods #982

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

mirceanis
Copy link
Member

What issue is this PR fixing

fixes #981

What is being changed

I renamed the credential status interfaces to ICredentialStatusVerifier and ICredentialStatusManager and created a ICredentialStatus plugin interface to represent both.
I moved these interfaces to @veramo/core and updated the plugin schema generated.
The schema generation had some issues because of some inline docs so I fixed the inline tsdocs that were creating issues.

There is an extra change in this PR that was not related to #981:
The ICredentialStatusVerifier.checkCredentialStatus method didDoc parameter was renamed to didDocumentOverride and is now optional.
If it is not provided, the IResolver plugin is called to resolve the issuer DID document. I also added a test for this.

Quality

Check all that apply:

  • I want these changes to be integrated
  • I successfully ran yarn, yarn build, yarn test, yarn test:browser locally.
  • I allow my PR to be updated by the reviewers (to speed up the review process).
  • I added unit tests.
  • I added integration tests.
  • I did not add automated tests because _________, and I am aware that a PR without tests will likely get rejected.

@mirceanis mirceanis requested a review from italobb August 9, 2022 13:57
Copy link
Contributor

@italobb italobb left a comment

Choose a reason for hiding this comment

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

Just one issue found related to the revoked field, in the CredentialStatus class, which is still an optional field but in the comments we say it MUST be filled by the implementations.

packages/core/src/types/vc-data-model.ts Outdated Show resolved Hide resolved
…ethods

docs: fix inline docs broken references

docs(kms-web3): add docs to kms-web3
@codecov
Copy link

codecov bot commented Aug 10, 2022

Codecov Report

Merging #982 (125bf42) into next (64f01cb) will increase coverage by 1.01%.
The diff coverage is 74.39%.

❗ Current head 125bf42 differs from pull request most recent head e00daa4. Consider uploading reports for the commit e00daa4 to get more accurate results

@@            Coverage Diff             @@
##             next     #982      +/-   ##
==========================================
+ Coverage   79.23%   80.25%   +1.01%     
==========================================
  Files         106      118      +12     
  Lines        3621     4056     +435     
  Branches      780      875      +95     
==========================================
+ Hits         2869     3255     +386     
- Misses        748      798      +50     
+ Partials        4        3       -1     

@italobb italobb merged commit a110e96 into next Aug 10, 2022
@italobb italobb deleted the 981_refactor_credential_status branch August 10, 2022 12:29
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.

[chore] clarify credential-status method names
2 participants