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 API - update vc status #890

Conversation

mishasizov-SK
Copy link
Contributor

@mishasizov-SK mishasizov-SK commented Oct 18, 2022

closes: #834
closes: #826

Signed-off-by: Mykhailo Sizov [email protected]

@cla-bot cla-bot bot added the cla-signed label Oct 18, 2022
@@ -150,8 +150,20 @@ type Crypto struct {
documentLoader ld.DocumentLoader
}

// SignCredentialLDP adds verifiable.LinkedDataProofContext to the vc.
func (c *Crypto) SignCredentialLDP(
func (c *Crypto) SignCredential(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

created a generic method instead of SignCredentialLDP and SignCredentialJWT

@mishasizov-SK mishasizov-SK force-pushed the feat_ID-3359_Credential_Status_API-update_vc_status branch from 30ca3b6 to a6fa11b Compare October 18, 2022 12:28
@codecov
Copy link

codecov bot commented Oct 18, 2022

Codecov Report

Base: 87.44% // Head: 87.42% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (a6fa11b) compared to base (03e09e9).
Patch coverage: 86.81% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
- Coverage   87.44%   87.42%   -0.02%     
==========================================
  Files          72       72              
  Lines        3862     3904      +42     
==========================================
+ Hits         3377     3413      +36     
- Misses        309      315       +6     
  Partials      176      176              
Impacted Files Coverage Δ
pkg/doc/vc/crypto/crypto.go 89.07% <0.00%> (-4.08%) ⬇️
...rvice/credentialstatus/credentialstatus_service.go 79.68% <92.00%> (+0.24%) ⬆️
pkg/restapi/v1/issuer/controller.go 91.19% <93.33%> (+2.04%) ⬆️
cmd/vc-rest/startcmd/start.go 84.15% <100.00%> (ø)
...rvice/didconfiguration/didconfiguration_service.go 100.00% <100.00%> (ø)
...service/issuecredential/issuecredential_service.go 100.00% <100.00%> (ø)
.../verifycredential/revocation/revocation_service.go 90.90% <100.00%> (ø)
pkg/storage/mongodbprovider/csl.go 72.54% <100.00%> (+6.51%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@@ -114,5 +115,5 @@ func (s *Service) sendHTTPRequest(req *http.Request, status int, token string) (
return nil, fmt.Errorf("failed to read response body for status %d: %s", resp.StatusCode, string(body))
}

return body, nil
return bytes.Trim(body, "\n"), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need trim here

Copy link
Contributor Author

@mishasizov-SK mishasizov-SK Oct 18, 2022

Choose a reason for hiding this comment

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

ctx.JSON(), ctx.String() and similar methods from echo adds \n to the end of payload body.
For LDP VC it's ok, but for jwt it makes problems in decoding.

Copy link
Contributor

@fqutishat fqutishat Oct 18, 2022

Choose a reason for hiding this comment

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

will be fixed in the next PR

@fqutishat fqutishat merged commit b427e63 into trustbloc:main Oct 18, 2022
@mishasizov-SK mishasizov-SK deleted the feat_ID-3359_Credential_Status_API-update_vc_status branch October 19, 2022 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Credential Status API Credential status management JWT support
3 participants