feat: [WACI-Issuance] Support to read credential manifest #574

talwinder50 · 2022-01-21T17:58:52Z

closes #582

Signed-off-by: talwinder50 talwinderkaur50@gmail.com

codecov · 2022-01-21T18:02:35Z

Codecov Report

Merging #574 (9ff8c67) into main (837ffab) will decrease coverage by 0.06%.
The diff coverage is 86.36%.

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
- Coverage   87.64%   87.58%   -0.07%     
==========================================
  Files          29       29              
  Lines        4201     4220      +19     
==========================================
+ Hits         3682     3696      +14     
- Misses        305      310       +5     
  Partials      214      214

Impacted Files	Coverage Δ
pkg/profile/issuer/profile.go	`89.47% <ø> (ø)`
pkg/restapi/issuer/operation/operations.go	`89.59% <85.00%> (-0.28%)`	⬇️
cmd/adapter-rest/startcmd/start.go	`82.24% <100.00%> (+0.03%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 837ffab...9ff8c67. Read the comment docs.

DRK3 · 2022-01-21T18:08:51Z

cmd/adapter-rest/startcmd/start.go

@@ -872,6 +872,7 @@ func addIssuerHandlers(parameters *adapterRestParameters, framework *aries.Aries
 		ExternalURL:          parameters.externalURL,
 		DidDomain:            parameters.trustblocDomain,
 		JSONLDDocumentLoader: ariesCtx.JSONLDDocumentLoader(),
+		CmOutputDescriptor:   cmOutputDescriptor,


Should be CMOutputDescriptor for consistency with the naming conventions

sudeshrshetty · 2022-01-24T19:43:02Z

cmd/adapter-rest/startcmd/start.go

@@ -892,7 +892,7 @@ func addIssuerHandlers(parameters *adapterRestParameters, framework *aries.Aries
 	if err != nil {
 		return fmt.Errorf("failed to init trustbloc did creator: %w", err)
 	}
-	// TODO #572 Pass the output descriptors to issuer
+	// TODO #575 Persist the manifest output descriptor in database


correction on TODO: it shouldn't be in database since you pass it everytime you start adapter, it should be in some kind of local file. based or in-memory cache.

add that new issue into current epic you are working on.

I don't see any issues in adapter numbered #575

updated and fixed the issue # as well

rolsonquadras · 2022-01-24T19:47:34Z

cmd/adapter-rest/startcmd/start.go

-	// TODO #572 Pass the output descriptors to issuer
-	_, err = readCMOutputDescriptorFile(parameters.cmOutputDescriptorsFilePath)
+
+	CMOutputDescriptor, err := readCMOutputDescriptorFile(parameters.cmOutputDescriptorsFilePath)


Shouldn't this local var name start with lower case ?

rolsonquadras · 2022-01-24T19:49:11Z

pkg/profile/issuer/profile.go

@@ -45,7 +46,7 @@ type ProfileData struct {
 	OIDCClientParams            *OIDCClientParams `json:"oidcParams,omitempty"`
 	CredentialScopes            []string          `json:"credScopes,omitempty"`
 	LinkedWalletURL             string            `json:"linkedWallet,omitempty"`
-	// Todo #issue Add credential manifest issuer object
+	CredentialManifestIssuer    cm.Issuer         `json:"issuer,omitempty"`


Can't we use Name from line 36 rather a new variable ?

I was wondering same, profile name & issuer names are same? remember issuer.Name name gets displayed in.wallet UI.

It is - same is displayed in DIDComm CHAPI screens as well during sharing (passed as part of credential.issuer field).

@talwinder50 remove cm.Issuer and use

Name - already there

IssuerID - put comment as it is going to be used as issuer ID in credential manifests being issued

Style - style object from aries credential manifest package, add go comments to explain it.

@talwinder50 I don't see any updates

sudeshrshetty · 2022-01-24T19:48:13Z

pkg/profile/issuer/profile.go

@@ -45,7 +46,7 @@ type ProfileData struct {
 	OIDCClientParams            *OIDCClientParams `json:"oidcParams,omitempty"`
 	CredentialScopes            []string          `json:"credScopes,omitempty"`
 	LinkedWalletURL             string            `json:"linkedWallet,omitempty"`
-	// Todo #issue Add credential manifest issuer object
+	CredentialManifestIssuer    cm.Issuer         `json:"issuer,omitempty"`


you do schema validation when CredentialManifestIssuer is supplied? Better to fail here rather than failing during transaction later during WACI.

Add some go doc comments about usage and expected format of this field.

From the spec, you'd need to make sure the ID is not empty:

The object MUST contain a id property, and its value MUST be a valid URI string that identifies who the issuer of the credential(s) will be.

My CredentialManifest validation code makes sure that the ID isn't empty, but only if you unmarshal a full CredentialManifest... (which will happen during the transaction, causing a failure)

#581 Validation of the manifest will be done as part of this todo. Which i will address in the follow up pr.

sudeshrshetty · 2022-01-24T19:48:51Z

pkg/restapi/issuer/operation/operations.go

@@ -104,6 +104,7 @@ const (

 	// WACI interaction constants
 	credentialManifestFormat       = "dif/credential-manifest/manifest@v1.0"
+	credentialManifestVersion      = "1.0.0"


sudeshrshetty · 2022-01-24T19:50:38Z

pkg/restapi/issuer/operation/operations.go

@@ -155,6 +156,7 @@ type Config struct {
 	ExternalURL          string
 	DidDomain            string
 	JSONLDDocumentLoader ld.DocumentLoader
+	CmOutputDescriptor   map[string][]*cm.OutputDescriptor


this object will be heavy, I would suggest you put a helper struct here which has functions to find and return subset of output descriptors by credential scope. So that you can swap underlying memory store with cache in future with little effort.

#580 I will address this optimization in the follow up pr , if its okay with you @sudeshrshetty

sudeshrshetty · 2022-01-24T19:53:05Z

pkg/restapi/issuer/operation/operations.go

+// TODO issue#561 Add credential manifest presentation definition
+func (o *Operation) readCredentialManifest(profileData *issuer.ProfileData,
+	txnCredScope string) *cm.CredentialManifest {
+	for scope, cmDesciptor := range o.cmOutputDescriptor {


loop can be replaced by a single line o.cmOutputDescriptor[txnCredScope]

sudeshrshetty · 2022-01-25T23:50:46Z

I don't see resolution for this comment : #574 (comment)

sudeshrshetty · 2022-01-25T23:49:08Z

pkg/restapi/issuer/operation/operations.go

+			ID:      uuid.NewString(),
+			Version: credentialManifestVersion,
+			Issuer: cm.Issuer{
+				ID:     profileData.ID,


you can not use profile ID here, this is not issuer ID

@sudeshrshetty In here when we are creating credential the issuer id we are using profile.url

adapter/pkg/restapi/issuer/operation/operations.go

Line 1568 in 837ffab

cred.Issuer.ID = profile.URL

Does this implies the issuer adapter is creating credential and thats why using profile URL ? Is this correct ??

this looks wrong, don't use that. Profile ID is some UUID generated to co-relate data in adapter it has nothing to do with actual identity of issuer.

Refer this example for credential manifest: https://identity.foundation/credential-manifest/#credential-manifest---all-features-exercised

For let profile creation option accept issuer ID & Style as input arguments as I mentioned here

@sudeshrshetty I have added issuerID and cmstyle in profile data as when New issuer onboards to adapter it need to provide issuer ID and manifest style for that specific profile.

where it is set?

sudeshrshetty · 2022-01-25T23:49:22Z

pkg/restapi/issuer/operation/operations.go

+			Issuer: cm.Issuer{
+				ID:     profileData.ID,
+				Name:   profileData.Name,
+				Styles: cm.Styles{},


Style has to be provided by issuer

sudeshrshetty · 2022-01-26T15:18:54Z

pkg/restapi/issuer/operation/operations.go

+			ID:      uuid.NewString(),
+			Version: credentialManifestVersion,
+			Issuer: cm.Issuer{
+				ID:     profileData.IssuerID,


I don't see anywhere these being set to profile data??
Where is profile creation step which accepts these 2 additional params - IssuerID & CMStyle?

adapter/pkg/restapi/issuer/operation/operations.go

Line 2102 in 3cbdf9e

IssuerID: data.IssuerID,

Its being set here

@talwinder50 I meant how an issuer can pass it? I don't see these as part of profile creation request

@sudeshrshetty It is mentioned here

adapter/pkg/restapi/issuer/operation/models.go

Line 31 in 33e8e04

IssuerID string `json:"issuerID,omitempty"`

sudeshrshetty · 2022-01-26T15:21:03Z

test/bdd/fixtures/adapter-rest/manifest-config/outputdescriptors.json

@@ -69,7 +69,7 @@
            }
        }
    ],
-    "prc": [
+    "PermanentResidentCard": [


why is it renamed?

local change pushed by mistake

sudeshrshetty · 2022-01-26T17:32:39Z

pkg/restapi/issuer/operation/models.go

@@ -27,6 +28,8 @@ type ProfileDataRequest struct {
 	OIDCClientParams            *OIDCClientParams `json:"oidcParams,omitempty"`
 	CredentialScopes            []string          `json:"scopes,omitempty"`
 	LinkedWalletURL             string            `json:"linkedWallet,omitempty"`
+	IssuerID                    string            `json:"issuerID,omitempty"`


follow up: please add comments explaining usage of these params so that it can show up in documentation.
(can be done in future PRs)

sudeshrshetty · 2022-01-26T17:33:35Z

pkg/restapi/issuer/operation/operations.go

 	return &cm.CredentialManifest{
 		ID: uuid.NewString(),
+		Issuer: cm.Issuer{
+			ID: profileData.ID,


you missed here? issuer ID instead?

closes trustbloc#561 Signed-off-by: talwinder50 <talwinderkaur50@gmail.com>

cla-bot bot added the cla-signed label Jan 21, 2022

talwinder50 force-pushed the issue-561 branch from 62f3bb0 to b1ddf7d Compare January 21, 2022 17:59

talwinder50 force-pushed the issue-561 branch from b1ddf7d to e7b3984 Compare January 21, 2022 18:03

DRK3 reviewed Jan 21, 2022

View reviewed changes

DRK3 approved these changes Jan 21, 2022

View reviewed changes

talwinder50 force-pushed the issue-561 branch 7 times, most recently from a98dfaf to 9f1210e Compare January 24, 2022 13:15

talwinder50 requested review from rolsonquadras, sudeshrshetty and Moopli January 24, 2022 13:45

talwinder50 force-pushed the issue-561 branch from 9f1210e to f721c3b Compare January 24, 2022 15:49

sudeshrshetty reviewed Jan 24, 2022

View reviewed changes

rolsonquadras reviewed Jan 24, 2022

View reviewed changes

sudeshrshetty suggested changes Jan 24, 2022

View reviewed changes

talwinder50 mentioned this pull request Jan 25, 2022

Create helper function for credential manifest output descriptors #580

Closed

talwinder50 force-pushed the issue-561 branch 2 times, most recently from da36692 to 2fb4094 Compare January 25, 2022 22:38

talwinder50 requested review from rolsonquadras and sudeshrshetty January 25, 2022 22:46

rolsonquadras approved these changes Jan 25, 2022

View reviewed changes

sudeshrshetty suggested changes Jan 25, 2022

View reviewed changes

talwinder50 force-pushed the issue-561 branch 2 times, most recently from 1b60db5 to 690b8e6 Compare January 26, 2022 14:45

talwinder50 force-pushed the issue-561 branch from 690b8e6 to f5fa920 Compare January 26, 2022 14:46

talwinder50 requested a review from sudeshrshetty January 26, 2022 14:48

talwinder50 force-pushed the issue-561 branch from f5fa920 to b46f080 Compare January 26, 2022 14:50

sudeshrshetty suggested changes Jan 26, 2022

View reviewed changes

sudeshrshetty reviewed Jan 26, 2022

View reviewed changes

talwinder50 force-pushed the issue-561 branch 2 times, most recently from 3cbdf9e to 33e8e04 Compare January 26, 2022 15:57

talwinder50 requested a review from sudeshrshetty January 26, 2022 15:59

sudeshrshetty reviewed Jan 26, 2022

View reviewed changes

feat: [WACI-Issuance] Support to read credential manifest

9ff8c67

closes trustbloc#561 Signed-off-by: talwinder50 <talwinderkaur50@gmail.com>

talwinder50 force-pushed the issue-561 branch from 33e8e04 to 9ff8c67 Compare January 26, 2022 17:36

talwinder50 requested a review from sudeshrshetty January 26, 2022 17:46

sudeshrshetty approved these changes Jan 26, 2022

View reviewed changes

talwinder50 merged commit fe16daf into trustbloc:main Jan 26, 2022

feat: [WACI-Issuance] Support to read credential manifest #574

feat: [WACI-Issuance] Support to read credential manifest #574

Conversation

talwinder50 commented Jan 21, 2022 • edited

codecov bot commented Jan 21, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudeshrshetty commented Jan 25, 2022

Choose a reason for hiding this comment

talwinder50 Jan 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudeshrshetty Jan 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sudeshrshetty Jan 26, 2022 • edited

Choose a reason for hiding this comment

sudeshrshetty Jan 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talwinder50 commented Jan 21, 2022 •

edited

codecov bot commented Jan 21, 2022 •

edited

talwinder50 Jan 26, 2022 •

edited

sudeshrshetty Jan 26, 2022 •

edited

sudeshrshetty Jan 26, 2022 •

edited

sudeshrshetty Jan 26, 2022 •

edited