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: adds support for linked attachments #320

Merged

Conversation

berendsliedrecht
Copy link
Contributor

Implements 0641.

It allows for attachments outside of the credential to be linked to a credential. This means that it will be way less resource intensive as it does not have to sign the entire attachment, but just the link (a sub 100 char string).

For now a default threshold has been set at 5 megabytes inside the mediator as with bigger images it would just get too cumbersome and as stated in the attachment RFC, messages should remain small. Attachments above 5 megabytes are possible, however they have to be hosted and the link(s) have to be supplied in the linked attachment object.

this will help a lot of mobile agents as after some testing with larger attachments (1mb+) in the Animo react native wallet, it crashed.

Setting of the threshold might be better to do somewhere else, but since the mediator is external it might be not something worth implementing.

@berendsliedrecht berendsliedrecht requested a review from a team as a code owner June 11, 2021 12:36
@berendsliedrecht berendsliedrecht changed the title Feature/attachments feat: adds support for linked attachments Jun 11, 2021
Copy link
Contributor

@TimoGlastra TimoGlastra left a comment

Choose a reason for hiding this comment

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

Overall looks good, just some suggestions :)

src/__tests__/proofs.test.ts Show resolved Hide resolved
src/modules/credentials/CredentialUtils.ts Outdated Show resolved Hide resolved
src/modules/credentials/CredentialUtils.ts Outdated Show resolved Hide resolved
src/modules/credentials/CredentialUtils.ts Outdated Show resolved Hide resolved
src/modules/credentials/CredentialsModule.ts Outdated Show resolved Hide resolved
src/utils/HashlinkEncoder.ts Outdated Show resolved Hide resolved
src/utils/HashlinkEncoder.ts Outdated Show resolved Hide resolved
src/utils/HashlinkEncoder.ts Outdated Show resolved Hide resolved
src/utils/HashlinkEncoder.ts Outdated Show resolved Hide resolved
src/utils/MultiHashEncoder.ts Outdated Show resolved Hide resolved
blu3beri added 2 commits June 16, 2021 12:01
Signed-off-by: blu3beri <[email protected]>
Signed-off-by: blu3beri <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2021

Codecov Report

Merging #320 (6a11460) into main (a79e4f4) will increase coverage by 0.13%.
The diff coverage is 94.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #320      +/-   ##
==========================================
+ Coverage   87.86%   87.99%   +0.13%     
==========================================
  Files         224      226       +2     
  Lines        4127     4223      +96     
  Branches      491      520      +29     
==========================================
+ Hits         3626     3716      +90     
- Misses        501      507       +6     
Impacted Files Coverage Δ
src/decorators/attachment/AttachmentExtension.ts 53.84% <ø> (ø)
src/modules/ledger/services/LedgerService.ts 84.84% <0.00%> (ø)
src/utils/attachment.ts 73.33% <73.33%> (ø)
src/modules/credentials/CredentialUtils.ts 98.07% <91.66%> (-1.93%) ⬇️
src/utils/LinkedAttachment.ts 92.85% <92.85%> (ø)
src/modules/credentials/CredentialsModule.ts 87.17% <100.00%> (+0.51%) ⬆️
...les/credentials/messages/IssueCredentialMessage.ts 96.00% <100.00%> (+0.16%) ⬆️
...les/credentials/messages/OfferCredentialMessage.ts 96.55% <100.00%> (+0.12%) ⬆️
...s/credentials/messages/ProposeCredentialMessage.ts 100.00% <100.00%> (ø)
...s/credentials/messages/RequestCredentialMessage.ts 96.15% <100.00%> (+0.15%) ⬆️
... and 10 more

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 a79e4f4...6a11460. Read the comment docs.

src/modules/proofs/services/ProofService.ts Show resolved Hide resolved
src/utils/HashlinkEncoder.ts Outdated Show resolved Hide resolved
src/utils/MultiHashEncoder.ts Outdated Show resolved Hide resolved
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
Signed-off-by: Berend Sliedrecht <[email protected]>
@TimoGlastra TimoGlastra linked an issue Jul 3, 2021 that may be closed by this pull request
@TimoGlastra TimoGlastra requested a review from jakubkoci July 6, 2021 11:46
@TimoGlastra TimoGlastra merged commit ea91559 into openwallet-foundation:main Jul 10, 2021
@berendsliedrecht berendsliedrecht deleted the feature/attachments branch August 31, 2022 07:18
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.

Support for attachments in verifiable credentials
3 participants