-
Notifications
You must be signed in to change notification settings - Fork 984
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
[#21557] feat: show from account page in swap flow #21611
Conversation
Jenkins BuildsClick to see older builds (42)
|
d516016
to
d730706
Compare
d730706
to
f2900c3
Compare
f2900c3
to
cddf6c2
Compare
62% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletOneDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
:fx (if (and multi-account? (= view-id :wallet-stack) (not from-account)) | ||
[[:dispatch [:open-modal :screen/wallet.swap-from]]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question: From reading the code, i can understand that this opens the account selection screen (from) screen if the user has more than one account (any type). This is different in send/bridge flow. Only if the asset is available on multiple accounts, that's when we open the from screen with list of accounts that has that asset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we should open this modal only if the selected asset is in multiple accounts and has any balance on more than one account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it's different according to QA's doc list of test cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to design we list all accounts but show account with zero balance with disabled state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah maybe just condition here has a wrong text, otherwise we have account with zero balance in design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the designs three accounts are shown, which two of them have balance, and that's why the third one is disabled. But (from my understanding) if the user has two (or more) accounts and only balance for the selected asset in only one of them, the account selection page should not be shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess better to check linked issue description, #21557 (comment)
also list of list of test cases
I implemented it accordingly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation looks correct based in the test cases document, I am just wondering if test cases are correctly set, as design implies that the From page should be shown only if asset exist in multiple accounts, but on the document there are some test cases that expect the From page to be always shown if the user has more than one account, no matter if the asset exist on more than one account. I think we should double check test cases. cc @Horupa-Olena
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@briansztamfater Thanks for your comments! I’ll review these cases one more time.
cddf6c2
to
b2f6b15
Compare
@mohsen-ghafouri Thank you for your PR! |
Hey @Horupa-Olena yes please, thanks |
b2f6b15
to
f83deb5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise, it looks good to me!
@Horupa-Olena @pavloburykh, could you check this comment and verify that we are not misrepresenting the expected behavior? Thanks 🙏 #21611 (comment)
@briansztamfater @mohsen-ghafouri Thank you, guys! Here’s what I believe should be the correct behavior: The FROM page should only appear if more than one account holds the token selected for the swap.
Let me know your thoughts or if we need further alignment! cc @xAlisher |
Looks good to me |
thank you @xAlisher |
@mohsen-ghafouri @briansztamfater I agree with this behavior. If it’s implemented this way, I will start testing. |
Thank you @pavloburykh , could you please correct the description of linked issue and also test case document? as it will be reference for dev and QA. @Horupa-Olena I need to apply some changes to cover Scenario 1, will let you know once it's ready for review. |
@mohsen-ghafouri I have corrected issue description. Regarding the document , @VolodLytvynenko could you please correct it, as I have View only permissions. |
I just corrected the table thanx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the modifications @mohsen-ghafouri 🚀
Hey @Horupa-Olena, it's ready for test. thanks in advance |
88% of end-end tests have passed
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
|
@mohsen-ghafouri Thanks for your work! Unfortunately, I’m unable to trigger the From page. I restored a profile using a seed phrase that had two accounts with assets. Could you please take a look and check what might be wrong? Let me know if logs are needed. |
Hey @Horupa-Olena, a video of test would be helpful, as you can see in my last comment the result with my account is like this #21611 (comment) swap with account that has balance worked fine for my accounts |
9143d15
to
cc11a5f
Compare
Just retest it with latest changes, the result is the same, you should have an asset available at least in 2 accounts to see the "From" page in swap flow scenarios #21611 (comment) Simulator.Screen.Recording.-.iPhone.13.-.2024-11-21.at.18.51.09.mp4 |
@mohsen-ghafouri Sorry, I rushed to switch to another task and seem to have sent the comment without waiting for the video to upload. Screenrecorder-2024-11-21-18-15-29-966.mp4 |
@mohsen-ghafouri I retest on last build. The same result. ISSUE 1: From page not appears for multichain account Step
Expected results: Actual results: video_2024-11-21_18-26-56.mp4 |
Hi @mohsen-ghafouri, I am also reproducing the issue on the latest builds of current PR. I’ve tried two different users with varying numbers of accounts, some with more than three accounts holding assets. However, I am still unable to see the FROM page: FROM.mp4Devices:
|
cc11a5f
to
95cc78b
Compare
I might find the source if issue, it can't detect the root screen, i guess if you switch between tabs and then click on wallet tab it will start working, I just pushed a fix. will test and let you know if it's ready. |
All looks good now, in latest PR's build on my iPhone device, please test and let me know @Horupa-Olena . thanks 2024-11-21.21.22.23.mp4 |
95cc78b
to
a234559
Compare
@mohsen-ghafouri Thanks for your fix! Everything is finally working. screen-20241122-135719.mp4However, I can’t fully test the entire flow and verify that the assets are swapped on the correct account (chosen on the From page) because there’s a bug in the swap flow. I’m waiting for this PR to resolve the issue. I’d greatly appreciate it if you could also take a look at this issue next, as it’s related to the Swap flow and makes sense to address after fixing the problem with the From page. |
@mohsen-ghafouri You can merge it PR. |
a234559
to
398289a
Compare
Hey @Horupa-Olena thank you for testing this PR, sure i will try to check that as well |
fixes #21557
Summary
The "From" page should appear, allowing the user to choose the account for the swap.
Areas that maybe impacted
Steps to test
Result
Simulator.Screen.Recording.-.iPhone.13.-.2024-11-13.at.23.25.52.mp4
status: ready