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.

Sign up for GitHub

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

Jump to bottom

feat: sell eth warning for limit/twap #3573

Merged
merged 15 commits into from
Jan 11, 2024
Merged

feat: sell eth warning for limit/twap #3573

merged 15 commits into from
Jan 11, 2024

Conversation

alfetopito
Copy link
Collaborator

Summary

Part of 2024 roadmap.

Improve sell ETH UX for limit orders.

While UX wise the ethflow contract is not usable IMO for Limit orders, the current UX is also not optimal.
When the user tries to sell ETH for anything other than WETH (a wrap), the UI silently switches it to WETH.

This PR makes it so:

  1. ETH is selectable both on limit and twap
  2. The trade button is disabled
  3. A warning with a short explanation and the options available are displayed

Screenshot 2024-01-04 at 14 34 06

Think of it as a poor's man version of the classic eth flow.

Notes

1: While Safe Apps could in theory do the wrap + order placement as it's available now for SWAPs on LIMIT and TWAP, it's out of the scope of this PR, mainly for a similar reason as for the ethflow contract.
While on LIMIT and TWAP the funds are NOT locked in the ethflow contract, they will end up in a different token and might be wrapped needlessly, in case the trades don't happen.

2: Text is definitely not final. Open to suggestions.

To Test

  1. Open the limit orders with an EOA
  2. Pick eth as sell token
  • Should not be tradable
  • Should show warning
  1. Click on Switch to WETH
  • Should select WETH as the sell token. The rest of the parameters should remain unchanged
  1. Go back and click on Wrap ETH to WETH
  • Should switch to wrapping ETH for ETH. The rest of the parameters should remain unchanged
  1. Repeat 2-4 with a Safe App
  • Should have the same results
  1. Repeat 2-4 with a Safe App, on TWAP form
  • Should have the same results

@alfetopito alfetopito self-assigned this Jan 4, 2024
@vercel Vercel
Copy link

vercel bot commented Jan 4, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
cosmos ✅ Ready (Inspect) Visit Preview Jan 11, 2024 4:12pm
explorer ✅ Ready (Inspect) Visit Preview Jan 11, 2024 4:12pm
swap-dev ✅ Ready (Inspect) Visit Preview Jan 11, 2024 4:12pm
widget-configurator ✅ Ready (Inspect) Visit Preview Jan 11, 2024 4:12pm

@alfetopito alfetopito requested review from shoom3301, elena-zh and a team and removed request for shoom3301 January 4, 2024 14:47
@alfetopito alfetopito requested review from a team and shoom3301 January 5, 2024 16:19
@elena-zh
Copy link
Contributor

elena-zh commented Jan 9, 2024

Hey @alfetopito , great job!

Some notes from my side:

  1. I'd add a link to the Swap trade as well (will navigate to the swap page)
    add link to swap
  2. I'd make links more noticeable (at least, always underlined like we do on TWAP warnings)
    image
    links more visible
  3. I'd not to show the warning unless the 2nd token is not selected as user may select WETH for wrapping.
    not to show
  4. When not connected, the warning is displayed on the Limit orders page, and is not displayed on the TWAP page. I think, the behavior should be the same for both forms
    limit
    twap
  5. For a buy order on the Limit orders page, when I press on the wrap button, it takes a buy amount to wrap instead of the value in the Sell field
    buy order
    takes buy amount

Thanks!

@alfetopito alfetopito force-pushed the feat/sell-eth-warning-limit branch from 38dfcca to ab12657 Compare January 9, 2024 12:27
anxolin
anxolin reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@anxolin anxolin left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is great! thanks for doing it.

I would like to suggest to do another message in this direction (maybe a bit better presented):

image

@alfetopito alfetopito force-pushed the feat/sell-eth-warning-limit branch from ab12657 to c740d71 Compare January 9, 2024 18:01
@alfetopito
Copy link
Collaborator Author

Hey @alfetopito , great job!

Some notes from my side:

1. I'd add a link to the Swap trade as well (will navigate to the swap page)
   ![add link to swap](https://private-user-images.githubusercontent.com/70885163/295195980-36292c78-1357-4557-bf2e-7daec24d933b.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTU5ODAtMzYyOTJjNzgtMTM1Ny00NTU3LWJmMmUtN2RhZWMyNGQ5MzNiLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQ2NDFjOTljOTAzZmJiZjM2YjkwMjAwNjYwNjEzNGEwZDc0MWNkY2FkNTdhYmQ4MDljMWNiN2U3Zjc3ZWJhMmQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.7s0YvjFJj0Kd5BcTrB7VW3mpR4ZMNMMtpd0j6yK6q4w)

UI/UX folks rather not add it, as that's not user intention when being on LIMIT page, so we'll leave without it.

2. I'd make links more noticeable (at least, always underlined like we do on TWAP warnings)
   ![image](https://private-user-images.githubusercontent.com/70885163/295196190-53f1dba2-5f0b-4f47-9413-1b1aefcc0375.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTYxOTAtNTNmMWRiYTItNWYwYi00ZjQ3LTk0MTMtMWIxYWVmY2MwMzc1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWVhZThjMDE3YzQzYjQ0NTY3MjZjOWVkMjRhMzU5NzQwODllNzBjMWQwZGRkNGRjODJmYmQwOWE3OGZiMzBhOWQmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.kgx0wyNsVQXe8Vn9Suz3Qxvj5HKTlxwYAWV8dCa_qNU)
   ![links more visible](https://private-user-images.githubusercontent.com/70885163/295196236-a4d65a37-7755-4fc5-9d37-2cd57fcb0c43.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTYyMzYtYTRkNjVhMzctNzc1NS00ZmM1LTlkMzctMmNkNTdmY2IwYzQzLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQ2MTUyMjYzNzVkNTA2NDgwY2RhOGI2ZDg3NjViYTA0ZTFlZjE3OTg0MTMxYWM4YzhlNzZkOTExNjc3MGVkYWEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.47oe5pUnNolSWXlsA24Mr0bNgEUo9ijD66wA-egJAK8)

Done

3. I'd not to show the warning unless the 2nd token is not selected as user may select WETH for wrapping.
   ![not to show](https://private-user-images.githubusercontent.com/70885163/295196736-00563c00-e2b2-4d79-8f74-f52c4e06333a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTY3MzYtMDA1NjNjMDAtZTJiMi00ZDc5LThmNzQtZjUyYzRlMDYzMzNhLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTcyMTZhYTZmMmI3YzIyYjBhZmY1Y2QxNmY4Yjc0ZTU1YzU3YzdiMDcyZDMzYTZlOWQ0MmRlYmFhMzM4YjczNWYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.J2MvmuucfP8HWZcPb6CiSsFHvER_JEVk1NN6Dllkgmo)

Done

4. When not connected, the warning is displayed on the Limit orders page, and is not displayed on the TWAP page. I think, the behavior should be the same for both forms
   ![limit](https://private-user-images.githubusercontent.com/70885163/295196988-58b91f3c-304d-41aa-947b-6b810f6c6d6e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTY5ODgtNThiOTFmM2MtMzA0ZC00MWFhLTk0N2ItNmI4MTBmNmM2ZDZlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTlkYTA1NTVmYzE2NmIxZTU3OGRmMjliYzgwMmY5NjZiZmJlYjYxOTdiOTJhOGIzZTU1OGZkYjZjOGQ1ZjVmZDYmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.1vGH6vxZkwGWqrD89ZjO3GFprCnLqbrJq9-Wc6XgoF8)
   ![twap](https://private-user-images.githubusercontent.com/70885163/295196995-399922f0-0be1-4c25-9663-c465492a796f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTY5OTUtMzk5OTIyZjAtMGJlMS00YzI1LTk2NjMtYzQ2NTQ5MmE3OTZmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTkxOWE1ODliZjg4ZmE1YjBmYWQzN2JkNDYwNTkyMDc3NjU1OWU5NGEzOGQwZGMzMTlhNDUwODRjYWVjZmE1NjkmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.vkAXMlH2xu0JEa6T4_1-7vVwQK-z85v1YCkinEGQ6Lo)

Done

5. For a buy order on the Limit orders page, when I press on the wrap button, it takes a buy amount to wrap instead of the value in the Sell field
   ![buy order](https://private-user-images.githubusercontent.com/70885163/295197394-44f5724e-e78f-430d-aed9-890d49c191f4.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTczOTQtNDRmNTcyNGUtZTc4Zi00MzBkLWFlZDktODkwZDQ5YzE5MWY0LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTBjYzMyYTNkYzE5ZWZkNzFlNTgzYjJlYTZlNGVmY2M0OGJkMzYyNzU0YTZlY2U3ODFmZDQ1YTk1ODhhYjg1MjcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.odj5y3_u1d5T2GbcwHcy_rQdzNaubACwX-wNZh0rfyg)
   ![takes buy amount](https://private-user-images.githubusercontent.com/70885163/295197401-e6052d3b-3d09-4383-99e8-ac2858893735.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MDQ4MDM0NTEsIm5iZiI6MTcwNDgwMzE1MSwicGF0aCI6Ii83MDg4NTE2My8yOTUxOTc0MDEtZTYwNTJkM2ItM2QwOS00MzgzLTk5ZTgtYWMyODU4ODkzNzM1LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDAxMDklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwMTA5VDEyMjU1MVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWM2MGIzYWRkNzA4MzFlYTlhNmFlY2QxYjJlNzdlMjdhY2I5MmYyZDIzNjJlOWMxZjNjYTQxZTc4M2EyZTY0ZTcmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.cwnB3OucjoatQtuRQt5f0de59ZAtHLIN9ulr3JFa8R0)

Working on it

@elena-zh
Copy link
Contributor

Hey @alfetopito , all changes LGTM!
Waiting for the last fix.
One more issue I faced only once:

  1. I opened the PR link
  2. specified a Buy limit order with ETH token
  3. and from the Limit orders page connected to the wallet --> I got a crash
    conenct from Limit orders page

But I was not able to reproduce the issue once again :(

@alfetopito alfetopito force-pushed the feat/sell-eth-warning-limit branch from c740d71 to 2f4c3cb Compare January 11, 2024 10:34
elena-zh
elena-zh approved these changes Jan 11, 2024
View reviewed changes
Copy link
Contributor

@elena-zh elena-zh 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, thank you!

@alfetopito
feat: add SellNativeWarningBanner
7f004ec
@alfetopito
feat: show native sell warning on limit
67a8ec2
@alfetopito
feat: enable native selling on limit
a85ef7f
@alfetopito
refactor: rename variable to showNativeSellWarning
d87bd00
@alfetopito
feat: show eth sell warning for safe app too
e5d1ca8
@alfetopito
feat: show sell native warning for TWAP
95c0a00
@alfetopito
refactor: add SellNativeWarningBanner non-pure
d8ccdc8
@alfetopito
refactor: remove duplication as form validation already knows to show…
4aaef40 
… warning
@alfetopito
fix: add underline under links
04930c2
@alfetopito
chore: rely on button validation only
338a52e
@alfetopito
chore: do the warning before to take precedence over empty input
febfa73
@alfetopito
feat: add parameterizeTradeSearch
3ce5475
@alfetopito
feat: optionally pass sell/buy amount when navigating
db8f703
@alfetopito
feat: use parameterizeTradeSearch on Twap banner
925dbc9
@alfetopito
feat: use sell amount when chosing to use wrapped token
ac6a97c
@alfetopito alfetopito force-pushed the feat/sell-eth-warning-limit branch from 5787984 to ac6a97c Compare January 11, 2024 16:10
@alfetopito alfetopito merged commit ff14c4b into develop Jan 11, 2024
11 checks passed
@alfetopito alfetopito deleted the feat/sell-eth-warning-limit branch January 11, 2024 16:56
@github-actions github-actions bot locked and limited conversation to collaborators Jan 11, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@anxolin anxolin anxolin left review comments

@elena-zh elena-zh elena-zh approved these changes

@shoom3301 shoom3301 shoom3301 approved these changes

Assignees

@alfetopito alfetopito

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alfetopito @elena-zh @anxolin @shoom3301