Wallet: create account using recovery phrase #19702
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OmarBasem
commented
Apr 18, 2024
| (ns status-im.contexts.onboarding.enter-seed-phrase.view | ||
| (ns status-im.common.enter-seed-phrase.view |
There was a problem hiding this comment.
Moved enter-seed-phrase to be in a common namespace
OmarBasem
commented
Apr 18, 2024
Comment on lines
23
to
25
| {:icon :i/keycard-card | ||
| :accessibility-label :import-from-keycard | ||
| :label (i18n/label :t/import-from-keycard)} |
Jenkins BuildsClick to see older builds (25)
|
OmarBasem
requested review from
shivekkhurana,
vkjr,
flexsurfer,
briansztamfater,
J-Son89 and
ulisesmac
J-Son89
reviewed
Apr 18, 2024
| :text suggestions-text | ||
| :words (keyboard-suggestions last-word) | ||
| :on-press pick-suggested-word}]])]) | ||
| (finally | ||
| (.remove keyboard-show-listener) | ||
| (.remove keyboard-hide-listener)))) | ||
|
|
||
| (defn enter-seed-phrase | ||
| (defn view |
J-Son89
reviewed
Apr 18, 2024
J-Son89
reviewed
Apr 18, 2024
src/status_im/contexts/wallet/add_account/create_account/events.cljs
Outdated
Show resolved
Hide resolved
J-Son89
reviewed
Apr 18, 2024
| @@ -37,6 +37,15 @@ | |||
| effects (events/new-keypair-continue {:db db} props)] | |||
| (is (match? effects {:fx expected-effects})))) | |||
|
|
|||
| (deftest seed-phrase-entered-test | |||
J-Son89
reviewed
Apr 18, 2024
...status_im/contexts/wallet/add_account/create_account/new_keypair/check_your_backup/view.cljs
Outdated
Show resolved
Hide resolved
J-Son89
reviewed
Apr 18, 2024
| @@ -8,7 +8,9 @@ | |||
| :effects.wallet/create-account-from-mnemonic | |||
| (fn [{:keys [secret-phrase keypair-name]}] | |||
| (native-module/create-account-from-mnemonic | |||
| {:MnemonicPhrase (string/join " " secret-phrase)} | |||
| {:MnemonicPhrase (if (string? secret-phrase) | |||
There was a problem hiding this comment.
should we make this safer? 🤔
i.e if it's a string do this, if it's a vector use string/join and otherwise return an empty string/nil? 🤔
There was a problem hiding this comment.
@J-Son89 But we shouldn't get to that point with an empty string. It will be of no use and the status-go call will fail. So probably better to make it fail earlier?
There was a problem hiding this comment.
tbh, I'm not sure. Just flaggin this to be careful. Whatever you think is best! 🙏
J-Son89
reviewed
Apr 18, 2024
J-Son89
reviewed
Apr 18, 2024
J-Son89
approved these changes
Apr 18, 2024
There was a problem hiding this comment.
some minor details that would be good to address.
Otherwise looks great, nice one @OmarBasem! 🚀
OmarBasem
force-pushed
the
wallet/recovery-phrase
branch
from
707d5f3 to
4aee51a
Compare
96% of end-end tests have passedExpected to fail tests (2)Click to expandClass TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (50)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
|
Hi @OmarBasem . Thank you for PR. Take a look found issues: PR_ISSUE 1: Users are able to add an account using an invalid seed phrase when 18 or 24 random valid words are enteredSteps:
Actual result:The account is successfully added, even though the seed phrase is invalid. Expected result:Potential solution:
Let's see what design team can reply OS:IOS, Android Devices:
|
PR_ISSUE 2: The key pair field requires 6 words instead of 5 and the validation message "Key pair name must be at least 5 characters" is not displayed in case if less than 5 chars enteredSteps:
Actual result:
Expected result:
OS:IOS, Android Devices:
|
PR_ISSUE 3: The color of the 'continue' button does not inherit the color of the accountSteps:
Actual result:The 'continue' button does not inherit the color of the account Expected result:The 'continue' button inherits the color of the account OS:IOS, Android Devices:
|
90% of end-end tests have passedFailed tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestWalletMultipleDevice:
Expected to fail tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Passed tests (47)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestWalletOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Hi @OmarBasem, thank you for your reply. Could you please check #19702 (comment) if this is not an issue? |
@VolodLytvynenko sorry missed the question. The derivation path will work correctly after that issue is fixed. |
|
Hi @OmarBasem
I didn't understand you here. Is it correct the buttons are always blue, regardless of the account color that is set up?
Thank you for the clarification! |
|
@OmarBasem PR is tested and ready to be merged. Still, some follow-ups are required, which I will create after approval from the design team here |
OmarBasem
force-pushed
the
wallet/recovery-phrase
branch
from
0cf9d07 to
4cde270
Compare
OmarBasem
force-pushed
the
wallet/recovery-phrase
branch
from
cfbb480 to
4dc2250
Compare
|
Thanks for your testing @VolodLytvynenko.
Yes correct the button color when creating an account depends on the profile color - not the account yet to be created. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes: #19514
Summary:
This PR implements recovering a keypair using recovery phrase and creating a wallet account from that keypair.
The
enter-seed-phrasescreen inside onboarding moved it to a common namespace to be used for both of wallet and onboarding recovery.Designs
For QA testing, areas that may be impacted:
Demo:
Screen_Recording_20240418_112738_Status.mp4