Skip to content
This repository has been archived by the owner on Jan 15, 2021. It is now read-only.

1163/non intrusive price suggestion #1164

Merged
merged 20 commits into from
Jul 6, 2020

Conversation

alfetopito
Copy link
Contributor

@alfetopito alfetopito commented Jun 29, 2020

Update July 2

  • Added swap icon
  • No longer showing infinity prices
  • Added tooltip. Please validate text contents:
    screenshot_2020-07-02_13-53-28

Update Jun 30

  • Did a minimal styling, properly aligning things and matching other parts of the app
  • Adjusted widget height to fit the new entry
  • No longer displaying infinity for bad prices. Using N/A instead
  • Disabling input when N/A
  • Added base/quote token suffix to be explicit on what this price refers too (although being a bit redundant)
  • Added selling word to sentence to make it clear what the amount refers to: Onchain orderbook price for selling <amount> <symbol|name|address>
  • Fixed the displayed symbol in the sentence. Previously it was changing every time the price was inverted
  • Displaying second price estimation only when amount is different from default of 1 unit.

Worst case scenario (long token symbols):
screenshot_2020-06-30_14-49-37

Regular use case (short token symbols):
screenshot_2020-06-30_15-05-39

TODO:

  • Show % difference between prices (slippage?)
  • Add tooltips
  • Add swap price buttons

Proposal

No longer automatically update price inputs with price suggestion.
Instead, show a button and allow the user to make the choice.

Closes #1163

This change adds mostly the logic of fetching the prices and displaying it in a button.

I added 1 price for a single unit and one price for the selected amount.
This way the user has a reference of the slippage given his amount.

When switching to the inverse price, the suggestion is also inverted.
When clicking in a button, both prices are filled.

screenshot_2020-06-29_15-09-52

It's hideous, I know. Will work on that.

As always, open for suggestions/feedback.

TODO:

  • Style
  • Do not show infinity when one of the prices is too big/too small
  • More testing, look for edge cases, etc

@alfetopito alfetopito self-assigned this Jun 29, 2020
@alfetopito alfetopito requested review from Velenir, W3stside and anxolin and removed request for Velenir June 29, 2020 22:15
@ghost
Copy link

ghost commented Jun 29, 2020

Travis automatic deployment:
https://pr1164--dexreact.review.gnosisdev.com

@W3stside
Copy link
Contributor

@alfetopito careful on the height of the div considering the added content aka price suggestion. If it gets too tall (e.g when the user puts in too much sell and gets the over balance warning) the submit button will wrap to the right and look shitty af

Copy link
Contributor

@Velenir Velenir left a comment

Choose a reason for hiding this comment

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

Really like the new non-intrusive price flow 👍

@alfetopito
Copy link
Contributor Author

@alfetopito careful on the height of the div considering the added content aka price suggestion. If it gets too tall (e.g when the user puts in too much sell and gets the over balance warning) the submit button will wrap to the right and look shitty af

Indeed. In that case, what's the best way to handle it? Just increase the default max height?

@alfetopito alfetopito requested a review from W3stside June 30, 2020 22:14
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I think I never added the feedback from yesterday here.

I'll add it just for documenting.


I think the proposal is super good, and allow us to grow the number of price references, possibly showing also external feeds or the off-chain order book

I have some suggestions:

  1. Invert the price icon 🔁 I understand, they should also have the icon, and it will invert all the prices. Including the one he inputed. Of course the other way around. If he clicks in the 🔁 in the limit price, it inverts also the suggestions

  2. Show an indicator when our price is +10% ---> we overpay (not necessarily this PR / 1st version)

  3. Show an indicator when our price is -10% ----> we underpay (maybe it’s not executed in 1 batch) (not necessarily this PR / 1st version)

  4. Show an indicator of how liquid/illiquid is a market ---> how can we do this? all deposit tokens can be a good start, but it doesn’t mean it’s liquid. Trades could give you an idea. Also the volume for certain amounts. What people think about this?. This info could be used for showing a warning or some indication to the user (the price shouldn’t be trusted for these illiquid markets). ---> Chen mentioned that this one is tricky, he will think about a possible trust score definitely not necessarily this PR / 1st version`)

  5. Units: I think we have still room for adding the units in the price. ie. 1.00117 DAI

  6. Add tooltips for the prices. This way, we can properly explain, what does it mean Onchain price for that volume. (good to have)

  7. nit: Price Icon: If we aggregate more price in the future. Add a small icon for each price, as dex aggregator do -> (not necessarily this PR / 1st version)

  8. nit: Highlight or tag the onchain price as the recommended option. If we have the liquidity indicator, we could remove the recommendation (not necessarily this PR / 1st version)

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I really love it ❤️

I think it's good already as a first version, and we can handle improvements like warnings and so on in different PRs. This way, we handle most of the issues and we deliver sooner a new version (that has many things already).

Some thoughts and comments:

  1. hahah, I didn't caught you on this one! Good :)

image

  1. At the beginning, I didn't like that we show this in multiple ways. But now I've been using it a bit, I actually like it more. The suggestions show the market, and the users price show in QUOTE ber BASE. Let's double check with Chen and @Rafanator , but I'm good with that:

image

  1. Can we make this also clickable? or add the 🔁 icon next to it?
    image

  2. I think you are aware already, but there's an INFiNiTE in the price when you load it.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Other than a minor things, like the INFINITE, and probably the tooltip, I think it's good to go.

The others we can add it to our backlog. Feel free to continue working on them since you are on 🔥
But I think we should freeze one version with what we have and deliver

@alfetopito alfetopito force-pushed the 1163/non-intrusive-price-suggestion branch from 6dfd889 to 4215afd Compare July 2, 2020 19:46
@alfetopito
Copy link
Contributor Author

Other than a minor things, like the INFINITE, and probably the tooltip, I think it's good to go.

The others we can add it to our backlog. Feel free to continue working on them since you are on fire
But I think we should freeze one version with what we have and deliver

@anxolin addressed your comments. I'll wait for your feedback on the tooltip text before merging, though.

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I think it's perfect.

As an optional, and subjective nit. I think it look nice to add a dotted underline to the concept in there you are providing contextual help.

image

I would also add a link to to this: https://docs.gnosis.io/protocol/docs/introduction1/

@alfetopito alfetopito force-pushed the 1163/non-intrusive-price-suggestion branch from df8270e to 7154f03 Compare July 6, 2020 16:53
@alfetopito alfetopito merged commit b8eb3a0 into develop Jul 6, 2020
@alfetopito alfetopito deleted the 1163/non-intrusive-price-suggestion branch July 6, 2020 17:05
@alfetopito alfetopito mentioned this pull request Jul 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make price estimation non-intrusive
4 participants