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.

Sign up for GitHub

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

Jump to bottom

Get rid of bcrypto dependency #693

Merged
merged 3 commits into from
Sep 19, 2023
Merged

Get rid of bcrypto dependency #693

merged 3 commits into from
Sep 19, 2023

Conversation

r-czajkowski
Copy link
Contributor

@r-czajkowski r-czajkowski commented Sep 6, 2023 •
edited by lukasz-zimnoch
Loading

Refs: #695

We use bcrypto to compute hashes, but since this library is a bit problematic, here we replace it with hashing algorithms from the ethers library. ethers has no hash160 algorithm but under the hood HASH160 = RIPEMD160(SHA256(X)) so it's possible to compute hash160 using ethers.

@r-czajkowski r-czajkowski added the 🔌 typescript TypeScript library label Sep 6, 2023
@r-czajkowski r-czajkowski changed the title Ger rid of bcrypto dependency Get rid of bcrypto dependency Sep 6, 2023
@r-czajkowski
Get rid of bcrypto dependency
e022bdc 
We use `bcrypto` to compute hashes, but since this library is a bit
problematic, here we replace it with hashing algorithms from the
`ethers` library. `ethers` has no `hash160` algorithm but under the hood
`HASH160 = RIPEMD160(SHA256(X))` so it's possible to compute `hash160`
using `ethers`.
@Shadowfiend
Copy link
Contributor

Glory hallelujah.

@mhluongo
Copy link
Member

I am shocked at the size of this PR, wow!

Would be really nice to get test vectors confirming a few hashes 🙏

@lukasz-zimnoch
Copy link
Member

Nice work! However... bcrypto still remains a transitive dependency. This is because bcoin depends on bcrypto as well. Are we sure this PR fully solves the problem?

@tomaszslabon
Copy link
Contributor

tomaszslabon commented Sep 12, 2023 •
edited
Loading

Looks good to me.

@lukasz-zimnoch

Nice work! However... bcrypto still remains a transitive dependency. This is because bcoin depends on bcrypto as well. Are we sure this PR fully solves the problem?

I think this PR is meant to be the very first step.
If we want to get rid of bcrypto entirely we will have to either get rid of bcoin or modify bcoin to use some other library for cryptography.

@Shadowfiend
Copy link
Contributor

Nice work! However... bcrypto still remains a transitive dependency. This is because bcoin depends on bcrypto as well. Are we sure this PR fully solves the problem?

You're breaking my heart 😆 (This makes a lot more sense.)

@lukasz-zimnoch lukasz-zimnoch mentioned this pull request Sep 13, 2023
Closed
@lukasz-zimnoch lukasz-zimnoch mentioned this pull request Sep 15, 2023
Closed
@michalsmiarowski
Add basic tests to compute hashes functions
85c1a45 
Adds basic tests to `computeHash160`, `computeHash256` and `computeScriptHash`
functions so that we are sure they are working exactly the same as when we used
`bcrypto` library inside them.
@lukasz-zimnoch lukasz-zimnoch merged commit 8d64d8c into main Sep 19, 2023
38 checks passed
@lukasz-zimnoch lukasz-zimnoch deleted the get-rid-of-bcrypto branch September 19, 2023 09:30
@lukasz-zimnoch lukasz-zimnoch added this to the typescript/v1.4.0 milestone Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalsmiarowski michalsmiarowski michalsmiarowski left review comments

@lukasz-zimnoch lukasz-zimnoch lukasz-zimnoch approved these changes

@tomaszslabon tomaszslabon Awaiting requested review from tomaszslabon

Assignees

@michalsmiarowski michalsmiarowski

Labels
🔌 typescript TypeScript library
Projects
None yet
Milestone
typescript/v1.4.0
Development

Successfully merging this pull request may close these issues.
6 participants
@r-czajkowski @Shadowfiend @mhluongo @lukasz-zimnoch @tomaszslabon @michalsmiarowski