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.

Sign up for GitHub

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

Jump to bottom

FxPool rounding errors fixes #393

Merged
merged 57 commits into from
Jul 4, 2023
Merged

FxPool rounding errors fixes #393

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
57 commits
Select commit Hold shift + click to select a range
bf44477
WIP fxPool rounding error
andreiashu May 25, 2023
37b6ef3
fixed regression in xaveFxPool.spec.ts
andreiashu May 26, 2023
e9f50e1
🍜 minor amends; removed obsolete bnum conversions; rearranged checks …
andreiashu May 29, 2023
c1f880b
🎐 wip
andreiashu May 31, 2023
1ab80f6
wip; broken branch for now
andreiashu May 31, 2023
a44635b
⛽ fixed 10 wei precision issue; still wip;
andreiashu Jun 2, 2023
f0b3516
breaking polygon test
0xAplki Jun 6, 2023
519d418
🍅 fix the subgraph values. Tests passing on Polygon
andreiashu Jun 8, 2023
29f16c1
added new values for subgraph return
0xAplki Jun 8, 2023
7b753bd
🏈 code formatting
andreiashu Jun 10, 2023
fdeb41b
🍿 minor changes: code reuse
andreiashu Jun 10, 2023
7a79166
🦄 added documentation
andreiashu Jun 10, 2023
c1f4c35
Removed obsolete documentation
andreiashu Jun 11, 2023
91a3076
⛑ switched back to `ALCHEMY_URL`; fixed one test in `xaveFxPool.spec.ts`
andreiashu Jun 12, 2023
b9d6c90
🎣 fixed several test cases; still wip
andreiashu Jun 14, 2023
6e35042
🎣 added tests for expectedSwapOutput values
andreiashu Jun 14, 2023
4e736d8
🎤 minor changes
andreiashu Jun 14, 2023
ba76227
⚓ added _inHigherPrecision function to workaround the SOR limitation …
andreiashu Jun 14, 2023
559e0bd
⛄ pull from upstream/develop
andreiashu Jun 14, 2023
820bf11
🏈 lint fixes
andreiashu Jun 14, 2023
adc7836
🍁 added xaveFxPool.math.spec.ts
andreiashu Jun 14, 2023
c33c58c
🌳 removed debug module
andreiashu Jun 14, 2023
e464e24
🌳 removed debug module
andreiashu Jun 14, 2023
19ad8be
🚗 removed FXPoolMath tests from integration
andreiashu Jun 14, 2023
49094af
🐙 removed code change from `src/pools/index.ts` (oops)
andreiashu Jun 14, 2023
09253c6
🍳 enabled FX pool type
andreiashu Jun 15, 2023
a965343
🍵 xavePool run tests against polygon fork; better organize test files;
andreiashu Jun 15, 2023
c578d17
🐟 fixed bug where OldBigNumber.config.DECIMAL_PLACES was left as 36 i…
andreiashu Jun 15, 2023
4d98aa8
💻 removed comment about innacuracy test since it is irrelevant now
andreiashu Jun 15, 2023
2576c4d
test cases + bug
0xAplki Jun 20, 2023
c2590b0
🦩 new fxpool test cases
0xAplki Jun 21, 2023
af3eb5c
🍎 removed rounding of values in xaveFxPool.spec.ts; the values for `e…
andreiashu Jun 22, 2023
61a9774
🐨 fixed bug whereby the way we calculated our derivative would have p…
andreiashu Jun 23, 2023
f62a047
🐨 lint fixes
andreiashu Jun 23, 2023
9c82d4b
⛰ removed obsolete comments
andreiashu Jun 23, 2023
60071d9
💾 do not use floats in calculations; found a place where we were roun…
andreiashu Jun 23, 2023
090466f
⛏ viewRawAmount and viewNumeraireAmount using BigInt internally
andreiashu Jun 26, 2023
5e537cc
🐼 calculateMicroFee is using BigNumber now
andreiashu Jun 26, 2023
2f8f87b
😮 oops I deleted some of the test cases by mistake. fixed now
andreiashu Jun 26, 2023
94b2616
🐬 calculateTrade uses BigNumber internally
andreiashu Jun 26, 2023
71ab58b
👽 alpha, beta, delta, epsilon, viewRawAmount, viewNumeraire using Big…
andreiashu Jun 26, 2023
ac20365
tokenOutLatestFXPrice and tokenInLatestFXPrice are BigNumber now
andreiashu Jun 27, 2023
a3d7557
⚽ amount -> amount_36 [wip]
andreiashu Jun 27, 2023
2673734
🎻 viewRawAmount amount -> amount_36
andreiashu Jun 27, 2023
83cc506
ParsedFxPoolData uses BigNumber
andreiashu Jun 27, 2023
b90f725
🖖 spotPriceBeforeSwap uses BigNumber amount
andreiashu Jun 27, 2023
4635ed9
🚣 params to param_64
andreiashu Jun 27, 2023
e5b7a8d
_derivativeSpotPriceAfterSwapTokenInForExactTokenOut and _derivativeS…
andreiashu Jun 27, 2023
3471d7d
🐺 viewRawAmount and spotPriceBeforeSwap return BigNumber
andreiashu Jun 27, 2023
edc0b95
🍳bnum(10).pow(36) -> OLD_ONE_36
andreiashu Jun 27, 2023
2350377
⛰ default to 8 for `fxOracleDecimals`
andreiashu Jun 27, 2023
1e0d545
🍜 removed redundant operations; thanks @brunoguerios
andreiashu Jun 28, 2023
2cf04c2
🏺 parseFixedCurveParam test
andreiashu Jun 28, 2023
1bbc298
🙅🏻‍♂️ removed the conditional that was previously used on other funct…
0xAplki Jun 29, 2023
3d347f5
🍳 removed the rounding down from spot price related functions;
andreiashu Jul 3, 2023
7191c7b
⛲ formatting
andreiashu Jul 3, 2023
6a15ae4
🍎 reverted the changes in `_derivativeSpotPriceAfterSwapExactTokenInF…
andreiashu Jul 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 121 additions & 42 deletions src/pools/xaveFxPool/fxPool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { getAddress } from '@ethersproject/address';
import { BigNumber, formatFixed, parseFixed } from '@ethersproject/bignumber';
import { Zero } from '@ethersproject/constants';
import { BigNumber as OldBigNumber, ZERO, bnum } from '../../utils/bignumber';

import { parseFixedCurveParam } from './parseFixedCurveParam';
import { isSameAddress } from '../../utils';
import { universalNormalizedLiquidity } from '../liquidity';
import {
Expand Down Expand Up @@ -29,13 +31,15 @@ type FxPoolToken = Pick<
>;

export type FxPoolPairData = PoolPairBase & {
alpha: BigNumber;
beta: BigNumber;
lambda: BigNumber;
delta: BigNumber;
epsilon: BigNumber;
alpha: OldBigNumber;
beta: OldBigNumber;
lambda: OldBigNumber;
delta: OldBigNumber;
epsilon: OldBigNumber;
tokenInLatestFXPrice: OldBigNumber;
tokenInfxOracleDecimals: OldBigNumber;
tokenOutLatestFXPrice: OldBigNumber;
tokenOutfxOracleDecimals: OldBigNumber;
};

export class FxPool implements PoolBase<FxPoolPairData> {
Expand All @@ -46,11 +50,11 @@ export class FxPool implements PoolBase<FxPoolPairData> {
totalShares: BigNumber;
tokens: FxPoolToken[];
tokensList: string[];
alpha: BigNumber;
beta: BigNumber;
lambda: BigNumber;
delta: BigNumber;
epsilon: BigNumber;
alpha: OldBigNumber;
beta: OldBigNumber;
lambda: OldBigNumber;
delta: OldBigNumber;
epsilon: OldBigNumber;

static fromPool(pool: SubgraphPoolBase): FxPool {
if (
Expand Down Expand Up @@ -95,11 +99,11 @@ export class FxPool implements PoolBase<FxPoolPairData> {
this.totalShares = parseFixed(totalShares, 18);
this.tokens = tokens;
this.tokensList = tokensList;
this.alpha = parseFixed(alpha, 18);
this.beta = parseFixed(beta, 18);
this.lambda = parseFixed(lambda, 18);
this.delta = parseFixed(delta, 18);
this.epsilon = parseFixed(epsilon, 18);
this.alpha = parseFixedCurveParam(alpha);
this.beta = parseFixedCurveParam(beta);
this.lambda = parseFixedCurveParam(lambda);
this.delta = parseFixedCurveParam(delta);
this.epsilon = parseFixedCurveParam(epsilon);
}
updateTotalShares: (newTotalShares: BigNumber) => void;
mainIndex?: number | undefined;
Expand Down Expand Up @@ -137,6 +141,8 @@ export class FxPool implements PoolBase<FxPoolPairData> {

if (!tO.token?.latestFXPrice || !tI.token?.latestFXPrice)
throw 'FX Pool Missing LatestFxPrice';
if (!tO.token?.fxOracleDecimals || !tI.token?.fxOracleDecimals)
throw 'FX Pool Missing tokenIn or tokenOut fxOracleDecimals';

const poolPairData: FxPoolPairData = {
id: this.id,
Expand All @@ -154,8 +160,20 @@ export class FxPool implements PoolBase<FxPoolPairData> {
lambda: this.lambda,
delta: this.delta,
epsilon: this.epsilon,
tokenInLatestFXPrice: bnum(tI.token.latestFXPrice), // decimals is formatted from subgraph in rate we get from the chainlink oracle
tokenOutLatestFXPrice: bnum(tO.token.latestFXPrice), // decimals is formatted from subgraph in rate we get from the chainlink oracle
tokenInLatestFXPrice: bnum(
parseFixed(
tI.token.latestFXPrice,
tI.token.fxOracleDecimals
).toString()
), // decimals is formatted from subgraph in rate we get from the chainlink oracle
tokenOutLatestFXPrice: bnum(
parseFixed(
tO.token.latestFXPrice,
tO.token.fxOracleDecimals
).toString()
), // decimals is formatted from subgraph in rate we get from the chainlink oracle
tokenInfxOracleDecimals: bnum(tI.token.fxOracleDecimals),
tokenOutfxOracleDecimals: bnum(tO.token.fxOracleDecimals),
};

return poolPairData;
Expand All @@ -181,34 +199,50 @@ export class FxPool implements PoolBase<FxPoolPairData> {
getLimitAmountSwap(
poolPairData: FxPoolPairData,
swapType: SwapTypes
): OldBigNumber {
return this._inHigherPrecision(
this._getLimitAmountSwap,
poolPairData,
swapType
);
}

_getLimitAmountSwap(
poolPairData: FxPoolPairData,
swapType: SwapTypes
): OldBigNumber {
try {
const parsedReserves = poolBalancesToNumeraire(poolPairData);

const alphaValue = Number(formatFixed(poolPairData.alpha, 18));
const alphaValue = poolPairData.alpha.div(bnum(10).pow(18));

const maxLimit = (1 + alphaValue) * parsedReserves._oGLiq * 0.5;
const maxLimit = alphaValue
.plus(1)
.times(parsedReserves._oGLiq)
.times(0.5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor Author

@andreiashu andreiashu Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@brunoguerios brunoguerios Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😃

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


if (swapType === SwapTypes.SwapExactIn) {
const maxLimitAmount =
maxLimit - parsedReserves.tokenInReservesInNumeraire;

return bnum(
viewRawAmount(
maxLimitAmount,
poolPairData.tokenInLatestFXPrice.toNumber()
).toString()
const maxLimitAmount = maxLimit.minus(
parsedReserves.tokenInReservesInNumeraire
);
} else {
const maxLimitAmount =
maxLimit - parsedReserves.tokenOutReservesInNumeraire;

return bnum(
viewRawAmount(
maxLimitAmount,
poolPairData.tokenOutLatestFXPrice.toNumber()
).toString()
return viewRawAmount(
maxLimitAmount,
bnum(poolPairData.decimalsIn),
poolPairData.tokenInLatestFXPrice,
poolPairData.tokenInfxOracleDecimals
).div(bnum(10).pow(poolPairData.decimalsIn));
} else {
const maxLimitAmount = maxLimit.minus(
parsedReserves.tokenOutReservesInNumeraire
);

return viewRawAmount(
maxLimitAmount,
bnum(poolPairData.decimalsOut),
poolPairData.tokenOutLatestFXPrice,
poolPairData.tokenOutfxOracleDecimals
).div(bnum(10).pow(poolPairData.decimalsOut));
}
} catch {
return ZERO;
Expand All @@ -233,8 +267,12 @@ export class FxPool implements PoolBase<FxPoolPairData> {
amount: OldBigNumber
): OldBigNumber {
try {
return _exactTokenInForTokenOut(amount, poolPairData);
} catch {
return this._inHigherPrecision(
_exactTokenInForTokenOut,
amount,
poolPairData
);
Comment on lines +280 to +284
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Screenshot 2023-06-28 at 11 26 00

} catch (e) {
return ZERO;
}
}
Expand All @@ -244,7 +282,11 @@ export class FxPool implements PoolBase<FxPoolPairData> {
amount: OldBigNumber
): OldBigNumber {
try {
return _tokenInForExactTokenOut(amount, poolPairData);
return this._inHigherPrecision(
_tokenInForExactTokenOut,
amount,
poolPairData
);
} catch {
return ZERO;
}
Expand All @@ -255,7 +297,8 @@ export class FxPool implements PoolBase<FxPoolPairData> {
amount: OldBigNumber
): OldBigNumber {
try {
return _spotPriceAfterSwapExactTokenInForTokenOut(
return this._inHigherPrecision(
_spotPriceAfterSwapExactTokenInForTokenOut,
poolPairData,
amount
);
Expand All @@ -269,7 +312,8 @@ export class FxPool implements PoolBase<FxPoolPairData> {
amount: OldBigNumber
): OldBigNumber {
try {
return _spotPriceAfterSwapTokenInForExactTokenOut(
return this._inHigherPrecision(
_spotPriceAfterSwapTokenInForExactTokenOut,
poolPairData,
amount
);
Expand All @@ -283,7 +327,8 @@ export class FxPool implements PoolBase<FxPoolPairData> {
amount: OldBigNumber
): OldBigNumber {
try {
return _derivativeSpotPriceAfterSwapExactTokenInForTokenOut(
return this._inHigherPrecision(
_derivativeSpotPriceAfterSwapExactTokenInForTokenOut,
amount,
poolPairData
);
Expand All @@ -297,12 +342,46 @@ export class FxPool implements PoolBase<FxPoolPairData> {
amount: OldBigNumber
): OldBigNumber {
try {
return _derivativeSpotPriceAfterSwapTokenInForExactTokenOut(
return this._inHigherPrecision(
_derivativeSpotPriceAfterSwapTokenInForExactTokenOut,
amount,
poolPairData
);
} catch {
return ZERO;
}
}

/**
* Runs the given function with the BigNumber config set to 36 decimals.
* This is needed since in the Solidity code we use 64.64 fixed point numbers
* for the curve math operations. This makes the SOR default of 18 decimals
* not enough.
*
* @param funcName
* @param args
* @returns
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/ban-types
_inHigherPrecision(funcName: Function, ...args: any[]): OldBigNumber {
const prevDecimalPlaces = OldBigNumber.config({}).DECIMAL_PLACES;
OldBigNumber.config({
DECIMAL_PLACES: 36,
});

try {
const val = funcName.apply(this, args);
OldBigNumber.config({
DECIMAL_PLACES: prevDecimalPlaces,
});
return val;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
} catch (err: any) {
// restore the original BigNumber config even in case of an exception
OldBigNumber.config({
DECIMAL_PLACES: prevDecimalPlaces,
});
throw err;
}
}
}
Loading