makerdao / exchange-callees

Exchange Callee contracts for LIQ2.0 Flash Loans
GNU Affero General Public License v3.0
31 stars 11 forks source link

Uniswap V3 Split Router Callee #35

Closed KirillDogadin-std closed 1 year ago

KirillDogadin-std commented 1 year ago

This PR adds new Uniswap Split Route callee contract and its tests.

Notes:

Functional testing

  1. Create .env file (in the root of the project) containing valid rpc url under FOUNDRY_ETH_RPC_URL name
  2. Run tests
    • Using hardcoded alpha router data: forge test --match testTakeLinkUniswapSplitProfit
    • Using freshly fetched alpha router data: (cd scripts/uniswap-split-route-callee && npm ci && node index.js)
rockyfour commented 1 year ago

@KirillDogadin-std @valiafetisov in the 1Inch integration the actual collateral slice amount and owed debt to be paid back are re-encoded in the callee itself - https://github.com/makerdao/exchange-callees/blob/1f9ba7b0e5d202774212c8ccfb9b0ac929ee9124/src/OneInchCallee.sol#L131-L132

Here this is not done. So in practice, the slice parameter, which is calculated in the clipper is ignored, and you might end up not swapping all the collateral you got from the clipper.

Was the approach from 1Inch changed intentionally?

KirillDogadin-std commented 1 year ago

https://github.com/makerdao/exchange-callees/pull/35#issuecomment-1457771576

Was the approach from 1Inch changed intentionally?

the calldata that is generated by AlphaRouter has a non-trivial structure and is supposed to be (expected to be) highly optimised for the specific input amount. This means we should choose either or:

The latter was preferred according to https://github.com/makerdao/exchange-callees/pull/34#discussion_r1066712189

rockyfour commented 1 year ago

Almost there. I do see this failure (let's ignore other instabilities for the purpose of merging this): [FAIL. Reason: STF] testTakeLinkUniswapSplitProfit() (gas: 8260641125390436681) this was on block 16941906

Will approve after that test is fixed.

KirillDogadin-std commented 1 year ago

So this test fails sometimes due to the link price fluctuation (solidity test determines the link amount used in the test based on dust and link price at a block height). With the hardcoded amount there's a risk that eventually the test will fail either with STF or InsufficientProfit. To avoid this, the initial dynamic computation of the link amount was introduced.

I've adjusted the amount of link that the script provides, it works out now, but i am not even sure that when you will take a look at this next time it will pass.

If you have a preference on how to address this (if this should be addressed) please let me know.

rockyfour commented 1 year ago

I've adjusted the amount of link that the script provides, it works out now, but i am not even sure that when you will take a look at this next time it will pass.

Ok, you didn't update the test itself with the new txData, so I added a suggestion for it above.

With that change the test passes (block 16959704). Note that I didn't go deeply into the new amount you set and whether it could be made more robust. You can consider looking at that further and maybe making changes in a separate PR if needed.

Please commit the above suggestion when you can, then I will approve and merge the PR.

KirillDogadin-std commented 1 year ago

Committed, thanks for including suggestion.