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

Migrate Tick tests from HardHat to Foundry #322

Merged
merged 25 commits into from
Aug 18, 2023
Merged

Migrate Tick tests from HardHat to Foundry #322

merged 25 commits into from
Aug 18, 2023

Conversation

gitcoindev
Copy link
Contributor

Related Issue

This pull request migrates Tick tests from HardHat to Foundry (32 test cases), a part of #315 .

Description of changes

  • migrated 32 Tick tests to Foundry
  • removed HardHat implementation after migration

Note: This PR must be merged after #317 since it uses ./utils/Constants.sol.

Running 32 tests for test/foundry-tests/Tick.t.sol:TickTestTest
[PASS] test_clear_deletesAllTheDataInTheTick() (gas: 56733)
[PASS] test_cross_flipsTheGrowthVariables() (gas: 76504)
[PASS] test_cross_twoFlipsAreNoOp() (gas: 78262)
[PASS] test_getFeeGrowthInside_returns0ForTwoUninitializedTicksIfTickIsAbove() (gas: 81433)
[PASS] test_getFeeGrowthInside_returns0ForTwoUninitializedTicksIfTickIsBelow() (gas: 81354)
[PASS] test_getFeeGrowthInside_returnsAllForTwoUninitializedTicksIfTickIsInside() (gas: 61717)
[PASS] test_getFeeGrowthInside_subtractsLowerTickIfAbove() (gas: 105748)
[PASS] test_getFeeGrowthInside_subtractsUpperAndLowerTickIfInside() (gas: 149809)
[PASS] test_getFeeGrowthInside_subtractsUpperTickIfBelow() (gas: 105746)
[PASS] test_getFeeGrowthInside_worksCorrectlyWithOverflowOnInsideTick() (gas: 149989)
[PASS] test_tickSpacingToMaxLiquidityPerTick_gasCost60TickSpacing() (gas: 5903)
[PASS] test_tickSpacingToMaxLiquidityPerTick_gasCostMaxTickSpacing() (gas: 5886)
[PASS] test_tickSpacingToMaxLiquidityPerTick_gasCostMinTickSpacing() (gas: 5893)
[PASS] test_tickSpacingToMaxLiquidityPerTick_returnsTheCorrectValueFor1() (gas: 7098)
[PASS] test_tickSpacingToMaxLiquidityPerTick_returnsTheCorrectValueFor2302() (gas: 7056)
[PASS] test_tickSpacingToMaxLiquidityPerTick_returnsTheCorrectValueForEntireRange() (gas: 7236)
[PASS] test_tickSpacingToMaxLiquidityPerTick_returnsTheCorrectValueForHighFeeTickSpacing() (gas: 9524)
[PASS] test_tickSpacingToMaxLiquidityPerTick_returnsTheCorrectValueForLowFeeTickSpacing() (gas: 9425)
[PASS] test_tickSpacingToMaxLiquidityPerTick_returnsTheCorrectValueForMediumFeeTickSpacing() (gas: 9491)
[PASS] test_update_assumesAllGrowthHappensBelowTicksLteCurrentTick() (gas: 142130)
[PASS] test_update_doesNotFlipFromNonzeroToGreaterNonzero() (gas: 43294)
[PASS] test_update_doesNotFlipFromNonzeroToLesserZero() (gas: 43371)
[PASS] test_update_doesNotSetAnyGrowthFieldsForTicksGtCurrentTick() (gas: 101654)
[PASS] test_update_doesNotSetAnyGrowthFieldsIfTickIsAlreadyInitialized() (gas: 144881)
[PASS] test_update_flipsFromNonzeroToZero() (gas: 30501)
[PASS] test_update_flipsFromZeroToNonzero() (gas: 40570)
[PASS] test_update_liquidityParsingParsesMaxInt128StoredLiquidityGrossAfterUpdate() (gas: 104229)
[PASS] test_update_liquidityParsingParsesMaxInt128StoredLiquidityGrossBeforeUpdate() (gas: 104137)
[PASS] test_update_liquidityParsingParsesMaxUint128StoredLiquidityGrossAfterUpdate() (gas: 104240)
[PASS] test_update_liquidityParsingParsesMaxUint128StoredLiquidityGrossBeforeUpdate() (gas: 103892)
[PASS] test_update_netsTheLiquidityBasedOnUpperFlag() (gas: 50871)
[PASS] test_update_revertsOnOverflowLiquidityGross() (gas: 46268)
Test result: ok. 32 passed; 0 failed; finished in 3.20s

@gitcoindev gitcoindev changed the title Feature/migrate tick to foundry Migrate Tick tests from HardHat to Foundry Jul 27, 2023
@gitcoindev
Copy link
Contributor Author

Hi @ewilz I also refactored this PR to use forge-gas-snapshot, ready for the review.

@gitcoindev gitcoindev mentioned this pull request Jul 31, 2023
Merged
@gitcoindev
Copy link
Contributor Author

The PR needs changes similar to #317 , in progress...

@gitcoindev gitcoindev force-pushed the feature/migrate_Tick_toFoundry branch from e3bfd50 to 63c0321 Compare July 31, 2023 15:49
@gitcoindev
Copy link
Contributor Author

Hi @ewilz this PR is ready for the next review round, too.

@gitcoindev
Copy link
Contributor Author

Hi @0xb10ckdev thank you for the review, I am on it!

@gitcoindev
foundry-test: Migrate Tick#tickSpacingToMaxLiquidityPerTick test skel…
3ed4fd2 
…eton to Foundry.

Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate Tick#tickSpacingToMaxLiquidityPerTick to Foundry.
0d937e8 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate Tick#getFeeGrowthInside to Foundry.
1545632 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate Tick#update to Foundry.
5f2e9ae 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate liquidity parsing Tick#update to Foundry.
2274265 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate Tick#clear to Foundry.
34db7e3 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate Tick#cross to Foundry.
fddb086 
Ref: Uniswap#315
@gitcoindev
foundry-test: Refactor Tick#tickSpacingToMaxLiquidityPerTick to use
bdc4841 
MIN_TICK and MAX_TICK constants.

Ref: Uniswap#315
@gitcoindev
test: Remove migrated Tick from HardHat to Foundry.
6512d6f 
Ref: Uniswap#315
@gitcoindev
foundry-test: Tick: Use forge-gas-snapshot to snapshot gas used.
600ef19
@gitcoindev
test: Tick: Call library directly instead of test contract.
f09f850 
The test contract was a middle layer between library and was used by
javascript / HardHat tests. Due to the middle layer using Foundry,
gas cost usage was calculated incorrectly.

Generate correct gas snapshots using direct library calls.
Delete unnecessary comments.

Functions moved and internalized from TickTest.sol:

* ticks()
* tickSpacingToMaxLiquidityPerTick()
* setTick(),
* getFeeGrowthInside()
* update()
* clear()

Ref: Uniswap#315
@gitcoindev
test: Update Tick Foundry gas snapshots.
7261dc6 
Ref: Uniswap#315
@gitcoindev gitcoindev force-pushed the feature/migrate_Tick_toFoundry branch from 577986d to 7261dc6 Compare August 10, 2023 07:12
@gitcoindev
Copy link
Contributor Author

Hi @0xb10ckdev I rebased to the latest main branch and applied all suggestions. PR is ready for another review round.

@ewilz
Copy link
Member

ewilz commented Aug 17, 2023 •
edited
Loading

Need to delete js snapshots & TIckTest.sol + could delete TickEchidnaTest and convert it into forge fuzz test

@gitcoindev
Copy link
Contributor Author

Need to delete js snapshots & TIckTest.sol + could delete TickEchidnaTest and convert it into forge fuzz test

I am on it.

@gitcoindev
Copy link
Contributor Author

Hi @ewilz the PR is ready for another round of the review.

@gitcoindev gitcoindev requested a review from ewilz August 18, 2023 10:01
@ewilz
Copy link
Member

ewilz commented Aug 18, 2023

pushed up some small changes to avoid annoying churn. Thanks so much for all the work you've been doing on testing!!!

@ewilz ewilz merged commit 7393880 into Uniswap:main Aug 18, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewilz ewilz ewilz approved these changes

@0xb10ckdev 0xb10ckdev Awaiting requested review from 0xb10ckdev

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gitcoindev @ewilz @0xb10ckdev