Staking params upgradability #2166

abandeali1 · 2019-09-17T16:56:54Z

Description

This PR:

Creates a unified interface for upgrading any params within the staking contracts. wethAssetProxy, ethVault, rewardVault, and zrxVault are all now initialized in init and upgradable via setParams. Their old setter methods have been removed.
Note that attachStakingContract now also requires these params to be passed in. However, they will be ignored in favor of the already stored params if null addresses are passed in.

Testing instructions

Types of changes

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.

…ttern

coveralls · 2019-09-17T18:08:08Z

Coverage remained the same at 75.804% when pulling bb46f18 on feat/3.0/staking-upgradability into 1d1dd4b on feat/3.0/cleanup-staking-contracts.

dorothy-zbornak

Just some small changes then g2g! 💯

dorothy-zbornak · 2019-09-17T19:17:55Z

contracts/test-utils/src/address_utils.ts

+/**
+ * Generates a random address.
+ */
+export function randomAddress(): string {


I don't know why it took us so long. 🙃

dorothy-zbornak · 2019-09-17T19:48:07Z

contracts/staking/test/migration.ts

+                    'InitAddresses',
+                );
+                for (const args of logsArgs) {
+                    expect(args.wethProxyAddress).to.eq(wethProxyAddress);


Why are we just asserting wethProxyAddress and not the others?

Whoops, forgot to add those in 😅

dorothy-zbornak · 2019-09-17T19:48:16Z

contracts/staking/test/migration.ts

+                    'InitAddresses',
+                );
+                for (const args of logsArgs) {
+                    expect(args.wethProxyAddress).to.eq(wethProxyAddress);


jalextowle

Looks good to me! Just a couple of nits and thoughts

jalextowle · 2019-09-17T19:15:35Z

contracts/coordinator/test/libs.ts

@@ -13,7 +13,7 @@ const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper);
 describe('Libs tests', () => {
    let coordinatorContract: CoordinatorContract;
    let chainId: number;
-    const exchangeAddress = addressUtils.generatePseudoRandomAddress();
+    const exchangeAddress = randomAddress();


jalextowle · 2019-09-17T19:21:14Z

contracts/staking/test/utils/types.ts

@@ -4,11 +4,15 @@ import { constants } from './constants';

 export interface StakingParams {
    epochDurationInSeconds: BigNumber;
-    rewardDelegatedStakeWeight: BigNumber;
+    rewardDelegatedStakeWeight: number | BigNumber;


Nit: Let's just use Numberish here, unless there is a reason why Strings shouldn't be allowed. I feel like we should just make this change everywhere that accepts BigNumber.

I was using Numberish, but some of the contract wrapper functions don't accept strings as inputs and had to revert back to this.

jalextowle · 2019-09-17T20:11:09Z

contracts/staking/contracts/src/sys/MixinParams.sol

-    /// @dev Assert param values before initializing them.
-    /// This must be updated for each migration.
-    function _assertMixinParamsBeforeInit()
+    /// @dev Initialzize storage belonging to this mixin.


small nit: Initialzize => Initialize

jalextowle · 2019-09-17T20:12:58Z

contracts/staking/contracts/src/sys/MixinParams.sol

+        // Set up defaults.
+        // These cannot be set to variables, or we go over the stack variable limit.
+        _setParams(
+            2 weeks,                       // epochDurationInSeconds


Nit: Rather than setting these to variables, it would probably be good to make these constants, in case we ever want to change them. Additionally, if they are constants, we can use the internal function pattern to stub out these values, if necessary.

Had to do this to get around stack limitations unfortunately.

jalextowle · 2019-09-17T20:16:49Z

contracts/staking/contracts/src/sys/MixinParams.sol

+    /// @param _ethVaultAddress Address of the EthVault contract.
+    /// @param _rewardVaultAddress Address of the StakingPoolRewardVault contract.
+    /// @param _zrxVaultAddress Address of the ZrxVault contract.
+    function _assertValidAddresses(


Since these addresses should all be contracts and this function will be called extremely infrequently, I think it might be worth checking that these accounts are in fact deployed contracts. I think this could be good for trustlessness and as a sanity check on the addresses that are registered.

Is it really trustless if it's just a contract that allows someone to arbitrarily withdraw funds? 🤷‍♂

moodlezoup

looks good to me, just some small nits

moodlezoup · 2019-09-17T20:13:28Z

contracts/staking/test/params.ts

@@ -20,29 +23,9 @@ blockchainTests('Configurable Parameters', env => {
    });

    blockchainTests.resets('setParams()', () => {
-        interface Params {


moodlezoup · 2019-09-17T20:17:53Z

contracts/staking/contracts/src/stake/MixinZrxVault.sol

-    {
-        zrxVault = IZrxVault(zrxVaultAddress);
-    }
-
    /// @dev Deposits Zrx Tokens from the `owner` into the vault.
    /// @param owner of Zrx Tokens
    /// @param amount of tokens to deposit.
    function _depositFromOwnerIntoZrxVault(address owner, uint256 amount)
        internal
    {
        IZrxVault _zrxVault = zrxVault;


we no longer need to cache this

I'm probably going to just delete this mixin in a separate PR. It doesn't really do anything at the moment.

moodlezoup · 2019-09-17T20:18:56Z

contracts/staking/contracts/src/stake/MixinZrxVault.sol

@@ -57,10 +44,6 @@ contract MixinZrxVault is
        internal
    {
        IZrxVault _zrxVault = zrxVault;


moodlezoup · 2019-09-17T20:19:02Z

contracts/staking/contracts/src/stake/MixinZrxVault.sol

@@ -72,10 +55,6 @@ contract MixinZrxVault is
        returns (uint256)
    {
        IZrxVault _zrxVault = zrxVault;


moodlezoup · 2019-09-17T20:36:28Z

contracts/staking/test/migration.ts

-                    from: notOwnerAddress,
-                });
+                const tx = proxyContract.attachStakingContract.awaitTransactionSuccessAsync(
+                    attachedAddress,


nit: initTargetContract.address for consistency

abandeali1 added 8 commits September 17, 2019 08:45

Update visibility, events, and naming in vaults

3843c64

Set wethAssetProxy, ethVault, rewardVault, and zrxVault using init pa…

b70db37

…ttern

Remove redundant setters and require statements

16ebdfa

Do not initialize stakingProxy in vault constructors

de9527c

Fix build

6641af2

Fix typo across files

de567da

Add addresses back as optional params to attachStakingContract

4705b15

Fix build an tests

9473844

abandeali1 requested a review from hysz as a code owner September 17, 2019 16:56

Remove use of generatePseudoRandomAddress in favor of randomAddress

bb46f18

abandeali1 force-pushed the feat/3.0/staking-upgradability branch from 157e03a to bb46f18 Compare September 17, 2019 17:47

dorothy-zbornak self-assigned this Sep 17, 2019

moodlezoup self-assigned this Sep 17, 2019

dorothy-zbornak approved these changes Sep 17, 2019

View reviewed changes

Add missing checks to attachStakingContract tests

0d441a8

abandeali1 changed the base branch from feat/3.0/cleanup-staking-contracts to 3.0 September 17, 2019 20:03

jalextowle approved these changes Sep 17, 2019

View reviewed changes

abandeali1 mentioned this pull request Sep 17, 2019

Sync Rewards + Refactored Reward Vault #2156

Merged

4 tasks

moodlezoup approved these changes Sep 17, 2019

View reviewed changes

Fix typos and remove redundant cached variables

2d125cd

abandeali1 merged commit 2ea354f into 3.0 Sep 17, 2019

abandeali1 mentioned this pull request Sep 21, 2019

Mutli-Block Finalization #2155

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Staking params upgradability #2166

Staking params upgradability #2166

abandeali1 commented Sep 17, 2019

coveralls commented Sep 17, 2019

dorothy-zbornak left a comment

dorothy-zbornak Sep 17, 2019

dorothy-zbornak Sep 17, 2019

abandeali1 Sep 17, 2019

dorothy-zbornak Sep 17, 2019

jalextowle left a comment

jalextowle Sep 17, 2019

jalextowle Sep 17, 2019

abandeali1 Sep 17, 2019

jalextowle Sep 17, 2019

jalextowle Sep 17, 2019

abandeali1 Sep 17, 2019

jalextowle Sep 17, 2019

abandeali1 Sep 17, 2019

moodlezoup left a comment

moodlezoup Sep 17, 2019

moodlezoup Sep 17, 2019

abandeali1 Sep 17, 2019

moodlezoup Sep 17, 2019

moodlezoup Sep 17, 2019

moodlezoup Sep 17, 2019

Staking params upgradability #2166

Staking params upgradability #2166

Conversation

abandeali1 commented Sep 17, 2019

Description

Testing instructions

Types of changes

Checklist:

coveralls commented Sep 17, 2019

dorothy-zbornak 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 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

moodlezoup 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