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

Swap flows (launch from home / launch from account) #21269

Open
wants to merge 2 commits into
base: feature/20332-20333
Choose a base branch
from

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Sep 16, 2024

fixes #20337
fixes #20338

Implements all the swap flows. Based on #21140 that is getting reviewed individually.

In includes:

  1. Launching from the home screen.
  2. Launching from the account screen.
  3. Launching using the dedicated swaps button.

In all three cases the (pretty smart) selection of accounts being done, account to pay is always different from account to recieve + networks are being selected correctly.

Here is how it works:

Screen.Recording.2024-09-17.at.00.07.49.mov

Platforms

  • Android
  • iOS

status: ready

@alwx alwx self-assigned this Sep 16, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Sep 16, 2024

Jenkins Builds

Click to see older builds (19)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 0f7237c #1 2024-09-16 12:43:18 ~4 min tests 📄log
✔️ 0f7237c #1 2024-09-16 12:45:30 ~7 min android 🤖apk 📲
✔️ 0f7237c #1 2024-09-16 12:46:09 ~7 min android-e2e 🤖apk 📲
✔️ 0f7237c #1 2024-09-16 12:50:37 ~12 min ios 📱ipa 📲
2eec8e2 #2 2024-09-16 15:01:20 ~3 min tests 📄log
✔️ 2eec8e2 #3 2024-09-16 15:05:24 ~7 min android-e2e 🤖apk 📲
✔️ 2eec8e2 #3 2024-09-16 15:05:56 ~8 min android 🤖apk 📲
✔️ 2eec8e2 #3 2024-09-16 15:14:21 ~16 min ios 📱ipa 📲
be9ad7a #3 2024-09-16 21:44:14 ~2 min tests 📄log
✔️ be9ad7a #4 2024-09-16 21:48:06 ~6 min android 🤖apk 📲
✔️ be9ad7a #4 2024-09-16 21:49:00 ~7 min android-e2e 🤖apk 📲
✔️ be9ad7a #4 2024-09-16 21:53:15 ~11 min ios 📱ipa 📲
89b019c #4 2024-09-16 21:57:35 ~2 min tests 📄log
✔️ 89b019c #5 2024-09-16 22:02:30 ~7 min android-e2e 🤖apk 📲
✔️ 89b019c #5 2024-09-16 22:02:58 ~8 min android 🤖apk 📲
f05770f #5 2024-09-16 22:07:19 ~2 min tests 📄log
✔️ f05770f #6 2024-09-16 22:12:11 ~7 min android-e2e 🤖apk 📲
✔️ f05770f #6 2024-09-16 22:12:39 ~7 min android 🤖apk 📲
✔️ f05770f #6 2024-09-16 22:21:13 ~16 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
c07abca #7 2024-09-19 12:32:06 ~3 min tests 📄log
✔️ c07abca #8 2024-09-19 12:36:09 ~7 min android-e2e 🤖apk 📲
✔️ c07abca #8 2024-09-19 12:36:29 ~7 min android 🤖apk 📲
✔️ c07abca #8 2024-09-19 12:45:57 ~17 min ios 📱ipa 📲
bd42749 #8 2024-09-19 13:02:52 ~3 min tests 📄log
✔️ bd42749 #9 2024-09-19 13:06:18 ~6 min android 🤖apk 📲
✔️ bd42749 #9 2024-09-19 13:07:07 ~7 min android-e2e 🤖apk 📲
✔️ bd42749 #9 2024-09-19 13:17:44 ~18 min ios 📱ipa 📲

@alwx alwx changed the base branch from develop to feature/20332-20333 September 16, 2024 14:57
@alwx alwx marked this pull request as ready for review September 16, 2024 22:12
Copy link
Member

@clauxx clauxx left a comment

Choose a reason for hiding this comment

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

Looks good! Also thanks for the function docs 👍

src/status_im/contexts/wallet/common/token_value/view.cljs Outdated Show resolved Hide resolved
src/status_im/contexts/wallet/swap/events.cljs Outdated Show resolved Hide resolved
Comment on lines -88 to -102
:stack-id :wallet-stack
:start-flow? true
:owners token-owners})
bridge-params (if selected-account?
{:token token-data
:bridge-disabled? (or (not has-balance?)
(send-utils/bridge-disabled? token-symbol))
:stack-id :screen/wallet.accounts
:start-flow? true
:owners token-owners}
{:token-symbol token-symbol
:bridge-disabled? (send-utils/bridge-disabled? token-symbol)
:stack-id :wallet-stack
:start-flow? true
:owners token-owners})]
Copy link
Member

@briansztamfater briansztamfater Sep 17, 2024

Choose a reason for hiding this comment

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

Removing these changes may affect bridge and send flows navigation. Are you sure we want to remove them?
We should ask @status-im/mobile-qa team to verify we are not introducing regressions with these changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are not getting removed here, it's simply a small restructuring because some of the code is redundant.

Copy link
Contributor

@ulisesmac ulisesmac left a comment

Choose a reason for hiding this comment

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

Hi @alwx !

Thanks for the PR, I left a couple of comments

(action-receive selected-account)
(when (ff/enabled? ::ff/wallet.swap)
(action-swap params))
(when (seq (seq token-owners))
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it already existed, but I wonder if this (seq (seq x)) makes sense 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, fixed it

src/status_im/contexts/wallet/swap/events.cljs Outdated Show resolved Hide resolved
src/status_im/contexts/wallet/swap/utils.cljs Outdated Show resolved Hide resolved
@mariia-skrypnyk mariia-skrypnyk self-assigned this Sep 18, 2024
@mariia-skrypnyk
Copy link

Hey @alwx !
Can you please rebase your PR?

@alwx
Copy link
Contributor Author

alwx commented Sep 18, 2024

@mariia-skrypnyk I can't. It's based on another PR that's not merged yet.
#21140

@status-im-auto
Copy link
Member

86% of end-end tests have passed

Total executed tests: 7
Failed tests: 1
Expected to fail tests: 0
Passed tests: 6
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: Looking for a message by text: Message AFTER edit 2 (Edited)
    Device 2: Find `ChatElementByText` by `xpath`: `//*[starts-with(@text,'Message AFTER edit 2 (Edited)')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']`

    critical/chats/test_public_chat_browsing.py:378: in test_community_message_edit
        self.channel_2.set_reaction(message_text_after_edit)
    ../views/chat_view.py:1053: in set_reaction
        self.chat_element_by_text(message).long_press_until_element_is_shown(element)
    ../views/base_element.py:327: in long_press_until_element_is_shown
        element = self.find_element()
    ../views/chat_view.py:116: in find_element
        self.wait_for_visibility_of_element(20)
    ../views/base_element.py:147: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: ChatElementByText by xpath:`//*[starts-with(@text,'Message AFTER edit 2 (Edited)')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element
    



    Device sessions

    Passed tests (6)

    Click to expand

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    Device sessions

    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

    @mariia-skrypnyk
    Copy link

    mariia-skrypnyk commented Sep 18, 2024

    Hey @alwx!
    Thanks for your PR.

    ISSUE 1: wrong icon for swap option

    Design vs Implementation

    Screenshot 2024-09-18 at 15 43 42

    QUESTION 1:

    If I have a specific token only on Account 2 and I do such steps:

    1. Long tap on the token from the home screen.
    2. Choose swap option.
    3. Change account on Swap screen
    4. Go back (sometimes I need 2-3 tries to repeat 3 and 4 steps)
    5. Make long tap on the same token from step 1
    6. Choose swap option.

    Result: strange bottom sheet appears
    photo_2024-09-18_15-54-20

    Is it it the scope of your PR or I should report it separately?

    @alwx
    Copy link
    Contributor Author

    alwx commented Sep 19, 2024

    @mariia-skrypnyk to be honest, I think both issues should be reported separately. First of all, because swaps are still behind the feature flag.
    The one with the icon is definitely not important — we currently don't have the icon you're talking about in our resources. I can add it but we can also leave it for the part when we're doing the design reviews.
    The second one is a bit of an edge case — I would be happy to investigate but I'm currently busy with another issue and I think that might wait.

    @mariia-skrypnyk
    Copy link

    mariia-skrypnyk commented Sep 19, 2024

    Ok @alwx , will create followups and ask @status-im/design-team for an icon for swap option from ISSUE 1.

    What fixes did you push while ago? What changed?

    @alwx
    Copy link
    Contributor Author

    alwx commented Sep 19, 2024

    @mariia-skrypnyk what do you mean? I've basically just rebased this one.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Status: IN TESTING
    Development

    Successfully merging this pull request may close these issues.

    Launch from account Launch from home
    6 participants