[#9749] Support importing private key and seed #10100
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (15)
|
| @@ -24,7 +24,7 @@ | |||
| (contains? multiaccounts key-uid)) | |||
|
|
|||
| (re-frame/reg-fx | |||
| ::validate-mnemonic | |||
| :validate-mnemonic | |||
| (fx/defn import-new-account-seed | ||
| [{:keys [db]} passphrase hashed-password] | ||
| {:db (assoc-in db [:add-account :step] :generating) | ||
| :validate-mnemonic [(security/safe-unmask-data passphrase) |
There was a problem hiding this comment.
keep the namespaced keywords and import via alias, if there is a circular dep it just means that the effect is not in the right place from a code organization pov
ISSUE 1: field seed phrase is case-sensitive and contains autosuggestionSteps:
Expected result: Actual result: Also, keyboard has turned on autosuggestion when focusing in field (referred to the issue #9034) |
ISSUE 2: Can add account with invalid passwordSteps:
Expected result: Actual result: |
flexsurfer
force-pushed
the
feature/rework-new-accounts-in-wallet
branch
from
d64fc99 to
18d2221
Compare
|
issue1 : fixed |
|
@churik can't reproduce issue 2, btw for importing seed and private key there is no wrong password, because you can use any password you want, yeah i know its confusing |
|
summon @hesterbruikman , so when you import accounts with seed or private key, you can use any new password to import them , that might be confusing for sure |
If I recall correctly this should be your existing password. Also as it says, If it is indeed the existing password, validation is missing |
|
@hesterbruikman we just store new account on the device we can use any password, it's not related anyhow to multiaccount, so its up to us how we want to implement it, if we want new password we should ask it twice , if multiaccount, we need to verify it somehow @gravityblast ? |
|
I'm pretty sure the idea was to simply ask for the same password as to not let people juggle different passwords to sign within the same multiaccount in Status. That's why there's no confirm password and we only say |
|
100% In my mind, from our past discussions, a new account imported from a private key should be protected the same way an account on the BIP44 tree is, and we should not create a new password to avoid any added complexity. If the private key is encrypted in a json file (but I don't think this issue is about this case) of course, that's something else and at the time of importing, this encryption password is being asked. |
flexsurfer
force-pushed
the
feature/rework-new-accounts-in-wallet
branch
from
18d2221 to
34b4372
Compare
|
@churik issue 2 fixed, also now we check if password is the same as for multiaccount |
ISSUE 3: Keychain suggests to save passwordSteps:
Expected result: Actual result: |
ISSUE 4: Can add 2 accounts with same seedSteps:
Expected result: Actual result: |
|
ISSUE 3 - hm is this solved somehow for other password fields ? i remember we had this but not sure how we solved it |
|
@flexsurfer |
ISSUE 5: Can't export private keySteps:
Expected result: Actual result: |
98% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (92)Click to expand |
ISSUE 6: Seed phrase field is overlapped on Android
|
ISSUE 7: With whitspaces between, after or before seed phrase restores different accountSee #9670 Steps:
Expected result: Actual result: |
|
@flexsurfer here fix for several cases will be required (and ideally 2 new cases). |
| {:address address | ||
| :public-key publicKey | ||
| :type type | ||
| :path (if (string/starts-with? path "m/") |
There was a problem hiding this comment.
can we extract this path derivation part to a separate fn?
| "/" (last (string/split path "/"))) | ||
| path)}]))))))))))) | ||
|
|
||
| (def pass-error "cannot retrieve a valid key for a given account: could not decrypt key with given password") |
There was a problem hiding this comment.
Is that returned form status-go side? We probably need to start using error codes or something, because comparing messages might not be the best idea and definitely isn't a very reliable way.
| (status/verify address hashed-password | ||
| #(re-frame/dispatch [:wallet.accounts/add-new-account-password-verifyied | ||
| % | ||
| hashed-password])))) |
There was a problem hiding this comment.
(status/verify
address hashed-password
#(re-frame/dispatch
[:wallet.accounts/add-new-account-password-verifyied % hashed-password]))))not a big deal, but that lonely percent sign looks weird to me
flexsurfer
force-pushed
the
feature/rework-new-accounts-in-wallet
branch
from
34b4372 to
6575b94
Compare
|
@churik thanks all issue fixed |
flexsurfer
force-pushed
the
feature/rework-new-accounts-in-wallet
branch
from
6575b94 to
134a675
Compare
|
@corpetty |
flexsurfer
force-pushed
the
feature/rework-new-accounts-in-wallet
branch
from
83e137d to
f706a61
Compare
100% of end-end tests have passedPassed tests (4)Click to expand
|
|
Tested:
Apart from #9895 and #8837 don't see other issues. |
Signed-off-by: Andrey Shovkoplyas <motor4ik@gmail.com>
flexsurfer
force-pushed
the
feature/rework-new-accounts-in-wallet
branch
from
9515f03 to
7b7f567
Compare
delete-merged-branch
bot
deleted the
feature/rework-new-accounts-in-wallet
branch
|
👋@flexsurfer what's the behavior for Keycard in this current implementation? @rasom mentioned we're missing designs for Add account > Generate keys in case of Keycard. Assuming the same is true here. |
|
the behavior is that Keycard is not supported in this PR , here is the issue #10101 |
fixes #9749
QA: keycard is not supported #10101