Eth_Broker fix and test changes from js to ts #17

saksijas · 2021-11-26T09:28:08Z

No description provided.

packages/chain/test/curve.test.ts

significance · 2022-01-19T13:14:45Z

packages/chain/contracts/Eth_broker.sol

@@ -339,7 +338,7 @@ contract Eth_broker {
            "Curve burn failed"
        );
        // Getting expected ETH for DAI
-        uint256 ethMin = sellRewardDai(_minDaiSellValue);
+        uint256 ethMin = sellRewardDai(dai_.balanceOf(address(this)));


why does this revert https://github.com/ethersphere/bzzaar-contracts/pull/7/files ?

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? 😁

More context: #7

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.
On the most rudimentary level, the function sellRewardDai says that _daiAmount is the amount of dai to be traded for ETH and this is indeed dai_.balanceOf(address(this)) and not _minDaiValue. However, I see that #7 changed the value passed in at this line (which is now reverted), but did not update the comment for the function sellRewardDai.

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)

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

Eknir

Dan asked me to review this.

Eknir · 2022-01-19T16:15:02Z

packages/chain/contracts/Eth_broker.sol

@@ -22,7 +22,6 @@ contract Eth_broker {
    // -------------------------------------------------------------------------
    // Events
    // -------------------------------------------------------------------------
-


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?

Eknir · 2022-01-19T16:21:42Z

packages/chain/contracts/Eth_broker.sol

@@ -263,7 +262,7 @@ contract Eth_broker {
        mutex()
        returns (bool)
    {
-        (uint256 daiNeeded, uint256 ethReceived) = _commonMint(
+        (uint256 daiNeeded, uint256 ethReceived, uint256 ethSpent) = _commonMint(


ethReceived is an unused variable now, better don't assign it

(uint256 daiNeeded, , uint256 ethSpent) =

Not part of this changeset, but I notice that the comments to the internal function _commonMint don't document the last return value (ethSpent).

Eknir · 2022-01-19T16:23:08Z

packages/chain/contracts/Eth_broker.sol

@@ -217,7 +216,7 @@ contract Eth_broker {
        mutex()
        returns (bool)
    {
-        (uint256 daiNeeded, uint256 ethReceived) = _commonMint(
+        (uint256 daiNeeded, uint256 ethReceived, uint256 ethSpent) = _commonMint(


See my comment on line 265

Eknir · 2022-01-19T16:30:32Z

packages/chain/contracts/Eth_broker.sol

@@ -339,7 +338,7 @@ contract Eth_broker {
            "Curve burn failed"
        );
        // Getting expected ETH for DAI
-        uint256 ethMin = sellRewardDai(_minDaiSellValue);
+        uint256 ethMin = sellRewardDai(dai_.balanceOf(address(this)));


More context: #7

Eknir · 2022-01-19T16:33:53Z

packages/chain/contracts/Eth_broker.sol

@@ -339,7 +338,7 @@ contract Eth_broker {
            "Curve burn failed"
        );
        // Getting expected ETH for DAI
-        uint256 ethMin = sellRewardDai(_minDaiSellValue);


By removing this value, the passed in value _minDaiSellValue (line 301) is not anymore used (only to emit it in an event). Given that this value is not used, it is not best practice to emit it in the event, so you could just remove it from the function signature and also don't emit it in the event. I understand there might be reasons for backwards compatibility that you keep the function signature and event as is (and that this particular change is not even wanted because it is reverting #7 )

Eknir · 2022-01-19T16:39:28Z

packages/chain/contracts/Eth_broker.sol

@@ -339,7 +338,7 @@ contract Eth_broker {
            "Curve burn failed"
        );
        // Getting expected ETH for DAI
-        uint256 ethMin = sellRewardDai(_minDaiSellValue);
+        uint256 ethMin = sellRewardDai(dai_.balanceOf(address(this)));


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.
On the most rudimentary level, the function sellRewardDai says that _daiAmount is the amount of dai to be traded for ETH and this is indeed dai_.balanceOf(address(this)) and not _minDaiValue. However, I see that #7 changed the value passed in at this line (which is now reverted), but did not update the comment for the function sellRewardDai.

Eknir · 2022-01-19T16:40:53Z

packages/chain/contracts/Eth_broker.sol

@@ -339,7 +338,7 @@ contract Eth_broker {
            "Curve burn failed"
        );
        // Getting expected ETH for DAI
-        uint256 ethMin = sellRewardDai(_minDaiSellValue);
+        uint256 ethMin = sellRewardDai(dai_.balanceOf(address(this)));


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)

Eknir · 2022-01-19T16:43:16Z

packages/chain/contracts/Eth_broker.sol

@@ -361,7 +361,7 @@ contract Eth_broker {
            msg.sender, 
            _tokenAmount, 
            daiReward, 
-            ethMin, 
+            swapOutputs[1], 


I find this a very strange change and don't understand why you pass 0 here (the array is initialized but never filled)

The outputs from the uniswap broker return the number required here in swapOutputs[1]

Eknir · 2022-01-19T16:48:12Z

packages/chain/contracts/Eth_broker.sol

            daiNeeded, 
            getPath(true), 
            address(this), 
            _deadline
        );
        // Getting the amount of ETH received
        ethReceived = address(this).balance;
+        ethSpent = swapOutputs[0];


You never use this variable. Why burdening yourself with assigning it (also to initialize and assign the array on lines 421 and 423)

To match the uniswap broker, the issue with testing was that the values for swapOutputs wasn't being returned in the mocks

saksijas added 11 commits November 16, 2021 13:14

Merge branch 'main' into ry-fix

32cc116

removed artifacts and cache

72c9d1c

Added licence

6289c5d

Fixed readme

a2be417

fix: removed editorconfig

437e25c

fix: renamed file name

44a7138

added old folder to gitignore

3ff6b92

fix: updated to existing ci

fef1bae

added line to ci file

c90be69

fix: removed scripts readme file

d28260f

removed old deployment script

c9b1e7c

bee-runner bot added the pull-request label Nov 26, 2021

saksijas added 2 commits November 30, 2021 17:35

fix: removed commented unused test expects

32d4303

Merge branch 'ry-fix' of github.com:wschwab/bzzaar-contracts into ry-fix

cd881fc

ralph-pichler reviewed Dec 2, 2021

View reviewed changes

packages/chain/test/curve.test.ts Outdated Show resolved Hide resolved

wschwab added 2 commits December 5, 2021 15:03

removed commented out tests

7e337f1

sure hope this is all of them

ade4e24

wschwab mentioned this pull request Dec 9, 2021

migrated and updated old README and docs #18

Closed

ralph-pichler approved these changes Dec 9, 2021

View reviewed changes

significance reviewed Jan 19, 2022

View reviewed changes

Eknir reviewed Jan 19, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eth_Broker fix and test changes from js to ts #17

Eth_Broker fix and test changes from js to ts #17

saksijas commented Nov 26, 2021

significance Jan 19, 2022

significance Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

RyRy79261 Jan 19, 2022

Eknir left a comment

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

Eknir Jan 19, 2022

RyRy79261 Feb 7, 2022

Eknir Jan 19, 2022

RyRy79261 Feb 7, 2022

Eth_Broker fix and test changes from js to ts #17

Are you sure you want to change the base?

Eth_Broker fix and test changes from js to ts #17

Conversation

saksijas commented Nov 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eknir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment