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

[USD Price] Add coingecko api and hooks #1356

Merged
merged 16 commits into from
Sep 7, 2021
Merged

Conversation

W3stside
Copy link
Contributor

Summary

Adds Coingecko as a source for USD estimation on tokens to MAINNET and XDAI (no rinkeby or other test nets)

Currently it is part of a hook that get the GREATER of the 2 USD prices from USDC estimation vs Coingecko api resp. This can be discussed.

You can see in the logs the price found by coingecko:
image

To Test

  1. On either Mainnet or Xdai try picking tokens and adding amounts
  2. Check the networks tabs/console tabs for logs and info
  3. check the FE for the amounts being displayed correctly
  4. check xdai that more tokens have usd estimations (not that many unfort)

Next potentially

  1. Add the A -> B -> A check mentioned by @anxolin as a fallback
  2. Block swap button in some cases? maybe not part of this PR
    i. paraswap blocks swap button if price impact is -15%
    ii. if we get a returned USD price of -15% OR if we get no estimation and a bad A -> B -> A slippage then also block

@W3stside W3stside requested review from a team August 27, 2021 17:22
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

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.

Code is good, the prices look ok, but I don't see the price impact.

Left this PR, right BARN
screenshot_2021-08-27_14-20-53

@W3stside
Copy link
Contributor Author

hmm i can see it 🤔 not able to get the non-price impact. let me keep trying

@elena-zh
Copy link

LGTM besides price impact issue.
Btw, I picked at least half of the xDAI token list and did not noticed significant changes in USD estimations in xDAI =(
Also, Coingecko source is added to the console log when connect to an unsupported network/no wallet is connected. However, as I assume, it is not an issue.
wrong netw

@alfetopito
Copy link
Contributor

hmm i can see it thinking not able to get the non-price impact. let me keep trying

I see it now 🤷

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.

Awesome!!

I can tell at least, it cought the bad trade of last week where one user got REKTED!

image

image

const fiatValueOutput = useUSDCValue(trade.outputAmountWithoutFee)
// const fiatValueInput = useUSDCValue(trade.inputAmountWithoutFee)
// const fiatValueOutput = useUSDCValue(trade.outputAmountWithoutFee)
const fiatValueInput = useHigherUSDValue(trade.inputAmountWithoutFee)
Copy link
Contributor

Choose a reason for hiding this comment

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

higher or highest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well technically "highest" would mean just that, the "highest" whereas "higher" implies of the choices given, which is "highest"

but yes it's very nitpicky here. happy to change it to whatever we like best @alfetopito @Anxo @nenadV91

Copy link
Contributor

Choose a reason for hiding this comment

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

whatever

tokenAddress,
})
.then(toPriceInformation)
.then((response) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious, why do you expose the original data instead of just the, priceInformation directly?

Copy link
Contributor

Choose a reason for hiding this comment

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

This response i think is to generic name. maybe priceInfo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think it was because in the other APIs it's done similarly ( i think ) - and for paraswap there are other useful props that could posisbly want to be used elsewhere before the priceInformation schema mapping. In the case of coingecko there isn't any useful info so i could change the fn directly 👍

const coingeckoUsdPrice = useCoingeckoUSDValue({
tokenAddress: currencyAmount ? currencyId(currencyAmount.currency) : '',
currencyAmount,
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any kind of timeout logic? probably not for this PR, also less important than other things, but maybe good to track it.

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.

I'm able to see price impact now.
BTW, there is an issue related to DAI token estimation #1363

@MareenG
Copy link

MareenG commented Aug 31, 2021

I could the the price impact once I did a hard refresh.

I came across this issue additionally to #1363
Bildschirmfoto 2021-08-31 um 12 08 07

@W3stside
Copy link
Contributor Author

I could the the price impact once I did a hard refresh.

I came across this issue additionally to #1363
Bildschirmfoto 2021-08-31 um 12 08 07

@MareenG this is fixed now please check again once the statuses are green :)

@W3stside W3stside merged commit 66b89ff into develop Sep 7, 2021
@W3stside W3stside deleted the add-coingecko-usd-query branch September 7, 2021 14:28
W3stside added a commit that referenced this pull request Sep 22, 2021
* coingecko api

* coingecko usd hooks

* use in swapmod

* replace useUSDCValue with useHigherUSDValue

* comment

* mod computeFiatValue fn

* made dumb comp

* smarter container

* naming

* remove unnecessary mod

* parse usd price from API as USDC

* fix infiniLoop (TM)

* remove trans from priceImpact

* fix baseAmount
This was referenced Sep 22, 2021
anxolin pushed a commit to anxolin/cowswap that referenced this pull request Apr 1, 2022
* coingecko api

* coingecko usd hooks

* use in swapmod

* replace useUSDCValue with useHigherUSDValue

* comment

* mod computeFiatValue fn

* made dumb comp

* smarter container

* naming

* remove unnecessary mod

* parse usd price from API as USDC

* fix infiniLoop (TM)

* remove trans from priceImpact

* fix baseAmount
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.

5 participants