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

fix Lost the syncing state in fallback recovery, when first enter the seed phrase for the wrong account #21298

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

Parveshdhull
Copy link
Member

fixes #21258

status: ready

@status-im-auto
Copy link
Member

status-im-auto commented Sep 20, 2024

Jenkins Builds

Click to see older builds (21)
Commit #️⃣ Finished (UTC) Duration Platform Result
27eda90 #1 2024-09-20 07:52:30 ~3 min tests 📄log
✔️ 27eda90 #1 2024-09-20 07:57:42 ~8 min android-e2e 🤖apk 📲
✔️ 27eda90 #1 2024-09-20 07:58:15 ~9 min android 🤖apk 📲
✔️ 27eda90 #1 2024-09-20 07:59:37 ~10 min ios 📱ipa 📲
✔️ 0fe1e3c #2 2024-09-20 08:11:32 ~4 min tests 📄log
✔️ 0fe1e3c #2 2024-09-20 08:14:39 ~7 min android 🤖apk 📲
✔️ 0fe1e3c #2 2024-09-20 08:15:09 ~7 min android-e2e 🤖apk 📲
✔️ 0fe1e3c #2 2024-09-20 08:18:51 ~11 min ios 📱ipa 📲
✔️ 1cfa9c5 #5 2024-09-23 11:43:23 ~4 min tests 📄log
✔️ 1cfa9c5 #5 2024-09-23 11:45:57 ~7 min android-e2e 🤖apk 📲
✔️ 1cfa9c5 #5 2024-09-23 11:46:32 ~7 min android 🤖apk 📲
✔️ 1cfa9c5 #5 2024-09-23 11:49:49 ~10 min ios 📱ipa 📲
0b1f557 #6 2024-09-24 06:18:33 ~2 min tests 📄log
✔️ 0b1f557 #6 2024-09-24 06:22:23 ~6 min android 🤖apk 📲
✔️ 0b1f557 #6 2024-09-24 06:22:58 ~7 min android-e2e 🤖apk 📲
✔️ 0b1f557 #6 2024-09-24 06:25:43 ~9 min ios 📱ipa 📲
✔️ 97a906b #7 2024-09-24 07:06:30 ~4 min tests 📄log
✔️ 97a906b #7 2024-09-24 07:09:39 ~7 min android-e2e 🤖apk 📲
✔️ 97a906b #7 2024-09-24 07:11:36 ~9 min android 🤖apk 📲
✔️ 97a906b #7 2024-09-24 07:15:49 ~13 min ios 📱ipa 📲
✔️ 1e6b812 #10 2024-09-24 15:25:38 ~4 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 1bb1f3b #11 2024-09-24 15:30:20 ~4 min tests 📄log
✔️ 1bb1f3b #11 2024-09-24 15:32:58 ~6 min android-e2e 🤖apk 📲
✔️ 1bb1f3b #11 2024-09-24 15:33:56 ~7 min android 🤖apk 📲
✔️ 1bb1f3b #11 2024-09-24 15:36:03 ~9 min ios 📱ipa 📲
✔️ 4f6f9c0 #12 2024-09-25 14:02:41 ~4 min tests 📄log
✔️ 4f6f9c0 #12 2024-09-25 14:05:46 ~7 min android-e2e 🤖apk 📲
✔️ 4f6f9c0 #12 2024-09-25 14:06:13 ~8 min android 🤖apk 📲
✔️ 4f6f9c0 #12 2024-09-25 14:09:13 ~11 min ios 📱ipa 📲

@Horupa-Olena Horupa-Olena self-assigned this Sep 23, 2024
@status-im-auto
Copy link
Member

100% of end-end tests have passed

Total executed tests: 7
Failed tests: 0
Expected to fail tests: 0
Passed tests: 7

Passed tests (7)

Click to expand

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
Device sessions

Class TestWalletMultipleDevice:

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

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
Device sessions

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions

2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
Device sessions

@Horupa-Olena
Copy link

@Parveshdhull
I created a new account for testing and noticed that this screen appears, but it shouldn’t. Becuase it's not fallback flow

@Parveshdhull
Copy link
Member Author

@Parveshdhull I created a new account for testing and noticed that this screen appears, but it shouldn’t. Becuase it's not fallback flow

Hi @Horupa-Olena,

Thank you very much for testing the PR. Could you please share a screen recording so that I can replicate the issue?

@Horupa-Olena
Copy link

@Parveshdhull Sorry, I forgot add the video because it's simple flow of account creation and don't need any special actions.

Screenrecorder-2024-09-23-14-18-58-880.mp4

@Parveshdhull
Copy link
Member Author

@Parveshdhull Sorry, I forgot add the video because it's simple flow of account creation and don't need any special actions.

Thank you, @Horupa-Olena, for sharing the video. Interestingly, I was unable to reproduce this issue when I tried to create an account from the profiles screen instead of the intro. The issue should be fixed now.

@Parveshdhull Parveshdhull force-pushed the fix/syncing-state branch 2 times, most recently from 16e7240 to 1cfa9c5 Compare September 23, 2024 11:38
@Parveshdhull
Copy link
Member Author

The issue should be fixed now.

Also rebased the PR, so that fix from #21300 is also included.

@status-im-auto
Copy link
Member

100% of end-end tests have passed

Total executed tests: 7
Failed tests: 0
Expected to fail tests: 0
Passed tests: 7

Passed tests (7)

Click to expand

Class TestCommunityOneDeviceMerged:

1. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
Device sessions

2. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions

Class TestWalletMultipleDevice:

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

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
Device sessions

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
Device sessions

@Horupa-Olena
Copy link

Horupa-Olena commented Sep 23, 2024

@Parveshdhull The main issue has been fixed.

However, on iOS, I encountered the following issue:

ISSUE 1: ios onboarding: missing screen after password creation, "generating keys..." screen appears instead

Steps:

  1. Device A1 try to sync with Device A2
  2. Initiate fallback flow
  3. Look for onboarding screen after entered seed phrase

Expected results:
This screen should appears.

Actual results:
Instead, the default screen with the message "Generating keys..." appears.

video.1.mp4

@Horupa-Olena
Copy link

@Parveshdhull @jrainville
I also noticed two issues with notifications in the AC after sync:

  1. After a successful sync that doesn’t use the fallback flow (but follows the main path), this notification appears in the AC. However, in this case, no notification should be shown.

  2. After a successful sync using the fallback flow, this notification appears in the AC, but it should display this notification instead (this can also be seen in the end of video

video.1.mp4

I assume some changes were made on the status-go side, as these issues were not present in the main PR for the fallback flow. Could you confirm that notifications changes by the status-go side or I mistake? So I can create separately bug reports for this issues

@Parveshdhull
Copy link
Member Author

I also noticed two issues with notifications in the AC after sync:

Hi @Horupa-Olena,

Thank you very much for reporting these issues. Could you please confirm whether you are using a new account for testing? (Old notifications may have been fetched from the last fallback recovery flow.) - #21090 (comment)

@Horupa-Olena
Copy link

@Parveshdhull Thank you for your quick fix.

This issue still reproduces if I use the old profile. The difference is that in this PR, if I use a new account, I receive an incorrect notification. Previously, the first sync on a new account worked fine.
For the old account, these two issues now just overlap.

@Parveshdhull
Copy link
Member Author

ISSUE 1: ios onboarding: missing screen after password creation, "generating keys..." screen appears instead

Thank you for finding this issue. Should be fixed now.

@Parveshdhull
Copy link
Member Author

This issue still reproduces if I use the old profile. The difference is that in this PR, if I use a new account, I receive an incorrect notification. Previously, the first sync on a new account worked fine. For the old account, these two issues now just overlap.

Issue already logged here and have not been worked on yet: #21260

@Parveshdhull
Copy link
Member Author

if I use a new account, I receive an incorrect notification.

Oh, you mean even if for new accounts we are showing wrong notification? Sorry I missed this.

@Parveshdhull
Copy link
Member Author

if I use a new account, I receive an incorrect notification.

Oh, you mean even if for new accounts we are showing wrong notification? Sorry I missed this.

Weird, I just tested latest build and I am seeing correct notifications.

@Horupa-Olena
Copy link

if I use a new account, I receive an incorrect notification.

Oh, you mean even if for new accounts we are showing wrong notification? Sorry I missed this.

Weird, I just tested latest build and I am seeing correct notifications.

@Parveshdhull Agreed. I think (but not sure) it can be some changes on status-go side, because this issue was found in nightly build. I see it only once in #21090, but now it reproduce more oten.

After a successful sync that doesn’t use the fallback flow (but follows the main path), this notification appears in the AC. However, in this case, no notification should be shown.

@status-im-auto
Copy link
Member

100% of end-end tests have passed

Total executed tests: 7
Failed tests: 0
Expected to fail tests: 0
Passed tests: 7

Passed tests (7)

Click to expand

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
Device sessions

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions

2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
Device sessions

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
Device sessions

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

@Horupa-Olena
Copy link

@Parveshdhull Thank for your fix!

After a successful sync using the fallback flow, this notification appears in the AC, but it should display this notification instead (this can also be seen in the end of video

This fix is ok!

ISSUE 1: ios onboarding: missing screen after password creation, "generating keys..." screen appears instead

This fix is ok, too.

@Horupa-Olena
Copy link

@Parveshdhull

After a successful sync that doesn’t use the fallback flow (but follows the main path), this notification appears in the AC. However, in this case, no notification should be shown.

Could you confirm, for this issue, we need to create a separate bug report, right?

@jrainville
Copy link
Member

@Parveshdhull

After a successful sync that doesn’t use the fallback flow (but follows the main path), this notification appears in the AC. However, in this case, no notification should be shown.

Could you confirm, for this issue, we need to create a separate bug report, right?

@Parveshdhull I just tested this on Desktop -> Desktop and I couldn't reproduce, so my guess is that you call finishPairingThroughSeedPhraseProcess when it's not needed?

As far as I can see, it's not a status-go issue, because it would reproduce on the Desktop side then.

cc @Horupa-Olena

@Parveshdhull
Copy link
Member Author

Parveshdhull commented Sep 24, 2024

Could you confirm, for this issue, we need to create a separate bug report, right?

If this is happening even for fresh account then I think its probably a new bug in UI side. But if its happening on the same account which we used earlier or fallback recovery then its the same issue. - #21090 (comment)

@Horupa-Olena
Copy link

Could you confirm, for this issue, we need to create a separate bug report, right?

If this is happening even for fresh account then I think its probably a new bug in UI side. But if its happening on the same account which we used earlier or fallback recovery then its the same issue. - #21090 (comment)

@Parveshdhull unfortunately it's reproduce on new accounts too

@Parveshdhull
Copy link
Member Author

@Parveshdhull unfortunately it's reproduce on new accounts too

Thank you for confirming, issue should be fixed now.

@Parveshdhull Parveshdhull force-pushed the fix/syncing-state branch 2 times, most recently from 1f274ee to 1e6b812 Compare September 24, 2024 15:21
@Parveshdhull
Copy link
Member Author

Please let me know if PR also fixes #21260

@status-im-auto
Copy link
Member

71% of end-end tests have passed

Total executed tests: 7
Failed tests: 2
Expected to fail tests: 0
Passed tests: 5
IDs of failed tests: 727230,727229 

Failed tests (2)

Click to expand
  • Rerun failed tests

  • Class TestWalletMultipleDevice:

    1. test_wallet_send_asset_from_drawer, id: 727230
    critical/test_wallet.py:163: in test_wallet_send_asset_from_drawer
        self.wallet_1.navigate_back_to_wallet_view()
    ../views/base_view.py:455: in navigate_back_to_wallet_view
        while not element.is_element_displayed(1) and counter <= attempts:
    ../views/base_element.py:221: in is_element_displayed
        return self.wait_for_visibility_of_element(sec, ignored_exceptions=ignored_exceptions)
    ../views/base_element.py:145: in wait_for_visibility_of_element
        .until(expected_conditions.visibility_of_element_located((self.by, self.locator)))
    ../../../../status-app-prs-rerun@tmp/venv/lib/python3.10/site-packages/selenium/webdriver/support/wait.py:86: in until
        value = method(self._driver)
    ../../../../status-app-prs-rerun@tmp/venv/lib/python3.10/site-packages/selenium/webdriver/support/expected_conditions.py:152: in _predicate
        return _element_if_visible(driver.find_element(*locator))
    ../../../../status-app-prs-rerun@tmp/venv/lib/python3.10/site-packages/appium/webdriver/webdriver.py:409: in find_element
        return self.execute(RemoteCommand.FIND_ELEMENT, {'using': by, 'value': value})['value']
    ../../../../status-app-prs-rerun@tmp/venv/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:345: in execute
        self.error_handler.check_response(response)
    ../../../../status-app-prs-rerun@tmp/venv/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:122: in check_response
        raise exception_class(msg=message, stacktrace=format_stacktrace(stacktrace))
     A session is either terminated or not started
    E   Stacktrace:
    E   NoSuchDriverError: A session is either terminated or not started
    E       at asyncHandler (/mnt/sauce/appium/appium-v2.11.0/packages/base-driver/lib/protocol/protocol.js:309:15)
    E       at /mnt/sauce/appium/appium-v2.11.0/packages/base-driver/lib/protocol/protocol.js:512:15
    E       at Layer.handle [as handle_request] (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/layer.js:95:5)
    E       at next (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/route.js:149:13)
    E       at Route.dispatch (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/route.js:119:3)
    E       at Layer.handle [as handle_request] (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/layer.js:95:5)
    E       at /mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:284:15
    E       at param (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:365:14)
    E       at param (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:376:14)
    E       at Function.process_params (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:421:3)
    E       at next (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:280:10)
    E       at logger (/mnt/sauce/appium/appium-v2.11.0/node_modules/morgan/index.js:144:5)
    E       at Layer.handle [as handle_request] (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/layer.js:95:5)
    E       at trim_prefix (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:328:13)
    E       at /mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:286:9
    E       at Function.process_params (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:346:12)
    E       at next (/mnt/sauce/appium/appium-v2.11.0/node_modules/express/lib/router/index.js:280:10)
    E       at /mnt/sauce/appium/appium-v2.11.0/node_modules/body-parser/lib/read.js:137:5
    E       at AsyncResource.runInAsyncScope (node:async_hooks:204:9)
    E       at invokeCallback (/mnt/sauce/appium/appium-v2.11.0/node_modules/raw-body/index.js:238:16)
    E       at done (/mnt/sauce/appium/appium-v2.11.0/node_modules/raw-body/index.js:227:7)
    E       at IncomingMessage.onEnd (/mnt/sauce/appium/appium-v2.11.0/node_modules/raw-body/index.js:287:7)
    E       at IncomingMessage.emit (node:events:513:28)
    E       at endReadableNT (node:internal/streams/readable:1359:12)
    E       at processTicksAndRejections (node:internal/process/task_queues:82:21)
    



    2. test_wallet_send_eth, id: 727229

    Expected amount of confirmations is 7, in fact 9604
    Expected amount of confirmations is 7, in fact 9678

    critical/test_wallet.py:142: in test_wallet_send_eth
        self.network_api.wait_for_confirmation_of_transaction(address=self.sender['wallet_address'],
    ../support/api/network_api.py:131: in wait_for_confirmation_of_transaction
        pytest.fail('The last transaction was not confirmed, address is %s, still has %s confirmations' % (
     The last transaction was not confirmed, address is 0x83DFF07F40642Fb42FEBB5deB6A314c8b725749E, still has 9678 confirmations
    



    Passed tests (5)

    Click to expand

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_edit, id: 702843
    Device sessions

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Device sessions

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    Device sessions

    @Horupa-Olena
    Copy link

    @Parveshdhull Hey! Thanks for your fix!

    If this is happening even for fresh account then I think its probably a new bug in UI side.

    This issue is fixed. But for old accounts (old = account that have few tries to syncs before) issue from #21260 still reproduce.

    @Horupa-Olena
    Copy link

    @Parveshdhull I don't see any other issues. The PR is ready to be merged.

    @Parveshdhull Parveshdhull merged commit 59c2b79 into develop Sep 25, 2024
    6 checks passed
    @Parveshdhull Parveshdhull deleted the fix/syncing-state branch September 25, 2024 14:10
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    Lost the syncing state in fallback recovery, when first enter the seed phrase for the wrong account
    6 participants