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

feat: load x ledger wallets #1059

Merged
merged 22 commits into from Mar 7, 2019

Conversation

@alexbarnsley
Copy link
Member

commented Feb 4, 2019

Proposed changes

This feature allows Ledger users to load X amount of Ledger wallets. The reason for this is, for example, some people may want to see a larger amount of wallets. The other reason is for users to access their Phantom tokens where the Airdrop took place on Ledger wallet 10 for example (which may also happen in the future with other chains).
this also improves the loading of ledger wallets and has the ability to stop any currently ongoing process.

Types of changes

  • New feature (non-breaking change which adds functionality)

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

@alexbarnsley alexbarnsley requested review from j-a-m-l and luciorubeens Feb 4, 2019

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@j-a-m-l @luciorubeens - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@j-a-m-l

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

@alexbarnsley I'd not merge this PR until having the Ledger profile settings, when I'd move this setting to that tab, what do you think?

@alexbarnsley

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

@j-a-m-l I wouldn't consider this a setting. It's a task to load X ledger wallets as per a request from @boldninja. If the ledger wallets aren't cached then the list of ledger wallets resets on profile change or wallet reload

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Mar 1, 2019

Some issues while using it:

  1. I can select 0 wallets to load (it will load 1)

schermafbeelding 2019-03-01 om 16 59 11

  1. Text not centered on smaller screen

schermafbeelding 2019-03-01 om 16 58 45

  1. When I load 3 wallets, then turn caching on, then want to load 1 wallet; it will load 3 wallets again
  2. Maybe "add ledger wallets" is not the right name for the button, as it can also be used to remove ledger wallets? E.g. you are shown 10 and then set it to 3 with that button, you'll have removed 7. I don't know what to call it yet though.
  3. A warning to say that loading too many ledger wallets can take a very long time, and the ledger device may sign out before it's finished
@alexbarnsley

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

you wasn't supposed to find issues 😭

@alexbarnsley alexbarnsley requested a review from faustbrian as a code owner Mar 6, 2019

@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Mar 6, 2019

The ci/circleci: test-node-11 job is failing as of 35c74c74b33542c6d4ef499d6b9de2228a292c3e. Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

@alexbarnsley alexbarnsley force-pushed the feat/add-ledger-wallets branch from 45ef958 to 88cfbef Mar 6, 2019

@ItsANameToo

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

Looks good. Only thing I run into is that if I have a ledger connected it will spam [16:40:51.098] Error: Ledger device: Incorrect length (0x6700) messages. It stops when I open the ark app on the ledger 🤔

@ItsANameToo ItsANameToo merged commit 3f404fd into develop Mar 7, 2019

1 check passed

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

@ArkEcosystemBot ArkEcosystemBot deleted the feat/add-ledger-wallets branch Mar 7, 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.