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.

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

fix/test(CL Fees): swap out given in fee growth tests; clean up swap tests #4127

Merged
merged 9 commits into from
Jan 30, 2023

Conversation

p0mvn
Copy link
Member

@p0mvn p0mvn commented Jan 28, 2023

Closes: #XXX

What is the purpose of the change

Due to a merge conflict between #4021 and #4097, the fee growth validations were not added.

This PR adds the validations for the fee growth accumulator.

The changes from #4021 interfere with the assertions since they also relate to updating the fee growth accumulator. As a result, they were moved into a separate test fixture called TestSwapOutAmtGivenIn_TickUpdates to enable the expected fee growth validations in the original test.

Lastly, it cleans up the swap test suite. See changelog for details. This clean up is necessary for #4131 and is separated here for ease of review.

Brief Changelog

  • move tick update tests into a separate fixture TestSwapOutAmtGivenIn_TickUpdates
  • enable validation of the fee growth accumulator in TestCalcAndSwapOutAmtGivenIn
  • do the first two steps for swap in given out in preparation for feat/test(CL Fees): Swap fees for in given out #4131
  • group fee-specific tests in swapOutGivenInFeeCases global variable
    • this separation stems from the need to avoid running fee tests in tick updates or inverse tests
  • group all test case fields in struct SwapTest and reuse it where applicable

Testing and Verifying

This change is already covered by existing tests

Documentation and Release Note

  • Does this pull request introduce a new feature or user-facing behavior changes? no
  • Is a relevant changelog entry added to the Unreleased section in CHANGELOG.md? no
  • How is the feature or change documented? not applicable

@p0mvn p0mvn added the V:state/compatible/no_backport State machine compatible PR, depends on prior breaks label Jan 28, 2023
@p0mvn p0mvn requested review from mattverse and czarcas7ic January 28, 2023 07:31
@p0mvn p0mvn marked this pull request as ready for review January 29, 2023 07:19
@p0mvn p0mvn requested a review from AlpinYukseloglu January 29, 2023 07:19
@p0mvn p0mvn marked this pull request as draft January 29, 2023 07:53
@p0mvn p0mvn changed the title fix/test(CL Fees): swap out given in fee growth tests fix/test(CL Fees): swap out given in fee growth tests; clean up swap tests Jan 29, 2023
@@ -581,232 +560,28 @@ var (
expectErr: true,
},
}
)

func (s *KeeperTestSuite) TestCalcAndSwapOutAmtGivenIn() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this is a problem with diff, the test is present down below

@p0mvn p0mvn marked this pull request as ready for review January 29, 2023 16:49
Copy link
Member

@czarcas7ic czarcas7ic left a comment

Choose a reason for hiding this comment

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

Approved with the understanding that we are adding swapInGivenOutFeeCases post fee logic PR. Nicely done, very clean (the diff was super odd but reviewing the final file instead was much easier to grok)

@p0mvn
Copy link
Member Author

p0mvn commented Jan 30, 2023

merging since this is a simple test refactor

@p0mvn p0mvn merged commit 42426cf into main Jan 30, 2023
@p0mvn p0mvn deleted the roman/fix-swap-tests branch January 30, 2023 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/concentrated-liquidity V:state/compatible/no_backport State machine compatible PR, depends on prior breaks
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants