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.

Sign up for GitHub

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

Jump to bottom

feat: custom derivation path - 2nd attempt #1049

Merged
merged 14 commits into from
Sep 6, 2023
Merged

feat: custom derivation path - 2nd attempt #1049

merged 14 commits into from
Sep 6, 2023

Conversation

0xKheops
Copy link
Contributor

@0xKheops 0xKheops commented Aug 30, 2023 •
edited
Loading

  • import account form
  • new account form

@0xKheops 0xKheops marked this pull request as ready for review September 1, 2023 10:31
@0xKheops 0xKheops requested a review from chidg September 1, 2023 10:31
@0xKheops 0xKheops linked an issue Sep 4, 2023 that may be closed by this pull request
Custom Derivation Path #989
Closed
chidg
chidg reviewed Sep 5, 2023
View reviewed changes
apps/extension/src/ui/apps/onboard/routes/AddAccount/AccountAddDerivedPage.tsx

return (
<AccountAddWrapper
title={t("Create a new account")}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The t function is called on the props given inside AccountAddWrapper. I wasn't entirely sure if this would work though and hadn't tested it yet. If it doesn't work, we should also update AccountAddWrapper to remove the t call there.

@chidg
Copy link
Contributor

chidg commented Sep 5, 2023 •
edited
Loading

A few general UX observations:

  • When I go to derive a 2nd account from an existing stored mnemonic, and choose custom derivation path, the placeholder displays the first derivation path, and the error message "Address already exists" appears. Can we delay validation until the input has been touched?

  • Unchecking the 'custom derivation path' input does not clear this error message, and the error is still not cleared when I re-check it and enter something in the input in some cases

  • There is a layout issue on the form, notice the top of the checkbox has been clipped by the element above
    Screenshot 2023-09-05 at 11 35 18 am

  • Maybe it would be preferable to have the derivation path as a default value on the input rather than a placeholder - then it could be edited in place rather than entered from scratch as it is on the import from recovery phrase screen.

  • Entering an invalid derivation path is not caught by the form
    Screenshot 2023-09-05 at 11 43 20 am

  • For the import recovery phrase screen, the form is valid when custom derivation derivation path is chosen for an EVM account, but the derivation path input is empty. Not sure if this is intended or not?

@0xKheops
Copy link
Contributor Author

0xKheops commented Sep 5, 2023

For the import recovery phrase screen, the form is valid when custom derivation derivation path is chosen for an EVM account, but the derivation path input is empty. Not sure if this is intended or not?

Yes empty derivation path is valid

chidg
chidg approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@chidg chidg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@0xKheops 0xKheops merged commit 9b8e380 into release/1.19 Sep 6, 2023
4 checks passed
@0xKheops 0xKheops deleted the feat/custom-derivation-path-2 branch September 6, 2023 02:07
chidg added a commit that referenced this pull request Sep 14, 2023
@chidg @0xKheops
Release 1.19 (#1031)
e724c66 
* feat: add migrations store

* feat: run migrations on startup when installed or updated

* refactor: tidy up legacy migrations

* chore: changed name to runner and tidied up

* refactor: move migrations code to libs

* feat: wip: multiple mnemonic seed store mostly works

* feat: fix problem migrating mnemonic when changing password

* fix: give password to migrations in context allowing seed access during migration

* fix: migration runner tests

* fix: layout dead click zone at bottom

* feat: accordion className

* feat: mnemonics page

* feat: accounts iconstack

* wip: manage recovery phrases ui

* feat: delete mnemonic

* feat: save mnemonic when importing account(s)

* feat: backup only from recovery phrases page

* fix: migration mnemonic find error

* fix: attach accounts to mnemonic on onboarding

* fix: import without derivation path

* chore: set package version

* feat: disabled item style in context menu

* feat: backup mnemonic

* Onboarding v5 (#993)

* refactor: onboarding welcome page

* feat: add progress bar to onboarding and update password input stage

* feat: wip: cleanup privacy onboarding page

* chore: replace some styled components with tailwind classes

* chore: remove more styled components

* refactor: reduce background opacity as onboard stages progress

* fix: improve layout of privacy page

* refactor: remove unused

* feat: add pictures to onboarding background

* fix: improve performance of mystical background

* feat: add do it later button to onboard progress bar

* refactor: remove account related tasks from onboard handler

* refactor: update handlers to allow onboarding without account creation

* feat: handle case where user navigates back in onboarding after setting pw

* feat: complete basic entry menu UI for add account flow

* chore: tidy up and ensure cross browser compat

* feat: add onboarding success page

* chore: remove onboard status messages and rely on app state subscription only

* chore: tidy up

* feat: update fullscreen sidebar to handle no accounts

* feat: update popup to handle no accounts

* feat: add nice empty state message to fullscreen portfolio

* chore: tidy up

* feat: use new account selection menu for account creation in dashboard

* feat: all create new account via onboarding

* feat: allow create new account by json import in onboarding

* feat: allow account creation via import seed phrase in onboarding

* feat: allow import of ledger account in onboarding

* fix: remove @ui react-app-env types rule

* fix: prevent onboard background animation pausing on hover

* fix: minor UI improvements on add account screen

* fix: failing tests

* fix: checkPassword if pw not hashed (v1) (#1029)

* feat: fine tune backup & delete modals

* feat: icon to reveal mnemonic

* fix: backup modal adjustments

* fix: backup modal layout shift

* fix: unused imports

* Remove mystical acolyte (#1032)

* chore: kill mystical background acolyte

* chore: changeset

* chore: cleanup hook (#1033)

---------

Co-authored-by: Kheops <26880866+0xKheops@users.noreply.github.com>

* feat: show mnemonic before creating account (#1036)

* feat: set PV certifier & sort mnemonics/accounts

* fix: eslint

* fix: import from PK

* wip: show mnemonic before account create

* feat: new mnemonic for new account

* feat: obfuscate account creation msg

* feat: allow removal of root account

* fix: expotable origins

* fix: style

* feat: can't mark as backed up until revealed

* fix: number color

* feat: copied text

* feat: account tooltip

* fix: hasFunds false by default

* chore: remove account tooltip

* fix: multi account import from mnemonic

* chore: cleanup

* refactor: performance improvement

Co-authored-by: Chid Gilovitz <chid@talisman.xyz>

* fix: suggestion

* refactor: mnemonic fetch

* fix: tests (wip)

* fix: tests

* fix: signing tests

---------

Co-authored-by: Chid Gilovitz <chid@talisman.xyz>

* More onboarding improvements (#1047)

* fix: prefer Balances rather than Balance[] on account import

* chore: rename AccountCreate -> AccountAdd for consistency

* feat: add watched account as onboarding account creation option

* fix: yeet Kusama

* refactor: tidy up

* refactor: add AccountAddWrapper to unify onboarding account add component UI

* feat: allow onboarding with qr code account

* chore: turn off i18n debug

* feat: prevent creation of account in onboarding when user is not logged in

* fix: improve copy on onboarding success page

* fix: navigation after password in onboarding

* fix: prevent flicker on fullscreen portfolio when no accounts

* fix: broken link on watched account from portfolio no accounts empty state

* feat: enable DCent account creation in onboarding

* feat: add nice empty state page to popup when no accounts

* fix: pr review issues

* fix: display add account menu at top of page rather than centred in fullscreen view

* fix: revert part of last

* feat: custom derivation path - 2nd attempt (#1049)

* feat: form container normal casing for errors

* feat: import account with custom derivation path

* feat: derivation path in new account screen

* fix: wording

* feat: derivation path validation

* chore: cleanup

* chore: cleanup

* chore: cleanup

* feat: missing i18n

* fix: useTranslation usage

* fix: review fixes & predict next dp

* fix: derivation path validation

* fix derivation path validation

* More mnemonic store fixes (#1050)

* refactor: changed name of seedPhraseStore to mnemonicsStore

* fix: remove "Vault" as source value for stored mnemonic

* refactor: move mnemonic messages from accounts handler

* chore: changed some instances of "seed" to "mnemonic" or "recovery phrase"

* fix: remove references to legacy verifier certificate store

* fix: more merge updates

* fix: more mnemonic store tweaks

* chore: rename seed -> suri in account creation method

* feat: use mnemonic creation modal to allow creation of mnemonic for vault verifier certificate

* feat: ensure mnemonic generate for vault verifier certificate works properly

* refactor: migrate account types

* refactor: replace "hardware" with "ledger" in account creation

* chore: tidy up

* fix: typo

* fix: flicker in popup load showing empty state

* chore: changeset

* fix: mnemonic backup behaviour for new settings

* chore: yeet styled components (#1072)

* chore: remove dead code

* fix: memoized background

* chore: onboard layout without SC

* feat: account pill without SC

* chore: yeet styled components

* chore: adjustments & changeset

* fix: fade in duration

* Make migrate password mandatory (#1075)

* chore: prevent ignoring password upgrade

* refactor: improve mandatory password migration ui behaviour

* fix: enable logging in with legacy root account

* fix: use recoil atom for "should migrate password" state

* fix: include support link on migrate password error boundary

* fix: copy and buttons on manage accounts screen

---------

Co-authored-by: Kheops <26880866+0xKheops@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chidg chidg chidg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom Derivation Path
2 participants
@0xKheops @chidg