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 SqrtPriceMath tests from HardHat to Foundry #317

Merged
merged 15 commits into from
Jul 31, 2023
Merged

Migrate SqrtPriceMath tests from HardHat to Foundry #317

merged 15 commits into from
Jul 31, 2023

Conversation

gitcoindev
Copy link
Contributor

Related Issue

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

Description of changes

  • migrated 41 SqrtPriceMath tests to Foundry
  • refactored constants to separate library to be re-used by multiple tests
  • removed HardHat implementation after migratiopn
Running 41 tests for test/foundry-tests/SqrtPriceMath.t.sol:SqrtPriceMathTestTest
[PASS] test_getAmount0Delta_gasCostForAmount0WhereRoundUpIsFalse() (gas: 6795)
[PASS] test_getAmount0Delta_gasCostForAmount0WhereRoundUpIsTrue() (gas: 6959)
[PASS] test_getAmount0Delta_returns0IfLiquidityIs0() (gas: 6634)
[PASS] test_getAmount0Delta_returns0IfPricesAreEqual() (gas: 6634)
[PASS] test_getAmount0Delta_returns0_1Amount1ForPriceOf1To1_21() (gas: 8814)
[PASS] test_getAmount0Delta_worksForPricesThatOverflow() (gas: 9380)
[PASS] test_getAmount1Delta_gasCostForAmount1WhereRoundUpIsFalse() (gas: 6805)
[PASS] test_getAmount1Delta_gasCostForAmount1WhereRoundUpIsTrue() (gas: 6902)
[PASS] test_getAmount1Delta_returns0IfLiquidityIs0() (gas: 6635)
[PASS] test_getAmount1Delta_returns0IfPricesAreEqual() (gas: 6623)
[PASS] test_getAmount1Delta_returns0_1Amount1ForPriceOf1To1_21() (gas: 8855)
[PASS] test_getNextSqrtPriceFromInput_amountInGreaterThanType_uint96_maxAndZeroForOneEqualsTrue() (gas: 6996)
[PASS] test_getNextSqrtPriceFromInput_anyInputAmountCannotUnderflowThePrice() (gas: 6852)
[PASS] test_getNextSqrtPriceFromInput_canReturn1WithEnoughAmountInAndZeroForOneEqualsTrue() (gas: 6777)
[PASS] test_getNextSqrtPriceFromInput_inputAmountOf0_1Currency0() (gas: 7222)
[PASS] test_getNextSqrtPriceFromInput_inputAmountOf0_1Currency1() (gas: 7036)
[PASS] test_getNextSqrtPriceFromInput_returnsInputPriceIfAmountInIsZeroAndZeroForOneEqualsFalse() (gas: 6658)
[PASS] test_getNextSqrtPriceFromInput_returnsInputPriceIfAmountInIsZeroAndZeroForOneEqualsTrue() (gas: 6400)
[PASS] test_getNextSqrtPriceFromInput_returnsTheMinimumPriceForMaxInputs() (gas: 6925)
[PASS] test_getNextSqrtPriceFromInput_revertsIfInputAmountOverflowsThePrice() (gas: 9303)
[PASS] test_getNextSqrtPriceFromInput_revertsIfLiquidityIsZero() (gas: 9116)
[PASS] test_getNextSqrtPriceFromInput_revertsIfPriceIsZero() (gas: 9129)
[PASS] test_getNextSqrtPriceFromInput_zeroForOneEqualsFalseGas() (gas: 6980)
[PASS] test_getNextSqrtPriceFromInput_zeroForOneEqualsTrueGas() (gas: 7155)
[PASS] test_getNextSqrtPriceFromOutput_outputAmountOf0_1Currency0() (gas: 6944)
[PASS] test_getNextSqrtPriceFromOutput_outputAmountOf0_1Currency1() (gas: 7298)
[PASS] test_getNextSqrtPriceFromOutput_puzzlingEchidnaTest() (gas: 9101)
[PASS] test_getNextSqrtPriceFromOutput_returnsInputPriceIfAmountInIsZeroAndZeroForOneEqualsFalse() (gas: 6627)
[PASS] test_getNextSqrtPriceFromOutput_returnsInputPriceIfAmountInIsZeroAndZeroForOneEqualsTrue() (gas: 6869)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfAmountOutIsImpossibleInOneForZeroDirection() (gas: 9120)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfAmountOutIsImpossibleInZeroForOneDirection() (gas: 9165)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfLiquidityIsZero() (gas: 9137)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfOutputAmountIsExactlyTheVirtualReservesOfCurrency0() (gas: 9101)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfOutputAmountIsExactlyTheVirtualReservesOfCurrency1() (gas: 9165)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfOutputAmountIsGreaterThanTheVirtualReservesOfCurrency0() (gas: 9152)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfOutputAmountIsGreaterThanTheVirtualReservesOfCurrency1() (gas: 9165)
[PASS] test_getNextSqrtPriceFromOutput_revertsIfPriceIsZero() (gas: 9085)
[PASS] test_getNextSqrtPriceFromOutput_succeedsIfOutputAmountIsJustLessThanTheVirtualReservesOfCurrency1() (gas: 6579)
[PASS] test_getNextSqrtPriceFromOutput_zeroForOneEqualsFalseGas() (gas: 7285)
[PASS] test_getNextSqrtPriceFromOutput_zeroForOneEqualsTrueGas() (gas: 6968)
[PASS] test_swapComputation_sqrtPTimessqrtQOverflows() (gas: 9374)
Test result: ok. 41 passed; 0 failed; finished in 4.84ms

@gitcoindev gitcoindev force-pushed the feature/migrate_SqrtPriceMath branch from ab5bcd1 to f7bce47 Compare July 25, 2023 12:35
@gitcoindev
Copy link
Contributor Author

I force pushed the branch after signing commits, no changes were made. PR ready for the review.

@gitcoindev
foundry-test: Refactor constants to a library.
4ff1283 
The constants library will allow to directly access constants from
multiple Foundry tests.

Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate SqrtPriceMath#getNextSqrtPriceFromInput to
1cc5abd 
Foundry.

Ref: Uniswap#315
@gitcoindev
foundry-test: Move MAX_UINT128 and MAX_UINT256 to Constants.
d8678d3 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate SqrtPriceMath#getNextSqrtPriceFromOutput to Fou…
488137a 
…ndry.

Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate SqrtPriceMath#getAmount0Delta to Foundry.
83e3f25 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate SqrtPriceMath#getAmount1Delta to Foundry.
64e147c 
Ref: Uniswap#315
@gitcoindev
foundry-test: Migrate SqrtPriceMath#swapComputation to Foundry.
3b44c34 
Ref: Uniswap#315
@gitcoindev
test: Remove migrated SqrtPriceMath from HardHat to Foundry.
3e2eb6e 
Ref: Uniswap#315
@gitcoindev gitcoindev force-pushed the feature/migrate_SqrtPriceMath branch from f7bce47 to 3e2eb6e Compare July 25, 2023 19:11
@gitcoindev
Copy link
Contributor Author

Rebased to latest main branch.

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

hi @ewilz I applied the suggested changes, the pr is ready for another review round.

@ewilz
Copy link
Member

ewilz commented Jul 31, 2023

cool! can you please delete test/_snapshots_ run yarn run snapshots to regenerate that folder to account for the snaps you deleted, and also run forge test to generate the forge snapshots?

@gitcoindev
Copy link
Contributor Author

cool! can you please delete test/_snapshots_ run yarn run snapshots to regenerate that folder to account for the snaps you deleted, and also run forge test to generate the forge snapshots?

done added at a72796e

@gitcoindev
Copy link
Contributor Author

aha! I noticed one copy-paste error, as one snapshot was duplicated, fixing asap

@gitcoindev
test: Update correct SqrtPriceMath gas snapshot for
c56e765 
getNextSqrtPriceFromInput_zeroForOneEqualsFalseGas.
@gitcoindev
Copy link
Contributor Author

@ewilz new forge gas snapshots were generated and pushed, PR is ready for the new review round.

@gitcoindev
test: SqrtPriceMath: Call library directly instead of test contract.
9ca00b0 
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.

Ref: Uniswap#315
@gitcoindev
Copy link
Contributor Author

gitcoindev commented Jul 31, 2023 •
edited
Loading

@ewilz gas costs look much better now, see 9ca00b0 , pr ready for the next review round.

@gitcoindev gitcoindev mentioned this pull request Jul 31, 2023
Merged
@ewilz ewilz merged commit 3ddc63c into Uniswap:main Jul 31, 2023
@gitcoindev
Copy link
Contributor Author

Thank you for merging! I will apply the same changes to #320 and #322

@ewilz ewilz mentioned this pull request Jul 30, 2023
Closed
6 tasks
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

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

Successfully merging this pull request may close these issues.
2 participants
@gitcoindev @ewilz