Querier quotes tolerance_bps parameter and total_bps/slippage_bps returned field use different reference output amounts
Discord context:
https://discord.com/channels/838986635756044328/842253131105566760/1239470501668065301
Endpoint example:
https://thornode-v1.ninerealms.com/thorchain/pool/AVAX.USDT-0X9702230A8EA53601F5CD2DC00FDBC13D4DF4A8C7?height=15947000
"balance_asset": "19203591597176",
AVAX.USDT pool holds ~192.0k USDT.
A swap from 189.7k AVAX.USDT to ETH.USDT:
https://thornode-v1.ninerealms.com/thorchain/quote/swap?from_asset=AVAX.USDT-0X9702230A8EA53601F5CD2DC00FDBC13D4DF4A8C7&to_asset=ETH.USDT-0XDAC17F958D2EE523A2206206994597C13D831EC7&amount=18970000000000&height=15947000
"expected_amount_out": "4775297198300",
USDT expected out is ~47,8k USDT, about 25% (since the inbound was close to its pool's depth).
"fees": {
"asset": "ETH.USDT-0XDAC17F958D2EE523A2206206994597C13D831EC7",
"affiliate": "0",
"outbound": "672452100",
"liquidity": "4777046048000",
"total": "4777718500100",
"slippage_bps": 5000,
"total_bps": 5000
},
"slippage_bps": 5000,
"streaming_slippage_bps": 5000,
I chose the inbound so the basis points would be a round 5000: these basis points are of the XYK emitted amount, not relative to the starting pool prices.
Now, adding tolerance_bps
greater than 5000:
https://thornode-v1.ninerealms.com/thorchain/quote/swap?from_asset=AVAX.USDT-0X9702230A8EA53601F5CD2DC00FDBC13D4DF4A8C7&to_asset=ETH.USDT-0XDAC17F958D2EE523A2206206994597C13D831EC7&amount=18970000000000&height=15947000&tolerance_bps=7000
{"error":"failed to simulate swap: emit asset 4775969650400 less than price limit 5708839981050"}
The "emit asset" amount here is close to the expected_amount_out
previously, namely ~47.8k USDT.
The price limit for tolerance_bps
7000 is ~57k USDT;
57 / 189.7 ~= 30%, appropriate for if having tolerance of 70% (7000 basis points).
However, it seems to be understandably confusing for a user if the tolerance_bps
and total_bps
are using different reference output amounts for their calculations!
Relevant code:
https://gitlab.com/thorchain/thornode/-/blob/v1.132.0/x/thorchain/querier_quotes.go#L681-687
// convert to a limit of target asset amount assuming zero fees and slip
feelessEmit, err := quoteConvertAsset(ctx, mgr, fromAsset, swapAmount, toAsset)
if err != nil {
return quoteErrorResponse(err)
}
limit = feelessEmit.MulUint64(10000 - toleranceBasisPoints.Uint64()).QuoUint64(10000)
https://gitlab.com/thorchain/thornode/-/blob/v1.132.0/x/thorchain/querier_quotes.go#L181
amount = fromPool.AssetValueInRune(amount)
This I think is code that calculates the price limit from the inbound's value-in-output based on the pool prices before the swap,
which I currently think is an intuitive reference output amount for a swapper.
https://gitlab.com/thorchain/thornode/-/blob/v1.132.0/x/thorchain/querier_quotes.go#L341-358
// compute slip based on emit amount instead of slip in event to handle double swap
slippageBps := liquidityFee.MulUint64(10000).Quo(emitAmount.Add(liquidityFee))
// build fees
totalFees := affiliateFee.Add(liquidityFee).Add(outboundFeeAmount)
fees := openapi.QuoteFees{
Asset: msg.TargetAsset.String(),
Affiliate: wrapString(affiliateFee.String()),
Liquidity: liquidityFee.String(),
Outbound: wrapString(outboundFeeAmount.String()),
Total: totalFees.String(),
SlippageBps: slippageBps.BigInt().Int64(),
TotalBps: totalFees.MulUint64(10000).Quo(emitAmount.Add(totalFees)).BigInt().Int64(),
}
// build response from simulation result events
return &openapi.QuoteSwapResponse{
ExpectedAmountOut: emitAmount.String(),
This I think is code that returns basis points values wholly in reference to the emit amount and liquidity fees, and outbound fee,
after pool slippage has already taken place,
which seems as though for a swapper it could unintuitively hide the hypothetical-output-value basis points lost in pool slippage.
(In practice this has confused users of the swap endpoint,
in that a tolerance_bps
parameter higher than the parameter-not-present total_bps
/slippage_bps
returns an error.)
I am happy to dig into why/how different values are returned, but if possible I would prefer that someone else more familiar with the code
( @the_eridanus / @ursa9r ?) write any code changes for this, if to increase consistency/intuitiveness in a satisfactory way.