-
Notifications
You must be signed in to change notification settings - Fork 987
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
Conversation
Jenkins BuildsClick to see older builds (23)
|
0f7237c
to
2eec8e2
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.
Looks good! Also thanks for the function docs 👍
: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})] |
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.
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.
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.
They are not getting removed here, it's simply a small restructuring because some of the code is redundant.
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.
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)) |
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 know it already existed, but I wonder if this (seq (seq x))
makes sense 🤔
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.
Haha, fixed it
src/status_im/contexts/wallet/swap/select_asset_to_pay/view.cljs
Outdated
Show resolved
Hide resolved
Hey @alwx ! |
@mariia-skrypnyk I can't. It's based on another PR that's not merged yet. |
86% of end-end tests have passed
Failed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (6)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Hey @alwx! ISSUE 1: wrong icon for swap option Design vs Implementation QUESTION 1: If I have a specific token only on Account 2 and I do such steps:
Result: strange bottom sheet appears Is it it the scope of your PR or I should report it separately? |
fe3aa38
to
052e412
Compare
5c725d9
to
c07abca
Compare
@mariia-skrypnyk to be honest, I think both issues should be reported separately. First of all, because swaps are still behind the feature flag. |
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? |
@mariia-skrypnyk what do you mean? I've basically just rebased this one. |
@alwx oh, I see I thought it is smth new Will retest after rebase and let you know. |
0% of end-end tests have passed
Failed tests (7)Click to expandClass TestWalletOneDevice:
Class TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
86% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletMultipleDevice:
Passed tests (6)Click to expandClass TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
@alwx PR can be merged. Thanks. I will create followups. |
97a8640
to
dd2fcae
Compare
8d65295
to
e8f1fd3
Compare
bd42749
to
e3a8111
Compare
fixes #20337
fixes #20338
Implements all the swap flows. Based on #21140 that is getting reviewed individually.
In includes:
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
status: ready