-
Notifications
You must be signed in to change notification settings - Fork 624
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.
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
refactor: remove cache context from swap out given in #5176
Changes from all commits
5369d50
c15cc47
b982ed9
161af01
b9bb749
4d69456
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1442,7 +1442,7 @@ var ( | |
} | ||
) | ||
|
||
func (s *KeeperTestSuite) TestCalcAndSwapOutAmtGivenIn() { | ||
func (s *KeeperTestSuite) TestComputeAndSwapOutAmtGivenIn() { | ||
tests := make(map[string]SwapTest, len(swapOutGivenInCases)+len(swapOutGivenInFeeCases)+len(swapOutGivenInErrorCases)) | ||
for name, test := range swapOutGivenInCases { | ||
tests[name] = test | ||
|
@@ -1484,11 +1484,17 @@ func (s *KeeperTestSuite) TestCalcAndSwapOutAmtGivenIn() { | |
poolBeforeCalc, err := s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId()) | ||
s.Require().NoError(err) | ||
|
||
// perform calc | ||
_, tokenIn, tokenOut, updatedTick, updatedLiquidity, sqrtPrice, err := s.App.ConcentratedLiquidityKeeper.CalcOutAmtGivenInInternal( | ||
s.Ctx, | ||
// Refetch the pool | ||
pool, err = s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId()) | ||
s.Require().NoError(err) | ||
|
||
// perform compute | ||
cacheCtx, _ := s.Ctx.CacheContext() | ||
tokenIn, tokenOut, updatedTick, updatedLiquidity, sqrtPrice, err := s.App.ConcentratedLiquidityKeeper.ComputeOutAmtGivenIn( | ||
cacheCtx, | ||
pool.GetId(), | ||
test.tokenIn, test.tokenOutDenom, | ||
test.swapFee, test.priceLimit, pool.GetId()) | ||
test.swapFee, test.priceLimit) | ||
|
||
if test.expectErr { | ||
s.Require().Error(err) | ||
|
@@ -2278,9 +2284,9 @@ func (s *KeeperTestSuite) TestSwapExactAmountOut() { | |
} | ||
} | ||
|
||
// TestCalcOutAmtGivenInWriteCtx tests that writeCtx successfully performs state changes as expected. | ||
// We expect writeCtx to only change fee accum state, since pool state change is not handled via writeCtx function. | ||
func (s *KeeperTestSuite) TestCalcOutAmtGivenInWriteCtx() { | ||
// TestComputeOutAmtGivenIn tests that ComputeOutAmtGivenIn successfully performs state changes as expected. | ||
// We expect to only change fee accum state, since pool state change is not handled by ComputeOutAmtGivenIn. | ||
func (s *KeeperTestSuite) TestComputeOutAmtGivenIn() { | ||
// we only use fee cases here since write Ctx only takes effect in the fee accumulator | ||
tests := make(map[string]SwapTest, len(swapOutGivenInFeeCases)) | ||
|
||
|
@@ -2316,11 +2322,11 @@ func (s *KeeperTestSuite) TestCalcOutAmtGivenInWriteCtx() { | |
s.Require().NoError(err) | ||
|
||
// perform calc | ||
writeCtx, _, _, _, _, _, err := s.App.ConcentratedLiquidityKeeper.CalcOutAmtGivenInInternal( | ||
_, _, _, _, _, err = s.App.ConcentratedLiquidityKeeper.ComputeOutAmtGivenIn( | ||
s.Ctx, | ||
pool.GetId(), | ||
test.tokenIn, test.tokenOutDenom, | ||
test.swapFee, test.priceLimit, pool.GetId()) | ||
s.Require().NoError(err) | ||
test.swapFee, test.priceLimit) | ||
|
||
// check that the pool has not been modified after performing calc | ||
poolAfterCalc, err := s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId()) | ||
|
@@ -2331,28 +2337,81 @@ func (s *KeeperTestSuite) TestCalcOutAmtGivenInWriteCtx() { | |
s.Require().Equal(poolBeforeCalc.GetLiquidity(), poolAfterCalc.GetLiquidity()) | ||
s.Require().Equal(poolBeforeCalc.GetTickSpacing(), poolAfterCalc.GetTickSpacing()) | ||
|
||
// check that fee accum has been correctly updated. | ||
feeAccum, err := s.App.ConcentratedLiquidityKeeper.GetFeeAccumulator(s.Ctx, 1) | ||
s.Require().NoError(err) | ||
|
||
feeAccumValue := feeAccum.GetValue() | ||
s.Require().Equal(0, feeAccumValue.Len()) | ||
s.Require().Equal(1, | ||
s.Require().Equal(1, feeAccumValue.Len()) | ||
s.Require().Equal(0, | ||
Comment on lines
+2345
to
+2346
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't really get why this changed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Previously, this was a non-mutative method. As a result, we expected no accumulator updates. It would only mutate upon |
||
additiveFeeGrowthGlobalErrTolerance.CompareBigDec( | ||
osmomath.BigDecFromSDKDec(test.expectedFeeGrowthAccumulatorValue), | ||
osmomath.BigDecFromSDKDec(feeAccum.GetValue().AmountOf(test.tokenIn.Denom)), | ||
), | ||
) | ||
}) | ||
} | ||
} | ||
|
||
// System under test | ||
writeCtx() | ||
// TestCalcOutAmtGivenIn_NonMutative tests that CalcOutAmtGivenIn is non-mutative. | ||
func (s *KeeperTestSuite) TestCalcOutAmtGivenIn_NonMutative() { | ||
// we only use fee cases here since write Ctx only takes effect in the fee accumulator | ||
tests := make(map[string]SwapTest, len(swapOutGivenInFeeCases)) | ||
|
||
// now we check that fee accum has been correctly updated upon writeCtx | ||
feeAccum, err = s.App.ConcentratedLiquidityKeeper.GetFeeAccumulator(s.Ctx, 1) | ||
for name, test := range swapOutGivenInFeeCases { | ||
tests[name] = test | ||
} | ||
|
||
for name, test := range tests { | ||
test := test | ||
s.Run(name, func() { | ||
s.SetupTest() | ||
s.FundAcc(s.TestAccs[0], sdk.NewCoins(sdk.NewCoin("eth", sdk.NewInt(10000000000000)), sdk.NewCoin("usdc", sdk.NewInt(1000000000000)))) | ||
s.FundAcc(s.TestAccs[1], sdk.NewCoins(sdk.NewCoin("eth", sdk.NewInt(10000000000000)), sdk.NewCoin("usdc", sdk.NewInt(1000000000000)))) | ||
|
||
// Create default CL pool | ||
pool := s.PrepareConcentratedPool() | ||
|
||
// add default position | ||
s.SetupDefaultPosition(pool.GetId()) | ||
|
||
// add second position depending on the test | ||
if !test.secondPositionLowerPrice.IsNil() { | ||
newLowerTick, err := math.PriceToTickRoundDown(test.secondPositionLowerPrice, pool.GetTickSpacing()) | ||
s.Require().NoError(err) | ||
newUpperTick, err := math.PriceToTickRoundDown(test.secondPositionUpperPrice, pool.GetTickSpacing()) | ||
s.Require().NoError(err) | ||
|
||
_, _, _, _, _, _, _, err = s.App.ConcentratedLiquidityKeeper.CreatePosition(s.Ctx, pool.GetId(), s.TestAccs[1], DefaultCoins, sdk.ZeroInt(), sdk.ZeroInt(), newLowerTick.Int64(), newUpperTick.Int64()) | ||
s.Require().NoError(err) | ||
} | ||
|
||
poolBeforeCalc, err := s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId()) | ||
s.Require().NoError(err) | ||
|
||
feeAccumValue = feeAccum.GetValue() | ||
s.Require().Equal(1, feeAccumValue.Len()) | ||
s.Require().Equal(0, | ||
// perform calc | ||
_, err = s.App.ConcentratedLiquidityKeeper.CalcOutAmtGivenIn( | ||
s.Ctx, | ||
poolBeforeCalc, | ||
test.tokenIn, test.tokenOutDenom, | ||
test.swapFee) | ||
s.Require().NoError(err) | ||
|
||
// check that the pool has not been modified after performing calc | ||
poolAfterCalc, err := s.App.ConcentratedLiquidityKeeper.GetPoolById(s.Ctx, pool.GetId()) | ||
s.Require().NoError(err) | ||
|
||
s.Require().Equal(poolBeforeCalc.GetCurrentSqrtPrice(), poolAfterCalc.GetCurrentSqrtPrice()) | ||
s.Require().Equal(poolBeforeCalc.GetCurrentTick(), poolAfterCalc.GetCurrentTick()) | ||
s.Require().Equal(poolBeforeCalc.GetLiquidity(), poolAfterCalc.GetLiquidity()) | ||
s.Require().Equal(poolBeforeCalc.GetTickSpacing(), poolAfterCalc.GetTickSpacing()) | ||
|
||
feeAccum, err := s.App.ConcentratedLiquidityKeeper.GetFeeAccumulator(s.Ctx, 1) | ||
s.Require().NoError(err) | ||
|
||
feeAccumValue := feeAccum.GetValue() | ||
s.Require().Equal(0, feeAccumValue.Len()) | ||
s.Require().Equal(1, | ||
additiveFeeGrowthGlobalErrTolerance.CompareBigDec( | ||
osmomath.BigDecFromSDKDec(test.expectedFeeGrowthAccumulatorValue), | ||
osmomath.BigDecFromSDKDec(feeAccum.GetValue().AmountOf(test.tokenIn.Denom)), | ||
|
@@ -2876,7 +2935,7 @@ func (s *KeeperTestSuite) TestFunctionalSwaps() { | |
expectedTokenOut := sdk.NewInt(983645) | ||
|
||
// Compare the expected and actual values with a multiplicative tolerance of 0.0001% | ||
s.Require().Equal(0, multiplicativeTolerance.CompareBigDec(expectedSqrtPrice, actualSqrtPrice)) | ||
s.Require().Equal(0, multiplicativeTolerance.CompareBigDec(expectedSqrtPrice, actualSqrtPrice), "expected sqrt price: %s, actual sqrt price: %s", expectedSqrtPrice, actualSqrtPrice) | ||
s.Require().Equal(0, multiplicativeTolerance.Compare(expectedTokenIn, totalTokenIn.Amount)) | ||
s.Require().Equal(0, multiplicativeTolerance.Compare(expectedTokenOut, totalTokenOut.Amount)) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should be calling and testing CalcOutAmtGivenIn here instead of directly testing and calling computeOutAmtGivenIn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test name had to be changed, good catch. Done