Skip to content

feat: remove async from crypto API #1558

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

Merged

Conversation

AngelCastilloB
Copy link
Member

@AngelCastilloB AngelCastilloB commented Jan 14, 2025

Context

The crypto interface currently is async because it uses libsodium underneath which requires an async call to sodium.ready. We can remove the async from the interface and hoist it to the strategy constructor or a global method async Crypto.ready, that can be called by the client code at their convenience.

Resolves #1529

Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Looks good! Please update commit message to indicate breaking changes

@AngelCastilloB AngelCastilloB marked this pull request as draft January 14, 2025 07:00
@AngelCastilloB AngelCastilloB marked this pull request as ready for review January 14, 2025 08:42
mirceahasegan
mirceahasegan previously approved these changes Jan 14, 2025
Copy link
Contributor

@mirceahasegan mirceahasegan left a comment

Choose a reason for hiding this comment

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

Fantastic work @AngelCastilloB 🚀

mkazlauskas
mkazlauskas previously approved these changes Jan 14, 2025
Copy link
Member

@mkazlauskas mkazlauskas left a comment

Choose a reason for hiding this comment

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

Nice work! 🚀

@AngelCastilloB AngelCastilloB force-pushed the feat/lw-12104-remove-async-from-crypto-interfaces branch from f122c41 to dfc1811 Compare January 15, 2025 01:21
mkazlauskas
mkazlauskas previously approved these changes Jan 15, 2025
BREAKING CHANGE: The package now exports an async `ready` function that must be called before any of crypto related functions can be called
- Bip32PrivateKe async functions are all now sync
- Bip32PublicKey class async functions are all now sync
- Ed25519PrivateKey class async functions are all now sync
- Ed25519PublicKey class async functions are all now sync
- Bip32Ed25519 interface async functions are all now sync
- SodiumBip32Ed25519 cosntructor is now private
- SodiumBip32Ed25519 now has a new async factory method create
@AngelCastilloB AngelCastilloB force-pushed the feat/lw-12104-remove-async-from-crypto-interfaces branch from 1719108 to 91b7fa2 Compare January 15, 2025 07:29
@AngelCastilloB AngelCastilloB merged commit ea0b2c2 into master Jan 15, 2025
5 of 6 checks passed
@AngelCastilloB AngelCastilloB deleted the feat/lw-12104-remove-async-from-crypto-interfaces branch January 15, 2025 08:17
mkazlauskas added a commit that referenced this pull request Jan 21, 2025
Bip32Ed25519 was refactored to synchronous methods by hoisting
sodium await to top level in #1558, which makes it difficult
to use SigningCoordinator without top level await
mkazlauskas added a commit that referenced this pull request Jan 21, 2025
Bip32Ed25519 was refactored to synchronous methods by hoisting
sodium await to top level in #1558, which makes it difficult
to use SigningCoordinator without top level await
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.

remove the use of await sodium.ready
3 participants