dwn-sdk-js icon indicating copy to clipboard operation
dwn-sdk-js copied to clipboard

refactor: replaced built-in crypto library with @web5/crypto

Open Toheeb-Ojuolape opened this issue 1 year ago • 7 comments
trafficstars

What type of PR is this? (check all applicable)

  • [x] ♻️ Refactor
  • [ ] ✨ New Feature
  • [ ] 🐛 Bug Fix
  • [ ] 📝 Documentation Update
  • [ ] 👷 Example Application
  • [ ] 🧑‍💻 Code Snippet
  • [ ] 🎨 Design
  • [ ] 📖 Content
  • [ ] 🧪 Tests
  • [ ] 🔖 Release
  • [ ] 🚩 Other

Description

This PR replaces the built-in crypto library with @web5/crypto in the encryption.ts file

Related Tickets & Documents

Resolves https://github.com/TBD54566975/dwn-sdk-js/issues/672

Mobile & Desktop Screenshots/Recordings

Added code snippets?

  • [ ] 👍 yes
  • [x] 🙅 no, because they aren't needed

Added tests?

  • [ ] 👍 yes
  • [ ] 🙅 no, because they aren't needed
  • [ ] 🙋 no, because I need help
  • [x] 🥺 I ran unit tests for encryption.ts file and it passed

[optional] Are there any post-deployment tasks we need to perform?

Please run npm install or make sure @web5/crypto package is installed before testing

[optional] What gif best describes this PR or how it makes you feel?

Demo GIF

Toheeb-Ojuolape avatar Oct 04 '24 20:10 Toheeb-Ojuolape

Hi @Toheeb-Ojuolape! Thanks for this 🙏.

It seems like the tests are failing around encryption, please see: https://github.com/TBD54566975/dwn-sdk-js/actions/runs/11186574275/job/31314960415?pr=816#step:9:1057

LiranCohen avatar Oct 09 '24 19:10 LiranCohen

Hi @Toheeb-Ojuolape! Thanks for this 🙏.

It seems like the tests are failing around encryption, please see: https://github.com/TBD54566975/dwn-sdk-js/actions/runs/11186574275/job/31314960415?pr=816#step:9:1057

Hi @LiranCohen thanks for your message. I have refactored my code for encryption using web5/crypto and ran the failing npm run test:node command on my local and all tests passed. I think all should be well now. Looking forward to your feedback

Toheeb-Ojuolape avatar Oct 11 '24 22:10 Toheeb-Ojuolape

Codecov Report

Attention: Patch coverage is 94.33962% with 6 lines in your changes missing coverage. Please review.

Project coverage is 98.95%. Comparing base (8b6eb13) to head (3d68bc2). Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/utils/encryption.ts 94.28% 6 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #816      +/-   ##
==========================================
+ Coverage   98.71%   98.95%   +0.23%     
==========================================
  Files          74       74              
  Lines       11469    11560      +91     
  Branches     1652     1680      +28     
==========================================
+ Hits        11322    11439     +117     
+ Misses        141      115      -26     
  Partials        6        6              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Oct 16 '24 13:10 codecov-commenter

Hey @Toheeb-Ojuolape! Thanks for the update!

There seems to still be a failure in the browser tests: https://github.com/TBD54566975/dwn-sdk-js/actions/runs/11299952497/job/31617946660?pr=816

you can run npm run test:browser locally to reproduce the failure. It seems to be around base64url encoding, you can take a look at https://github.com/TBD54566975/dwn-sdk-js/blob/main/src/utils/encoder.ts for some common methods we use which could be helpful.

Additionally if you could add test cases for the failures to get patch coverage to 100%, the Codecov link above will be helpful in ensuring that.

LiranCohen avatar Oct 16 '24 14:10 LiranCohen

Hi @LiranCohen I've refactored the code it seems all should be fine now. I ran the browser test locally before I pushed and it worked fine

Toheeb-Ojuolape avatar Oct 17 '24 11:10 Toheeb-Ojuolape

Hi @Toheeb-Ojuolape - can you resolve to the comments above so that @LiranCohen may review/merge? Thank you!

taniandjerry avatar Oct 23 '24 17:10 taniandjerry

Hi @Toheeb-Ojuolape - can you resolve to the comments above so that @LiranCohen may review/merge? Thank you!

Hi @taniashiba I think I've resolved all the comments now and I think the PR is ready. Fingers crossed 🤞🏽

Toheeb-Ojuolape avatar Oct 24 '24 18:10 Toheeb-Ojuolape

Hey @LiranCohen - could you please review this PR and if approved, add the hacktoberfest-accepted label to it as well as soon as you can? Thank you!

taniandjerry avatar Oct 31 '24 18:10 taniandjerry