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

performed modifications after adding sqrt computation #26

Merged
merged 23 commits into from
Sep 16, 2024

Conversation

ovatman
Copy link
Contributor

@ovatman ovatman commented Sep 5, 2024

Changes include:

  • UniswapV2Router02.addLiquidity() and UniswapV2Pair.mint() functions are added (together with functions they use such as math_sqrt())
  • testRouterAddLiquidity() test added that checks for newly added functionality
  • output from regression tests are updated

- construction of Router and Pair contracts are done outside `UniswapV2Swap` for testing purposes
- interfaces for `UniswapV2Router02` and `UniswapV2Pair` are added
- construction of pairs are moved from the UniswapV2Swap's constructor to the test setup
- `UniswapV2Router02.addLiquidity()` and `UniswapV2Pair.mint()` functions are added (together with functions they use such as `math_sqrt()`)
- `testRouterAddLiquidity()` test added that checks for newly added functionality
- some additional formatting changes (done by forge formatter)
@ovatman ovatman self-assigned this Sep 5, 2024
mariaKt
mariaKt previously requested changes Sep 5, 2024
Copy link
Contributor

@mariaKt mariaKt left a comment

Choose a reason for hiding this comment

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

I believe that this is failing due to compound assignment no longer being supported.

@mariaKt mariaKt self-requested a review September 6, 2024 19:44
Copy link
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

Please also make sure you are applying the same modifications to the version of the contract in test/regression/swaps.sol. The two files differ slightly, but we want to keep them enough in sync that they are running the same computations. Also, that version is the only version that is currently being regression tested.

@dwightguth
Copy link
Contributor

I will happily investigate the failures in the semantics if there are any, but now that we have a working version of the semantics on one version of the contract, changes to the contract need to only go through if they don't break the semantics.

@dwightguth
Copy link
Contributor

Also, just fyi, auto formatting a file in the same PR that you make other modifications to that file is a bad idea; it makes it vastly more difficult to review the code.

@mariaKt
Copy link
Contributor

mariaKt commented Sep 9, 2024

I tried this version of Uniswap locally, and it does not run. It fails in setUp (

). Looking very briefly, it seems that is attempts to create a UniswapV2Swap using two different constructors.
_uni = new UniswapV2Swap(address(_weth), address(_dai), address(_usdc));

and
_uni = new UniswapV2Swap(address(_weth), address(_dai), address(_usdc), address(_router));

Only the one with 3 parameters exists though.

@ovatman
Copy link
Contributor Author

ovatman commented Sep 10, 2024

@mariaKt I have addressed the issue below; Uniswap is compiling now.

I tried this version of Uniswap locally, and it does not run. It fails in setUp (


). Looking very briefly, it seems that is attempts to create a UniswapV2Swap using two different constructors.

_uni = new UniswapV2Swap(address(_weth), address(_dai), address(_usdc));

and

_uni = new UniswapV2Swap(address(_weth), address(_dai), address(_usdc), address(_router));

Only the one with 3 parameters exists though.

@ovatman
Copy link
Contributor Author

ovatman commented Sep 10, 2024

@dwightguth last commit performs this synchronization.

Please also make sure you are applying the same modifications to the version of the contract in test/regression/swaps.sol. The two files differ slightly, but we want to keep them enough in sync that they are running the same computations. Also, that version is the only version that is currently being regression tested.

@ovatman
Copy link
Contributor Author

ovatman commented Sep 10, 2024

@dwightguth @mariaKt as far as I can see current state of semantics don't work well with the sqrt() related modifications. I believe it should be possible to keep the older version but still add sqrt() related functionality.
However, I'm not certain how it will work with retesteth related stuff.

@ovatman ovatman requested a review from dwightguth September 10, 2024 11:24
@dwightguth
Copy link
Contributor

Can you show me what the change to the contract and the resulting stuck configuration were?

@mariaKt
Copy link
Contributor

mariaKt commented Sep 10, 2024

@dwightguth if you meant me: I made no changes to the contract, and simply added the transaction

txn(1, 2, 0, 1724300000, testRouterAddLiquidity, )

after the others (so it is similar to the UniswapTest.txn file in this PR). Here is the stuck configuration.
conf.txt

@ovatman
Copy link
Contributor Author

ovatman commented Sep 11, 2024

@dwightguth @mariaKt I went over the error and performed necessary modifications. krun progressed when I replaced integer literals in the addLiquidity() call with local variables keeping the same values.

I have also updated the reference output accordingly, I wasn't able to see any problems with the output reference please check if you can see anything odd.

@ovatman ovatman enabled auto-merge (squash) September 13, 2024 06:08
@dwightguth dwightguth disabled auto-merge September 13, 2024 14:04
Copy link
Contributor

@dwightguth dwightguth left a comment

Choose a reason for hiding this comment

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

I'm approving so as to make you not stuck, but please address my comment prior to merging.

@@ -1,2 +1,3 @@
create(1, 0, 1724300000, UniswapV2SwapTest, ),
txn(1, 2, 0, 1724300000, testSwapLoop, )
txn(1, 2, 0, 1724300000, testSwapLoop, ),
txn(1, 2, 0, 1724300000, testRouterAddLiquidity, )
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be here. This program tests just the swap function.

@ovatman ovatman dismissed mariaKt’s stale review September 16, 2024 05:20

it has been addressed

@ovatman ovatman merged commit 95f5df6 into main Sep 16, 2024
1 check passed
@ovatman ovatman deleted the tolga/uniswap-sqrt-updates branch September 16, 2024 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants