-
Notifications
You must be signed in to change notification settings - Fork 102
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
refactor: replaced built-in crypto library with @web5/crypto #816
base: main
Are you sure you want to change the base?
Conversation
Hi @Toheeb-Ojuolape! Thanks for this 🙏. It seems like the tests are failing around encryption, please see: |
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 |
Codecov ReportAttention: Patch coverage is
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 ☔ View full report in Codecov by Sentry. |
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 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. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally got around to doing a more thorough review now that tests + build passes.
@Toheeb-Ojuolape Just a few small things and a couple of questions and this should be good! Appreciate you taking this on! 🙇
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 🤞🏽 |
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! |
What type of PR is this? (check all applicable)
Description
This PR replaces the built-in crypto library with @web5/crypto in the encryption.ts file
Related Tickets & Documents
Resolves #672
Mobile & Desktop Screenshots/Recordings
Added code snippets?
Added tests?
[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?