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 from record method to cred preview #428

Conversation

TimoGlastra
Copy link
Contributor

Two api improvements

  1. register credential definition doesn't require signatureType parameter anymore (just add bloat to the API)
  2. add fromRecord method to credential preview to be able to create a preview from json object. this makes it a lot simpler:

Before

const credentialPreview = new CredentialPreview({
  attributes: [
    new CredentialPreviewAttribute({
      name: 'name',
      mimeType: 'text/plain',
      value: 'John',
    }),
    new CredentialPreviewAttribute({
      name: 'age',
      mimeType: 'text/plain',
      value: '99',
    }),
  ],
})

After

const credentialPreview = CredentialPreview.fromRecord({
  name: 'John',
  age: '99',
})

Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra requested a review from a team as a code owner August 19, 2021 15:00
@codecov-commenter
Copy link

codecov-commenter commented Aug 19, 2021

Codecov Report

Merging #428 (9d9cf84) into main (0e2338f) will decrease coverage by 0.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   86.24%   85.61%   -0.64%     
==========================================
  Files         249      249              
  Lines        5264     5346      +82     
  Branches      784      798      +14     
==========================================
+ Hits         4540     4577      +37     
- Misses        724      769      +45     
Impacted Files Coverage Δ
.../modules/credentials/messages/CredentialPreview.ts 84.61% <100.00%> (+2.79%) ⬆️
packages/core/src/modules/ledger/LedgerModule.ts 90.00% <100.00%> (ø)
packages/core/src/agent/MessageSender.ts 71.92% <0.00%> (-2.30%) ⬇️
packages/core/src/agent/Agent.ts 94.68% <0.00%> (-2.20%) ⬇️
.../core/src/modules/connections/ConnectionsModule.ts 72.15% <0.00%> (-1.88%) ⬇️
...ckages/core/src/transport/WsOutboundTransporter.ts 3.94% <0.00%> (-1.32%) ⬇️
...ckages/core/src/modules/routing/RecipientModule.ts 63.04% <0.00%> (-0.96%) ⬇️
.../modules/connections/models/did/service/Service.ts 100.00% <0.00%> (ø)
.../modules/connections/services/ConnectionService.ts 98.83% <0.00%> (+0.01%) ⬆️

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 0e2338f...9d9cf84. Read the comment docs.

@berendsliedrecht
Copy link
Contributor

Once test passes, I will merge.

@@ -53,7 +48,7 @@ export async function e2eTest({
issuerConnectionId: senderRecipientConnection.id,
credentialTemplate: {
credentialDefinitionId: definition.id,
preview: previewFromAttributes({
preview: CredentialPreview.fromRecord({
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this improvement. I would consider calling it fromJSON to prevent confusion with other framework's records, that's what I would expect as a consumer at least.

To me, the best API would be just passing JSON objects, but this is definitely step forward 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with fromJSON is that it clashes with toJSON.

Currently, toJSON transforms the class instance to the JSON representation. If we have a fromJSON method, I'd expect it to be the exact opposite of toJSON. Which it is not.

The reason I didn't change the constructor is because the fromRecord method removes the option to set a mime-type. Most time we just use text/plain, but it should be possible to set it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not set on fromRecord, so if you have suggestions I'm happy to create a new PR

Signed-off-by: Timo Glastra <timo@animo.id>
@berendsliedrecht berendsliedrecht enabled auto-merge (squash) August 20, 2021 08:10
@berendsliedrecht berendsliedrecht merged commit 895f7d0 into openwallet-foundation:main Aug 20, 2021
@TimoGlastra TimoGlastra deleted the feat/api-simplifications branch October 28, 2021 10:11
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.

4 participants