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

Don't call fee/price endpoint on Wrapping (ETH/WETH || XDAI/WXDAI etc) #1920

Merged
merged 6 commits into from
Dec 8, 2021

Conversation

W3stside
Copy link
Contributor

@W3stside W3stside commented Nov 25, 2021

Summary

Closes tech debt: dont call fee/price on wrapping op

Closes #1853 and closes #1927

Dont call fee/price on wrapping operation, any chain

image

To Test

  1. any chain
  2. select native token IN/OUT
  3. inverse = wrapped native
  4. enter amount
  5. check networks tab that no requests are made
  6. can still wrap etc

@W3stside W3stside requested review from a team November 25, 2021 13:27
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@W3stside W3stside changed the title isWrapping check in price updater Don't call fee/price endpoint on Wrapping (ETH/WETH || XDAI/WXDAI etc) Nov 25, 2021
Copy link

@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.

LGTM!

Copy link
Contributor

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

It is working fine.

Only caveat is that, when not connected, the behaviour is quite weird

I had another token selected as buy token.
The price loaded.
Then I picked ETH as the buy token and got this:
Screenshot from 2021-11-25 08-22-10

Switching around and manually changing the input resulted in 0 for the opposite
Screenshot from 2021-11-25 08-33-11

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.

Thanks for the fix, it doesn't do the request anymore, but before merging I would review the case mentioned by @alfetopito , cause I think that's an undesired effect

Maybe is cause chainId is undefined? If that's the case, for the isWrapping check we should assume mainnet (as the app does)

@W3stside W3stside force-pushed the dont-call-fee-weth-eth branch from 83238f3 to 466ef3f Compare December 3, 2021 16:43
@alfetopito
Copy link
Contributor

I can still reproduce the issue when not connected.
Screenshot from 2021-12-03 12-27-20
Screenshot from 2021-12-03 12-27-03

And when connected I even got slippage 😅
Screenshot from 2021-12-03 12-26-17

@W3stside W3stside requested a review from anxolin December 6, 2021 10:59
@W3stside W3stside changed the base branch from develop to release/v1.7.0 December 6, 2021 11:08
@W3stside W3stside added the RELEASE Included in the release that is being closed label Dec 6, 2021
@elena-zh
Copy link

elena-zh commented Dec 6, 2021

Hey @W3stside , I still face issues mentioned above:
image

image

Moreover, lots of strange numbers
image

See the video: https://watch.screencastify.com/v/jxR0fTCN0HxQVxdJHKr2

@elena-zh
Copy link

elena-zh commented Dec 7, 2021

Steps to reproduce the issue with weird amounts:

  1. Do not connect a wallet
  2. leave WETH token in the from field
  3. select any token in the from (not ETH), wait till prices are calculated
  4. then change to ETH token in the To field
  5. The you can swap ETH/WETH, you will always see weird prices and amounts

@elena-zh
Copy link

elena-zh commented Dec 7, 2021

Besides, there is another issue: currently, amount of a wrapped/unwrapped token shows 0 to a not connected user. See comparison with Prod
issue #1
issue #1-2

@W3stside
Copy link
Contributor Author

W3stside commented Dec 7, 2021

right now when disconnected we dont show an output for a wrapping operation. on the flip side it no longer does the strange calcuation of a trade when swithcing to ETH from another token as outlined by Elena.

it also gets rid of the fee / price messages when wrapping

@elena-zh
Copy link

elena-zh commented Dec 7, 2021

Hey @W3stside , now I do not see weird numbers.
I found another edge case for fee request displaying, but I reported it in a separate ticket #1980 .
As for this issue #1920 (comment), I will report a separate low priority ticket for this.

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.

4 participants