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.

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

markets "contractSize" wrong data type #11123

Closed
xmatthias opened this issue Jan 4, 2022 · 13 comments
Closed

markets "contractSize" wrong data type #11123

xmatthias opened this issue Jan 4, 2022 · 13 comments
Assignees

Comments

@xmatthias
Copy link
Contributor

xmatthias commented Jan 4, 2022

  • OS: linux
  • Programming Language version: all (although i personally only really care about python for now)
  • CCXT version: latest main - v1.65.19

the type of contractSize seems to be a string throughout the exchanges I've tested - which seems and feels wrong to me.

Based on the documentation i'd expect it to be a float, which would align the "position structure" with the response of the market response.

Screenshot_20220104_192835

I also don't think any value other than a float will be a possible/allowed value for contractSize - as we'd expect to multiply (or divide) by that number to go from amount to what the exchange expects and viceversa.

Having this as string makes all calculations necessary for futures quite odd, as we'll have to do explicitly convert contract-size to number (e.g. amount * float(market['contractSize'])).

 node examples/js/cli.js okex market BTC/USDT:USDT
okex.market (BTC/USDT:USDT)
0 ms
{         limits: { leverage: { min: undefined, max: 125 },
                      amount: { min: 1, max: undefined },
                       price: { min: 0.1, max: undefined },
                        cost: { min: 0.1, max: undefined }  },
       precision: { amount: 1, price: 0.1 },
       tierBased:    undefined,
      percentage:    undefined,
           taker:    0.0005,
           maker:    0.0002,
              id:   "BTC-USDT-SWAP",
          symbol:   "BTC/USDT:USDT",
            base:   "BTC",
           quote:   "USDT",
          baseId:   "BTC",
         quoteId:   "USDT",
        settleId:   "USDT",
          settle:   "USDT",
            info: {     alias: "",
                      baseCcy: "",
                     category: "1",
                       ctMult: "1",
                       ctType: "linear",
                        ctVal: "0.01",
                     ctValCcy: "BTC",
                      expTime: "",
                       instId: "BTC-USDT-SWAP",
                     instType: "SWAP",
                        lever: "125",
                     listTime: "1636620057000",
                        lotSz: "1",
                        minSz: "1",
                      optType: "",
                     quoteCcy: "",
                    settleCcy: "USDT",
                        state: "live",
                          stk: "",
                       tickSz: "0.1",
                          uly: "BTC-USDT"       },
            type:   "swap",
            spot:    false,
         futures:    false,
            swap:    true,
        contract:    true,
          option:    false,
          linear:    true,
         inverse:    false,
          active:    true,
    contractSize:   "0.01",    <-- would like to be a number ... 
          expiry:    undefined,
  expiryDatetime:    undefined                               
@xmatthias xmatthias changed the title load_markets "contractSize" wrong type load_markets "contractSize" wrong data type Jan 4, 2022
@xmatthias xmatthias changed the title load_markets "contractSize" wrong data type markets "contractSize" wrong data type Jan 4, 2022
@frosty00
Copy link
Member

frosty00 commented Jan 5, 2022

@xmatthias it may seem like it is the wrong data type but we actually store it as a string on purpose since we need to do stringMaths using contractSize inside of parsePosition and some other places

Having this as string makes all calculations necessary for futures quite odd, as we'll have to do explicitly convert contract-size to number (e.g. amount * float(market['contractSize'])).

that's cause you are using floating point maths

Based on the documentation i'd expect it to be a float, which would align the "position structure" with the response of the market response.

I'll update the docs

@xmatthias
Copy link
Contributor Author

xmatthias commented Jan 5, 2022

@frosty00 the linked commit has 2 issues in my opinion.

If you update the datatype, you should also update the sample data -> 'contractSize': '100' (which i've noticed now was fixed in a second commit).

It's however also wrong in another sense.

The documentation section is about the "position structure" -> the position return value - which seems to be a number:
https://github.com/ccxt/ccxt/blob/master/js/ftx.js#L1962
https://github.com/ccxt/ccxt/blob/master/js/gateio.js#L2954

...
it does make sense to keep contractSize as float in positions (i never argued this, and think this should remain this way).

It makes however the value of 'contractSize' of a different type depending on the endpoint - as market has it as string, while fetch_positions has it as float (this is what i'm asking above).

I've also been unable to find any instance where contractSize was actually used for string calculations (string calculations to me means "100" + "100" = "100100" - which i don't think makes any sense in finance).

The only instances i did find were with Precise.stringMul - where it should be easily possible to also accepts floats into in one central location.

@samgermain
Copy link
Member

@xmatthias it may seem like it is the wrong data type but we actually store it as a string on purpose since we need to do stringMaths using contractSize inside of parsePosition and some other places

Having this as string makes all calculations necessary for futures quite odd, as we'll have to do explicitly convert contract-size to number (e.g. amount * float(market['contractSize'])).

that's cause you are using floating point maths

Based on the documentation i'd expect it to be a float, which would align the "position structure" with the response of the market response.

I'll update the docs

I think that we should convert contractSize to a string when we need to do string math internally, but the value returned from fetchMarkets should be a float. Almost every user would want the contract size to be a float, and would expect it to be a float, and returning a string is just kind of weird to the user when it's only a string to help with internal use

@xmatthias
Copy link
Contributor Author

xmatthias commented Jan 6, 2022

That's exactly my point here: #11128 (comment)

Alternatively (to avoid potentially repeat converting) - maybe keep it in contractSize_str (naming is obviously random) - so both are accessible (it should be pretty easy to copy this over during exchange init - or as part of fetchMarkets).
that'll give the user the flexibility to use either the float, or string version (whatever the library user requires at that moment).

@frosty00
Copy link
Member

frosty00 commented Jan 6, 2022

I think that we should convert contractSize to a string when we need to do string math internally, but the value returned from fetchMarkets should be a float. Almost every user would want the contract size to be a float, and would expect it to be a float, and returning a string is just kind of weird to the user when it's only a string to help with internal use

Technically we have deprecated floats everywhere already, in other words there is no such things as a float type in ccxt, just .number which is customisable by the user

@xmatthias
Copy link
Contributor Author

xmatthias commented Jan 6, 2022

Technically we have deprecated floats everywhere already

@frosty00 I must've missed that anouncement - can you point me to it?

also, the famous .number property seems to be missing from the docs completely ... (at least i cannot find it).

@frosty00
Copy link
Member

frosty00 commented Jan 6, 2022

#522

@xmatthias
Copy link
Contributor Author

xmatthias commented Jan 6, 2022

#522

That actually looks like a (still open) enhancement request - not an announcement.
If that's an official ccxt standpoint - i would expect to see it

  • in the docs
  • as pinned anouncement - like the contract naming conventions

As neither of the 2 is true at this point as far as i can see - so i'm sorry, but it's just a wish in my eyes.

You can't realistically expect users to take "announcements" from some discussion buried in the 20th comment in a 4 year old issue.

@frosty00
Copy link
Member

frosty00 commented Jan 6, 2022

the support for it isn't complete, basically there is a reason why we have let the numbers in ccxt be customizable. So it's not as simple as "just setting it to a float" cause then the next person will come in and say "why is contractSize a float I want a decimal.Decimal" or whatever type. So that's why the type of our numbers are now exchange.number (whatever class the user points that to), and why we cannot set contractSize to a float. The alternative is to set it to a number in fetchMarkets however then we can't use it later for stringMath.

@frosty00
Copy link
Member

frosty00 commented Jan 6, 2022

That actually looks like a (still open) enhancement request - not an announcement.
If that's an official ccxt standpoint - i would expect to see it

in the docs
as pinned anouncement - like the contract naming conventions

There's nothing to announce since it isn't complete and the change is backwards compatible

@xmatthias
Copy link
Contributor Author

xmatthias commented Jan 6, 2022

So that's why the type of our numbers are now exchange.number (whatever class the user points that to)

works for me - but https://docs.ccxt.com/en/latest/manual.html#exchange-structure (nor other documentation pages) even mention this possibility (let me know if you'd like me to raise this as a separate issue to have it as "todo").

and why we cannot set contractSize to a float

Again - works for me, but then contractSize should be a string in both instances (or use the .number logic in both instances).
Assuming i set .number to float - i'd want to get it as number from fetch_markets too (which is currently not the case - and hence i consider this a bug).

I understand the concern about string math internally - but i think there's quite simple workarounds to this, which have been pointed out above.

There's nothing to announce since it isn't complete and the change is backwards compatible

You mentioned it's deprecated above - which has to be either documented/anounced (so people know about it) - or a personal wish from your side.

@frosty00
Copy link
Member

frosty00 commented Jan 6, 2022

Yeah I think probably we'll need to have a variable called contractSizeString or similar

@kroitor
Copy link
Member

kroitor commented Jan 6, 2022

Totally agree with @xmatthias and @samgermain on this issue.

@frosty00 I think, the correct way to address this is to have both a float and a string in each market and each position. I suggest to revert contractSize to safeNumber (defaulting to a float). We can also add contractSizeString alongside if it's really necessary, but i don't think it is. The values of contractSize are mostly integers and floats up to 8 decimals, which is why we can safely convert those values in place for internal string-based maths and in most cases we don't even have to expose the string-math in the userland.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants