Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

[ABA impact] - Re-enable Fiat Price Impact + minor optimisations #1986

Merged
merged 5 commits into from
Dec 13, 2021

Conversation

W3stside
Copy link
Contributor

@W3stside W3stside commented Dec 9, 2021

Re-enables the fiat price impact.

  1. Fiat Price Impact > Fallback ABA Price Impact
  2. Minor optimisations to UI/UX

Testing

  1. should get fiat price impacts
  2. XDAI should still calc fallback price impact on some pairs that dont have fiat (WXDAI <> 1INCH)

@W3stside W3stside changed the base branch from develop to release/v1.7.0 December 9, 2021 11:11
@W3stside W3stside requested review from a team December 9, 2021 11:11
@W3stside W3stside added the RELEASE Included in the release that is being closed label Dec 9, 2021
@W3stside W3stside mentioned this pull request Dec 9, 2021
2 tasks
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2021

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@W3stside
Copy link
Contributor Author

W3stside commented Dec 9, 2021

@elena-zh this PR doesn't have the logic from #1985 so you will see jumping values here.

@elena-zh
Copy link

elena-zh commented Dec 9, 2021

Hey @W3stside , is this OK to see the message along with a price impact value?
image

I'm clarifying due to it is a bit unclear to me what else, besides displaying price impacts even without USD estimations, to test.

@W3stside
Copy link
Contributor Author

W3stside commented Dec 9, 2021

Hey @W3stside , is this OK to see the message along with a price impact value? image

I'm clarifying due to it is a bit unclear to me what else, besides displaying price impacts even without USD estimations, to test.

hmm interesting, let me review this

@elena-zh
Copy link

elena-zh commented Dec 9, 2021

Hey @W3stside , great! Now the message is displayed only if a price impact is missing.
But could we do anything with this jumping message effect (see the video from 00:09 sec):

blink.mp4

@W3stside
Copy link
Contributor Author

W3stside commented Dec 9, 2021

Hey @W3stside , great! Now the message is displayed only if a price impact is missing. But could we do anything with this jumping message effect (see the video from 00:09 sec):

blink.mp4

should be fixed when #1985 is merged together

@W3stside
Copy link
Contributor Author

W3stside commented Dec 9, 2021

@gnosis/gp-qa @gnosis/gp-frontend I am preemptively merging please post merge review

@W3stside
Copy link
Contributor Author

W3stside commented Dec 9, 2021

rebased #1984 which has #1985

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.

Not related to this PR, but I didn't know in which to write it.

If you are connected, we fail to show the user if there's an issue with the price:
image

image

@elena-zh
Copy link

Hey @W3stside , I see an eternal loader when I select From token and its amount, but has not yet selected To token.
And vice versa: To+amount is specified, no From token.
image
I think, we could show it when a token pair and an amount are specified.

@elena-zh
Copy link

elena-zh commented Dec 10, 2021

Also, I can see an eternal loader when all tokens and amounts are specified. Sometimes I can't even press on the Swap button when the impact is being loaded (in regular mode):
image
image

This might be a result of failed BE requests or so.. But as an option, could we set something like a timer for this loading indicator?
If we do not get a calculation when timer's countdown ends, just show a warning that price impact is unknown.
@W3stside , @anxolin , WDYT?

@W3stside
Copy link
Contributor Author

@elena-zh infinite loader is fixed in latest commit

@elena-zh
Copy link

@W3stside , this issue

I see an eternal loader when I select From token and its amount, but has not yet selected To token.
And vice versa: To+amount is specified, no From token.

has been fixed.

But still I can face this issue
image

@elena-zh
Copy link

elena-zh commented Dec 13, 2021

Hey @W3stside , I have been able to face this: I see Price impact in percentage, but get price impact unknown warning. This is due to buy amount is too low to cover fees for the inverse sell order.
image

image

I think we shouldn't show percentage in this case.

The same is when there is insufficient liquidity for an inverse trade
image

Actually, it might be displayed even when there is enough liquidity for both trades, and amounts cover fees
image
I might assume that this is due to price/quotes loading errors...
image

@elena-zh
Copy link

elena-zh commented Dec 13, 2021

Also, I noticed that tokens' USD estimation appears disappears on the form (XDAI network)
https://watch.screencastify.com/v/LXxwuxocGvZWVVFz2GFe

@elena-zh
Copy link

Not related to this PR, but I didn't know in which to write it.

If you are connected, we fail to show the user if there's an issue with the price: image

image

Reported as a separate issue #1989

@elena-zh
Copy link

Changes LGTM besides this warning flashing issue
https://watch.screencastify.com/v/omnuebGgf9WziElOXQWi

@W3stside , let me know please if I need to create a separate issue for this case.

Thanks

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.

Seems alright to me, if this passes Elena tests im good :)

Some unrelated issue I see, is that ABA won't show ever more than 50%, when in reality it could be 100%

image

src/custom/hooks/usePriceImpact/useFallbackPriceImpact.ts Outdated Show resolved Hide resolved
src/custom/hooks/usePriceImpact/useFallbackPriceImpact.ts Outdated Show resolved Hide resolved
src/custom/hooks/usePriceImpact/useQuoteAndSwap.ts Outdated Show resolved Hide resolved
@W3stside
Copy link
Contributor Author

merging after successful build (addressed @Anxo comments)

@W3stside W3stside merged commit 5622d90 into release/v1.7.0 Dec 13, 2021
@elena-zh
Copy link

Seems alright to me, if this passes Elena tests im good :)

Some unrelated issue I see, is that ABA won't show ever more than 50%, when in reality it could be 100%

image

@anxolin , we have an issue for this on the board: #1979

Actually, the max 49.99% price impact appears when there is insufficient liquidity for the inverse trade

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
RELEASE Included in the release that is being closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants