Eth_Broker fix and test changes from js to ts #17
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,7 +22,6 @@ contract Eth_broker { | |
| // ------------------------------------------------------------------------- | ||
| // Events | ||
| // ------------------------------------------------------------------------- | ||
| // Emitted when tokens are minted | ||
| event mintTokensWithEth( | ||
| address indexed buyer, // The address of the buyer | ||
|
|
@@ -217,7 +216,7 @@ contract Eth_broker { | |
| mutex() | ||
| returns (bool) | ||
| { | ||
| (uint256 daiNeeded, uint256 ethReceived, uint256 ethSpent) = _commonMint( | ||
|
|
||
| _tokenAmount, | ||
| _maxDaiSpendAmount, | ||
| _deadline, | ||
|
|
@@ -228,7 +227,7 @@ contract Eth_broker { | |
| msg.sender, | ||
| _tokenAmount, | ||
| daiNeeded, | ||
| ethSpent, | ||
| _maxDaiSpendAmount | ||
| ); | ||
| // Returning that the mint executed successfully | ||
|
|
@@ -263,7 +262,7 @@ contract Eth_broker { | |
| mutex() | ||
| returns (bool) | ||
| { | ||
| (uint256 daiNeeded, uint256 ethReceived, uint256 ethSpent) = _commonMint( | ||
|
There was a problem hiding this comment. ethReceived is an unused variable now, better don't assign it (uint256 daiNeeded, , uint256 ethSpent) =There was a problem hiding this comment. Not part of this changeset, but I notice that the comments to the internal function |
||
| _tokenAmount, | ||
| _maxDaiSpendAmount, | ||
| _deadline, | ||
|
|
@@ -275,7 +274,7 @@ contract Eth_broker { | |
| _to, | ||
| _tokenAmount, | ||
| daiNeeded, | ||
| ethSpent, | ||
| _maxDaiSpendAmount | ||
| ); | ||
| // Returning that the mint executed successfully | ||
|
|
@@ -339,7 +338,7 @@ contract Eth_broker { | |
| "Curve burn failed" | ||
| ); | ||
| // Getting expected ETH for DAI | ||
|
There was a problem hiding this comment. By removing this value, the passed in value |
||
| uint256 ethMin = sellRewardDai(dai_.balanceOf(address(this))); | ||
|
Member
There was a problem hiding this comment. why does this revert https://github.com/ethersphere/bzzaar-contracts/pull/7/files ?
Member
There was a problem hiding this comment. a bit more info - i believe the original change was made in order to allow the caller of the function to account for slippage. using the full amount of DAI received by the contract to calculate the minimum amount of ETH that would be received as a result of the transaction would only work in the case that the slippage is 0%. @ralph-pichler @RyRy79261 could you advise please guys? 😁 There was a problem hiding this comment. I would be really interested to hear Ralph has to say about this, since he approved #7 by Nicca42. Completely tracing this down to the source is probably warranted as this value might have an impact. I can help with that, but that will take a bit more time than just doing a review. There was a problem hiding this comment. Veronica writes that this could not be properly tested because the solidity versions between the router and this contract are different. If you do this change, at the very least I would call for caution and manually test that this works (and share the test artifacts)
Contributor
There was a problem hiding this comment. Hi there, sorry it's been a while since I saw this code base so still need to pull it down but if I recall correctly, in trying to eliminate all potential entropy from the contracts when trying to fix the tests, I think this approach is more explicit as it takes the active balance as directly as possible That being said it's be a while so I can't recall and specifically deliberate requirement for this line change |
||
| // Approving the router as a spender | ||
| require( | ||
| dai_.approve( | ||
|
|
@@ -349,6 +348,7 @@ contract Eth_broker { | |
| "DAI approve failed" | ||
| ); | ||
| // Selling DAI received for ETH | ||
| uint[] memory swapOutputs = new uint[](2); | ||
| router_.swapExactTokensForETH( | ||
| daiReward, | ||
| ethMin, | ||
|
|
@@ -361,7 +361,7 @@ contract Eth_broker { | |
| msg.sender, | ||
| _tokenAmount, | ||
| daiReward, | ||
| swapOutputs[1], | ||
|
There was a problem hiding this comment. I find this a very strange change and don't understand why you pass
Contributor
There was a problem hiding this comment. The outputs from the uniswap broker return the number required here in |
||
| _minDaiSellValue | ||
| ); | ||
| // Returning that the burn executed successfully | ||
|
|
@@ -406,7 +406,8 @@ contract Eth_broker { | |
| internal | ||
| returns( | ||
| uint256 daiNeeded, | ||
| uint256 ethReceived, | ||
| uint256 ethSpent | ||
| ) | ||
| { | ||
| // Getting the exact needed amount of DAI for desired token amount | ||
|
|
@@ -417,14 +418,17 @@ contract Eth_broker { | |
| "DAI required for trade above max" | ||
| ); | ||
| // Swapping sent ETH for exact amount of DAI needed | ||
| uint256[] memory swapOutputs = new uint[](2); | ||
|
|
||
| swapOutputs = router_.swapETHForExactTokens.value(msg.value)( | ||
| daiNeeded, | ||
| getPath(true), | ||
| address(this), | ||
| _deadline | ||
| ); | ||
| // Getting the amount of ETH received | ||
| ethReceived = address(this).balance; | ||
| ethSpent = swapOutputs[0]; | ||
|
There was a problem hiding this comment. You never use this variable. Why burdening yourself with assigning it (also to initialize and assign the array on lines 421 and 423)
Contributor
There was a problem hiding this comment. To match the uniswap broker, the issue with testing was that the values for swapOutputs wasn't being returned in the mocks |
||
| // Approving the curve as a spender | ||
| require( | ||
| dai_.approve(address(curve_), daiNeeded), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it is not part of this changeset, but I notice that you are using Solidity version 0.5.0 which is rather outdated. Is it in the books to update this?