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

fix: several BIP 38 issues and refactor the implementation #1115

Merged
merged 11 commits into from Mar 6, 2019

Conversation

@j-a-m-l
Copy link
Contributor

commented Mar 6, 2019

Proposed changes

This PR removes the duplication that was used to handle BIP 38. Apart from that, it fixes several things:

  • The Bip38 worker wasn't stopped in some cases
  • Failing to encrypt or decrypt the passphrase wasn't displaying errors
  • The Bip38 worker was forked without being necessary in several occasions (which could be expensive in some computers)
  • The tests were failing locally due the background worker plugin

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Test (adding missing tests or fixing existing tests)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
@codecov-io

This comment has been minimized.

Copy link

commented Mar 6, 2019

Codecov Report

Merging #1115 into develop will increase coverage by 0.69%.
The diff coverage is 19.04%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1115      +/-   ##
===========================================
+ Coverage    40.22%   40.91%   +0.69%     
===========================================
  Files          209      211       +2     
  Lines         5467     5404      -63     
  Branches      1073     1051      -22     
===========================================
+ Hits          2199     2211      +12     
+ Misses        3147     3069      -78     
- Partials       121      124       +3
Impacted Files Coverage Δ
src/renderer/pages/Wallet/WalletNew.vue 40% <ø> (+4.44%) ⬆️
src/renderer/workers/bip38-worker.js 0% <ø> (ø) ⬆️
src/renderer/main.js 0% <ø> (ø) ⬆️
src/renderer/pages/Wallet/mixin-on-create.js 0% <0%> (ø)
...ransaction/TransactionForm/TransactionFormVote.vue 10.98% <0%> (+2.49%) ⬆️
...TransactionForm/TransactionFormSecondSignature.vue 13.51% <0%> (+3.4%) ⬆️
...actionForm/TransactionFormDelegateRegistration.vue 12.12% <0%> (+3.47%) ⬆️
src/renderer/pages/Wallet/WalletImport.vue 11.42% <0%> (+3.38%) ⬆️
...action/TransactionForm/TransactionFormTransfer.vue 6.89% <0%> (+1.59%) ⬆️
...nts/Transaction/TransactionForm/mixin-on-submit.js 0% <0%> (ø)
... and 12 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 26da980...d2b2ba6. Read the comment docs.

@luciorubeens
Copy link
Member

left a comment

Nice work! 👍

@j-a-m-l j-a-m-l merged commit 7644362 into develop Mar 6, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details

@ArkEcosystemBot ArkEcosystemBot deleted the fix-bip38 branch Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.