- We have decided to now support calling our executors normally (without using delegatecalls) since they now support specifying token transfers.
- This was disabled for UniswapV3 in the past also because of data decoding issues. This seems to be the solution, though.
In both encoding and contracts:
- Make sure that the tests names/encoder match what is happening (we had test_split_... that was only a single swap).
- Made modules inside the encodings tests
- In swap_encoders: there is one module per protocol
- In strategy_encoders: there is one module per strategy and an extra protocol_integration module. Inside the sequential module there is another module called chained_swaps
- In contracts, each strategy has its own file. Integration tests per strategy should also be here. Renamed TychoRouterIntegration to TychoRouterProtocolIntegration. Only protocol integration tests should be in this file
--- don't change below this line ---
ENG-4327 Took 1 hour 13 minutes
- In Tycho Router
- Fix integration tests
- If the trade is cyclical, we can't do the balance check for the correctness of the amountOut transfers
- In encoding,
- if it is a wrap trade, the transfer should be just a normal Transfer (not TransferFrom nor a Permit2Transfer)
- add test names to println! statements
--- don't change below this line ---
ENG-4315 Took 1 hour 0 minutes
Took 30 seconds
Took 21 seconds
Took 16 seconds
Took 17 seconds
Add transfer in Executor and pass receiver address in encoding
This is done by using the TAKE action instead of TAKE_ALL
--- don't change below this line ---
ENG-4315 Took 1 hour 36 minutes
Took 2 minutes
Add transfer in Executor and pass receiver address in encoding
Remove integration test with StETH pool because we don't support it at simulation level and there is something weird with the amounts (that it is not worth it to investigate now)
--- don't change below this line ---
ENG-4315 Took 34 minutes
Took 2 minutes
- The out transfer is now a responsibility of the Executors -> remove this from router methods
- Also adding a check that the receiver got the full amount out
- In encoding, if it is the last swap, pass the receiver as the trade receiver and not the router address (fix encoding tests)
- Fixed some solidity tests (after rebasing with a PR that is still open, I will fix them all)
TODO: Adapt curve and uniswap v4 to support this
--- don't change below this line ---
ENG-4315 Took 3 hours 7 minutes
Took 20 minutes
Took 59 seconds
Took 7 minutes
- This needed to be in BalancerV2 so that the executor can also take care of transfers from the user into the tycho router, to avoid having transfer actions mixed between the router and executors
- This needed to be in Curve so that the executor can also take care of transfers from the user into the tycho router, to avoid having transfer actions mixed between the router and executors
- For protocols like Balancer and Curve, which expect funds to be in the router at the time of swap, we must support also transferring funds from the user into the router. Doing this in the router would mean we are dealing with transfers in two different places: in the router main methods and in the executors. To avoid this, we are now performing transfers just in the executors, and two transfer types have been added to support transfers into the router.
TODO:
- Add this for Balancer and Curve (only added for USV4 atm).
- TODO consider renaming TRANSFER_FROM and TRANSFER_PERMIT2 to include "pool" in the name
- Renamed ExecutorTransferMethods to TokenTransfer to avoid leaking information about how the transfer happens into the executor. The executor shouldn't care if there are multiple methods or one single method that takes care of everything.
- Also renamed TransferMethod to TransferType to match the rust encoding
- Properly decode, update tests with proper decoding
- Added test case for each transfer method
- Also fully tested permit2 transferFrom and it works perfectly.
TODO:
- Fix integration tests once encoding is implemented.
- Also do some renamings and comment improvements:
transfer method -> type
- Remove integration tests since we no longer support direct calls to USV3 executor, even for testing.
- Renamed ExecutorTransferMethods to TokenTransfer to avoid leaking information about how the transfer happens into the executor. The executor shouldn't care if there are multiple methods or one single method that takes care of everything.
- Also renamed TransferMethod to TransferType to match the rust encoding
- For now, hardcode them to TRANSFER on the rust encoder side. This will be fixed in an upcoming PR.
- Remove the split swap simple route integration test - seemed overkill since simple routes are tested in many other integration tests, the original rust test named doesn't exist anymore, and simple routes should anyway be passing through the single endpoint.
- Properly decode, update tests with proper decoding
- Added test case for each transfer method
- Also fully tested permit2 transferFrom and it works perfectly.
NOTE:
UniswapV3 doesn't support NONE as a transfer method.
TODO:
- Fix integration tests once encoding is implemented.
- Properly decode, update tests with proper decoding
- Added test case for each transfer method
- Also fully tested permit2 transferFrom and it works perfectly.
TODO:
- Fix integration tests once encoding is implemented.
- Add more test cases for sequential swap and single swap to match those of split swap (fee, slippage), to catch errors more easily if someone makes a small code change to either the single or sequential methods
- Excluded USV3 and USV4 tests on sequential and single swap, since these tests were more to test USV3 and USV4 executor functionality than the high level sswap methods - they should already be sufficiently tested.
- Remove `testSplitSwapSimple` and `testSplitSwapSingleUSV3` since this is already tested by several other high-level methods (see single USV3 and single USV4 tests). We should prioritize integration-testing public methods over private methods.
- The old way was useful when we just had split swaps. Unfortunately, we now have split, sequential, and single swaps, which don't always require token indices or split percentages, so we need to decode differently for each case.
- Add some basic tests (will add more in next commits)
- Adapt sequential tests (which originally tested split swap)
- Adapt a forgotten single swap test
TODO:
- Fix encoding of single and sequential swaps to not expect the split sawp format every time (the split and the token indices are not necessary and consume unnecessary gas).
