Overall review: bug fixes, unit tests, new sources, stock publication. #56

Zapata · 2018-06-07T17:16:18Z

sorry for this massive pull request, it's not in my habits, but I feel like it will be more efficient like this.
However, I have reported most of the issues seperately.

My initial goal was to implement stock price publication (#54), but when I saw #35 and @clockworkgr 's comment, I thought it needs a global review.

This pull request solve most of the currently open issues.
Here is the detailed content:

New feature:

Add unit tests (fix Add unit tests #39)
Add ability to publish Stock prices. (fix Add ability to publish Stock prices. #54)
Add Binance source
Add WorldCoinIndex source
Add BitsharesFeed source (to retrieve the current MPA feed)

Bug fix:

Fix error when asset has no specific cer defined (fix CER for external assets are not calculating properly #27).
Update default configuration for CNY CER value (fix Change CER for CNY to 1.2 #28)
Clean config (fix Fixup: settings for default.yaml #31)
Fix volume bug (fix Volume bug #34).
Fix Asset with type formula (fix Hertz.yaml - NameError: name 'sin' is not defined/ NameError: name 'pi' is not defined #37, Hertz.yaml & Hero.yaml - assert "price" not in args[0], "You cannot provide a 'price' this way" #38)
Add unit tests Add unit tests #39 (fix Add unit tests #39)
Remove closed exchanges (fix Remove closed exchanges. #40)
Remove Yahoo finance (fix Remove Yahoo Finance API. #41)
Fix source Bitcoin Venezuela. (fix Fix source Bitcoin Venezuela. #42)
Fix ALTCAP.X computation in Coincap source. (fix Fix ALTCAP.X computation in Coincap source. #45)
Coinmarketcap: If multiple quotes are provided only the last one is returned. (fix Coinmarketcap: If multiple quotes are provided only the last one is returned. #46)
Coinmarketcap: ALTCAP and ALTCAP.X are computed but ignored. (fix Coinmarketcap: ALTCAP and ALTCAP.X are computed but ignored. #47)
Fix Fixer source: upgrade to new API. (fix Fix Fixer source: upgrade to new API. #49)
Fix Graphene source: If multiple quotes are provided only the last one is returned. (fix Fix Graphene source: If multiple quotes are provided only the last one is returned. #50)
Quandl: Quote inconsistency between Quandl and QuandlPlain. (fix Quandl: Quote inconsistency between Quandl and QuandlPlain. #52)
Wrong prices displayed on summary for formula assets (fix Wrong prices displayed on summary for formula assets #55)
Fix TypeError: can't subtract offset-naive and offset-aware datetimes (fix TypeError: can't subtract offset-naive and offset-aware datetimes #57)

Se the status of all the sources: https://github.com/Zapata/bitshares-pricefeed#sources

Any feedback/review welcome!

Updated CER to 20% in line with wackou's price feed script. Updated the comments to reflect recent changes.

- Handle configuration without equities; - Handle configuration with only equities. - Improve testing.

clockworkgr · 2018-06-07T19:02:34Z

Is "BitsharesFeed source" similar to what I've done with the SettleGraphene source?

Overall, I think that the extern/intern/formula stuff is added complexity for no reason.

E.g. My way of calculating Hero is a Hero source that provides Hero/USD through a formula.

And when I request a feed for Hero, I add that source and the USD/BTS from SettleGraphene (which returns current settlement price for USD) only. Thus I only get 1 price which is the correct one.

This allows a simpler way of adding algorithmic feeds (by copying a source template and replacing a calculation) and keeps the pricefeed code cleaner and simpler. Part of the cleanup I want to do is keep only extern from now on.

Zapata · 2018-06-08T11:25:06Z

Yes BitsharesFeed source is the same idea as your SettleGraphene: it returns asset['bitasset_data']['current_feed']['settlement_price'].

I agree formula computation is too complex, not sure we need the ref_asset, I agree with your approach (just compute HERO/USD then let the usual algorithm derive the HERO/BTS).

BitsharesFeed was introduced to rely on other to compute intermediary prices, but if as a publisher you already compute a USD/BTS, it's better to use it instead of the one from the DEX, this will be more consistent. (same discussion here).

clockworkgr · 2018-06-08T11:59:59Z

Hi I agree with using the same login for price computation. But I believe HERO price was designed to specifically use the bitUSD settlement price now? Hence why I'm only using that SettleGraphene source for hero rather than the "generic" USD/BTS price reached.

Same goes for Hertz I think

…roc#57)

xeroc · 2018-06-25T12:54:57Z

bitshares_pricefeed/cli.py

@@ -42,6 +42,11 @@
    help="Skip critical feeds",
    default=False,
 )
+@click.option(
+    "--node",
+    metavar='<wss://host:port>',


Coool. Didn't know metavar.

xeroc · 2018-06-25T13:09:44Z

bitshares_pricefeed/pricefeed.py

@@ -2,14 +2,14 @@
 import numpy as num
 import time
 from pprint import pprint
-from math import fabs, sqrt
+from math import *


That's not an ideal solution. Please define which modules you need when you import them directly.

This is related to issue #37.

In this case only sin should be enough.
Unfortunately we don't know what user may need in their formula, that why I used *.
That's why I agree with @clockworkgr approach (and other pricefeed implementations), and use python modules for algorithmic assets instead of the configuration based approach. But I didn't want to change the approach in this pull request.

xeroc · 2018-06-25T13:10:05Z

bitshares_pricefeed/pricefeed.py

@@ -98,31 +98,35 @@ def obtain_flags(self, symbol):

        # Feed too old
        feed_age = self.price_result[symbol]["current_feed"]["date"] if self.price_result[symbol]["current_feed"] else datetime.min
-        if (datetime.utcnow() - feed_age).total_seconds() > self.assetconf(symbol, "maxage"):
+        if (datetime.now(timezone.utc) - feed_age).total_seconds() > self.assetconf(symbol, "maxage"):


why that change?

See issue #57 and this commit

More specifically utcnow() returns a date with timezone=None, and current_feed.date is, since the last python-bitshares upgrade, with a timezone.

xeroc · 2018-06-25T13:10:50Z

bitshares_pricefeed/pricefeed.py

@@ -276,7 +282,7 @@ def derive2Markets(self, asset, target_symbol):
                            symbol,
                            target_symbol,
                            float(self.data[interasset][target_symbol][idx]["price"] * ratio["price"]),
-                            float(self.data[interasset][target_symbol][idx]["price"] * ratio["price"]),
+                            float(self.data[interasset][target_symbol][idx]["volume"] * ratio["price"]),


nice catch!

To be fair it's not mine, see #36 . I don't know why @notchxor abandonned his changes.

Original change cherrypicked.

xeroc · 2018-06-25T13:11:48Z

bitshares_pricefeed/sources/coincap.py

-                              if 'position24' in coin and
-                              int(coin['position24']) <= 11 and
-                              coin['short'] != "BTC"]
+                              for coin in coincap_front[0:11]


Can we make that 11 and the 10 a parameter maybe?

Yep, it should be a named constant. I'll change it.

see
e5f2551

xeroc · 2018-06-25T13:14:36Z

tests/formula/conftest.py

+import yaml
+import math
+
+@pytest.fixture


sweet. wanted to learn about pytest.fixtures anyways :D

xeroc · 2018-06-25T13:15:45Z

Excellent work. If you may improve on the comments I made, I am fine merging this in.

Zapata · 2018-06-25T21:24:47Z

Code updated according to comments + spotted an unitialized parameter in Alphavantage source.
If it's fine for you I think it can be merged and we can close the related issues.

xeroc · 2018-06-26T14:37:59Z

Excellent piece of work. Thank you for your contribution!

Zapata added 30 commits June 7, 2018 15:36

Updated Hertz CER & Comments

f537a19

Updated CER to 20% in line with wackou's price feed script. Updated the comments to reflect recent changes.

Ignore vscode files and pytest cache.

2a90964

Fix error when asset has no specific cer defined (fix xeroc#27).

de51e98

Handle "*" as source.

0c7ef07

When derive quotes, ignore intermediate_assets without data.

0663124

Fix volume bug (resolves xeroc#34).

7a8041b

Add equities price retrieval to Google source.

805c761

Add test for AEX source.

9f0985f

Test and Fix Bitcoin Venezuela source.

7fd0efb

Add test for Bitstamp.

8a4c611

Add test for Bittrex.

056ccf7

Remove BTC38 as it has been shutdown.

31466e7

Remove BtcChina as it has been shutdown.

78cb8c5

Remove Bter as it has shutdown.

3323207

Add test for ChBTC.

ef8db03

Add test for CoinCap and fix price compute of ALTCAP.X .

4167d0b

Test Coinmarketcap and fix computation of all pairs and ALTCAP price.

1e9bb50

Add test for Currencylayer source.

6d1cfff

Add test for Fixer.io source.

75fa388

Test Graphene source and fix issue when multiple quotes are provided.

33027ca

Test LBank source and improve error handling.

91c1baf

Test OkCoin source.

590d6df

Test OpenExchangeRates source and clean code.

474701a

Add test for Poloniex source.

b271eda

Test Quantl source, fix price inconsistency, and add api_key for plain.

b251a5a

Remove Yahoo Finance source as it has been shutdown.

d225968

Remove Yunbi as it has shutdown.

1f27c8b

Test ZB source.

8e1bbaf

Fix issues in Google source:

db7ab0a

- Handle configuration without equities; - Handle configuration with only equities. - Improve testing.

Add AlphaVantage source and handle equities.

619dd2b

Zapata added 10 commits June 7, 2018 15:39

Add WorldCoinIndex source.

e14b872

Add Coindesk source.

1b7075e

Fix default CNY CER

94f87a3

Document how to run tests.

5da0e45

Document available sources and their status.

7759b0d

Document API Keys in default config.

2884618

Add BitsharesFeed source to retrieved feed prices from DEX.

19a0185

Add sample config to publish stock price feeds.

2f74004

Test assets with formula, fix import issues and summary display.

2e138fc

Fixup: settings for default.yaml (fix xeroc#31)

fe5685d

Zapata added 5 commits June 8, 2018 21:23

Add ability to configure Bitshares node.

d7761e8

Fix default maximum short squeeze ration for 'derivatives-dex' assets.

2d65275

Add source 'manual' for test purpose.

066b4c6

Replace all datetime.utcnow() with datetime.now(timezone.utc) (fix xe…

b549016

…roc#57)

Add Docker.

5de8627

xeroc reviewed Jun 25, 2018

View reviewed changes

tests/formula/conftest.py

import yaml

import math

@pytest.fixture

Copy link

Owner

xeroc Jun 25, 2018

Choose a reason for hiding this comment

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

sweet. wanted to learn about pytest.fixtures anyways :D

Zapata added 2 commits June 26, 2018 00:13

Avoid magic numbers.

e5f2551

Source Alphavantage: Initialize timeout variable.

7dcac01

xeroc merged commit 095ffa4 into xeroc:develop Jun 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overall review: bug fixes, unit tests, new sources, stock publication. #56

Overall review: bug fixes, unit tests, new sources, stock publication. #56

Zapata commented Jun 7, 2018 •

edited

Loading

clockworkgr commented Jun 7, 2018

Zapata commented Jun 8, 2018

clockworkgr commented Jun 8, 2018

xeroc Jun 25, 2018

xeroc Jun 25, 2018

Zapata Jun 25, 2018

xeroc Jun 25, 2018

Zapata Jun 25, 2018

Zapata Jun 25, 2018

xeroc Jun 25, 2018

Zapata Jun 25, 2018

xeroc Jun 25, 2018

Zapata Jun 25, 2018

Zapata Jun 25, 2018

xeroc Jun 25, 2018

xeroc commented Jun 25, 2018

Zapata commented Jun 25, 2018

xeroc commented Jun 26, 2018

Overall review: bug fixes, unit tests, new sources, stock publication. #56

Overall review: bug fixes, unit tests, new sources, stock publication. #56

Conversation

Zapata commented Jun 7, 2018 • edited Loading

clockworkgr commented Jun 7, 2018

Zapata commented Jun 8, 2018

clockworkgr commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xeroc commented Jun 25, 2018

Zapata commented Jun 25, 2018

xeroc commented Jun 26, 2018

Zapata commented Jun 7, 2018 •

edited

Loading