Conditional numberInString. Allow passing number | string to order endpoints

[felds]

Currently, the values in NewSpotOrderParams only accept numbers. Since all numbers in JS are floats, this leads to some numbers having recurring decimals when converting to base 10, tripping the filter "Precision is over the maximum defined for this asset." even after normalizing the number (with Math.floor(qty / precision) * precision).

Allowing passing numberInString or simply string (as numberInString | number) would allow the user to trim the passed value with Number.toFixed.

eg:

const qty = 10/3; //  => 3.3333333333333335
const precision = 0.0001;
const normalized = Math.floor(qty / precision) * precision; //  => 3.3333000000000004 (rejected)
const trimmed = normalized.toFixed(4) //  => "3.3333" (accepted)

[tiagosiebler]

JS can be quite bad at precise math, as you also noticed here. Even when trimming and rounding to try and work around this, you're likely to see this happen again and cause other problems in your workflows.

If you're dealing with sensitive math (such as order amounts, values, prices, anything money related), I really recommend using a decimal library such as big.js or decimal.js for any calculations that require precision. Once your calculations are done you can still extract a number (or a string if you prefer) from this special type.

[felds]

That part is sorted out. My suggestion is just to change the typings to allow strings, so people don't need to use hacks like this to circumvent the requirement for some values to be numbers:

{
  quoteOrderQty: amount.toFixed(4) as unknown as number
}

[tiagosiebler]

That part is sorted out. My suggestion is just to change the typings to allow strings, so people don't need to use hacks like this to circumvent the requirement for some values to be numbers:

{
  quoteOrderQty: amount.toFixed(4) as unknown as number
}

If the API accepts strings for these fields (even the docs say numbers only), I'm absolutely open to that. It was also suggested here - just need to update the type to accept number | string in those fields, as long as each field has been tested to support that.

[felds]

Before using your library, I was passing everything as strings because the request is made using form data and every param is sent as string anyways (as opposed to JSON, that has bools, numbers, null etc).

I'd love to be able to change that but I can't make the tests pass as is in my environment. Are there any documentation on that?

[tiagosiebler]

Before using your library, I was passing everything as strings because the request is made using form data and every param is sent as string anyways (as opposed to JSON, that has bools, numbers, null etc).

That's a good point!

I'd love to be able to change that but I can't make the tests pass as is in my environment. Are there any documentation on that?

The tests take API credentials for the private portion. I have a sub-account dedicated to these tests, although I did notice the API key expired (since it doesn't have IP whitelisting enabled), so I'm fixing that shortly.

What're your tests failing with? Missing credentials?

If you're in a unix or macOS shell, you can just pass API credentials using env vars as such:

API_KEY_COM="yourapikeyhere" API_SECRET_COM="yourapisecrethere" npm run test:watch

That's how I normally do full test runs locally and that's how the CI run executes these tests too.

[felds]

(just as a clarification about my suggested type: I suggested numberInString because it's an existing type used widely on the project that resolves to number | string)

[tiagosiebler]

Also a good point. I'm undecided about using it in a request scenario where either of the two types is OK...because it's currently used for responses, but only responses with the beautifier enabled will always return a number for this field, or if the beautifier is disabled they will always return a number in a string.

I had hoped to eventually improve how types resolve so that typescript knows "this client was instanced with the beautifier enabled, so all numberInString fields are definitely number and never string. I don't have an immediate solution for this but it's a point I've wanted to reach with that specific response property type. If that point is reached, I'm not sure it would cause conflict or confusion for requests fields that are not either or but can be both...

[felds]

I had hoped to eventually improve how types resolve so that typescript knows "this client was instanced with the beautifier enabled, so all numberInString fields are definitely number and never string.

I don't know enough about TS to help you with that right now, but I think it would be a fun subject to study! :)

I'll let you know as soon as I have a working example.

[tiagosiebler]

I had hoped to eventually improve how types resolve so that typescript knows "this client was instanced with the beautifier enabled, so all numberInString fields are definitely number and never string.

I don't know enough about TS to help you with that right now, but I think it would be a fun subject to study! :)

I'll let you know as soon as I have a working example.

That would be extremely helpful! I'm also very curious how one might resolve that. It would be easy if the boolean controlling the beautifier were included in a function call (although that would be an ugly design choice too in my opinion), but right now any API call would somehow need to know that this part is going via the beautifier so any numberInString is actually a number...
https://github.com/tiagosiebler/binance/blob/master/src/util/BaseRestClient.ts#L244

Maybe passing in a generic TNumber. If via the beautifier, any numberInString<TNumber> would actually be numberInString<number> (thus returning only a number type), else numberInString<string> (thus this property contains a string, since the beautifier didn't resolve it).

Tricky but interesting challenge. Open to design changes too if it makes it easier to solve, within reason.

[felds]

Good new is that it seems possible with conditional types:

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBDAnmApnA3nAogRwK4CGANgDTYAeqAxvAL5wBmUEIcA5AAJKoC0VAFsSIoAdgHMUAZwD0eGMCKS2AWABQa7mgBCKAnOANgKACYBhIkZHwAvBjgAjXfoaIAXAih40tDcjQAlAgB3c0sbO0c9eRd3BmJJb19UOFDRcJ0ogyMzCzS4AB84QJDcqzUktAA5PBBHKAB5KABlGChgcQAeABU4FHIYUWNJFNKYAD44Wx6+gZEhuAznbNSrOAB+OBEaurh3SVb2sXLVTTgqAgTh2wBtNTgKahgO3EIiDuralAbmg870SOcbg8XloYzIW0+UDGYLuDxQNGe+GI722X0aLTafwB0SBcUUKFBZH2mLE0JIagAukA

[tiagosiebler]

I thought it might be in that direction, good stuff, though I still wonder how the implementation might look on something like the rest clients...does each method need a pass-through generic in the return value?

[felds]

The type of the client can be inferred by the values in the constructor like this:

https://www.typescriptlang.org/play?#code/C4TwDgpgBAQghgV2ASwGbIgEwApwE5wC2UJAvFAN5QBGEiKqIAXFMHgtAL4CwAUKJCgAlOAHdcBYiWnTyVWvTTMoqOABsAzlz4DoAYTUYAdsAlENMqOQAU8JGgw58RKAB9hYs4QCUUAGSUAPbAABYQeCwARGDOhBqRPLw64NAAcgiEtHgA8ngAymzIRgDmADwAKlAQAB7AEEaYFgbGprEaAHxWUJU1dQ0WdgyOXlAA-FBGGVlQLBqFJXx8mBAAxmr40GtwGk2G9cAVVbX1jVDN+14dlHwkK4FGc+wrwIF41jGSGizl3gDcN1BihBgAA1dQcazeFjpTLhXIFPBFMrldr-XiJPh3B7AKBrFoAQS6RggojOexM1goCnsjBYbA4ABooMEwhEoNE2glvJjycB8QA6IGg8EQSGLXhYua43kwIkksktSnUhjKVSaCBMlnhKIfcxcnktGCC4FgtQQ7m8IA

(hover the getValue() calls to see the inferred types)

[tiagosiebler]

Ah that is quite nice, all that's left then is passing in the generic to the type return by each method. Nice!

Edit: that's also a good reason to use number | string instead of the numberInString type for any request parameters, so this numberInString is reserved for response types.