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 support for Multibase, Multihash and Hashlinks #263

Merged

Conversation

berendsliedrecht
Copy link
Contributor

@berendsliedrecht berendsliedrecht commented May 11, 2021

Implements Multihash, Multibase and Hashlinks.

Adds decoding, encoding and validating for multibase, multihash and hashlinks.

Hashlinks have been implemented accoring to draft-sporny-hashlink-07

The main reason for this implementation is to add multibase for the upcoming did exchange protocol and hashlink for linking credentials to attachments.

Signed-off-by: Berend Sliedrecht berend@animo.id

@berendsliedrecht berendsliedrecht requested a review from a team as a code owner May 11, 2021 13:55
@codecov-commenter
Copy link

Codecov Report

Merging #263 (71863c8) into main (33c85ba) will increase coverage by 0.16%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
+ Coverage   89.47%   89.64%   +0.16%     
==========================================
  Files         195      198       +3     
  Lines        3581     3668      +87     
  Branches      393      406      +13     
==========================================
+ Hits         3204     3288      +84     
- Misses        377      380       +3     
Impacted Files Coverage Δ
src/decorators/attachment/Attachment.ts 100.00% <ø> (ø)
src/utils/BufferEncoder.ts 84.61% <50.00%> (-15.39%) ⬇️
src/utils/hashlink.ts 98.38% <98.38%> (ø)
src/utils/MultibaseEncoder.ts 100.00% <100.00%> (ø)
src/utils/MultihashEncoder.ts 100.00% <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 33c85ba...71863c8. Read the comment docs.

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.

Some small suggestions

src/utils/MultibaseEncoder.ts Outdated Show resolved Hide resolved
src/utils/MultihashEncoder.ts Outdated Show resolved Hide resolved
src/utils/__tests__/MultibaseEncoder.test.ts Outdated Show resolved Hide resolved
src/utils/hashlink.ts Outdated Show resolved Hide resolved
src/utils/hashlink.ts Outdated Show resolved Hide resolved
@TimoGlastra
Copy link
Contributor

TimoGlastra commented May 12, 2021

Although conventional commits allows to use upper case types, I prefer feat: over Feat: for consistency

berendsliedrecht and others added 6 commits May 12, 2021 15:11
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Co-authored-by: Timo Glastra <timo@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Co-authored-by: Timo Glastra <timo@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Co-authored-by: Timo Glastra <timo@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Co-authored-by: Timo Glastra <timo@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
Signed-off-by: Berend Sliedrecht <berend@animo.id>
@TimoGlastra TimoGlastra changed the title Feature: Add support for Multibase, Multihash and Hashlinks feat: add support for Multibase, Multihash and Hashlinks May 14, 2021
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.

Good job. Code is clean, readable, and well-structured 👍 I could resist adding a few suggestions but nothing critical :)

We should rename the files according to a class naming convention at least.

package.json Show resolved Hide resolved
src/utils/MultibaseEncoder.ts Outdated Show resolved Hide resolved
src/utils/MultihashEncoder.ts Outdated Show resolved Hide resolved
src/utils/__tests__/MultihashEncoder.test.ts Outdated Show resolved Hide resolved
src/utils/__tests__/hashlinkEncoder.test.ts Outdated Show resolved Hide resolved
src/utils/__tests__/hashlinkEncoder.test.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
Signed-off-by: berend <berend@animo.id>
package.json Outdated Show resolved Hide resolved
blu3beri added 2 commits May 29, 2021 15:47
Signed-off-by: blu3beri <dev@blueberi.nl>
Signed-off-by: blu3beri <dev@blueberi.nl>
Signed-off-by: blu3beri <berend@animo.id>
Signed-off-by: blu3beri <berend@animo.id>
@TimoGlastra TimoGlastra merged commit 36ceaea into openwallet-foundation:main Jun 2, 2021
@berendsliedrecht berendsliedrecht deleted the feature/hashlink 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.

4 participants