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

Price Feeds + Restructuring #28

Closed
wants to merge 10 commits into from
Closed

Price Feeds + Restructuring #28

wants to merge 10 commits into from

Conversation

shubhagarwal03
Copy link
Contributor

@shubhagarwal03 shubhagarwal03 commented Aug 8, 2024

This PR includes the following changes:

  1. There is now functionality to fetch token prices from other providers (i.e. Dune and Moralis) in case it is unable to
    fetch from coingecko.
  2. The function write_coingecko_prices() has been changed to write_prices(). There is a new table called "slippage_prices"
    in the solver_slippage DB with an additional field for the price source.

Restructures:

  1. The coingecko_pricing file does not use a hardcoded JSON and instead makes an API call to fetch the coingecko token-
    ids list in a fixed interval (currently, every 24 hours) to account for any updated lists.

src/dune_pricing.py Outdated Show resolved Hide resolved
src/dune_pricing.py Outdated Show resolved Hide resolved
Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

Have not checked the code in detail yet, but the overall code structure looks good to me.

I would probably move all price providers into a prices folde for not clutter the main folder too much.

One additional price provider might be native prices from the auction. That would require the backend database for now.

Copy link
Contributor

@bram-vdberg bram-vdberg left a comment

Choose a reason for hiding this comment

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

Looks great to me. I found some small formatting things but overall looks good.

src/coingecko_pricing.py Outdated Show resolved Hide resolved
src/coingecko_pricing.py Outdated Show resolved Hide resolved
src/coingecko_pricing.py Outdated Show resolved Hide resolved
src/moralis_pricing.py Outdated Show resolved Hide resolved
src/moralis_pricing.py Outdated Show resolved Hide resolved
src/moralis_pricing.py Outdated Show resolved Hide resolved
src/coingecko_pricing.py Outdated Show resolved Hide resolved
src/coingecko_pricing.py Outdated Show resolved Hide resolved
@shubhagarwal03 shubhagarwal03 marked this pull request as ready for review August 9, 2024 04:58
Copy link

socket-security bot commented Aug 9, 2024

New dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
pypi/[email protected] environment, filesystem, network 0 183 kB bh2smith, dune.com
pypi/[email protected] environment, eval, filesystem, network 0 10.2 MB ErnoW

View full report↗︎

Copy link
Contributor

@fhenneke fhenneke left a comment

Choose a reason for hiding this comment

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

Edit: I moved all my comments to the other PR
I added a few comment. Some of the things are easy to address, some can just be turned into issues.

Did you plan on restructuring more, e.g. splitting the daemon file? That could be done in some other PR as well.

I do not see any tests for the added code. I assume some parts of the code did change behavior. How did you check correctness of the code locally? Maybe that could be turned into a test.

src/constants.py Show resolved Hide resolved
src/constants.py Show resolved Hide resolved
src/constants.py Show resolved Hide resolved
src/daemon.py Show resolved Hide resolved
src/daemon.py Show resolved Hide resolved
src/sql/insert_price.sql Show resolved Hide resolved
src/daemon.py Show resolved Hide resolved
src/daemon.py Show resolved Hide resolved
src/daemon.py Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
@fhenneke
Copy link
Contributor

Actually, ignore the comments for now. Will look at the actual restructuring PR first.

@fhenneke
Copy link
Contributor

Is this PR still relevant? It seems all the changes are contained in #29 as well.

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.

3 participants