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.

Sign up for GitHub

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

Jump to bottom

Fix Pricing Bug when Using Default Exchange #207

Merged
merged 6 commits into from
Nov 4, 2023
Merged

Fix Pricing Bug when Using Default Exchange #207

merged 6 commits into from
Nov 4, 2023

Conversation

macanudo527
Copy link
Collaborator

What?

When the default Kraken exchange was assigned to another exchange the Kraken CSV reader was not being used. This would cause prices with longer timeframes to be used for prices further in the past.

This PR fixes that bug and prevents double instantiation of the CSV reader.

Also included in this PR are some minor formatting fixes to previously submitted code.

@ndopencode
Copy link
Contributor

I did a quick dirty test with these code changes and believe I still see 1 week prices.

@macanudo527
Copy link
Collaborator Author

Can you post the warning messages you are getting? When anything bigger than a 1m candle is used for pricing, warning messages should be sent to stdout. If you could post those, it might help to find the issue.

@ndopencode
Copy link
Contributor

Sure! And thanks for looking into this @macanudo527

I didn't see any WARN or WARNING logs in my log file but I did run it with LOG_LEVEL=DEBUG so I got the following DEBUG statements that maybe relevant. Note, for this test, I only let one transaction from the plugins to be sent to the transaction_resolver so that way I can control the logic easier since the list size is always one and I know what gets sent to the transaction_resolver can only be the single transaction and as such can ascertain what the values should be (i.e., spot_price).

P.S. Curious could you explain what this line in the log means? If I only I have one transaction how come there are numerous logs regarding graph snapshots?

2023-09-25 13:31:23,393/CCXT-converter/high/DEBUG: Added graph snapshot AVLTree for kraken for timestamp: 2023-08-24 00:00:00+00:00

2023-09-25 13:31:23,393/CCXT-converter/high/DEBUG: Added graph snapshot AVLTree for kraken for timestamp: 2023-08-24 00:00:00+00:00
2023-09-25 13:31:23,578/CCXT-converter/high/DEBUG: Added graph snapshot AVLTree for kraken for timestamp: 2023-09-07 00:00:00+00:00
2023-09-25 13:31:23,750/CCXT-converter/high/DEBUG: Added graph snapshot AVLTree for kraken for timestamp: 2023-09-14 00:00:00+00:00
2023-09-25 13:31:23,751/CCXT-converter/high/DEBUG: Found market - BTCUSD on single exchange, skipping routing.
2023-09-25 13:31:23,932/CCXT-converter/high/DEBUG: Got historical_data: [[1695619920000, 26110.8, 26110.8, 26099.4, 26099.4, 5.72510357]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 1m
2023-09-25 13:31:23,932/CCXT-converter/high/DEBUG: For BTC/USD requested candle for 2019-10-20 17:11:44+00:00 (ms 1571591504000), but got 2023-09-25 05:32:00+00:00. Continuing with larger timeframe.
2023-09-25 13:31:24,975/CCXT-converter/high/DEBUG: Got historical_data: [[1695447300000, 26558.2, 26558.2, 26558.1, 26558.2, 0.0896251]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 5m
2023-09-25 13:31:24,976/CCXT-converter/high/DEBUG: For BTC/USD requested candle for 2019-10-20 17:11:44+00:00 (ms 1571591504000), but got 2023-09-23 05:35:00+00:00. Continuing with larger timeframe.
2023-09-25 13:31:25,951/CCXT-converter/high/DEBUG: Got historical_data: [[1695015900000, 26669.9, 26670.0, 26669.9, 26670.0, 4.71505263]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 15m
2023-09-25 13:31:25,951/CCXT-converter/high/DEBUG: For BTC/USD requested candle for 2019-10-20 17:11:44+00:00 (ms 1571591504000), but got 2023-09-18 05:45:00+00:00. Continuing with larger timeframe.
2023-09-25 13:31:26,951/CCXT-converter/high/DEBUG: Got historical_data: [[1693072800000, 26009.0, 26009.0, 26005.4, 26006.3, 6.11007192]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 1h
2023-09-25 13:31:26,951/CCXT-converter/high/DEBUG: For BTC/USD requested candle for 2019-10-20 17:11:44+00:00 (ms 1571591504000), but got 2023-08-26 18:00:00+00:00. Continuing with larger timeframe.
2023-09-25 13:31:27,965/CCXT-converter/high/DEBUG: Got historical_data: [[1685304000000, 27534.9, 28257.0, 27534.9, 28090.0, 1037.94396534]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 4h
2023-09-25 13:31:27,966/CCXT-converter/high/DEBUG: For BTC/USD requested candle for 2019-10-20 17:11:44+00:00 (ms 1571591504000), but got 2023-05-28 20:00:00+00:00. Continuing with larger timeframe.
2023-09-25 13:31:28,987/CCXT-converter/high/DEBUG: Got historical_data: [[1633478400000, 51492.9, 55770.0, 50366.4, 55350.2, 7380.2335466]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 1d
2023-09-25 13:31:28,988/CCXT-converter/high/DEBUG: For BTC/USD requested candle for 2019-10-20 17:11:44+00:00 (ms 1571591504000), but got 2021-10-06 00:00:00+00:00. Continuing with larger timeframe.
2023-09-25 13:31:29,972/CCXT-converter/high/DEBUG: Got historical_data: [[1571875200000, 7473.9, 10580.5, 7360.0, 9166.2, 64211.72154619]] with ms_timestamp - 1571591504000 with exchange <class 'ccxt.kraken.kraken'> with timeframe - 1w
2023-09-25 13:31:29,972/dali/DEBUG: Fetched high conversion rate 10580.5 for 2019-10-20 17:11:44+00:00/BTC->USD from plugin CCXT-converter: HistoricalBar(duration=datetime.timedelta(days=7), timestamp=datetime.datetime(2019, 10, 20, 17, 11, 44, tzinfo=UTC), open=Decimal('7473.9'), high=Decimal('10580.5'), low=Decimal('7360.0'), close=Decimal('9166.2'), volume=Decimal('64211.72154619'))

@macanudo527
Copy link
Collaborator Author

P.S. Curious could you explain what this line in the log means? If I only I have one transaction how come there are numerous logs regarding graph snapshots?

It makes snapshots for the week including your first transaction until the present no matter how many transactions you have.

2023-09-25 13:31:23,751/CCXT-converter/high/DEBUG: Found market - BTCUSD on single exchange, skipping routing.

I think this is the problem. It is attempting to pull the data from Kraken directly and bypassing the CSV reader. It's strange that it is optimizing from 2023-08-24 though. Is this a BTCUSD transaction at 2019-10-20 17:11:44+00:00 (ms 1571591504000) on Kraken? I'll spoof that transaction on my local setup and see what I can see.

@ndopencode
Copy link
Contributor

ndopencode commented Sep 26, 2023 •
edited

P.S. Curious could you explain what this line in the log means? If I only I have one transaction how come there are numerous logs regarding graph snapshots?

It makes snapshots for the week including your first transaction until the present no matter how many transactions you have.

Interesting so that means from 2019-10-20 to today, it will generate weekly snapshots and add it to the AVL Tree?

2023-09-25 13:31:23,751/CCXT-converter/high/DEBUG: Found market - BTCUSD on single exchange, skipping routing.

I think this is the problem. It is attempting to pull the data from Kraken directly and bypassing the CSV reader. It's strange that it is optimizing from 2023-08-24 though. Is this a BTCUSD transaction at 2019-10-20 17:11:44+00:00 (ms 1571591504000) on Kraken? I'll spoof that transaction on my local setup and see what I can see.

Correct. I also think I saw it use Coinbase Pro's fetchOHCLV() function when I was in the debugger FYI. That diagnosis was using the code from main.

@macanudo527 macanudo527 mentioned this pull request Sep 26, 2023
Open
@macanudo527
Copy link
Collaborator Author

macanudo527 commented Sep 27, 2023 •
edited

This is what I get spoofing the transaction:

2023-09-27 09:15:36,500/CCXT-converter/high/DEBUG: Found market - BTCUSD on single exchange, skipping routing.
2023-09-27 09:15:36,501/Kraken.com_CSVOHLCVT/DEBUG: Retrieving bar for XBTUSD at 1571591504
2023-09-27 09:15:36,501/Kraken.com_CSVOHLCVT/DEBUG: Retrieving cached bar for XBT, USD at 1571591504
2023-09-27 09:15:36,501/Kraken.com_CSVOHLCVT/DEBUG: Retrieving XBT -> USD at 1571591460 from 1570752000 stamped file.
2023-09-27 09:15:36,512/CCXT-converter/high/DEBUG: Retrieved bar from cache - [HistoricalBar(duration=datetime.timedelta(seconds=60), timestamp=datetime.datetime(2019, 10, 20, 17, 11, tzinfo=datetime.timezone.utc), open=Decimal('8076.9'), high=Decimal('8077.5'), low=Decimal('8074.3'), close=Decimal('8077.4'), volume=Decimal('1.01775872'))] for 2019-10-20 17:11:44+00:00/BTC->USD for Kraken
2023-09-27 09:15:36,513/dali/DEBUG: Fetched high conversion rate 8077.5 for 2019-10-20 17:11:44+00:00/BTC->USD from plugin CCXT-converter: HistoricalBar(duration=datetime.timedelta(seconds=60), timestamp=datetime.datetime(2019, 10, 20, 17, 11, tzinfo=datetime.timezone.utc), open=Decimal('8076.9'), high=Decimal('8077.5'), low=Decimal('8074.3'), close=Decimal('8077.4'), volume=Decimal('1.01775872'))

You made this transaction on Kraken, correct? Not on another exchange?

It's a little hard to find, but it retrieved the 1m candle as expected.

@ndopencode
Copy link
Contributor

Is it getting confused because its BTCUSD on my end but XBTUSD on your end?

@macanudo527
Copy link
Collaborator Author

Is it getting confused because its BTCUSD on my end but XBTUSD on your end?

It's BTC on my end, too. For Kraken CSV files, BTC is labeled XBT so the Kraken CSV reader has an alias built in for BTC/XBT. All logger messages for BTC from Kraken CSV will use the XBT ticker.

@macanudo527
Copy link
Collaborator Author

@ndopencode are you having further issues? Can we review and merge this?

@eprbell
Copy link
Owner

eprbell commented Oct 3, 2023

I took a quick look and it seems good to me: when the tests pass I'll review it one more time.

@macanudo527
Copy link
Collaborator Author

macanudo527 commented Oct 3, 2023 •
edited

@eprbell Something strange is happening. These tests fail on the main branch, too. Something changed in the last week that broke fiat. I'm guessing the exchangerates.host changed format or something. I'll have to investigate.

@macanudo527 macanudo527 mentioned this pull request Oct 3, 2023
Closed
@ndopencode
Copy link
Contributor

ndopencode commented Oct 9, 2023 •
edited

I have a brief break from my day job, I'll take a look at this this week!

@macanudo527 macanudo527 mentioned this pull request Oct 20, 2023
Merged
@macanudo527
Copy link
Collaborator Author

@eprbell The tests now pass with this one thanks to #212

eprbell
eprbell approved these changes Nov 4, 2023
View reviewed changes
Copy link
Owner

@eprbell eprbell left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry for taking so long... I've been snowed under lately.

@eprbell eprbell merged commit 18d0316 into eprbell:main Nov 4, 2023
16 checks passed
@macanudo527 macanudo527 deleted the fix_pricing_bug branch November 7, 2023 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eprbell eprbell eprbell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@macanudo527 @ndopencode @eprbell