[H01] Check for ETH deposited in buyOTokens. #185
Conversation
…_amt = 0, then buyOTokens will spend all of msg.value
|
I want to make sure it's ok to switch uniswapBuyOToken to internal ? |
|
FYI, it still does not "return any ETH deposited in excess." |
|
OK, I changed uniswapBuyOToken so that it always sends back excess ETH. Let me know if that sounds good? |
| @@ -241,20 +241,33 @@ contract OptionsExchange { | |||
| uniswapFactory.getExchange(address(oToken)) | |||
| ); | |||
|
|
|||
| uint256 ethToTransfer = exchange.getEthToTokenOutputPrice(_amt); | |||
| uint256 ethToTransfer; | |||
| uint256 amount = _amt; | |||
There was a problem hiding this comment.
Awesome work 🔥 , but why here re-assigning _amt into amount ?
There was a problem hiding this comment.
OpenZeppelin suggested providing a way to buy as many oTokens as msg.value can afford.
So, if _amt == 0, we get the amount from the exchange contract.
Otherwise, amount = _amt.
Then we emit the event and send the exchange transfer with amount.
Happy to do it a different way, or get rid of the _amt == 0 thing.
mshrieve
changed the title
contracts/OptionsExchange.sol
Outdated
| ethToTransfer = msg.value; | ||
| amount = exchange.getTokenToEthOutputPrice(msg.value); |
There was a problem hiding this comment.
Here u are assigning ethToTransfer = msg.value but using msg.value in getTokenToEthOutputPrice
There was a problem hiding this comment.
they are equal, but we can switch msg.value to ethToTransfer in line 258 if that's more clear.
There was a problem hiding this comment.
I think that would be more clear!
Added a check to make sure msg.value is sufficient to purchase the amount specified.
Also added functionality to set amount = 0 and spend all of msg.value.
This is in response to openZeppelin:
"Additionally, consider implementing a way to either return any ETH deposited in excess, or trade all ETH sent for oTokens."