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: add credential info to access attributes #254

Conversation

TimoGlastra
Copy link
Contributor

Fixes #190
Fixes #187

Example:

// credentialRecord.getCredentialInfo()
{
  claims: {
    name: 'Timo',
    date_of_birth: '1998-07-29',
    'country-of-residence': 'The Netherlands',
    'street name': 'Test street',
    age: '22',
  },
  metadata: {
    credentialDefinitionId: 'Th7MpTaRZVRYnPiabds81Y:3:CL:17:TAG',
    schemaId: 'TL1EaPFCZ8Si5aUrqScBDt:2:test-schema-1599055118161:1.0',
  },
}
// credentialInfo.getFormattedClaims()
{
  Age: '22',
  'Country Of Residence': 'The Netherlands',
  'Date Of Birth': '1998-07-29',
  'Street Name': 'Test street',
  Name: 'Timo',
}
const credentialInfo = credentialRecord.getCredentialInfo()
const claims = credentialInfo.claims
const formattedClaims = credentialInfo.getFormattedClaims()

@TimoGlastra TimoGlastra requested a review from JamesKEbert May 1, 2021 12:31
@TimoGlastra TimoGlastra requested a review from a team as a code owner May 1, 2021 12:31
@TimoGlastra
Copy link
Contributor Author

TimoGlastra commented May 1, 2021

This is dependant on #253, so once that's merged I'll rebase

@codecov-commenter
Copy link

codecov-commenter commented May 1, 2021

Codecov Report

Merging #254 (b4aa1d0) into main (e07b90e) will increase coverage by 0.04%.
The diff coverage is 82.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #254      +/-   ##
==========================================
+ Coverage   88.96%   89.01%   +0.04%     
==========================================
  Files         178      179       +1     
  Lines        3381     3422      +41     
  Branches      389      403      +14     
==========================================
+ Hits         3008     3046      +38     
- Misses        373      376       +3     
Impacted Files Coverage Δ
src/modules/credentials/CredentialsModule.ts 85.91% <0.00%> (ø)
...c/modules/credentials/models/IndyCredentialInfo.ts 60.00% <60.00%> (ø)
.../modules/credentials/services/CredentialService.ts 85.09% <77.27%> (-0.48%) ⬇️
src/modules/credentials/CredentialUtils.ts 100.00% <100.00%> (ø)
src/modules/credentials/models/Credential.ts 75.00% <100.00%> (ø)
src/modules/credentials/models/CredentialInfo.ts 100.00% <100.00%> (+40.00%) ⬆️
src/modules/credentials/models/index.ts 100.00% <100.00%> (ø)
...modules/credentials/repository/CredentialRecord.ts 97.82% <100.00%> (+0.52%) ⬆️
src/modules/proofs/services/ProofService.ts 81.96% <100.00%> (ø)

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 e07b90e...b4aa1d0. Read the comment docs.

Copy link
Contributor

@jakubkoci jakubkoci left a comment

Choose a reason for hiding this comment

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

I like the addition of credential preview and metadata but to be honest I don't think it's necessary to do the transformation from the array or the formatting operations. That should be the responsibility of the UI layer and not the framework. I realize that you put some effort into the implementation of these functions but maybe we could rather move them to Aries Bifold. What do you think?

From my experience from UI, you do the iteration through attributes over an array anyway:

Object.keys(attributes).map(key => <Item label={key} value={attributes[key]} />

Then it's more convenient to write:

attributes.map(attr => <Item label={attr.key} value={attr.value} />

@TimoGlastra
Copy link
Contributor Author

I'm fine with removing the formatting stuff, not so making it an array.

An array works well for one level of nesting, not so for multiple levels (IMO). Besides the natural structure of a credential is dict like, not an array. Making it an array would tailor the structure to the UI (which is the responsibility of the UI layer as you said)

Are you okay with that?

@jakubkoci
Copy link
Contributor

jakubkoci commented May 2, 2021

Yeah, of course, my point wasn't about making it array because of convenience for UI. But it makes sense to have the claims as a dict because it aligns more with the W3C spec 👍 (if I understand the spec correctly 🤷‍♂️ 😄 )

@JamesKEbert
Copy link
Contributor

Thanks for your efforts here @TimoGlastra! I agree with you that keeping the existing dictionary structure allows for more flexibility with claim formats.
I also initially agree with @jakubkoci on not performing formatting at the framework layer.
Also, is there overlap we want to consider in terms of internationalization for future work here? Some relevant conversation in the Aries RFCs. Likely a separate issue, but I figure potentially worth mentioning.

as discussed this is the responsiblity of the UI layer, not the framework

Signed-off-by: Timo Glastra <[email protected]>
@TimoGlastra TimoGlastra force-pushed the feature/credentials-attributes branch from 50c922d to 25f7163 Compare May 5, 2021 08:59
@TimoGlastra
Copy link
Contributor Author

OK. Thanks for the feedback both. Updated and ready to merge now

Copy link
Contributor

@jakubkoci jakubkoci left a comment

Choose a reason for hiding this comment

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

Sorry I wanted to go through that yet again after rebase. Overall good, just one non-blocking note about the naming.

// TODO: Ideally we don't throw here, but instead store that it's not equal.
// the credential may still have value, and we could just respond with an ack
// status of fail
if (credentialRecord.credentialAttributesValues) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems we could shorten credentialAttributesValues just to credentialAttributes. Does it contain a list of CredentialPreviewAttribute right? I thought that it contains just values from credential preview attributes but the call convertAttributesToValues(attributesValues) confused me a little bit. But maybe I'm missing something 🤷‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we only store the values we lose the mime-types. These are not stored by indy. So you'd need to extract them from one of the exchanged messages.

I think it's a burden the mime-types are not encoded in the values itself like data:image/png;base64,xxxxxxxx, but until that's the case (which is the pattern I've seen for W3C credentials) we need to store the preview attributes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But credentialAttributes sounds fine. Will update

@TimoGlastra TimoGlastra merged commit 2fef3aa into openwallet-foundation:main May 6, 2021
@TimoGlastra TimoGlastra deleted the feature/credentials-attributes branch May 6, 2021 07:37
berendsliedrecht pushed a commit to berendsliedrecht/credo-ts that referenced this pull request May 11, 2021
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.

Validate received credential against offer Credentials have no easy way to access its attributes
4 participants