-
Notifications
You must be signed in to change notification settings - Fork 988
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
feat: trigger swap transactions #21134
Conversation
2e5bba4
to
c131380
Compare
Jenkins BuildsClick to see older builds (60)
|
c131380
to
dfcf773
Compare
71% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
hi @briansztamfater could you please rebase the current PR? thanx |
@briansztamfater should the swap feature still be feature-flagged? In the latest build, I had to manually enable it in the feature flags to see the swap option |
@briansztamfater I think some commits might be missing in the latest build because the swap UI looks completely different from what was shown in the demo compared with the latest build where the issue is found. Could you please check? |
swap-approval-transaction? (= swap-approval-transaction-id tx-hash) | ||
swap-transaction-ids (get-in db [:wallet :swap-transaction-ids]) | ||
swap-transaction? (and swap-transaction-ids | ||
(contains? swap-transaction-ids tx-hash))] |
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.
Is swap-transactions-id
a set ?
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.
It is
receive-amount receive-token-symbol]} (get-in db | ||
[:wallet :transactions tx-hash | ||
:swap-data]) | ||
transaction-confirmed-or-failed? (#{:confirmed :failed} status) |
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.
What is this sorcery 😅
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.
Just checking if status of the transaction is the final one (either confirmed or failed). Saw this pattern in many places of the codebase, for example this:
(when (and (not dont-sync?) (#{:name :preferred-name} setting)) |
dfcf773
to
4b44668
Compare
We should remove it at some point, I'll raise a new PR removing the flag 👍 About issue 1, please try again, I have just updated the code. As we don't have a way (yet) to select the asset to receive, I hardcoded USDT for testing purposes, but SNT should be the default, which is a token available for all users. If you don't have USDT in your tokens list that may cause problems for now. Once #21140 is done that should be fine. |
Also I will rebase, it will take some time as there is some PR chaining so I need to rebase the dependant PRs first |
49888c6
to
37c70c8
Compare
4b44668
to
698a8ec
Compare
71% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletMultipleDevice:
Passed tests (5)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
@briansztamfater thank you for clarification. Unfortunately I am blocked to test the swap flow fully due status go issue . Only some UI issues are found: ISSUE 2: [Android] Device's native keyboard appears on Swap pageSteps:Go to the swap page within the app on an Android device. Actual result:The device's native keyboard automatically opens, resulting in two keyboards being displayed: the app's keyboard and the device's native keyboard. Expected result:Only the app's keyboard should be displayed, without triggering the device's native keyboard. ENV:Pixel 7a, Android 13 |
ISSUE 3: Max value balances not updating according to selected accountSteps:
Actual result:The max value balances displayed do not update to reflect the newly selected account's balances swapbalances.mp4Expected result:The max balances should update to reflect the selected account's available funds on the swap page. |
@briansztamfater Not sure if issue 2 and issue 3 are within the scope of this PR. If not, I'll create them separately. |
4c7487f
to
4321942
Compare
0ae3d1b
to
981100f
Compare
4321942
to
51487f6
Compare
981100f
to
ffde94f
Compare
issue 5 is not fixed fully. |
86% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletOneDevice:
Passed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
|
issue 2 is still reproducible. I missed one more scenario where it occurs: the keyboard appears when the app returns from the background. Steps:
Actual result:The device's native keyboard automatically opens, resulting in two keyboards being displayed: the app's keyboard and the device's native keyboard keyboard.mp4Device:Pixel 7a, Android 14 |
ISSUE 7: "View" button available after and during ERC-20 Token approval, leading to errorsSteps:
Actual result:The view_button_error.mp4Expected result:The |
It looks like Paraswap struggles with certain decimal values on the Swap Page. To prevent this, I suggest implementing rounding for entered amounts on the swap page, similar to what was done on the send page in PR #20915. This would round values so that the final decimal place represents a value between $0.01 and $0.10 USD, ensuring proper display of small amounts and avoiding issues with third-party integrations. WDYT, @xAlisher? |
ffde94f
to
85c05f1
Compare
@VolodLytvynenko issue 2 happening this way is weird, for now I am forcing keyboard dismiss on Android every time the input is focused. The trade-off of this approach is that the input will loose focus and cursor will not be shown. We can create an issue to do some further research on this. iOS behavior remains as usual. Also regarding issue 7, I removed the View button as it is not shown in Swap designs. |
IMO this deserves a bit more research, we can track it in a separate issue and add all the findings there, because I am not sure what are the actual errors (will add better error handling for mobile soon) and how is the best way to prevent them, if there is any. Also maybe this impacts Desktop implementation too. |
51487f6
to
fbe2bfc
Compare
4abc589
to
9940f3c
Compare
@briansztamfater For sure. You've done a lot of work in this PR. This feature needs a separate follow-up. |
Yes, we can work on this on a follow up, and discuss the desired approach with designers. Not sure if we should round the value returned by the swap provider, but we can discuss it in a separate issue. Let me know if there's something else to fix in this PR! |
86% of end-end tests have passed
Failed tests (1)Click to expandClass TestWalletMultipleDevice:
Passed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
|
Hi @briansztamfater, thank you for your work. Issues are fixed. PR is ready to be merged |
Signed-off-by: Brian Sztamfater <[email protected]>
9940f3c
to
f12e0e5
Compare
fixes #20381
fixes #20344
Summary
This PR integrates swaps transactions actually triggering after user confirms the transaction. Also adds proper toasts notifications as per designs.
executeswap.mp4
Platforms
Functional
Steps to test
status: ready