CCXT Historical Data Retrieval Methods + Tests #60
Conversation
* Added two methods for fetching historical trade data as well as OHLCV data from exchanges using the underlying CCXT wrapper * Created a test for CCXT testing the functionality of the two added methods * Tests don't currently pass due to a couple of reasons. First one is a lack of support in CCXT for OHLCV data fetching, and the other is strange mapping of symbols
* Add tests for fetching orderbook and ticker
* Update CCXT dependency's version to latest
package.json
Outdated
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "gdax-trading-toolkit", | |||
| "version": "0.1.18", | |||
| "version": "0.1.19", | |||
There was a problem hiding this comment.
Let us manage versioning; we can then batch a bunch of PRs together in a release.
| }); | ||
|
|
||
| it('is able to fetch historical trade data from an exchange', async () => { | ||
| const data = await wrapper.fetchHistTrades(productId); |
There was a problem hiding this comment.
I love the tests, but it looks like these requests are hitting the internet. This leads to slow and flaky tests (what if bitmex is down / delist the product?). We use the nock library for mocking out HTTP requests, and you can see examples of its use in some of the other tests here.
There was a problem hiding this comment.
delist the product
That's why we don't hardcode pairs and fetch them online where possible... Outdated product lists can be painful to maintain in the long term. Though we still have to keep unique product id ←→ common symbol mapping to remain consistent across exchanges... Did you know that CoinMarketCap lists BTM for two different coins (Bytom (HitBtc) vs Bitmark (Poloniex), ccxt/ccxt#348, ccxt/ccxt#350, etc...) ? There's plenty of that across exchanges...
Just my 2 cents.
There was a problem hiding this comment.
Interesting. Then there's BCH and BCC..
For the GTT, which is an abstraction layer, I think it's ok to maintain a canonical list of coin names and map them behind the interfaces.
So for example, if GTT users define and expect BCC to represent 'Bitcoin cash' then mappings to BCH should happen behind the scenes where appropriate. Then users only have to learn one set of ticker names and the GTT does most of the work. ccxt, being pretty close to the apis needs to work with this more carefully.
There was a problem hiding this comment.
@CjS77 we do the BCC → BCH mapping on the fly and the user doesn't have to know/learn it. We use BCH everywhere in our API because BCH is most widely used for Bitcoin Cash, while BCC also stands for Bitconnect sometimes, that causes confusion. If you notice BCC somewhere in CCXT let us know, but you should expect unified symbols from CCXT as written here: https://github.com/ccxt-dev/ccxt/wiki/Manual#naming-consistency ;) The unification mapping is overrideable per exchange, of course.
There was a problem hiding this comment.
Yeah, so we're advocating the same thing. I was just using BCH/BCC as an example and wasn't favoring one ticker over the other.
* Revert version bump
|
All network requests have been eliminated and mock data has been set up for all of the URLs requested internally by CCXT. I've also reverted the version bump. |
* Newer versions of CCXT seem to use a different route for fetching the product list. A regular expression is used to match both.
|
LGTM, nice work! |
This PR adds two CCXT-specific methods to the
CCXTExchangeWrapperfor downloading OHLCV candlestick data and historical trades. I've also added some tests for the CCXT wrapper for these two added features as well as orderbook+ticker fetching.