uniswapv3 amm adapter #10

snake-poison · 2023-02-07T01:18:24Z

Continues on #8
Additionally:

…r' into feature/uniswapv3-amm-adapter

feature interface was utilizing OZ's IERC20 in interface definition. This is unnecessarily type safe as the IERC20 functions aren't used.

Later compilers require valid spx identifiers.

constructor - public is ignored and has no meaning in that context. getProvideLiquiditySingleAssetCalldata, getRemoveLiquiditySingleAssetCalldata - restricted to pure.

Additionally, clarifies component matching pool test.

snake-poison · 2023-02-08T21:42:43Z

Note about checks:

ArrakisUniswapV3AmmAdapter.sol#L239 is not covered because recreating the situation in which this would trigger is impractical (creating a mock contract) and unlikely to occur in production.
There seem to be integration/forked tests unrelated to this feature that are broken.

ckoopmann

Mostly looking good. Only some nit-picking regarding the contracts (mostly documentation) as well as some suggestions on how to get the CI green.

ckoopmann · 2023-02-09T04:22:23Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

@@ -0,0 +1,277 @@
+/*
+    Copyright 2022 Set Labs Inc.


Should be 2023 and (maybe) Index Coop. No ?

ckoopmann · 2023-02-09T04:22:54Z

contracts/interfaces/external/IArrakisVaultV1.sol

@@ -0,0 +1,83 @@
+/*
+    Copyright 2022 Set Labs Inc.


Wrong date and Copy Right holder. (Should be 2023 IndexCoop no ? )

ckoopmann · 2023-02-09T04:24:14Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+
+/**
+ * @title UniswapV3AmmAdapter
+ * @author Zishan Sami


Without diminishing Zishans contribution I'd suggest changing this to "Index Coop" as we usually do for all contracts I think.

ckoopmann · 2023-02-09T04:26:08Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+        address setToken = _setToken;
+        address[] memory components = _components;
+        uint256[] memory maxTokensIn = _maxTokensIn;
+        uint256 minLiquidity = _minLiquidity;


Why are we doing these reassignments ? Seems like a waste of gas no ? (For the arrays I suggest just changing the type to "memory" in the call signature if those arrays need to be edited. If they are read only then calldata is cheaper anyway.

ckoopmann · 2023-02-09T04:26:58Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+        address[] memory components = _components;
+        uint256[] memory minTokensOut = _minTokensOut;
+        uint256 liquidity = _liquidity;
+        IArrakisVaultV1 arrakisVaultPool = IArrakisVaultV1(_pool);


Same here. Those reassignments seem unnecessary to me

ckoopmann · 2023-02-09T04:32:12Z

test/integration/arrakisUniswapV3AmmAdapter.spec.ts

+      method: "hardhat_reset",
+      params: [
+        {
+          forking: {


I think this hardhat reset is causing the (seemingly unrelated) test failures in the other tests, since this change will persist and therefore lead to the other tests running against a different block.

We have two options here:

Resetting to the state after running these tests to the previously pinned block number.

Adjusting the other tests to work on this block number as well and then updating it for all tests in the hardhat config. I can support here if needed.

I'm a bit conflicted on which is better, but I think long term it might be nice to have different tests running against different block numbers so that we don't have to continously update some of the tests which assume specific market conditions / product states.

Yet another option to update the blocknumber in hardhat-config and then do this "hardhat_reset" to the old blocknumber for all the test suites that are failing on the new block. I think this is probably the best solution as it should be the individual test suite's "responsibility" to ensure it is running agains the right block if it is sensitive to changes in market conditions / product composition etc.

Yet another option to update the blocknumber in hardhat-config and then do this "hardhat_reset" to the old blocknumber for all the test suites that are failing on the new block. I think this is probably the best solution as it should be the individual test suite's "responsibility" to ensure it is running agains the right block if it is sensitive to changes in market conditions / product composition etc.

hmm I thought that addSnapshotBeforeRestoreAfterEach would handle setting up a "resetting" to the snapshot after, but maybe its doing the snapshot after the fork which messes things up moving forward. I'll double check my understanding around this flow and likely try to implement this solution.

I think the "before" function is executed before "beforeEach" so the snapshots are probably taken after the fork change and therefore the changed forked block will persist.

I remember having faced similar issues elsewhere though, it could be that the "snapshot" / "restore" rpc calls just don't play well wit the "reset".

This is a great hint towards a solution. I think I can rework the before hook such that from a general level, the network is reset to the fork block for every test as expected.

ckoopmann · 2023-02-09T04:35:39Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+        try IArrakisVaultV1(_pool).token1() returns (address _token1) {
+            token1 = _token1;
+        } catch {
+            return false;


I don't quite understand why we can't have this line covered, especially since line 234 seems to be covered.

Am I missing anything here ?

Its not that we couldn't, but having a condition where the token0 external call succeeds and the token1 call fails would have to be done with a mock contract that exhibits this behavior.

Ah I see. That explains why we can cover the first call easily but not the second one.
I still think adding this little mock contract might be worth it if it helps us to just keep it at a nice 100% coverage.
Also might be worth looking into other mock options such as smock ( or whatever the cool kids are using nowadays)

I was scared of adding dependencies without upgrading existing dev deps, but I've heard of smock and would love to try it. If I run into an issue with configuring smock, I'll just do a small mock contract to get this to 100% which I agree is nice and why I added a constructor test :)

ckoopmann · 2023-02-09T04:36:20Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+
+    /**
+     * Return calldata for the add liquidity call for a single asset
+     */


Please make sure all external methods have full docstring documentation including all parameters.

ckoopmann · 2023-02-09T04:45:18Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+import "../../../interfaces/external/IArrakisVaultV1.sol";
+
+/**
+ * @title UniswapV3AmmAdapter


The title should probably be the same as the contract name.

ckoopmann · 2023-02-09T04:53:57Z

contracts/protocol/integration/amm/ArrakisUniswapV3AmmAdapter.sol

+
+        target = router;
+        value = 0;
+        data = abi.encodeWithSignature(


I think we could do something like:
abi.encodeWithSelector(IArrakis.removeLiquidity.selector ... here no ?

Advantages:

No need of signature string constant

Less error prone

Maybe more efficient if the selector is precomputed at compile time

also skipping typechecking on libraries

side steps stack too deep error in some cases.

fixes issue with coverage stack too deep.

zishansami102 and others added 23 commits July 12, 2022 16:30

init files

ce0add3

arrakis uniV3 adapter contract draft-1

10cde65

contract typos fix

b02754f

add arrakis abis, fixtures, and constructor tests

3aa13c7

add test for getSpenderAddress

6f096a1

typo fix

831a0b1

GUniPool to ArrakisVaultV1

4193b04

add isValidPool tests

0b475d5

add tests for singleAssetCalldata functions

c88ecd5

add more unit tests

44a940a

add integration tests

755137f

review fixes

375cd81

review fixes

206a476

Merge remote-tracking branch 'edward_set2/zishan/uniswapv3-amm-adapte…

36f9257

…r' into feature/uniswapv3-amm-adapter

chore: enabled 0.8.17 solidity compiler

3e4cfe1

refactor: widens type of IERC20 to address

be7d93e

feature interface was utilizing OZ's IERC20 in interface definition. This is unnecessarily type safe as the IERC20 functions aren't used.

ref: removes unused safemath library.

7c55055

style: update feature spdx identifiers.

7d44cf2

Later compilers require valid spx identifiers.

ref: update feature solidity pragma to 0.8.17

a016dbb

ref: fix warnings for visibility state mutability.

5e51ada

constructor - public is ignored and has no meaning in that context. getProvideLiquiditySingleAssetCalldata, getRemoveLiquiditySingleAssetCalldata - restricted to pure.

style: formatted spec file with prettier eslint

6a7bb40

feat(test): invalid constructor args.

18de1cf

test: adds missing branch in isvalid

3a0acb8

Additionally, clarifies component matching pool test.

snake-poison force-pushed the feature/uniswapv3-amm-adapter branch from 45c3327 to 3a0acb8 Compare February 8, 2023 20:16

snake-poison marked this pull request as ready for review February 8, 2023 20:17

snake-poison requested review from ckoopmann and edkim February 8, 2023 20:19

ckoopmann requested changes Feb 9, 2023

View reviewed changes

ckoopmann reviewed Feb 9, 2023

View reviewed changes

snake-poison added 13 commits February 9, 2023 15:16

chore: adds hardhat-network-helpers as dev dep.

2b65b92

test: reset fork to blocknumber in config

ba5e4ce

chore: upgraded some devdeps

aa6728f

also skipping typechecking on libraries

test: more strict revert strings

5f789f3

chore: setup smock dep dev.

6aadea0

test: added case for an invalid pool

f2a1145

style: doc string edits.

570b65f

chore: adds ext interface for arrakis v1 router.

6982692

refactor: no copy to memory and encodewithcall

4663019

chore: compiling viaIR for 0.8 contracts.

1df82d0

side steps stack too deep error in some cases.

test: sets expected calldata to match v1 router.

de8d270

fix: fix import line to grab all exports.

bfb9d68

chore: setup hardhat to emi yul properly

eec32a1

fixes issue with coverage stack too deep.

pblivin0x closed this Mar 26, 2024

uniswapv3 amm adapter #10

uniswapv3 amm adapter #10

Uh oh!

Conversation

snake-poison commented Feb 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snake-poison commented Feb 8, 2023

Uh oh!

ckoopmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckoopmann Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckoopmann Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckoopmann Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckoopmann Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ckoopmann Feb 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

snake-poison commented Feb 7, 2023 •

edited

Loading

ckoopmann Feb 9, 2023 •

edited

Loading

ckoopmann Feb 9, 2023 •

edited

Loading

ckoopmann Feb 9, 2023 •

edited

Loading

ckoopmann Feb 9, 2023 •

edited

Loading

ckoopmann Feb 9, 2023 •

edited

Loading