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

openbb-defillama extension to add DeFillama provider into the OpenBB Platform #6731

Open
wants to merge 87 commits into
base: develop
Choose a base branch
from

Conversation

the-praxs
Copy link
Contributor

@the-praxs the-praxs commented Oct 3, 2024

  1. Why?

    • Extends data providers to retrieve DeFi data
  2. What?

    • Adds a new DeFi provider DeFiLlama via the openbb-defillama extension using the defillama-curl module
    • Adds new routers - tvl, 'yields, fees, revenue, coins, stablecoins and volumes within the crypto router
  3. Impact
    High impact due to the addition of a new data provider and new endpoints

  4. Testing Done:
    Test files created for the fetcher and crypto API and Python tests

  5. Reviewer Notes:
    The following is hampering the PR test actions -

    • Cassette files are not being generated for the fetcher test functions
    • test_missing_python_integration_tests is failing for the newly added crypto endpoints
  6. Any other information:
    These are the additional tasks for extra bounty points -

    • Publish the package on PyPi
    • Write a blog post for the PR
    • Submit the blog post on LinkedIn, Twitter and Medium

@the-praxs the-praxs changed the title DeFillama provider for OpenBB [🕹️] openbb-defillama extension to add DeFillama provider into the OpenBB Platform Oct 6, 2024
Copy link

oss-gg bot commented Oct 11, 2024

The /assign command can only be used on issues, not on pull requests.

@DipeshK47
Copy link

/assign

Copy link

oss-gg bot commented Oct 11, 2024

The /assign command can only be used on issues, not on pull requests.

@the-praxs
Copy link
Contributor Author

the-praxs commented Oct 27, 2024

@IgorWounds @piiq I am having the following issues with tests -

  • No cassette files are generated while recording tests for the openbb_defillama provider.
  • test_missing_python_integration_tests fails for the added crypto endpoints.

What could be the possible reasons?

@the-praxs the-praxs changed the title [🕹️] [WIP] openbb-defillama extension to add DeFillama provider into the OpenBB Platform [🕹️] openbb-defillama extension to add DeFillama provider into the OpenBB Platform Oct 27, 2024
@the-praxs
Copy link
Contributor Author

@IgorWounds I am having the following issues with tests -

  • No cassette files are generated while recording tests for the openbb_defillama provider.
  • test_missing_python_integration_tests fails for the added crypto endpoints.

What could be the possible reasons?

Except this - everything is reading and working.

@deeleeramone can you test this and revert?

@piiq piiq added enhancement Enhancement and removed 🕹️ oss.gg labels Nov 2, 2024
@piiq piiq changed the title [🕹️] openbb-defillama extension to add DeFillama provider into the OpenBB Platform openbb-defillama extension to add DeFillama provider into the OpenBB Platform Nov 2, 2024
@piiq
Copy link
Contributor

piiq commented Nov 2, 2024

@the-praxs hacktoberfest is over. We're not reviewing or accepting any more oss.gg submissions.
But since this PR is particularly valuable you can work on it outside of hacktoberfest. If you don't have time - let me know if you would like to hand it off

@the-praxs
Copy link
Contributor Author

@the-praxs hacktoberfest is over. We're not reviewing or accepting any more oss.gg submissions.

But since this PR is particularly valuable you can work on it outside of hacktoberfest. If you don't have time - let me know if you would like to hand it off

No problems. I would like to continue with the work. I had tagged relevant people but no one reviewed it, the comments are already there. I will then make changes accordingly and then we can merge this.

P.S. Before merging, I will need to publish the PyPi package.

@piiq
Copy link
Contributor

piiq commented Nov 2, 2024

Do you mind if Darren publishes it into the OpenBB account? This way there will be significantly less maintenance burden on you moving forward

@the-praxs
Copy link
Contributor Author

I'd like to maintain it.

@piiq
Copy link
Contributor

piiq commented Nov 2, 2024

As a rule we have - if it ends up in the openbb repo we are the ones maintaining it. It would not be a problem to add you to the list of maintainers for the package, but every release would require coordination with us.
If you would like to be the sole maintainer of the package then it would be better if the code lives in a separate repository.
Let me know what you think

@the-praxs
Copy link
Contributor Author

As a rule we have - if it ends up in the openbb repo we are the ones maintaining it. It would not be a problem to add you to the list of maintainers for the package, but every release would require coordination with us. If you would like to be the sole maintainer of the package then it would be better if the code lives in a separate repository. Let me know what you think

Thanks for the insight. I feel that separating the package into a different repo for pip install is a good solution. That way I can maintain the package easily on my end, without having to interfere in the release process.

I will start out the process shortly and then close this PR.

@deeleeramone
Copy link
Contributor

deeleeramone commented Nov 12, 2024

  • No cassette files are generated while recording tests for the openbb_defillama provider.
  • test_missing_python_integration_tests fails for the added crypto endpoints.

What could be the possible reasons?

Except this - everything is reading and working.

@deeleeramone can you test this and revert?

@the-praxs, sorry for the delay in responding, this PR got buried by all the Hacktoberfest noise. Regarding the tests not capturing, I attempted locally and it captured only one test, and it appears as a valid response.

This is the URL from the captured test:

Screenshot 2024-11-12 at 8 51 17 AM

Screenshot 2024-11-12 at 8 56 27 AM

Digging a bit, @the-praxs, I think the reason for the tests not capturing is because of the defillama package. It is using pycurl instead of the requests or aiohttp library and pytest is not able to capture this.

Screenshot 2024-11-12 at 8 57 52 AM

Confirming this to be the case by bypassing their get(url) with requests.get(url). The cassette now captures.

Screenshot 2024-11-12 at 9 04 20 AM

@deeleeramone
Copy link
Contributor

deeleeramone commented Nov 12, 2024

A couple of general operational questions for you:

  • Are the historical endpoints supposed to be a time series, or do you have to request every data point individually? How do I get a table of the historical prices for a token, say ETH or BTC?
    • stablecoins.historical appears to have some historical data, but the response is a nested dictionary that needs to be flattened.
      Screenshot 2024-11-12 at 9 16 28 AM
    • coins.chart has a start_date and end_date parameter, but apparently only one is accepted.
      • Does this have a "chart", or is this a duplicate of "historical"?
      • "prices" is a nested array of dictionaries that should be flattened.
[Error] -> 1 validations error(s)
[Arg]  -> input: {'token': 'coingecko:ethereum', 'start_date': '2020-01-01', 'end_date': '2021-01-01'} -> Value error, Only one of start_date or end_date should be provided
  • What is the convention for entering a symbol? I can't seem to use "ETH" or the typical format of "TOKENCURRENCY" pairs. Is there a lookup/search function that lists the correct mappings and what you need to enter?

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

Successfully merging this pull request may close these issues.

6 participants