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

Feature/ledger multi #329

Merged
merged 17 commits into from
May 5, 2023
Merged

Feature/ledger multi #329

merged 17 commits into from
May 5, 2023

Conversation

ccali11
Copy link
Contributor

@ccali11 ccali11 commented May 4, 2023

@shanejearley - This is a PR related to the items outlined in #311. It doesn't necessarily make auth/login flow "flawless" yet, so it doesn't necessarily close out the task, but it is a significant improvement to previous flow as it:

  1. Enables Ledger login (with multiple account selection enabled)
  2. Enables Trezor login (with multiple account selection enabled)
  3. Re-enables WalletConnect login
  4. Decouples provider and account selection from login and /add-account for more modular use in our login flow (gives user the option to select one of multiple addresses they own)
  5. Adds more code structure and consistency

Some improvements to consider for future GH issues of mine:

  • There seem to be a few non-blocking TS issues that can be resolved, but are beyond me at the moment and I don't want to block the PR review.
  • We want to be smart about how many accounts to show each user at a time (Ledger has a little time lag, the more accounts we retrieve, the slower the UX)
  • Add account is not usable in this branch; need to integrate with new UI/UX so that we can await checkUserSession() at the appropriate time (in other words, I'll re-enable this very easily/simply when I integrate new UI/UX with these auth composables)
  • I haven't been able to find the HD derivation path for Goerli Testnet, so I'm not able to see my Goerli account on my Trezor (Trezor treats this differently than Ledger from what I can tell so far)
  • The Sign In With Ethereum message that shows up on the Trezor HW device is not human readable; might be a firmware limitation and a chance to contribute to Trezor if that firmware is open sourced.

If changes are appreciated, it would be nice to merge in to I can integrate it with new UI/UX for auth flow.

@ccali11 ccali11 requested a review from shanejearley May 4, 2023 17:08
@ccali11 ccali11 merged commit db016d6 into develop May 5, 2023
@ccali11 ccali11 deleted the feature/ledger-multi branch May 5, 2023 00:43
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.

None yet

2 participants