bmoscon / cryptofeed

Cryptocurrency Exchange Websocket Data Feed Handler
Other
2.19k stars 679 forks source link

Updating WHALE_ALERT feed to latest cryptofeed standards #401

Closed yohplala closed 3 years ago

yohplala commented 3 years ago

Hi @bmoscon, In the process of updating WHALE_ALERT feed, I have a trouble in fitting Whale Alert HTTP request into (what I understand is) new Cryptofeed logic. For this I am reviewing what you have done in COINGECKO feed (hence another question in Slack that I have also raised regarding Coingecko).

Ok, this ticket is about WHALE_ALERT HTTP request to be issued to retrieve data. This HTTP request has 'dynamic' parameters: start_time & cursor. They change for each new request.

Coingeck does not show me how to handle properly this. Its own HTTP request, once initialized, does not change any longer. In the code, I can see it is initialized in connect method (line 70 of coingecko.py). Once initialized, it is used to create a task in Feedhandler class (line 226 of feedhandler.py). Only then is started the loop (line 232 of feedhandler.py).

But for WHALE_ALERT, this HTTP request needs to be changed inside the Feedhanlder loop. Please, can you advise how managing that or maybe if an example already exists?

Thanks for your help, bests!

olibre commented 3 years ago

Hi @yohplala

Bryant re-factorized/unified the HTTP/websocket stuff around the Christmas holidays. Shortly after that I started a deeper connection refactoring, affecting Feed and FeedHandler (new file Runner.py). I am working on this PR to change (improve?) the way the websocket connection and HTTP polling are managed. Please see https://github.com/bmoscon/cryptofeed/pull/381

To ease the integration of this big PR, I moved some changes to smaller PRs. Some of these PRs have already been merged: https://github.com/bmoscon/cryptofeed/pull/400 https://github.com/bmoscon/cryptofeed/pull/399 https://github.com/bmoscon/cryptofeed/pull/393 And some others are still open: https://github.com/bmoscon/cryptofeed/pull/397 https://github.com/bmoscon/cryptofeed/pull/398 https://github.com/bmoscon/cryptofeed/pull/402

As for Whale Alert and CoinGecko, my work is based on your source code and Bryant's changes. But I rewrote a lot of stuff. The current rewritten versions of Whale Alert and CoinGecko are available on my branch refactor-http-ws-connections: https://github.com/olibre/cryptofeed/tree/refactor-http-ws-connections/cryptofeed/provider

However, I will revert most of my changes to provide the same behavior as the upstream master branch. This is the last big thing I need to clean up to finish this PR. Attention: I git push --force to my working branch, so you may not find my rewritten versions of Whale Alert and CoinGecko in the comming weeks.

On my computer, I am running my own versions of Whale Alert and CoinGecko. If you think you (or others) might be interested by my rewritten versions, we should think how to propose them. What are your thoughts? What about you Bryant?

I have added plenty of TODOs to the Cryptofeed source code to imrpove other things later (other PRs) depending on the Bryant feedback (and yours too). Moreover of these TODOs, I am not happy with my current HTTP polling implementation because the mechanism is rather opaque when we need to dynamically change the HTTP requests. I also plan to rework that part to provide more clarity and flexibility in a future PR.

Please give me your feedback on these two points:

  1. Are you interested in my rewritten versions of Whale Alert and CoinGecko? (tell me before I revert them, I have already started reverting the channel=SCORES)
  2. Do you want to join/participate/contribute in the design/refactoring of Cryptofeed?
yohplala commented 3 years ago

Hi @olibre, Thanks for your detailed feedbacks.

1. Are you interested in my rewritten versions of Whale Alert and CoinGecko? (tell me before I revert them, I have already started reverting the channel=SCORES) I am a strong believer in open source, and this is why I PR'ed in the 1st place COINGECKO and WHALE_ALERT feeds. The initial implementation fits my need, which does not mean it has to stay as it is. Already, I can only thank @bmoscon for his later changes. I have had no look at the ones you propose yet, but I believe that if you have made them, it is for a reason. It means something has to be improved. So yes, I am in favor about seeing how integrating your changes. I have to have a look to make sure this new version is still compatible with my own needs.

2. Do you want to join/participate/contribute in the design/refactoring of Cryptofeed? Yes but... As I could say it here and there, I am no software engineer, meaning I have no education, nor real background in coding. Interested in algo trading, I got my hands in that, but I am 100% use case driven. The only things I care in a code are:

In the current refactoring being processed, I may be wrong but it seems to me very 'code restructuration', and as I see it, I have no background to bring added-value. In truth, what is done is flying way above my head. I have 0 knowledge/background about asyncio/aiohttp/json which seems to me what this refactoring is about.

So, yes, if I can help, of course. At the moment, I will spend some time reviewing your changes in COINGECKO and WHALE_ALERT if it is ok for you.

A short question regarding COINGECKO. Why reverting the SCORE part? Is it because you had no feedbacks yet? I am sorry not to be more responsive, I will try to improve that. As said, yes, I don't think I can be very relevant on the refactoring so I stayed 'in the shadow' while there have been discussions about it. Yet, yes, I do should give feedback on COINGECKO and WHALE_ALERT.

Have a good day, bests

olibre commented 3 years ago

Perfect if you are an end user/trader of Cryptofeed. :+1: I want to deliver a clear source code that traders can easily understand, and you can help me with the design. The idea is not to bother end users too much with async/await semantic.

My personal version in CoinGecko writes one MARKET_INFO per symbol, in lieu of writing one MARKET_INFO for all symbols linked to one asset (e.g. BTC). Therefore, the score/rank metrics did not fit well with MARKET_INFO, and I created SCORE for that purpose.

Why reverting the SCORE part? Is it because you had no feedbacks yet?

I have made many many changes in Cryptofeed source code. But I cannot push all my changes at once: almost all files have been changed! I try to provide Bryant with small PRs so he can carefully review them in a few minutes/hours, no more. In my PR concerning the connection refactoring, I am reverting my CoinGecko/WhaleAlert changes to reduce the PR size. But in my personal modified Cryptofeed, I still keep my own CoinGecko/WhaleAlert versions.

If you want to use my CoinGecko and/or WhaleAlert versions, I can push another PR, to replace the current one(s) with my version(s).

yohplala commented 3 years ago

If you want to use my CoinGecko and/or WhaleAlert versions, I can push another PR, to replace the current one(s) with my version(s). Yes, this would be great. I can then review also the COINGECKO feed with your change regarding SCORE.

Regarding COINGECKO:

Besides, not for short term, do you know about COINPAPRIKA? Just recently, I have realized they aggregate liquidity information. I find this terrific. image

I don't know if they have it in their API, but if yes, I think in a longer term, this is a feed I would want to integrate as well. Just saying this to have your opinion in which channel this liquidity information would fit then? MARKET_INFO?

yohplala commented 3 years ago

hmm, this discussion is kind of drifting away, from WHALE_ALERT, to COINGECKO, to COINPAPRIKA... :)

olibre commented 3 years ago

Yep, we can add an Config option to let the user decide to produce MARKET_INFO keeping together all coin related symbols, or to produce MARKET_INFO for each individual symbol. :+1:

Concerning COINPAPRIKA, Yes, I am also interested by collecting their liquidity-aggregated information. However, I do not have time to investigate more about this data retrieval.

yohplala commented 3 years ago

If you want to use my CoinGecko and/or WhaleAlert versions, I can push another PR, to replace the current one(s) with my version(s).

Ok @olibre , so I am in stand-by mode to review this PR when you feel it is ok to be reviewed. Just let me know. Have a good evening, Bests,

olibre commented 3 years ago

OK, I will try to find some time during the week-end for another pull request...