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

Test suite improvements #390

Open
4 of 10 tasks
snreynolds opened this issue Nov 7, 2023 · 12 comments
Open
4 of 10 tasks

Test suite improvements #390

snreynolds opened this issue Nov 7, 2023 · 12 comments
Assignees
Labels

Comments

@snreynolds
Copy link
Member

snreynolds commented Nov 7, 2023

Component

General design optimization (improving efficiency, cleanliness, or developer experience)

Describe the suggested feature and problem it solves.

Deployers contract that we use in the testing suite has gotten a little bloated. I think we should clean this up and have a standard way of approving tokens and creating a new pool, and then enforce a standard way to perform all operations on top of that: adding liquidity/swapping/donating.

  • There are currently 2 ways we are deploying and minting currencies, one with a TokenFixture and one through Deployers. We should standardize 1) how we deploy currencies and 2) how to create pools from those currencies OR from new currencies handled outside the default deploy.
  • flow for a "default" pool with added liquidity
  • use updated test names test_{function}_{case} and gas or fuzz prefix added in the {case} section if its a gas or fuzz test
  • standardize snapshot names (no spaces, maybe something like : testFile.testName)
  • Separate the gas tests so we can have some that do include take/settle and some that are just the raw call
  • Update all uses of vm.expectRevert() to specify the selector of the error it should be throwing
  • Add explicit settle testing
  • Improve dynamic fee tests - currently they dont explicitly make it clear that the fee is changing with asserts
  • Lots of asserts missing on all swap tests - like the amount actually swapped
  • Helper function to set up a pool, approve the modifyPositionRouter for tokens, and give the pool initial liquidity

Describe the desired implementation.

No response

Describe alternatives.

No response

Additional context.

No response

@hensha256
Copy link
Contributor

Things I'd like to see improved

  • Lots of asserts missing on all swap tests - like the amount actually swapped
  • Helper function to set up a pool, approve the modifyPositionRouter for tokens, and give the pool initial liquidity

@hensha256
Copy link
Contributor

  • Add explicit settle testing
  • Improve dynamic fee tests - currently they dont explicitly make it clear that the fee is changing with asserts

@hensha256
Copy link
Contributor

  • Update all uses of vm.expectRevert() to specify the selector of the error it should be throwing

@hensha256
Copy link
Contributor

  • Proper tests for modify position!

@hensha256
Copy link
Contributor

  • Naming the same
  • Separate gas tests

@snreynolds
Copy link
Member Author

Added these notes as checkboxes above

@zhongeric
Copy link
Contributor

FYI we are splitting up modifyPosition tests with work in #402 so hopefully it'll be more fine grained

@hensha256
Copy link
Contributor

Add testing for interactions of NoOp #324 and Access Lock #404 once they are both merged.
For example testing a nested call to swap where the inner call NoOps

@hensha256
Copy link
Contributor

Test all functions revert without a lock

@hensha256 hensha256 changed the title Test suite: Improve Deployers/Set up Flow for pools Test suite improvements Dec 7, 2023
@hensha256
Copy link
Contributor

Remove vm.assume and replace with bound or actually test the other values revert !

@hensha256
Copy link
Contributor

Make a common SWAP_PARAMS in Deployers.sol instead of having
IPoolManager.SwapParams({zeroForOne: true, amountSpecified: -100, sqrtPriceLimitX96: SQRT_RATIO_1_2})
copied everywhere

@hensha256
Copy link
Contributor

helper functions _settle and _take could take a uint instead of an int

@hensha256 hensha256 added tests and removed triage labels Mar 19, 2024
This was referenced Mar 26, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue May 19, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue May 20, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue May 20, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue Jul 14, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue Jul 14, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue Jul 14, 2024
prosperring added a commit to prosperring/v4-core that referenced this issue Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants