FxPool rounding errors fixes

[andreiashu]

• enables FX Pool type
• using only OldBigNumber or bigint.
• updated tsconfig.json to use es2020. This was needed because in parseFixedCurveParam for being able to support bit-shifting for bigint values
• added _inHigherPrecision, which allows a function to run under higher-than-18 decimal precision for OldBigNumber
• polygon test (since that's where we have higher liquidity in our pools)
• fixed bug whereby the way we calculated our derivative would have put us as last of the pools in terms of normalized liquidity regardless of the true liquidity in the FXPool

[brunoguerios]

Hey @andreiashu 👋
I'm reviewing the PR over here and I realized you're updating from number to bignumber.js, but still using float points.
Do you think it's possible to move to ethers BigNumber or bigint and update the code to use fixed point math?
I ask that because that's what we're going to use on our new SOR version to keep consistency.
I know this would require a bigger change, but it would help a lot to migrate things later on.
Let me know if that makes sense or if you have any questions!

[andreiashu]

hi @brunoguerios, I appreciate you looking over this PR!

I just pushed a commit whereby we now pass the floats as strings to OldBigNumber.

re moving to ethers BigNumber or bigint: you mean that, ideally, we shouldn't be using OldBigNumber anymore and just stick with Ethers BigNumber?

re fixed point math: can you give me an example as to how you're approaching this in the new version of SOR? We're using ABDKMath64x64.sol in our contracts, and 36 decimals seemed to be enough precision on the SOR side for us (see _inHigherPrecision and parseFixedCurveParam ). We have several places where we round (down) to a specific number of decimals depending on the number of decimals that is expected (see viewRawAmount and viewNumeraireAmount). This part was somewhat more challenging to fix in the SOR code. In our smart contracts, converting from wei to fixed point 64x64 happens several times throughout the lifecycle of a swap. Each conversion operation has its own precision loss that ultimately was compounding to higher values of loss in total in our SOR code.

[brunoguerios]

Sure thing! Thanks for getting back to me with more context! 😊

Yes, moving from OldBigNumber to either ethers BigNumber or bigint would be the ideal solution, because it forces you to use fixed point instead of float point.
This is how that maxLimit calculation would look like in fixed point math:

const alphaFixed = parseFixed(alphaValue.toString(), 36);
const ONE_36 = parseFixed('1', 36);
const _oGliqFixed = parseFixed(
    parsedReserves._oGLiq.toString(),
    36
);
const maxLimitFixed = alphaFixed
    .add(ONE_36)
    .mul(_oGliqFixed)
    .div(ONE_36) // I left this step explicit, but this is better done using a mulDownFixed helper function
    .div(2);

Of course you'd already have all variables as BigNumber and those conversions wouldn't be necessary, but I added so it's easier to follow along what each of them represents.

You can also look at how other pools are handling that within the SOR.

Gyro pool for example uses 38 decimals fixed point math, which can be found on gyroSignedFixedPoint.ts

[andreiashu]

hi @brunoguerios I just pushed several commits that start to address this change.
still work in progress but wanted to get your feedback to make sure we're going in the right direction. See calculateMicroFee which I moved to BigNumber. Let me know what you think

[andreiashu]

ps calculateMicroFee is in src/pools/xaveFxPool/fxPoolMath.ts

[brunoguerios]

Hey @andreiashu 👋
Thanks for sharing your progress so I can provide feedback early 😊
Yeah, based on calculateMicroFee, you're definitely going in the right direction 👍
Thanks for taking the time and applying these changes!
They will help a lot to include FX pools on the new version of the SDK 😃

[andreiashu]

hi @brunoguerios we got rid of OldBigNumber pretty much across our codebase. The functions that are called by the SOR (_exactTokenInForTokenOut, _tokenInForExactTokenOut, _spotPriceAfterSwapExactTokenInForTokenOut and the _derivativeSpotPriceAfter[...]) are still converting back to OldBigNumber as expected.

Let me know what you think

[brunoguerios]

I added a few comments aiming for reducing complexity whenever possible, but none of them are critical, as they don't really change the end result.
The PR looks good to me 👍
I'll sync with John over here on the process of approving/merging/releasing on the next few days and I'll keep you updated.
Feel free to reach out in case you have any questions!

[brunoguerios]

You should be able to remove this div(ONE_36) and the mul(ONE_36) as they are cancelling themselves out.

[andreiashu]

For viewNumeraireAmount and viewRawAmount we end up with a 1 or more wei difference. There's rounding (down) happening on the division which replicates what Solidity does (along with the ABDK64x64 library operations).

the other instances your mentioned below are valid, will fix now. Thank you!

[brunoguerios]

Now that you're using fixed point math with 36 decimals on fxPoolMath, you should be able to remove this inHigherPrecision wrapper, right?
I'm just thinking of ways to reduce complexity whenever possible.

[andreiashu]

It seems this one might have to wait until we have SOR converted to fixed-point math as well.

For example, in _derivativeSpotPriceAfterSwapTokenInForExactTokenOut, because we still have to use OldBigNumber in _spotPriceAfterSwapTokenInForExactTokenOut, that means that for both spotPriceBeforeSwap and _spotPriceAfterSwapTokenInForExactTokenOut we're doing .div(OLD_ONE_36).decimalPlaces(....). I believe this will be easier to fix once we have fixed-point math across SOR. Thoughts?

[brunoguerios]

I already shared this on TG chat, but I believe it's worth highlighting again, as it seems pretty odd to me:

• Is it intentional that tokenInNumeraire is associated with balanceOut and decimalsOut on this scenario?

Based on the if/else, this results in USDC always being associated with tokenInNumeraire, disregard of it being tokenIn or tokenOut.

I know this has been like this for a while and that tests are passing, but I believe it's better to double check 😅

[andreiashu]

I think it's intentional. But I want to confirm with Ernest. Thanks for raising this

[0xAplki]

Hi @brunoguerios , thanks for the feedback. We already modified this based on the current way of things in our FxPool logic. It is already included in this PR.

This used to be another functionality of that helper function prior to optimization. Let us know if this looks good on your end.

[brunoguerios]

Sure thing! Thanks for taking the time to take a look at it 🙏

[brunoguerios]

Overall looks good!
Thanks for taking the time to update to fixed-point math.
I believe there are a few minor improvements that could be done regarding scaling up/down amounts, but I believe we can leave those optimizations to the next version of the SDK/SOR.