liqui: rounding in amountToPrecision() leads to InsufficientFunds exception

[mkutny]

Basically, createLimitSellOrder('BTC/USDT', 0.00206404549, 10000) results in InsufficientFunds exception as amountToPrecision rounds amount up to 0.00206405.

The following in the base class fixes the issue:

    this.amountToPrecision = function (symbol, amount) {
        //return parseFloat (amount).toFixed (this.markets[symbol].precision.amount)
        return this.truncate(amount, this.markets[symbol].precision.amount)
    }

At the same time I'm a bit unsure to submit a PR because I'm bothered with the following questions:

• I see several places in code where feeToPrecision() and costToPrecision() were overridden in a child's class with a similar fix. At the same time, it looks more natural to me if the base class contains the code with truncation so there is no need to override. Why it's not the case?

• What's the convention for passing amount? Those overrides do parseFloat(amount) before truncation. Is it expected that amount might be a String as well? I see that truncate converts amount to a String anyway down the road.

[kroitor]

Hi, @mkutny !

Thx for opening this issue!

At the same time, it looks more natural to me if the base class contains the code with truncation so there is no need to override. Why it's not the case?

In general, we need proper values for these entities:

• price
• amount
• cost
• fee

Unfortunately, some exchanges round these values and some exchanges truncate them inside their engines. So, any of those values upon any stage of fee calculation can be either truncated or rounded by the exchange. And we don't know beforehand what an exchange will do, because those guys never document their exact formulas for calculating fee precisions. This is very frustrating, because they make millions on traders, and they don't even bother to document their engines in a financially-sound way.

So we had to reverse-engineer it. We did that by placing market-making orders (to guarantee the exact price for which an order is closed). Then we waited for the order to close. And after that we did calculations on our side based on expected fees and rounding/truncation rules and compared the balance differences. If a balance difference was the same as expected, we carried that we have the calculation done right and fees were applied in correct amounts to correct sides. The balance difference has to be compared with respect to truncation or rounding of amount/price/cost and fee.

There's even more to it: some exchanges apply fees to this or that side of your balance (base or quote currency of the pair), depending on the side of your trade (Poloniex), whereas other exchanges always apply fees to quote side (Bittrex, Liqui), or always to the base side of the pair. And some of them allow to specify the desired fee side (Kraken).

So, we had to test all cases, namely both sell and buy orders to compare balances in all these cases. We did that for several exchanges, and Liqui was among them. This is why some exchanges use the rounding in *toPrecision methods (like bittrex and Liqui) and the other use truncation (like Kraken).

Is it expected that amount might be a String as well? I see that truncate converts amount to a String anyway down the road.

As the user might pass a string, we do the parsing/type-conversion as well. That is, until we obtain a fixed-point solution.

I hope it does answer your question... Let us know what you think of it, plz.

[kroitor]

The source of the test is:

const amount = exchange.amountToPrecision (symbol, 0.030)
const price_precision = market['precision']['price']
const amount_precision = market['precision']['amount']
const price_step = Math.pow (10, -price_precision)
const type = 'limit'
const side = (oldBalance[base] < (oldBalance[quote] / ticker['bid'])) ? 'buy' : 'sell'

// find the mid-spread price for placing a market-making order

const spread = ticker['ask'] - ticker['bid']
const price = exchange.priceToPrecision (symbol, (side == 'sell') ? (ticker['ask'] - spread / 2) : (ticker['bid'] + spread / 2))

const cost = exchange.costToPrecision (symbol, price * amount)
const fee = exchange.calculateFee (symbol, type, side, amount, price, 'maker')

log.green.bright (side.toUpperCase (), symbol)
log.green ('\t', 'Price:', price)
log.green ('\t', 'Amount:', amount)
log.green ('\t', 'Cost:', cost)
log.yellow ('\t', 'Fee:', fee)

let expectedBaseBalance = undefined
let expectedQuoteBalance = undefined
let expectedBaseDifference = undefined
let expectedQuoteDifference = undefined

let baseFeeCost  = (fee.currency == base)  ? fee.cost : 0
let quoteFeeCost = (fee.currency == quote) ? fee.cost : 0

if (side == 'sell') {

    expectedBaseDifference  = 0 - amount - baseFeeCost
    expectedQuoteDifference = 0 + cost   - quoteFeeCost

} else {

    expectedBaseDifference  = 0 + amount - baseFeeCost
    expectedQuoteDifference = 0 - cost   - quoteFeeCost
}

expectedBaseBalance  = oldBalance[base]  + expectedBaseDifference
expectedQuoteBalance = oldBalance[quote] + expectedQuoteDifference

log ('------------------------------------------------'.dim)
log ('expected balance:')
log (base,  exchange.amountToPrecision (symbol, expectedBaseBalance).green)
log (quote, exchange.priceToPrecision (symbol, expectedQuoteBalance).green)
log ('------------------------------------------------'.dim)
log ('expected difference:')
log (base,  ((expectedBaseDifference  >= 0) ? '+' : '').green + expectedBaseDifference.toFixed (amount_precision).green)
log (quote, ((expectedQuoteDifference >= 0) ? '+' : '').green + expectedQuoteDifference.toFixed (price_precision).green)
log ('------------------------------------------------'.dim)

log ('Placing', side, amount, base, 'for', price, quote)
const reply = await exchange.createOrder ('ETH/BTC', 'limit', side, amount, price)
log ('reply:', reply)

log ('------------------------------------------------'.dim)
const orderId = reply['id']
// re-check open orders in a continuous loop
// until it's either closed or a timeout of 2 minutes expires
// (makers are not guaranteed to close immediately)
const order = await untilOrderIsClosed (exchange, orderId, symbol, params)

log ('------------------------------------------------'.dim)
log ('order:', order)

log ('------------------------------------------------'.dim)
log ('old balance:')
printBalance2 (base, oldBalance[base], quote, oldBalance[quote])
log ('------------------------------------------------'.dim)
log ('new balance:')
const newBalance = await exchange.fetchFreeBalance ()
printBalance2 (base, newBalance[base], quote, newBalance[quote])

log ('------------------------------------------------'.dim)
log ('balance difference:')

const actualBaseDifference  = newBalance[base]  - oldBalance[base]
const actualQuoteDifference = newBalance[quote] - oldBalance[quote]

log (base,  ((actualBaseDifference  >= 0) ? '+' : '').green + exchange.amountToPrecision (symbol, actualBaseDifference).toString ().green)
log (quote, ((actualQuoteDifference >= 0) ? '+' : '').green + exchange.priceToPrecision (symbol, actualQuoteDifference).toString ().green)

log ('------------------------------------------------'.dim)

const actualCostToPrecision    = exchange.priceToPrecision (symbol, order.cost)
const expectedCostToPrecision  = exchange.priceToPrecision (symbol, cost)

const actualPriceToPrecision   = exchange.priceToPrecision (symbol, order.price)
const expectedPriceToPrecision = exchange.priceToPrecision (symbol, price)

// if our order landed as a maker, the price is guaranteed
// therefore both the cost and the price should be equal to what we expect

const orderCostIsMaker = (actualCostToPrecision === expectedCostToPrecision)
const orderPriceIsMaker = (actualPriceToPrecision === expectedPriceToPrecision)

// check basic order props

assert.property (order, 'filled')
assert.property (order, 'status')
assert.property (order, 'cost')
assert.property (order, 'fee')

// check if our order ended up being a maker indeed

assert.equal (orderCostIsMaker, true)
assert.equal (orderPriceIsMaker, true)

assert.equal (order.status, 'closed')

const expectedBaseDifferenceToPrecision  = exchange.amountToPrecision (symbol, expectedBaseDifference)
const actualBaseDifferenceToPrecision    = exchange.amountToPrecision (symbol, actualBaseDifference)

const expectedQuoteDifferenceToPrecision = exchange.priceToPrecision  (symbol, expectedQuoteDifference)
const actualQuoteDifferenceToPrecision   = exchange.priceToPrecision  (symbol, actualQuoteDifference)

// check expected balances on both sides against current balance received from the exchange

assert.equal (actualBaseDifferenceToPrecision,  expectedBaseDifferenceToPrecision)
assert.equal (actualQuoteDifferenceToPrecision, expectedQuoteDifferenceToPrecision)

const filledToPrecision       = exchange.amountToPrecision (symbol, order.filled)
const amountToPrecision       = exchange.amountToPrecision (symbol, amount)

assert.equal (filledToPrecision, amountToPrecision)

The above code snippet may be missing some non-copied methods, but the general structure of it should be clear. Standing by to hear more from you on this.

[mkutny]

Hmm, that's quite elaborate, it took me some time to read it (and ccxt code) thoughtfully.

(Just a side note: it might be just more convenient to calculate mid spread as (bid + ask) / 2).

Now I see that the problem is actually more complex than I previously thought so if you could elaborate a bit more on it that would be greatly appreciated:

1. I'm not sure you actually test amountToPrecision(). I see the point in calculating things on your side, then on exchange side, then to compare. But you don't do any calculations with amountToPrecision() on your side, you use it only during the last (comparison) step. Can it be actually tested this way?

2. You don't actually use amountToPrecision() for fees/costs/prices calculations. In ccxt code, I don't see other uses of this function rather than for preparing amounts for order placement. Do you use it somehow else? If not it could be switched to 'truncate' mode.

3. If you don't want me to switch amountToPrecision() to truncate mode I can write a one-liner amountToLots(). It won't handle lots that are not fractions of 10 (I'm still thinking on a proper way of doing it) but it will fix the immediate issue with InsufficientFunds and it could be elaborated to a more general case later.

[kroitor]

I'm not sure you actually test amountToPrecision(). I see the point in calculating things on your side, then on exchange side, then to compare. But you don't do any calculations with amountToPrecision() on your side, you use it only during the last (comparison) step. Can it be actually tested this way?

Because we had a fixed order amount of 0.03 ETH there, we were sure that we will avoid amount precision problems in the test.

You don't actually use amountToPrecision() for fees/costs/prices calculations. In ccxt code, I don't see other uses of this function rather than for preparing amounts for order placement. Do you use it somehow else? If not it could be switched to 'truncate' mode.

After reviewing the above code I can actually see now, that you are totally correct. So, I guess, we can switch to amount truncation, I don't foresee any problems there with respect to arithmetics.

If you don't want me to switch amountToPrecision() to truncate mode I can write a one-liner amountToLots(). It won't handle lots that are not fractions of 10 (I'm still thinking on a proper way of doing it) but it will fix the immediate issue with InsufficientFunds and it could be elaborated to a more general case later.

Actually, I think we'll be fine with truncation, and if it turns out to be a problem, then we can fall back any moment to a solution that does throw a "lot"-modulo exception. So, if you feel like you want to give it a try, go ahead and switch it to truncation, I'll merge it right away.

Thx!

[kroitor]

(Just a side note: it might be just more convenient to calculate mid spread as (bid + ask) / 2).

I know my code above isn't very clean %)) Your formula is much shorter, of course )