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

Keycard - Import a seed phrase into a new Keycard #21947

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

Parveshdhull
Copy link
Member

@Parveshdhull Parveshdhull commented Jan 17, 2025

fixes #21932

Video

signal-2025-01-19-184345.mp4

status: ready

Found Issues

  1. Flow UI issues

    • Fixed (checked by dev)
    • Verified (checked by QA)
  2. The app gets stuck while scanning a different Keycard

    • Fixed (checked by dev)
    • Verified (checked by QA)
  3. ISSUE 3: Not empty keycard can be used after PIN during Create Profile flow

    • Fixed (checked by dev)
    • Verified (checked by QA)

@Parveshdhull Parveshdhull self-assigned this Jan 17, 2025
@status-im-auto
Copy link
Member

status-im-auto commented Jan 17, 2025

Jenkins Builds

Click to see older builds (31)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 6b7a500 #1 2025-01-17 09:29:37 ~5 min tests 📄log
✔️ 6b7a500 #1 2025-01-17 09:33:46 ~9 min android-e2e 🤖apk 📲
✔️ 6b7a500 #1 2025-01-17 09:34:33 ~10 min android 🤖apk 📲
✔️ 6b7a500 #1 2025-01-17 09:36:40 ~12 min ios 📱ipa 📲
d16f13c #2 2025-01-19 09:26:20 ~2 min tests 📄log
✔️ d16f13c #2 2025-01-19 09:31:28 ~8 min android-e2e 🤖apk 📲
✔️ d16f13c #2 2025-01-19 09:31:58 ~8 min android 🤖apk 📲
✔️ d16f13c #2 2025-01-19 09:33:54 ~10 min ios 📱ipa 📲
✔️ 83260f4 #3 2025-01-19 11:37:16 ~4 min tests 📄log
✔️ 83260f4 #3 2025-01-19 11:41:22 ~9 min android-e2e 🤖apk 📲
✔️ 83260f4 #3 2025-01-19 11:42:00 ~9 min ios 📱ipa 📲
✔️ 83260f4 #3 2025-01-19 11:42:07 ~9 min android 🤖apk 📲
✔️ 361645b #4 2025-01-19 11:48:49 ~4 min tests 📄log
✔️ 361645b #4 2025-01-19 11:53:17 ~9 min android-e2e 🤖apk 📲
✔️ 361645b #4 2025-01-19 11:53:36 ~9 min ios 📱ipa 📲
✔️ 361645b #4 2025-01-19 11:53:44 ~9 min android 🤖apk 📲
✔️ e49c599 #5 2025-01-19 13:01:05 ~4 min tests 📄log
✔️ e49c599 #5 2025-01-19 13:05:15 ~8 min android-e2e 🤖apk 📲
✔️ e49c599 #5 2025-01-19 13:05:43 ~8 min android 🤖apk 📲
✔️ e49c599 #5 2025-01-19 13:09:33 ~12 min ios 📱ipa 📲
79174f7 #6 2025-01-20 09:20:10 ~33 sec ios 📄log
79174f7 #7 2025-01-20 09:22:43 ~15 sec ios 📄log
7d100bf #8 2025-01-20 09:23:46 ~14 sec ios 📄log
✔️ 7d100bf #7 2025-01-20 09:27:38 ~4 min tests 📄log
✔️ 7d100bf #7 2025-01-20 09:30:39 ~7 min android-e2e 🤖apk 📲
✔️ 7d100bf #7 2025-01-20 09:31:16 ~7 min android 🤖apk 📲
✔️ 7d100bf #9 2025-01-20 10:24:23 ~15 min ios 📱ipa 📲
✔️ f51f924 #8 2025-01-20 12:03:30 ~4 min tests 📄log
✔️ f51f924 #8 2025-01-20 12:07:48 ~9 min android-e2e 🤖apk 📲
✔️ f51f924 #8 2025-01-20 12:08:20 ~9 min android 🤖apk 📲
✔️ f51f924 #10 2025-01-20 12:09:44 ~11 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 8ce2561 #9 2025-01-22 10:16:04 ~4 min tests 📄log
✔️ 8ce2561 #9 2025-01-22 10:19:25 ~7 min android-e2e 🤖apk 📲
✔️ 8ce2561 #9 2025-01-22 10:21:15 ~9 min android 🤖apk 📲
✔️ 8ce2561 #11 2025-01-22 10:22:52 ~11 min ios 📱ipa 📲
✔️ 6518646 #10 2025-01-23 06:43:08 ~4 min tests 📄log
✔️ 6518646 #10 2025-01-23 06:47:09 ~9 min android-e2e 🤖apk 📲
✔️ 6518646 #10 2025-01-23 06:47:51 ~9 min android 🤖apk 📲
✔️ 6518646 #12 2025-01-23 06:48:32 ~10 min ios 📱ipa 📲

@Parveshdhull Parveshdhull force-pushed the feat/keycard-seed-phrase-flow branch from 6b7a500 to d16f13c Compare January 19, 2025 09:23
@Parveshdhull Parveshdhull force-pushed the feat/keycard-seed-phrase-flow branch from d16f13c to 64fb344 Compare January 19, 2025 11:30
@Parveshdhull Parveshdhull reopened this Jan 19, 2025
@Parveshdhull Parveshdhull force-pushed the feat/keycard-seed-phrase-flow branch 2 times, most recently from 361645b to e49c599 Compare January 19, 2025 12:56
@Parveshdhull Parveshdhull changed the title [WIP]: Keycard - Import a seed phrase into a new Keycard Keycard - Import a seed phrase into a new Keycard Jan 19, 2025
@Parveshdhull
Copy link
Member Author

Parveshdhull commented Jan 19, 2025

Summary

The PR implements a flow for importing a seed phrase into a new keycard and creating an account using that seed phrase. It is essentially a combination of the "recovery phrase flow" and the "Create profile using an empty keycard" flow. Some important points:-

  1. In the "Enter recovery phrase" flow in Figma, there is a screen displaying the error message: "The recovery phrase you entered is linked to a key pair already added to this device," along with a button labeled "Enter another recovery phrase." However, in our standard recovery flow, when :onboarding/seed-phrase-validated is dispatched, we show a popup(as shows in above video) that allows the user to open the profiles screen and access the already existing profile (profile selection functionality is currently broken, and I’ve added a comment in the code). Given this, I believe both flows should use the same logic, and we need to decide which approach we want to keep.

Unrelated, but it might be worth considering treating reusable flows as components and defining them in a single place to avoid discrepancies. We could then simply add a label when we need to duplicate a flow, similar to how we handle the "Try Again" button flows.

  1. Once the seed phrase is entered, we are using the keycard base flow, and since the base flow doesn’t include a biometric screen, this flow doesn’t either. By the way, the position of the biometric screen is not consistent across our app, which adds complexity to development. I think we should move the biometric step to show after account creation, as I also mentioned here.

  2. "Importing a seed phrase into a new keycard" flow includes an additional intermediary screen. Do we need it? I’m also unsure when we should display this screen, as mentioned in this issue, where even the visibility of the "Preparing Status for you" screen is sometimes too short.

I don’t think any of these points are blockers for the PR, but please let me know if you think otherwise. @flexsurfer @ilmotta

@Parveshdhull Parveshdhull marked this pull request as ready for review January 19, 2025 13:57
src/status_im/navigation/screens.cljs Outdated Show resolved Hide resolved
@status-im-auto
Copy link
Member

88% of end-end tests have passed

Total executed tests: 8
Failed tests: 1
Expected to fail tests: 0
Passed tests: 7
IDs of failed tests: 702843 

Failed tests (1)

Click to expand
  • Rerun failed tests

  • Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_edit, id: 702843

    Device 2: Tap on found: Button
    Device 2: Attempt 0 is successful clicking close-activity-center

    Test setup failed: critical/chats/test_public_chat_browsing.py:318: in prepare_devices
        self.home_2.handle_contact_request(self.username_1)
    ../views/home_view.py:388: in handle_contact_request
        chat_element.accept_contact_request()
    ../views/home_view.py:167: in accept_contact_request
        self.handle_cr("accept-contact-request")
    ../views/home_view.py:164: in handle_cr
        ).wait_for_rendering_ended_and_click()
    ../views/base_element.py:154: in wait_for_rendering_ended_and_click
        self.wait_for_visibility_of_element(20)
    ../views/base_element.py:138: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: Button by xpath:`//*[contains(@text, 'shoFcUTpGwfFF7WpjPmw')]/ancestor::*[@content-desc='activity']/*[@content-desc="accept-contact-request"]` is not found on the screen after wait_for_visibility_of_element
    



    Device sessions

    Passed tests (7)

    Click to expand

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_restore_multiaccount_with_waku_backup_remove_profile_switch, id: 703133
    Device sessions

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    2. test_wallet_balance_mainnet, id: 740490

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    Class TestWalletMultipleDevice:

    1. test_wallet_send_asset_from_drawer, id: 727230
    2. test_wallet_send_eth, id: 727229

    @mariia-skrypnyk mariia-skrypnyk self-assigned this Jan 20, 2025
    @Parveshdhull Parveshdhull force-pushed the feat/keycard-seed-phrase-flow branch 2 times, most recently from 7d100bf to f51f924 Compare January 20, 2025 11:58
    @mariia-skrypnyk
    Copy link

    mariia-skrypnyk commented Jan 20, 2025

    Hi @Parveshdhull !

    Thanks for such an important logic implementation!

    Here are 2 UI issues, by now I will group them into one :

    Issue 1: Flow UI issues

    • White dots instead of green ones as designed
      *we have a lot of Keycard flows using them, let me know if it's complicated to fix in scope of this PR

    Design vs Implementation

    Screenshot 2025-01-20 at 15 55 30

    https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?node-id=5045-52037&m=dev

    • Not the correct screen after PIN setting

    Design vs Implementation

    Screenshot 2025-01-20 at 15 46 11

    Steps:

    1. install app
    2. create user
    3. use empty keycard
    4. choose import recovery phrase
    5. scan empty keycard
    6. set PIN

    https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?node-id=5045-51925&m=dev

    @mariia-skrypnyk
    Copy link

    mariia-skrypnyk commented Jan 20, 2025

    ISSUE 2: The app gets stuck while scanning a different Keycard

    Steps:

    1. install app
    2. choose create new profile
    3. scan Keycard 1
    4. go through the flow
    5. scan Keycard 2 (with a different key pair on it)

    Result: The app gets stuck on Preparing screen

    IMG_4950.MP4

    Expected result:

    Screenshot 2025-01-20 at 17 08 09

    https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?node-id=5045-52070&m=dev

    Copy link
    Contributor

    @ilmotta ilmotta left a comment

    Choose a reason for hiding this comment

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

    In the "Enter recovery phrase" flow in Figma, there is a screen displaying the error message: "The recovery phrase you entered is linked to a key pair already added to this device," along with a button labeled "Enter another recovery phrase." However, in our standard recovery flow, when :onboarding/seed-phrase-validated is dispatched, we show a popup(as shows in above video) that allows the user to open the profiles screen and access the already existing profile (profile selection functionality is currently broken, and I’ve added a comment in the code). Given this, I believe both flows should use the same logic, and we need to decide which approach we want to keep.

    @Parveshdhull, I don't have any strong preference regarding this point, but I find the current solution showing a pop-up slightly worse than what's in Figma because the user loses context by being redirected to the profile selection. The solution in Figma is consistent with other error messages that can occur in the recovery phrase screen, so that's mainly why I prefer it. If the effort is small seems worth improving to match the UX in Figma. Not a blocker to this PR IMO.

    Unrelated, but it might be worth considering treating reusable flows as components and defining them in a single place to avoid discrepancies. We could then simply add a label when we need to duplicate a flow, similar to how we handle the "Try Again" button flows.

    Good idea @Parveshdhull.

    I think we should move the biometric step to show after account creation, as I also mentioned here.

    Agreed 👍🏼 This inconsistency is creating unnecessary complexity.


    "Importing a seed phrase into a new keycard" flow includes an additional intermediary screen. Do we need it? I’m also unsure when we should display this screen, as mentioned in this #21673, where even the visibility of the "Preparing Status for you" screen is sometimes too short.

    Just confirming if I understood your suggestion, per screenshots below, the user would go from the first screen to the second directly, possibly waiting a few seconds? If that's the case, I think it should be fine as long as it doesn't take too long on average (from my usage it's always been relatively fast). Nice suggestion to simplify.

    src/status_im/contexts/keycard/create/events.cljs Outdated Show resolved Hide resolved
    src/status_im/contexts/onboarding/events.cljs Outdated Show resolved Hide resolved
    @mariia-skrypnyk
    Copy link

    ISSUE 3: Not empty keycard can be used after PIN during Create Profile flow

    Steps:

    1. Install app
    2. Create Profile using empty Keycard
    3. Logout and go to Create Profile one more time
    4. Choose another empty Keycard in the beginning of Create Profile with empty Keycard flow
    5. Go through the flow till PIN set/check
    6. Scan Keycard from step 2 (with a key pair on it)

    Result:
    Keycard will be valid and key pair will be re-written with a new one

    Expected result:

    This is a different Keycard screen should be shown
    Screenshot 2025-01-21 at 11 51 50

    https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?node-id=3-89231&p=f&m=dev

    @Parveshdhull Parveshdhull force-pushed the feat/keycard-seed-phrase-flow branch from f51f924 to 8ce2561 Compare January 22, 2025 10:11
    @Parveshdhull Parveshdhull mentioned this pull request Jan 22, 2025
    6 tasks
    @Parveshdhull
    Copy link
    Member Author

    Parveshdhull commented Jan 22, 2025

    Thank you @ilmotta for reviewing the PR. Addressed required changes.

    If the effort is small seems worth improving to match the UX in Figma.

    Thank you, added task in #21970

    Just confirming if I understood your suggestion, per screenshots below, the user would go from the first screen to the second directly, possibly waiting a few seconds?

    Yes

    If that's the case, I think it should be fine as long as it doesn't take too long on average (from my usage it's always been relatively fast). Nice suggestion to simplify

    Thank you

    @Parveshdhull
    Copy link
    Member Author

    Thanks, @mariia-skrypnyk, for testing the PR.

    The "Import a seed phrase into a new Keycard" flow combines the existing seed phrase and Keycard base flows, so this PR inherits their limitations and issues.

    The reported issues are important but addressing them all at once would add complexity. I’ve created a separate issue for these bugs (including the info button update task from the old issue).

    Since these issues are already part of the develop branch and were not introduced by this PR, we can focus on testing only the new functionality of the PR. Please let me know if this is not the case for any of the issues.

    @mariia-skrypnyk
    Copy link

    mariia-skrypnyk commented Jan 22, 2025

    Hi @Parveshdhull !

    Understand your concern!
    Not sure I agree with Issue 3: the problem is that we have no check that the keycard already has a key pair on it and we rewrite it.

    Does it make sence? Or you still think it is out of the current PR's scope?

    UPD: I found issue in repo that we do not have this logic for now #21647 and probably that's why it was not included by you (nothing to include).

    So, Issue 3 with #21647 should also exist as a separate tasks.

    Failed e2e is not related.
    PR can be merged.

    @Parveshdhull Parveshdhull force-pushed the feat/keycard-seed-phrase-flow branch from 8ce2561 to 6518646 Compare January 23, 2025 06:37
    @Parveshdhull Parveshdhull merged commit 85d00e4 into develop Jan 23, 2025
    5 checks passed
    @Parveshdhull Parveshdhull deleted the feat/keycard-seed-phrase-flow branch January 23, 2025 06:53
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Status: DONE
    Development

    Successfully merging this pull request may close these issues.

    Keycard - Import a seed phrase into a new Keycard
    5 participants