Repository: ccxt/ccxt
Pull request: fetchOHLCV: switch between current and history candles
Edited how fetchOHLCV switches between history and current candles in the since handling
fixes: #20756
### Test 1 (history candles):
```
const hourInMs = 3600 * 1000;
since = this.milliseconds() - 1439 * hourInMs;
```
```
okx.fetchOHLCV (BTC/USDT, 1h, 1699862134346, 300)

okx GET https://www.okx.com/api/v5/market/history-candles?instId=BTC-USDT&bar=1H&limit=300&before=1699862134346&after=1700942134346

1700582400000 | 36974.8 | 37260.1 |   36750 | 37190.8 | 14441.17524511
1700586000000 | 37191.5 | 37509.7 | 36612.4 |   36900 | 90031.81273416
1700589600000 | 36903.9 | 36964.6 | 35779.1 | 35853.3 |    120.9956569
...
1700931600000 |   37744 | 37799.3 |   37700 |   37770 |   674.07239716
1700935200000 |   37770 | 37820.2 | 37755.4 | 37818.3 |   746.98028882
1700938800000 |   37819 |   37840 | 37790.2 | 37818.3 |   684.92567018
100 objects
```
### Test 2 (current candles):
```
const hourInMs = 3600 * 1000;
since = this.milliseconds() - 1438 * hourInMs;
```
```
okx.fetchOHLCV (BTC/USDT, 1h,1699865851198 , 300)

okx GET https://www.okx.com/api/v5/market/candles?instId=BTC-USDT&bar=1H&limit=300&before=1699865851198&after=1700945851198

1699866000000 | 37027.4 | 37040.1 | 36947.3 |   36982 |   1507.79655319
1699869600000 |   36984 | 37009.1 | 36914.9 | 36973.7 |   2561.28039174
1699873200000 | 36972.8 | 36973.7 | 36700.8 | 36817.3 |   5842.25295832
...
1700935200000 |   37770 | 37820.2 | 37755.4 | 37818.3 |    746.98028882
1700938800000 |   37819 |   37840 | 37790.2 | 37818.3 |    684.92567018
1700942400000 | 37818.3 | 37886.9 | 37766.6 | 37800.1 |    1017.9527583
300 objects
```

Problem statement:
Okx fetch_ohlcv candles/history candles automatic switch issue
### Operating System

Windows 10

### Programming Languages

_No response_

### CCXT Version

4.2.9

### Description

Okx states that limit is 300 for live candles, and 100 for history candles
And api may return 1440 live candles

If i query live candles started from 1200 candles back i expect to get 300 live candles, not 100 history ones

since = self.exchange.milliseconds() - 1200 * self.exchange.parse_timeframe('5m')
klines = self.exchange.fetch_ohlcv(symbol='BTCUSDT', timeframe='5m', since=since, limit=300, params=params)

Code returns only 100 results. But has to return 300.

With this code i can get 300 candles only for last 1140 candles (This is 1440 - 300), not for 1440


Why?

Because in okx fetch_ohlcv limit substracted from available 1440 candles. 
It has not to be substracted

### Code
okx:2080-2091
```
           if since is not None:
            now = self.milliseconds()
            difference = now - since
            durationInMilliseconds = duration * 1000
            # if the since timestamp is more than limit candles back in the past
            # additional one bar for max offset to round the current day to UTC
            calc = (1440 - limit - 1) * durationInMilliseconds
            if difference > calc:
                defaultType = 'HistoryCandles'
            startTime = max(since - 1, 0)
            request['before'] = startTime
            request['after'] = self.sum(startTime, durationInMilliseconds * limit)`

```

calc = (1440 - limit - 1) * durationInMilliseconds
has to be replaced to 
calc = 1439 * durationInMilliseconds

Hints from issue discussion:
Hey @Alfavisioner I think your question might be answered by the comment in the code snippet you posted:
`# if the since timestamp is more than limit candles back in the past` then the history candles with a limit of 100 are used, based on that I don't think it makes sense to remove limit from calc because then I'm not sure if the whole `if difference > calc` segment would work properly.

Here is my response from your example fetch_ohlcv call:
```
okx.fetchOHLCV (BTC/USDT, 5m, 1704955844524, 300)
2024-01-11T06:56:44.919Z iteration 0 passed in 396 ms

1704956100000 | 45767.6 | 45900 | 45765.8 | 45765.8 | 2.97980933
1 objects
```
And your proposed changes return the same result for me so I'm not sure what I'm missing.

If I'm misunderstanding your issue please elaborate on your **Description** section in a clear way, I found it a bit hard to follow what you were trying to say

You can also review this PR where the change that you are debating was introduced and see if the logic stands https://github.com/ccxt/ccxt/pull/18264

Thanks, Dan
Okx exchange uses 2 modes to return candles
1. Live mode for last 1440 candles with limit 300 and maximim 40 requests per 2 seconds
2. History mode with limit 100 and maximim 20 requests per 2 seconds

Look at cases:
1.
```
from ccxt import okx
ex = okx()

hourInMs = 3600 * 1000
since = ex.milliseconds() - 1439 * hourInMs
klines = ex.fetch_ohlcv('BTC/USDT', '1h', since, limit= 300)
print(len(klines))

### Output
100
Has to be 300

```



Case 1 queries klines 1440 periods back. We deduct 1 to take into account current unclosed candle.
According to okex documentation it has to return 300 candles.
It returns only 100 instead of 300

2.
```
from ccxt import okx
ex = okx()

hourInMs = 3600 * 1000
since = ex.milliseconds() - 1200 * hourInMs
klines = ex.fetch_ohlcv('BTC/USDT', '1h', since, limit= 300)
print(len(klines))

### Output
100
Has to be 300

```

Case 2 query 1200 klines back
Returns only 100 instead of 300

3.
```
from ccxt import okx
ex = okx()

hourInMs = 3600 * 1000
since = ex.milliseconds() - 1000 * hourInMs
klines = ex.fetch_ohlcv('BTC/USDT', '1h', since, limit= 300)
print(len(klines))

### Output
300
All is ok

```

Case 3 query 1000 klines back
Returns 300 as expected


**_Why we have this issue:_**
Because of deducted limit in calc variable. Code just switched from Candles to HisitoryCandles earlier than necessary:
calc = (1440 - limit - 1) * durationInMilliseconds
If you deduct limit variable from 1440, you just move minimum possible Candles timestamp on value of limit.  It becomes 1140 in our case.  


**_Research:_**
First of all i investigated issue: https://github.com/ccxt/ccxt/pull/18264

Found that it is posible 3 cases:
a) _before_ and _after_ are located left on timeline camparing to 1440 border. HistoryCandles mode has to be used:
b) _before_ and _after_ are located right on timeline camparing to 1440 border. Candles mode has to be used:
c) _before_ is left, _after_ is right   (case https://github.com/ccxt/ccxt/pull/18264). HistoryCandles mode has to be used:


**_My proposed solution:_**
To be frankly, calc variable is non descriptive and difference variable confusing too:
Propose to use historyBorder variable instead:
historyBorder = now - ((1440 - 1) * durationInMilliseconds)

Propose to change code from:
```
      now = self.milliseconds()
      difference = now - since
      durationInMilliseconds = duration * 1000
      # if the since timestamp is more than limit candles back in the past
      # additional one bar for max offset to round the current day to UTC
      calc = (1440 - limit - 1) * durationInMilliseconds
      if difference > calc:
          defaultType = 'HistoryCandles'
      startTime = max(since - 1, 0)
      request['before'] = startTime
      request['after'] = self.sum(startTime, durationInMilliseconds * limit)
```

to:

```
    now = self.milliseconds()
    durationInMilliseconds = duration * 1000
    # if the since timestamp is more than limit candles back in the past
    # additional one bar for max offset to round the current day to UTC
    historyBorder = now - ((1440 - 1) * durationInMilliseconds)
    startTime = max(since - 1, 0)
    if startTime >= historyBorder:
        defaultType = 'Candles'
    else:
        defaultType = 'HistoryCandles'
    request['before'] = startTime
    request['after'] = self.sum(startTime, durationInMilliseconds * limit)
```

Commit message: fix(fetchOHLCV): switch between current and history candles

```
const hourInMs = 3600 * 1000;
since = this.milliseconds() - 1440 * hourInMs;
```
```
okx.fetchOHLCV (BTC/USDT, 1h,1699862134346 , 300)

okx GET https://www.okx.com/api/v5/market/history-candles?instId=BTC-USDT&bar=1H&limit=300&before=1699862134346&after=1700942134346

1700582400000 | 36974.8 | 37260.1 |   36750 | 37190.8 | 14441.17524511
1700586000000 | 37191.5 | 37509.7 | 36612.4 |   36900 | 90031.81273416
1700589600000 | 36903.9 | 36964.6 | 35779.1 | 35853.3 |    120.9956569
...
1700931600000 |   37744 | 37799.3 |   37700 |   37770 |   674.07239716
1700935200000 |   37770 | 37820.2 | 37755.4 | 37818.3 |   746.98028882
1700938800000 |   37819 |   37840 | 37790.2 | 37818.3 |   684.92567018
100 objects
```
```
const hourInMs = 3600 * 1000;
since = this.milliseconds() - 1439 * hourInMs;
```
```
okx.fetchOHLCV (BTC/USDT, 1h,1699865851198 , 300)

okx GET https://www.okx.com/api/v5/market/candles?instId=BTC-USDT&bar=1H&limit=300&before=1699865851198&after=1700945851198

1699866000000 | 37027.4 | 37040.1 | 36947.3 |   36982 |   1507.79655319
1699869600000 |   36984 | 37009.1 | 36914.9 | 36973.7 |   2561.28039174
1699873200000 | 36972.8 | 36973.7 | 36700.8 | 36817.3 |   5842.25295832
...
1700935200000 |   37770 | 37820.2 | 37755.4 | 37818.3 |    746.98028882
1700938800000 |   37819 |   37840 | 37790.2 | 37818.3 |    684.92567018
1700942400000 | 37818.3 | 37886.9 | 37766.6 | 37800.1 |    1017.9527583
300 objects
```

Patch to review:
diff --git a/ts/src/okx.ts b/ts/src/okx.ts
index d66a18cceae37..2caf1efbf5b28 100644
--- a/ts/src/okx.ts
+++ b/ts/src/okx.ts
@@ -2146,17 +2146,14 @@ export default class okx extends Exchange {
         let defaultType = 'Candles';
         if (since !== undefined) {
             const now = this.milliseconds ();
-            const difference = now - since;
             const durationInMilliseconds = duration * 1000;
-            // if the since timestamp is more than limit candles back in the past
-            // additional one bar for max offset to round the current day to UTC
-            const calc = (1440 - limit - 1) * durationInMilliseconds;
-            if (difference > calc) {
+            // switch to history candles if since is past the cutoff for current candles
+            const historyBorder = now - (1440 * durationInMilliseconds);
+            if (since <= historyBorder) {
                 defaultType = 'HistoryCandles';
             }
-            const startTime = Math.max (since - 1, 0);
-            request['before'] = startTime;
-            request['after'] = this.sum (startTime, durationInMilliseconds * limit);
+            request['before'] = since;
+            request['after'] = this.sum (since, durationInMilliseconds * limit);
         }
         const until = this.safeInteger (params, 'until');
         if (until !== undefined) {


Code review comments:
diff_hunk: @@ -2146,17 +2146,14 @@ export default class okx extends Exchange {
         let defaultType = 'Candles';
         if (since !== undefined) {
             const now = this.milliseconds ();
-            const difference = now - since;
             const durationInMilliseconds = duration * 1000;
-            // if the since timestamp is more than limit candles back in the past
-            // additional one bar for max offset to round the current day to UTC
-            const calc = (1440 - limit - 1) * durationInMilliseconds;
-            if (difference > calc) {
+            // switch to history candles if since is past the cutoff for current candles
+            const historyBorder = now - (1440 * durationInMilliseconds);
path: ts/src/okx.ts
text: @user1:
i am not sure whether it's true what I am saying, but for safety and guaranteed behavior, i would have done this:

```suggestion
            const historyBorder = now - (1440 - 1) * durationInMilliseconds);
```

the reason is that the current bar might be considered as 1401th bar by exchange (idk, i just estimate it might be), so, it might be better to switch to `HistoryCandles` at 1439th bar (back from now) instead of exactly 1400th bar. 1 bar wouldn't much make any difference among users, however, it might save from unforeseen issues which might exist in exchange api because of current bar (which might be considered by API engine as 1401th bar) formation.

@author:
Okay I think that makes sense I'll add that change

@user2:
I also think -1 is necessary, because after we substract 1439 full periods 1 last nonclosed candle period left. 1440 total

@user2:
But if okx exchange returns live Candles for full 1440 candles plus 1 non-closed, than -1 is not necessary. All depends on exchange side

@user1:
@user2 yes, it was just added as a "possible" safety measure, which might arise (however, unconfirmed). anayw, 1 bar is not any significant, however, worth for that safety check