Team2 amm comments #24
Conversation
| // what's the reason for not providing all these parameters in the constructor? | ||
| // But at the same time, the pattern of assigning a global var, read then delete is interesting | ||
| // It's the first time I see this pattern |
There was a problem hiding this comment.
As I know, if the constructor has arguments, then the address of the contract will not be deterministic
There was a problem hiding this comment.
I have posted an explaination in the server, and I saw you already reacted to it lol
| @@ -282,6 +314,7 @@ contract Pool is IPool, PoolERC20 { | |||
|
|
|||
| //Assumption: token0 is ETH, so when you transfer to the user, always transfer token1 | |||
| function swapExactInEthForToken(address to) external payable onlyEthPool nonZeroAddress(to) { | |||
| // don't assume but should add a requirement checking token0 is ETH in the constructor | |||
There was a problem hiding this comment.
The factory always produce an ETH-Token pool with ETH as token0
There was a problem hiding this comment.
Ah okay. Then the comment is not exactly gud. Should say like it's guaranteed token0 is ETH (by the factory)
|
|
||
| // what's the reason for not providing all these parameters in the constructor? | ||
| // But at the same time, the pattern of assigning a global var, read then delete is interesting | ||
| // It's the first time I see this pattern |
There was a problem hiding this comment.
This was originally found in UniswapV3 and I thought it was interesting to use. As Lam said, we can't use a variables in the constructor if not the init code/creation code of Pool will change everytime since we have different argument values when constructing the Pool contract
The Params is being reused every single time a Pool is being deployed. This is different from initialize where the factory needs to deploy then make an external call to initialize. The reason for deleting I believe is because of the refund in gas (from STORAGEKILL? which is -15k since to SSTORE it'll take 20k)
There was a problem hiding this comment.
- Oh I don't know that this pattern is in UniV3, interesting
- Init Code issue has been explained in the server, please check
- Any benefits for the factory to not initialize but instead the pool calls the factory? I thought it's kinda the same?
- Yeah deleting is a very good thing actually
| // Also, this piece of logic is strange since now the calculation logic is in the contract already | ||
| // => From the amountIn, you can directly calculate the amount out, instead of the current logic | ||
| // where you calculate the amountOut and then call this function to once again verify the K | ||
|
|
There was a problem hiding this comment.
That is very true haha. Original intention of swap was to allow users to to do a flash swap and allow it as long as they return back whatever is needed, but then we did not implement a variant of UniswapV2Callee in the end so... this function kinda just became v flat LOL
There was a problem hiding this comment.
LOL I see. Even for flashswap right, the user can still provide the amountIn, you transfer the amountOut out, do the callback, then just simply checks enough amountIn has been transferred in. The pattern of amount0Out & amount1Out is actually appropriate only when the Router does all the logic & the pool contains minimal logic, which is not the case here
No description provided.