refactor: Serialization of Verifiable Credential with extended data model #564
Conversation
Codecov Report
@@ Coverage Diff @@
## master #564 +/- ##
==========================================
- Coverage 90.48% 90.28% -0.21%
==========================================
Files 77 77
Lines 4235 4302 +67
==========================================
+ Hits 3832 3884 +52
- Misses 223 231 +8
- Partials 180 187 +7
Continue to review full report at Codecov.
|
troyronda
requested review from
sandrask,
soluchok,
rolsonquadras and
fqutishat
| if err != nil { | ||
| return nil, nil, fmt.Errorf("failed to decode raw Verifiable Credential JWT claims: %w", err) | ||
| return nil, fmt.Errorf("failed to decode Verifiable Credential JWT claims: %w", err) |
There was a problem hiding this comment.
Wrapping messages should be kept succinct. https://github.com/uber-go/guide/blob/master/style.md#error-wrapping
There was a problem hiding this comment.
Agree! I think it deserves the creation of a separate issue, to clean-up the whole credential package.
#570
|
This PR/commit appears to be a "refactor: " rather than a "feat: " |
|
@troyronda yes, this is definitely "refactor:". |
kdimak
changed the title
| func decodeType(rc *rawCredential) ([]string, error) { | ||
| switch rType := rc.Type.(type) { | ||
| case string: | ||
| return []string{rType}, nil | ||
| case []interface{}: | ||
| types, err := stringSlice(rType) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to map credential types: %w", err) | ||
| } | ||
| return types, nil | ||
| default: | ||
| return nil, errors.New("credential type of unknown type") | ||
| } | ||
| } |
There was a problem hiding this comment.
I do not like this approach with different types string and []interface{}, are we able to change this logic? We can use comma-separated values instead of different types e.g type1 or type1,type3,... or only slice [type1] or [type1,type2]
@troyronda @rolsonquadras @fqutishat
There was a problem hiding this comment.
hey @soluchok
According to Verifiable Credentials spec, the type can be defined as either a single string value or array. We keep it raw in rawCredential struct (into which VC JSON is unmarshalled) and then decode it to universal and convenient []string to be used in Credential struct.
There was a problem hiding this comment.
I would, at least, add some comments in the code. It isn’t entirely clear to a reader when they see these case statements.
| @@ -63,7 +64,7 @@ type typedID struct { | |||
| type RefreshService typedID | |||
|
|
|||
| // TermsOfUse represents terms of use of Verifiable Credential by Issuer or Verifiable Presentation by Holder. | |||
| type TermsOfUse typedID | |||
| type TermsOfUse interface{} | |||
There was a problem hiding this comment.
I'm not too excited when we need to export types/struct that have interface{}
Can you give more context on this change?
There was a problem hiding this comment.
It was incorrectly defined before. It can be one or more TermsOfUse. Its structure is not precisely defined. They say that type field is mandatory and id field is optional, and
The precise contents of each term of use is determined by the specific termsOfUse type definition.
If we use typedID alias for termsOfUse and after decoding of VC want to marshal it back, we will loose all the original fields except type and id.
That was the reason I changed the TermsOfUse alias.
We can also stay with typedID alias and extend it to keep all extra fields in e.g. map[string]interface{} to be used during marshalling.
What do you think @troyronda?
There was a problem hiding this comment.
I thought we had typically used an unexported "raw" struct and filled a properly typed exported struct.
There was a problem hiding this comment.
I see. It's another situation with extended fields. I would think you would have defined the known parts and keep extra fields in map[string]interface{} like you did elsewhere in the PR.
Is it because it is "one or more"?
There was a problem hiding this comment.
Yes, because of "one or more" and also because of the flexible structure of the termsOfUse field.
There was a problem hiding this comment.
Filed a separate issue #586.
pkg/doc/verifiable/credential_jwt.go
Outdated
| nbfTime := nbf.Time().UTC() | ||
| raw.Issued = &nbfTime | ||
| vcMap["issuanceDate"] = nbfTime.Format(time.RFC3339) |
There was a problem hiding this comment.
some of these strings could make sense as named constants.
…odel closes hyperledger-archives#327 Signed-off-by: Dima <[email protected]>
closes #327
Signed-off-by: Dima [email protected]
This PR brings the following improvements to the Verifiable Credential (VC):
Credential: issuer, context, type (used genericinterface{}or[]interface{}before).*rawCredentialand serialized JSON ([]byte) of VC. Now it returns only serialized JSON which is then unmarshalled torawCredential.