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.

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

Add Uniswap price integration #6231

Closed
wants to merge 1 commit into from

Conversation

santteegt
Copy link
Contributor

@santteegt santteegt commented Mar 15, 2020

Description
  • The management command get_prices now fetch all ETH=>token and token=>USD prices from the Uniswap exchange by querying the Uniswap subgraph on mainnet
Refers/Fixes

Refs: #6192

@codecov
Copy link

codecov bot commented Mar 15, 2020

Codecov Report

Merging #6231 into master will increase coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6231      +/-   ##
==========================================
+ Coverage   27.08%   27.10%   +0.02%     
==========================================
  Files         290      290              
  Lines       26628    26626       -2     
  Branches     3942     3941       -1     
==========================================
+ Hits         7212     7218       +6     
+ Misses      19145    19141       -4     
+ Partials      271      267       -4     
Impacted Files Coverage Δ
app/economy/management/commands/get_prices.py 0.00% <0.00%> (ø)
app/dashboard/views.py 11.24% <0.00%> (ø)
...rketing/management/commands/no_applicants_email.py 0.00% <0.00%> (ø)
...eting/management/commands/assemble_leaderboards.py 40.48% <0.00%> (ø)
app/dashboard/embed.py 31.60% <0.00%> (+3.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6dc338c...73f7d02. Read the comment docs.

@owocki
Copy link
Contributor

owocki commented Mar 20, 2020

@santteegt just tried this and it works (mostly) like a charm!

a few testing notes:

  1. can we get all trading pairs, not just top 50?
  2. i get a value too long for type character varying(30) when i change the query_limit to 200. looks like something called REALTOKEN-16200-FULLERTON-AVE-DETROIT-MI breaks it. perhaps worth wrappin geach iteration of the for loop in a try catch?

@santteegt
Copy link
Contributor Author

Hi @owocki,

Just updated my PR so it now retrieves all exchange pairs from Uniswap. Exceptions due to long token names and zero value exchanges are also handled properly. Thanks @niran for pointing that the issue has already been solved

@danlipert
Copy link
Contributor

@santteegt I see theres a conflict here - is this PR now out of date given the recent changes?

@santteegt santteegt force-pushed the uniswap-getprices branch from de1865c to abc6d83 Compare April 1, 2020 14:39
@santteegt
Copy link
Contributor Author

Hi @danlipert,

Looks like @owocki has cherrypicked my commit, made a small change to fetch prices only for the Panvala token, and pushed this (commit) to master

I've rebased my PR excluding this change. In case it is required let me know to put it back again

@santteegt santteegt force-pushed the uniswap-getprices branch 3 times, most recently from 4268378 to ad18d47 Compare April 7, 2020 14:57
@danlipert
Copy link
Contributor

@owocki thoughts on this? I dont know if we want to limit the scope to Panvala or if we are good to source all the coins since this PR undoes the changes you did

@owocki
Copy link
Contributor

owocki commented Apr 22, 2020

last time we did all coins from uniswap it messed up price feeds across the site (some stable coins have low liquidity and were valued at 10x what they should have been). would prefer to keep the scope limited to just PAN for now. but maybe make it easy to add new uniswap tokens at a future date if people keep adding those coins.

@santteegt
Copy link
Contributor Author

Changes to my PR has now been restored. Now, my PR is basically doing nothing but worth mentioning that the initial implementation was already merged into master before approving this PR 😄

@thelostone-mc
Copy link
Member

@santteegt lol I'll be closing this one out cause like you said , your commit is already in :)
Thanks :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants