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

feat: swaps on send flow when amount is insufficient #6980

Merged
merged 34 commits into from
Jan 3, 2024

Conversation

tommasini
Copy link
Contributor

@tommasini tommasini commented Aug 7, 2023

Description
PROPOSAL:
During the send flow on the amount screen:

  • Possibility of swaps when ERC-20 is selected and the amount is insufficient.
  • Possibility of Buy when the main Token amount is insufficient.

UX to improve
After navigating to the swaps

  • Reason why we do not navigate back to the send flow on the end, it's because the transaction object on redux it's being replaced by the swaps transaction

Future development proposal
The transaction object should have a type (swaps, send_flow, dapp) or split the transaction objects, this way we can have more than one transaction in redux state, which could open opportunities to improve the UX, such as this example.

Screenshots/Recordings
Recording on buy: https://recordit.co/NBdij78xev
Recording on swap: https://recordit.co/4kUMifjSk6

E2E test
https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/55932a50-d34a-4599-9825-cca5aa681230

Issue

fixes #???

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

@tommasini tommasini requested a review from a team as a code owner August 7, 2023 20:20
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@gauthierpetetin gauthierpetetin added the needs-design Feature that requires UI/UX design label Aug 14, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (0601deb) 39.63% compared to head (a7f57fd) 39.61%.

Files Patch % Lines
app/components/UI/Swaps/index.js 0.00% 12 Missing ⚠️
app/components/Views/SendFlow/Amount/index.js 36.36% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6980      +/-   ##
==========================================
- Coverage   39.63%   39.61%   -0.03%     
==========================================
  Files        1233     1233              
  Lines       29810    29830      +20     
  Branches     2840     2840              
==========================================
  Hits        11816    11816              
- Misses      17302    17321      +19     
- Partials      692      693       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bschorchit
Copy link

Love this! Could the "Swap" tokens follow the same design pattern as the "Buy more" and be a link within the warning instead of new button?

@bschorchit
Copy link

This might be outside of scope, but it would be nice to add metrics on this so we can measure its impact and effectiveness. It could be triggering a Link Clicked event with properties:

  • location = insufficient_funds_warning
  • text = buy_more or swap_tokens

@tommasini
Copy link
Contributor Author

@bschorchit addressed, Here it goes the recording: https://recordit.co/4kUMifjSk6

@tommasini tommasini added needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) and removed needs-design Feature that requires UI/UX design labels Oct 11, 2023
app/components/Views/SendFlow/Amount/index.js Outdated Show resolved Hide resolved
app/components/UI/Swaps/index.js Outdated Show resolved Hide resolved
app/components/Views/SendFlow/Amount/index.js Outdated Show resolved Hide resolved
NicolasMassart
NicolasMassart previously approved these changes Oct 13, 2023
Copy link
Contributor

@NicolasMassart NicolasMassart 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 to me.

@tommasini tommasini added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Oct 19, 2023
@tommasini
Copy link
Contributor Author

Regression Pipeline running that have 2 swaps test flows https://app.bitrise.io/app/be69d4368ee7e86d/pipelines/8b0d7b14-8b16-4df9-a816-c50d08de6bf1

Important to notice that the swaps e2e are 4 test files, 2 of them are on the smoke e2e and 2 of them are on the regression pipeline

@tommasini tommasini changed the title feat: swaps on send flow when amount is insufficient (proposal) feat: swaps on send flow when amount is insufficient Nov 10, 2023
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Nov 10, 2023
@tommasini tommasini mentioned this pull request Nov 10, 2023
14 tasks
@tommasini
Copy link
Contributor Author

Last recording: https://recordit.co/O7a8J82Xg2

NicolasMassart
NicolasMassart previously approved these changes Dec 6, 2023
Copy link
Contributor

@NicolasMassart NicolasMassart 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 to me.

Copy link

@cortisiko cortisiko added QA Passed A successful QA run through has been done and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Dec 26, 2023
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

this looks good to me. Scenarios I also considered:

  • attempting to send funds on a network that does not support buying. Ensuring the alert which takes you to the buy flow is not displayed
  • attempting to send funds on a network that does not support swapping. Ensuring the alert which takes you to the swap flow is not displayed

@tommasini tommasini merged commit 034f549 into main Jan 3, 2024
28 of 29 checks passed
@tommasini tommasini deleted the feat/swaps-on-send-flow branch January 3, 2024 15:46
@github-actions github-actions bot locked and limited conversation to collaborators Jan 3, 2024
@metamaskbot metamaskbot added the release-7.15.0 Issue or pull request that will be included in release 7.15.0 label Jan 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template QA Passed A successful QA run through has been done release-7.15.0 Issue or pull request that will be included in release 7.15.0 team-mobile-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants