Feature/staking/protocol fees #2130

jalextowle · 2019-09-05T22:54:52Z

Description

This pull request introduces WETH support to the staking contract, implementing part of ZEIP-42. Despite the fact that supporting a new asset typically increases the complexity of a system, in this case, only the payProtocolFee and finalizeFees functions were made more complex.

payProtocolFee

The logic is now entirely compatible with ETH or WETH, and this works whether the maker is registered in a staking pool or not. These changes are also unit tested in the file test/protocol_fees.ts.

finalizeFees

Only a small change needed to be made to this function. Now this function will unwrap WETH and withdraw all of the unwrapped ETH to the staking contract. After this, finalization can carry on normally, so this is not a very big change.

Testing instructions

yall

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

Prefix PR title with [WIP] if necessary.
Add tests to cover changes as needed.
Update documentation as needed.
Add new entries to the relevant CHANGELOG.jsons.

coveralls · 2019-09-05T23:03:40Z

Coverage remained the same at 75.804% when pulling 1d5c175 on feature/staking/protocol-fees into abd479d on 3.0.

hysz

Nice work bud! Very concise implementation and test coverage lvl 💯

hysz · 2019-09-06T02:05:23Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

+
+        // Transfer the protocol fee to this address if it should be paid in WETH.
+        if (msg.value == 0) {
+            erc20Proxy.transferFrom(


This comment is more directed to architecture opposed to the logic, but I'm wondering if we should deploy a separate feeProxy.

(i)
When users set their allowance on the ERC20 proxy they permit our exchange contract to transfer tokens on their behalf. The user then explicitly consents to the transfer of token X in amount Y when they sign an order.

Here we're overloading the ERC20 proxy to collect a hidden or implicit fee. We can broadcast to our community that there now exists a fee, but it's still a passive agreement that the average user may not understand.

(ii)
The ERC20 proxy is super OP. It can transfer any ERC20 that the user has set allowance for. In this code, we really only need access to WETH.

(iii)
This also raises a question around the proxy used to stake ZRX. On one hand, this doesn't break (i) because the user still explicitly agrees to the transfer by calling stake(). However (ii) does apply because in that scenario we really only need access to ZRX.

I propose we have either (i) a wethFeeProxy and a zrxStakeProxy, or (ii) a combined stakingProxy that users can set allowances for WETH and ZRX on. Note that this can just be the generic ERC20 Proxy, but redeployed.

@abandeali1 curious to get your thoughts.

I have mixed feelings on it. I think I'm leaning towards having a separate wethAssetProxy but continuing to use the current ERC20Proxy for ZRX.

The good news is that we don't need to decide now since this can literally be a fork of the current ERC20Proxy. Let's just rename this variable wethAssetProxy (might as well rename ZrxVault.erc20Proxy to zrxAssetProxy while we're at it).

contracts/staking/contracts/src/fees/MixinExchangeManager.sol

contracts/staking/contracts/src/interfaces/IStructs.sol

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

contracts/staking/contracts/src/immutable/MixinConstants.sol

hysz · 2019-09-06T02:27:12Z

contracts/staking/test/protocol_fees.ts

+            // Ensure that the correct number of logs were recorded.
+            expect(receipt.logs.length).to.be.eq(1);
+
+            // Ensure that the correct log was recorded.


Wondering if we could move this into an actor or assertion wrapper, to follow similar patterns used elsewhere in our tests. Would also reduce a lot of the code that's repeated in these tests.

Yeah I can work on that. It should be a very simple actor for now.

hysz · 2019-09-06T02:36:11Z

contracts/staking/contracts/test/TestStaking.sol

+contract TestStaking is
+    Staking
+{
+    // Stub out `payProtocolFee` to be the naive payProtocolFee function so that tests will


Could we say a bit more here? I'm still not sure why tests would fail against the actual implementation.

I ended up just removing it since they do work with the current impl

hysz · 2019-09-06T02:37:20Z

contracts/staking/contracts/test/TestProtocolFees.sol

+import "../src/Staking.sol";
+
+
+contract TestProtocolFees is


Nice, I really like this pattern 💯

hysz · 2019-09-06T02:37:49Z

contracts/staking/test/protocol_fees.ts

+} from '../src';
+
+// tslint:disable:no-unnecessary-type-assertion
+blockchainTests('Protocol Fee Unit Tests', env => {


Solid coverage!

abandeali1

Super clean overall, just had a few nits!

contracts/staking/contracts/src/fees/MixinExchangeManager.sol

abandeali1 · 2019-09-06T02:44:18Z

contracts/staking/contracts/src/immutable/MixinConstants.sol

+    address constant internal WETH_ADDRESS = address(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
+
+    // abi.encodeWithSelector(IAssetData(address(0)).ERC20Token.selector, WETH_ADDRESS)
+    bytes constant internal WETH_ASSET_DATA = hex"f47261b0000000000000000000000000c02aaa39b223fe8d0a0e5c4f27ead9083c756cc2";


Since these values are hard coded, it could be good to assert that this actually equals abi.encodeWithSelector(IAssetData(address(0)).ERC20Token.selector, WETH_ADDRESS) in the constructor of this contract.

This isn't a merge blocker, but it's probably a good idea to add comments here with all of the testnet WETH addresses/assetDatas. We should also add WETH to the contract-addresses package now that it is used within the protocol.

abandeali1 · 2019-09-06T02:48:57Z

contracts/staking/contracts/src/immutable/MixinStorage.sol

@@ -18,6 +18,8 @@

 pragma solidity ^0.5.9;

+import "@0x/contracts-asset-proxy/contracts/src/interfaces/IAssetData.sol";


This import isn't being used at the moment.

It will actually be used now, since the check is in the constructor.

contracts/staking/contracts/src/interfaces/IStructs.sol

abandeali1 · 2019-09-06T02:52:51Z

contracts/staking/contracts/test/TestProtocolFees.sol

+        poolIdByMakerAddress[makerAddress] = poolId;
+    }
+
+    function getActivePoolsByEpoch()


We should probably add a getter for this in the staking contract if it doesn't already exist. Alternatively, should everything just be made public? If we go that route we can probably do it in another PR.

As discussed offline, a task has been made to implement this change in a separate PR, where naming conventions will also be updated.

contracts/staking/test/protocol_fees.ts

abandeali1 · 2019-09-06T03:37:18Z

contracts/staking/contracts/test/TestStaking.sol

+contract TestStaking is
+    Staking
+{
+    // Stub out `payProtocolFee` to be the naive payProtocolFee function so that tests will


What are your thoughts on creating some internal getter function for WETH_ASSET_DATA (getWethAssetData), using that in the actual payProtocolFee, and then stubbing it out in this contract? That way we can test with actual WETH transfers. It's probably a couple extra gas but the tradeoff feels worth it to me.

This could actually be a cool pattern to use for all deployment constants. Not sure if we have any similar situations with any of the other constants at the moment.

As discussed offline, I'm a big fan and I think that we should do it on an as-needed basis.

Since the integration tests aren't being implemented in this PR, I'm going to hold off on this until it's needed.

contracts/staking/test/protocol_fees.ts

hysz

I really like how you employed the actor pattern. Nice work! (~‾▿‾)~

hysz · 2019-09-09T20:04:33Z

contracts/staking/contracts/src/fees/MixinExchangeFees.sol

@@ -217,8 +263,9 @@ contract MixinExchangeFees is
            );

            // store pool stats
+            uint256 protocolFeeBalance = protocolFeesThisEpochByPool[poolId];


Is this an optimization?

It looks like it actually makes it less efficient. I think that it's a holdover from how it would have worked before WETH protocol fees.

hysz · 2019-09-09T20:32:49Z

contracts/staking/contracts/test/TestStaking.sol

+    // Stub out `payProtocolFee` to be the naive payProtocolFee function so that tests will
+    // not fail for WETH protocol fees. These tests will fail otherwise because many of them
+    // will transfer
+    function payProtocolFee(


Do we still need this, since it's commented out?

Nope, I'll remove.

jalextowle force-pushed the feature/staking/protocol-fees branch from b521a4d to 73dfb7d Compare September 5, 2019 23:06

jalextowle marked this pull request as ready for review September 5, 2019 23:07

jalextowle requested review from abandeali1, fabioberger and hysz as code owners September 5, 2019 23:07

hysz self-assigned this Sep 5, 2019

jalextowle assigned abandeali1 Sep 5, 2019

jalextowle force-pushed the feature/staking/protocol-fees branch from 73dfb7d to 724f9ab Compare September 6, 2019 00:55

jalextowle changed the base branch from 3.0 to feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed September 6, 2019 00:58

jalextowle changed the base branch from feature/staking/NewMechanicsSolidityOnly-Squashed-Tests-Squashed to feature/staking/NewMechanicsSolidityOnly-Squashed-Tests September 6, 2019 00:58

jalextowle changed the base branch from feature/staking/NewMechanicsSolidityOnly-Squashed-Tests to feature/staking/NewMechanicsSolidityOnly-Squashed September 6, 2019 00:58

hysz reviewed Sep 6, 2019

View reviewed changes

abandeali1 suggested changes Sep 6, 2019

View reviewed changes

jalextowle force-pushed the feature/staking/protocol-fees branch from 73d598b to f52f929 Compare September 6, 2019 23:45

jalextowle changed the base branch from feature/staking/NewMechanicsSolidityOnly-Squashed to 3.0 September 6, 2019 23:49

jalextowle force-pushed the feature/staking/protocol-fees branch 3 times, most recently from 85d99fd to d270581 Compare September 7, 2019 09:28

jalextowle added 3 commits September 7, 2019 13:41

@0x:contracts-staking Added WETH support to MixinExchangeFees

494dc47

@0x:contracts-staking Added tests for protocol fees

30fee43

@0x:contracts-staking Addressed some review comments

2fdd4e9

jalextowle force-pushed the feature/staking/protocol-fees branch from d270581 to 2fdd4e9 Compare September 9, 2019 17:25

abandeali1 approved these changes Sep 9, 2019

View reviewed changes

hysz approved these changes Sep 9, 2019

View reviewed changes

@0x:contracts-staking Fixed lingering review comments

1d5c175

jalextowle merged commit c21bce9 into 3.0 Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/staking/protocol fees #2130

Feature/staking/protocol fees #2130

jalextowle commented Sep 5, 2019 •

edited

Loading

coveralls commented Sep 5, 2019 •

edited

Loading

hysz left a comment

hysz Sep 6, 2019

abandeali1 Sep 6, 2019 •

edited

Loading

hysz Sep 6, 2019

jalextowle Sep 6, 2019

hysz Sep 6, 2019

jalextowle Sep 6, 2019

hysz Sep 6, 2019

hysz Sep 6, 2019

abandeali1 left a comment

abandeali1 Sep 6, 2019

abandeali1 Sep 6, 2019

abandeali1 Sep 6, 2019

jalextowle Sep 6, 2019

abandeali1 Sep 6, 2019

jalextowle Sep 6, 2019

abandeali1 Sep 6, 2019

abandeali1 Sep 6, 2019

jalextowle Sep 6, 2019

jalextowle Sep 6, 2019

hysz left a comment

hysz Sep 9, 2019

jalextowle Sep 9, 2019

hysz Sep 9, 2019

jalextowle Sep 9, 2019

		@@ -18,6 +18,8 @@

		pragma solidity ^0.5.9;

		import "@0x/contracts-asset-proxy/contracts/src/interfaces/IAssetData.sol";

Feature/staking/protocol fees #2130

Feature/staking/protocol fees #2130

Conversation

jalextowle commented Sep 5, 2019 • edited Loading

Description

payProtocolFee

finalizeFees

Testing instructions

Types of changes

Checklist:

coveralls commented Sep 5, 2019 • edited Loading

hysz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abandeali1 Sep 6, 2019 • edited Loading

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

abandeali1 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

hysz 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

jalextowle commented Sep 5, 2019 •

edited

Loading

coveralls commented Sep 5, 2019 •

edited

Loading

abandeali1 Sep 6, 2019 •

edited

Loading