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

Reuse credential offers and requests when possible #242

Merged
merged 7 commits into from
Oct 30, 2019

Conversation

andrewwhitehead
Copy link
Contributor

@andrewwhitehead andrewwhitehead commented Oct 28, 2019

Also adds tests and type info to the CredentialManager class, and allows create_offer to be called without a credential proposal.

…asses

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
…tion

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
@codecov-io
Copy link

codecov-io commented Oct 28, 2019

Codecov Report

Merging #242 into master will increase coverage by 1.07%.
The diff coverage is 66.98%.

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
+ Coverage   73.66%   74.74%   +1.07%     
==========================================
  Files         219      219              
  Lines       10045    10103      +58     
==========================================
+ Hits         7400     7551     +151     
+ Misses       2645     2552      -93

…to manager class

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
@andrewwhitehead andrewwhitehead marked this pull request as ready for review October 30, 2019 03:26
@@ -73,6 +63,12 @@ def context(self) -> InjectionContext:

"""

credential_definition_id = credential_proposal.cred_def_id
Copy link
Contributor

Choose a reason for hiding this comment

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

The proposal will not include a cred def id in future - rather, an issuer and schema id - so this will need some touching up at that time. Until then, it's fine.

sklump
sklump previously approved these changes Oct 30, 2019
Copy link
Contributor

@sklump sklump left a comment

Choose a reason for hiding this comment

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

Look good, but we will need to merge in the change from cred-stored to cred-ack, in PR#233. Likely easy in any case.

ianco
ianco previously approved these changes Oct 30, 2019
Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

Tested on my local, working as advertised

Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Copy link
Contributor

@swcurran swcurran left a comment

Choose a reason for hiding this comment

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

Reviewed by others; run by Ian

@swcurran swcurran merged commit ca34806 into openwallet-foundation:master Oct 30, 2019
@andrewwhitehead andrewwhitehead deleted the cred-req-reuse branch February 29, 2020 21:46
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.

5 participants