Add account derivation for mnemonic users #1853
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
b44438a to
f1cebbd
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
f1cebbd to
553c523
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
2 times, most recently
from
2365bb6 to
407c0da
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
407c0da to
d9a7944
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
d9a7944 to
8695e6c
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
8695e6c to
aacfb48
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
2 times, most recently
from
3996863 to
ca600ae
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
ca600ae to
b325753
Compare
| { | ||
| files: ["*.ts", "*.tsx"], | ||
| rules: { | ||
| "import/no-unused-modules": "off", |
There was a problem hiding this comment.
Why this rule was changed?
There was a problem hiding this comment.
the rule fixes eslint, which currently does not work properly in the components package (especially rule no-unused-modules)
| size="sm" | ||
| variant="ghost" | ||
| /> | ||
| <IconButton |
There was a problem hiding this comment.
I think, this button should only be displayed for mnemonic accounts 🤔
There was a problem hiding this comment.
From the business perspective this button should open Add new account modal for any accounts other than mnemonic
| </Center> | ||
| {accounts.map(account => { | ||
| {accounts?.map(account => { |
There was a problem hiding this comment.
Why accounts can be null here?
Maybe empty groups can be filtered out to avoid this check?
| expect(screen.getByText("First")).toBeVisible(); | ||
| }); | ||
|
|
||
| it("goToIndex should go back one step when is called with out-of-bounds index", async () => { |
There was a problem hiding this comment.
Why do we have both goBack & goToIndex with the same functionality?
Maybe we can remove goBack, or only allow goToIndex for correct indexes?
There was a problem hiding this comment.
I'd personally prefer to only allow goToIndex for correct indexes to simplify methods logic
There was a problem hiding this comment.
Previously, this logic was implemented in the goBack function, but after discussing it with @serjonya-trili, we decided to split this logic and keep both functions for different purposes - goBack for going directly to the previous disclosure and goToIndex for navigating through different disclosures
There was a problem hiding this comment.
We should check that account name is valid before proceeding (similar to RenameAccountModal check)
There was a problem hiding this comment.
we don't need these validation rules for this case since Account name field in this modal is optional.
Basically logic of NameAccountModal is the same as for desktop's NameAccountDisplay
There was a problem hiding this comment.
Although the warning is shown, maybe we can also make the form invalid on entering wrong password
There was a problem hiding this comment.
Password validation takes place inside useDeriveMnemonicAccount. We don't know if the password is valid or not until we process useDeriveMnemonicAccount
| expect(screen.getByTestId("master-password-modal")).toBeVisible(); | ||
| }); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Maybe we can also check the case with incorrect password
And that after successful deriving, the modal will be closed
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
2 times, most recently
from
8292145 to
49e2501
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
49e2501 to
ef5238d
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
ef5238d to
82d4fd1
Compare
OKendigelyan
force-pushed
the
add-account-derivation-for-mnemonic-users
branch
from
ce70f0f to
65c9fdc
Compare
Proposed changes
Task link
Types of changes
Screenshots
Add the screenshots of how the app used to look like and how it looks now
Screen.Recording.2024-09-09.at.14.23.11.mov
Checklist