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: showPublicKey bug with address import #780

Merged
merged 5 commits into from Dec 18, 2018

Conversation

@JeremiGendron
Copy link
Contributor

commented Dec 17, 2018

Proposed changes

#771 doesn't set the activeWallet, which means if you switch to a second wallet the fix is voided. This is fixed in this PR and refactored a little bit, so as to store the id of the wallet only and not the whole object.

The other change I've implemented here is to watch for changes in the currentWallet's publicKey in the WalletSidebar in order to check whether the wallet has one. If the wallet has no publicKey, it can cause a bug where the heading doesn't get reset.

  1. The wallet heading reset is still misbehaving
  • Before
    before_heading_noset

  • After
    after_heading_set

  1. The publicKey bug mentioned above (imported wallet with address only)
  • Before
    before_publickey_noreset

  • After
    after_publickey_reset

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)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build (changes that affect the build system)
  • Docs (documentation only changes)
  • Test (adding missing tests or fixing existing tests)
  • Other... Please describe:

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
  • I have added necessary documentation (if appropriate)
@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2018

Activewallet is set in resetHeading() tho:

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Dec 17, 2018

Also: I'm unable to reproduce what you show in your gif. With latest develop I can't get the heading to reset itself at random 🤷‍♂️

The public key issue is valid tho, good catch :D

@JeremiGendron

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

@ItsANameToo I admittedly didn't review the changes that well, let me see if I can reproduce in develop.

@JeremiGendron JeremiGendron changed the title fix: resetHeading() doesn't set activeWallet && showPublicKey bug with address import fix: showPublicKey bug with address import Dec 18, 2018

@ItsANameToo ItsANameToo merged commit 29c0780 into ArkEcosystem:develop Dec 18, 2018

1 check passed

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

@JeremiGendron JeremiGendron deleted the JeremiGendron:fix/wallet-heading-pubkey-issue branch Dec 18, 2018

@dated dated referenced this pull request Dec 19, 2018
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.