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

Remove node stream polyfill #1407

Merged
merged 7 commits into from
Feb 11, 2022
Merged

Remove node stream polyfill #1407

merged 7 commits into from
Feb 11, 2022

Conversation

davidyuk
Copy link
Member

@davidyuk davidyuk commented Feb 9, 2022

I'm planning to rename TxBuildHelper.decode/encode to something more general, but it is not ready yet.

The need to switch from bs58check to bs58 is explained in aeternity/aepp-calldata-js#122.

bip39 package can be configured to support different languages and this is not up to SDK to decide which of them should be included. Also, it depends on a set of unnecessary crypto libraries that is not easy to get rid of: bitcoinjs/bip39#170. It has a quite simple blockchain-agnostic interface. Our @aeternity/bip39 didn't get updates in 3 years and probably shouldn't be used. So, I'm proposing to make the wallet developer use bip39 package by himself and remove it from sdk.

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #1407 (4ee734c) into develop (481464c) will increase coverage by 19.32%.
The diff coverage is 100.00%.

❗ Current head 4ee734c differs from pull request most recent head dbeae88. Consider uploading reports for the commit dbeae88 to get more accurate results

Impacted file tree graph

@@             Coverage Diff              @@
##           develop    #1407       +/-   ##
============================================
+ Coverage    68.52%   87.84%   +19.32%     
============================================
  Files           23       54       +31     
  Lines         1258     3077     +1819     
  Branches        40       40               
============================================
+ Hits           862     2703     +1841     
+ Misses         385      353       -32     
- Partials        11       21       +10     
Impacted Files Coverage Δ
src/utils/errors.ts 69.53% <ø> (+22.32%) ⬆️
src/channel/handlers.js 78.97% <100.00%> (ø)
src/index.js 100.00% <100.00%> (ø)
src/tx/builder/helpers.js 98.93% <100.00%> (+25.12%) ⬆️
src/utils/crypto.js 98.03% <100.00%> (+8.33%) ⬆️
src/utils/hd-wallet.js 95.55% <100.00%> (-0.53%) ⬇️
src/utils/keystore.js 94.54% <100.00%> (ø)
src/tx/tx-object.js 94.73% <0.00%> (-0.10%) ⬇️
src/tx/builder/field-types.js
src/tx/validator.js 100.00% <0.00%> (ø)
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 481464c...dbeae88. Read the comment docs.

@@ -34,9 +34,8 @@
"SDK"
],
"dependencies": {
"@aeternity/aepp-calldata": "^1.1.0",
"@aeternity/aepp-calldata": "github:aeternity/aepp-calldata-js#a623a44a259cc53e99b1e358ecc7b3f88edcb45c",
Copy link
Collaborator

Choose a reason for hiding this comment

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

will there be a release with these changes included?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I think so

@davidyuk davidyuk merged commit 193d457 into develop Feb 11, 2022
@davidyuk davidyuk deleted the feature/bs58 branch February 11, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants