Routing #58
Routing #58
Conversation
single hops passing on exactOut
| poolManager.settle(currency); | ||
| } | ||
|
|
||
| function _pay(address token, address payer, address recipient, uint256 amount) internal virtual; |
There was a problem hiding this comment.
@hensha256 wanted to outsource this to UniversalRouter...wondering what you think of adding a generically named "pay" method which abstract contracts can call virtually.... 🤔
There was a problem hiding this comment.
So you mean like the UR would have a pay function with override in it? And then any of the contracts that it overrides can have an abstract pay function too?
There was a problem hiding this comment.
The V3SwapRouter inherits from Permit2Payments so it can then call payOrPermit2Transfer where it needs to do payments. So I guess we could change that to a pay function too and implement the permit2 functionality?
I'd be interested to see how it looks in the UR repo given that currently the hierarchy is
Permit2Payments -> V3SwapRouter -> Dispatcher
and the Dispatcher doesnt have to know anything about handling payments
There was a problem hiding this comment.
Ah I was wondering why this is virtual.. I guess that makes sense as UR has its own predefined internal "pay" methods
There was a problem hiding this comment.
yah we could leave it out of dispatcher, easiest case, have a V4SwapRouter contract in UR that inherits this and implements pay??
contracts/V4Router.sol
Outdated
| poolManager = _poolManager; | ||
| } | ||
|
|
||
| function v4Swap(SwapType swapType, bytes memory params) internal { |
contracts/V4Router.sol
Outdated
|
|
||
| function _swapExactInput(ExactInputParams memory params, address msgSender) private { | ||
| unchecked { | ||
| for (uint256 i = 0; i < params.path.length; i++) { |
There was a problem hiding this comment.
probably worth caching params.path.length given its accessed multiple times on every loop
contracts/interfaces/IV4Router.sol
Outdated
| } | ||
|
|
||
| struct PathKey { | ||
| Currency tradeCurrency; |
There was a problem hiding this comment.
I dont like the name tradeCurrency, its taken me a while to understand it. I guess I would call it like intermediateCurrency? Open to other ideas. just tradeCurrency wasn't immediately apparent to me
There was a problem hiding this comment.
great name, yah I just threw tradeCurrency in as a placeholder. I was hoping for something short to avoid annoying linting, but ultimately intermediateCurrency is a good option
| address recipient; | ||
| uint128 amountIn; | ||
| uint128 amountOutMinimum; | ||
| uint160 sqrtPriceLimitX96; |
There was a problem hiding this comment.
Why are both sqrtPriceLimitX96 and amountOutMinimum needed? Dont they both achieve slippage protection?
There was a problem hiding this comment.
In fact I'm pretty sure amountOutMinimum is unused?
There was a problem hiding this comment.
ohh it was actually on my TODO to see how UR/router-sdk deals with slippage, woops, still need to potentially incorporate that. amountOutMinimum is typically used from our UI. The only time Ive used sqrtPriceLimitX96 is with TWAMM which needs it (but not through a router).sqrtPriceLimitX96 is less about slippage and more about halting a swap midway through (you might still want to check you got enough out). Noah mentioned he was glad TWAMM needed it bc they were on the fence about adding the feature in. Anyway it's there for people if they need it, but it's not used much...
There was a problem hiding this comment.
I checked in UR and for V3 we only externally expose the amountMin/amountMaxs, for the sqrtPriceLimitX96 we set it to max/min tick always. So wonder if there is need to expose it? Not exactly sure what the use case is. Like I think exposing both means that you could set sqrtPriceLimitX96 incorrectly and cause reverts if it is too restrictive compared to the amountMinimum threshold
There was a problem hiding this comment.
I think it could be cool to keep it for others to use if they want (we wouldn't be using any of the swapSingle in UR anyway, which is the only place it's exposed) But if unused internal functions contribute to UR bytecode (can experiment with that) happy to take out
contracts/V4Router.sol
Outdated
|
|
||
| function _swapExactOutput(ExactOutputParams memory params, address msgSender) private { | ||
| unchecked { | ||
| for (uint256 i = params.path.length; i > 0; i--) { |
There was a problem hiding this comment.
ugh i dislike that this is from n to 1 not n-1 to 0... but I see the issue.
I guess it could be
for (uint256 i = params.path.length-1; i < params.path.length; i--) {
but thats also ugly so i guess theres no good way 😂
There was a problem hiding this comment.
dude yah, I actually searched all Uniswap for i-- to see how ppl deal with this type of thing. And I found ANOTHER POC v4 router that Pote wrote ages ago and he did the same thing baha
There was a problem hiding this comment.
I feel like this is typical for for loops iterating backwards through the path though.. like doesn't seem too weird to me. Is there a reason we force it to be backwards though? I guess compatibility with how other routers work but there is no reason we couldn't just encode the path the other way also
| /// @title UniswapV4Routing | ||
| /// @notice Abstract contract that contains all internal logic needed for routing through Uniswap V4 pools |
There was a problem hiding this comment.
nit: Can just have this natspec in the interface
| contract V4RouterImplementation is V4Router { | ||
| constructor(IPoolManager _poolManager) V4Router(_poolManager) {} | ||
|
|
||
| function swap(SwapType swapType, bytes memory params) external { |
There was a problem hiding this comment.
Could also make this a virtual function in the abstract contract. Beyond the _pay() function the inheriting contract needs some kind of external entrypoint function, so I think it would make it a little more clear exposing that in the interface and abstract contract.
There was a problem hiding this comment.
wonder if making this a virtual function on the actual router contract is too big of a security vulnerability? In UR, if we don't override it to do nothing, we have a potential reentrancy vulnerability that could swap on v4 with funds sitting in the router. If it's a vuln for us, could potentially be for other ppl inheriting into an aggregator-like contract.... 🤔
| address recipient; | ||
| uint128 amountIn; | ||
| uint128 amountOutMinimum; | ||
| uint160 sqrtPriceLimitX96; |
There was a problem hiding this comment.
I checked in UR and for V3 we only externally expose the amountMin/amountMaxs, for the sqrtPriceLimitX96 we set it to max/min tick always. So wonder if there is need to expose it? Not exactly sure what the use case is. Like I think exposing both means that you could set sqrtPriceLimitX96 incorrectly and cause reverts if it is too restrictive compared to the amountMinimum threshold
| poolManager.settle(currency); | ||
| } | ||
|
|
||
| function _pay(address token, address payer, address recipient, uint256 amount) internal virtual; |
There was a problem hiding this comment.
Ah I was wondering why this is virtual.. I guess that makes sense as UR has its own predefined internal "pay" methods
contracts/V4Router.sol
Outdated
|
|
||
| function _swapExactOutput(ExactOutputParams memory params, address msgSender) private { | ||
| unchecked { | ||
| for (uint256 i = params.path.length; i > 0; i--) { |
There was a problem hiding this comment.
I feel like this is typical for for loops iterating backwards through the path though.. like doesn't seem too weird to me. Is there a reason we force it to be backwards though? I guess compatibility with how other routers work but there is no reason we couldn't just encode the path the other way also
Probably of interest to note that foundry gas snapshots are not accounting for storage clear refunds, so before we get transient storage in here, gas is incredibly exaggerated.
Could cut down on bytecode by removing
Singleoptions