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

Add ledger account index in advanced settings #370

Closed
wants to merge 1 commit into from

Conversation

Duddino
Copy link
Member

@Duddino Duddino commented Jul 17, 2024

Abstract

This adds a temporary advanced setting to allow the user to customize their account index for ledger wallets. This PR will probably be reverted when multi account are going to be merged.

Testing

  • Toggle on advanced settings
  • Change the custom account index to a new value
  • Test that subsequent hardware wallet imports use this new account (I don't have access to a ledger, so I cannot test this myself)
  • Test that the account index is remembered when refreshing the page
  • Test that the account index is reset when toggling off advanced mode

Closes #357

@Duddino Duddino self-assigned this Jul 17, 2024
Copy link

netlify bot commented Jul 17, 2024

Deploy Preview for cheery-moxie-4f1121 ready!

Name Link
🔨 Latest commit 6fa3b42
🔍 Latest deploy log https://app.netlify.com/sites/cheery-moxie-4f1121/deploys/6697c0bae1cd800007f814a1
😎 Deploy Preview https://deploy-preview-370--cheery-moxie-4f1121.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@panleone
Copy link
Member

panleone commented Jul 19, 2024

I don't like that the account index is changed from the settings:

  • In this way it doesn't seem related to the wallet
  • The value is changeable when you have already imported/created a wallet ( this would be absolutely non-intuitive for any user, even if advanced)
  • It goes back to zero if you turn off advanced settings ( this is also non-intuitive)

Then there is also the problem that you have to remember all the indexes you have used, because we don't have a multi-account system yet.

So overall I think that this feature should be added directly with the multi-account system, but if we really want to go with this implementation we should at least move the account index selection away from the settings.

I would do something like:

  • User clicks on "Access my Ledger"
  • If in advanced mode it is asked: what account index you want to use?
  • User inserts the account index

@Duddino
Copy link
Member Author

Duddino commented Jul 25, 2024

I think it's not worth mantaining this since it's supposed to be a band aid until we finally implement multi account (right after ui-redesign I imagine), so I'm closing this, nevertheless if someone really needs this feature we could point them to this PR in the meantime

@Duddino Duddino closed this Jul 25, 2024
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

Successfully merging this pull request may close these issues.

Feature Request: Multiple Accounts Using Ledger
2 participants