Test suite improvements

[snreynolds]

trafficstars

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.

• [x] 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.
• [x] 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
• [x] 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
• [x] 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]

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]

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

[hensha256]

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

[hensha256]

• Proper tests for modify position!

[hensha256]

• Naming the same
• Separate gas tests

[snreynolds]

Added these notes as checkboxes above

[zhongeric]

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

[hensha256]

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]

Test all functions revert without a lock

[hensha256]

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

[hensha256]

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]

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