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

Use utils from ethereum-cryptography package #558

Closed
prichodko opened this issue Feb 21, 2022 · 5 comments
Closed

Use utils from ethereum-cryptography package #558

prichodko opened this issue Feb 21, 2022 · 5 comments

Comments

@prichodko
Copy link

As a part of the process of migrating from Node builtins, I would move to use utilities, like hexToBytes, bytesToHex from the ethereum-cryptography package.

As you can see in the implementation, they don't rely on the Buffer builtin.

@D4nte
Copy link
Contributor

D4nte commented Feb 22, 2022

Thanks @prichodko. Could you please clarify the gain of using implementations from ethereum-cryptography over the current implementation? I am not sure we want to add a new dependency.

@prichodko
Copy link
Author

prichodko commented Feb 22, 2022

Current implementation indirectly forces users to depend on a Buffer polyfill and essentially adds 2000 lines / 26kB of code. The proposed solution suggests only using what is needed for the package to work.

The ethereum-cryptography package seems to be tree-shakeable, therefore allowing bundlers to pull only the imported code and not the whole package. However, there may be other parts depending on the Buffer, which would need the refactoring as well.

@prichodko
Copy link
Author

Do you know, if any js-waku dependencies need Buffer polyfill?

@D4nte
Copy link
Contributor

D4nte commented Feb 23, 2022

Do you know, if any js-waku dependencies need Buffer polyfill?

This will be answered as part of #527

Current implementation indirectly forces users to depend on a Buffer polyfill and essentially adds 2000 lines / 26kB of code. The proposed solution suggests only using what is needed for the package to work.

hextToBytes and bytesToHex do not use Buffer:

https://github.com/status-im/js-waku/blob/c1c1858175940264845721bb8db24ae19b281e10/src/lib/utils.ts#L19

I think you are referring to hexToBuf and bufToHex who already are on their way out.

I guess we had a race condition between you investigating and me changing the code :).

@D4nte
Copy link
Contributor

D4nte commented May 20, 2022

The worry here is the usage of the Buffer polyfill, this is resolved with #726.

Note that ts-proto uses Buffer: tracked with #335.
Some dependencies are using buffer: Will review as part of #527

@D4nte D4nte closed this as completed May 20, 2022
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

No branches or pull requests

2 participants