From 41b372ffe6b83c5caba23a615abd23ea9fd6fa08 Mon Sep 17 00:00:00 2001 From: Greg Hysen Date: Tue, 20 Aug 2019 15:43:09 -0700 Subject: [PATCH] Updates from staking PR review --- .../contracts/src/interfaces/IAssetProxy.sol | 2 - .../contracts/src/fees/MixinExchangeFees.sol | 6 +-- .../src/immutable/MixinConstants.sol | 4 +- .../contracts/src/interfaces/IAssetProxy.sol | 42 ------------------- .../interfaces/IStakingPoolRewardVault.sol | 9 +--- .../contracts/src/libs/LibEIP712Hash.sol | 4 +- .../contracts/src/libs/LibRewardMath.sol | 1 + .../src/staking_pools/MixinStakingPool.sol | 13 +----- .../contracts/src/sys/MixinOwnable.sol | 28 ++----------- .../src/vaults/StakingPoolRewardVault.sol | 18 ++------ .../staking/contracts/src/vaults/ZrxVault.sol | 2 +- contracts/staking/package.json | 2 +- contracts/staking/src/artifacts.ts | 4 +- contracts/staking/src/wrappers.ts | 1 - contracts/staking/tsconfig.json | 1 - 15 files changed, 23 insertions(+), 114 deletions(-) delete mode 100644 contracts/staking/contracts/src/interfaces/IAssetProxy.sol diff --git a/contracts/asset-proxy/contracts/src/interfaces/IAssetProxy.sol b/contracts/asset-proxy/contracts/src/interfaces/IAssetProxy.sol index 41416a07b0..e2f381a9df 100644 --- a/contracts/asset-proxy/contracts/src/interfaces/IAssetProxy.sol +++ b/contracts/asset-proxy/contracts/src/interfaces/IAssetProxy.sol @@ -18,8 +18,6 @@ pragma solidity ^0.5.9; -import "./IAuthorizable.sol"; - contract IAssetProxy { diff --git a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol index 83b7d8ba37..f81b71ec69 100644 --- a/contracts/staking/contracts/src/fees/MixinExchangeFees.sol +++ b/contracts/staking/contracts/src/fees/MixinExchangeFees.sol @@ -166,8 +166,8 @@ contract MixinExchangeFees is } // step 1/3 - compute stats for active maker pools - IStructs.ActivePool[] memory activePools = new IStructs.ActivePool[](activePoolsThisEpoch.length); - for (uint i = 0; i != totalActivePools; i++) { + IStructs.ActivePool[] memory activePools = new IStructs.ActivePool[](totalActivePools); + for (uint256 i = 0; i != totalActivePools; i++) { bytes32 poolId = activePoolsThisEpoch[i]; // compute weighted stake @@ -203,7 +203,7 @@ contract MixinExchangeFees is } // step 2/3 - record reward for each pool - for (uint i = 0; i != totalActivePools; i++) { + for (uint256 i = 0; i != totalActivePools; i++) { // compute reward using cobb-douglas formula uint256 reward = LibFeeMath._cobbDouglasSuperSimplified( initialContractBalance, diff --git a/contracts/staking/contracts/src/immutable/MixinConstants.sol b/contracts/staking/contracts/src/immutable/MixinConstants.sol index 57d2611d2f..bf608364f6 100644 --- a/contracts/staking/contracts/src/immutable/MixinConstants.sol +++ b/contracts/staking/contracts/src/immutable/MixinConstants.sol @@ -25,12 +25,14 @@ contract MixinConstants is MixinDeploymentConstants { - uint64 constant internal MAX_UINT_64 = 2**64 - 1; + uint64 constant internal MAX_UINT_64 = 0xFFFFFFFFFFFFFFFF; uint256 constant internal TOKEN_MULTIPLIER = 1000000000000000000; // 10**18 + // The upper 16 bytes represent the pool id, so this would be pool id 1. See MixinStakinPool for more information. bytes32 constant internal INITIAL_POOL_ID = 0x0000000000000000000000000000000100000000000000000000000000000000; + // The upper 16 bytes represent the pool id, so this would be an increment of 1. See MixinStakinPool for more information. uint256 constant internal POOL_ID_INCREMENT_AMOUNT = 0x0000000000000000000000000000000100000000000000000000000000000000; bytes32 constant internal NIL_MAKER_ID = 0x0000000000000000000000000000000000000000000000000000000000000000; diff --git a/contracts/staking/contracts/src/interfaces/IAssetProxy.sol b/contracts/staking/contracts/src/interfaces/IAssetProxy.sol deleted file mode 100644 index e363bbaba7..0000000000 --- a/contracts/staking/contracts/src/interfaces/IAssetProxy.sol +++ /dev/null @@ -1,42 +0,0 @@ -/* - - Copyright 2018 ZeroEx Intl. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. - -*/ - -pragma solidity ^0.5.5; - - -contract IAssetProxy { - /// @dev Transfers assets. Either succeeds or throws. - /// @param assetData Byte array encoded for the respective asset proxy. - /// @param from Address to transfer asset from. - /// @param to Address to transfer asset to. - /// @param amount Amount of asset to transfer. - function transferFrom( - bytes calldata assetData, - address from, - address to, - uint256 amount - ) - external; - - /// @dev Gets the proxy id associated with the proxy address. - /// @return Proxy id. - function getProxyId() - external - pure - returns (bytes4); -} diff --git a/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol b/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol index 6d51a616fb..e3cae70557 100644 --- a/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/interfaces/IStakingPoolRewardVault.sol @@ -76,15 +76,10 @@ interface IStakingPoolRewardVault { uint8 operatorShare ); - /// @dev Default constructor. This contract is payable, but only by the staking contract. - function () - external - payable; - - /// @dev Deposit a reward in ETH. + /// @dev Default constructor. /// Note that this is only callable by the staking contract, and when /// not in catastrophic failure mode. - function deposit() + function () external payable; diff --git a/contracts/staking/contracts/src/libs/LibEIP712Hash.sol b/contracts/staking/contracts/src/libs/LibEIP712Hash.sol index e794bd96a9..0ae2e0ccd4 100644 --- a/contracts/staking/contracts/src/libs/LibEIP712Hash.sol +++ b/contracts/staking/contracts/src/libs/LibEIP712Hash.sol @@ -23,8 +23,8 @@ import "@0x/contracts-utils/contracts/src/LibEIP712.sol"; import "../interfaces/IStructs.sol"; -library LibEIP712Hash -{ +library LibEIP712Hash { + // EIP712 Domain Name value for the Staking contract string constant internal EIP712_STAKING_DOMAIN_NAME = "0x Protocol Staking"; diff --git a/contracts/staking/contracts/src/libs/LibRewardMath.sol b/contracts/staking/contracts/src/libs/LibRewardMath.sol index d43af7b112..8fca3ad1fe 100644 --- a/contracts/staking/contracts/src/libs/LibRewardMath.sol +++ b/contracts/staking/contracts/src/libs/LibRewardMath.sol @@ -101,6 +101,7 @@ library LibRewardMath { /// @dev Computes how much shadow asset to mint a member who wants to /// join (or delegate more stake to) a staking pool. + /// See MixinStakingPoolRewards for more information on shadow assets. /// @param amountToDelegateByOwner Amount of Stake the new member would delegate. /// @param totalAmountDelegated Total amount currently delegated to the pool. /// This does *not* include `amountToDelegateByOwner`. diff --git a/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol b/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol index 12bbf03823..b90daf6e94 100644 --- a/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol +++ b/contracts/staking/contracts/src/staking_pools/MixinStakingPool.sol @@ -263,17 +263,7 @@ contract MixinStakingPool is view returns (address[] memory _makerAddressesByPoolId) { - // Load pointer to addresses of makers - address[] storage makerAddressesByPoolIdPtr = makerAddressesByPoolId[poolId]; - uint256 makerAddressesByPoolIdLength = makerAddressesByPoolIdPtr.length; - - // Construct list of makers - _makerAddressesByPoolId = new address[](makerAddressesByPoolIdLength); - for (uint i = 0; i < makerAddressesByPoolIdLength; ++i) { - _makerAddressesByPoolId[i] = makerAddressesByPoolIdPtr[i]; - } - - return _makerAddressesByPoolId; + return makerAddressesByPoolId[poolId]; } /// @dev Returns the unique id that will be assigned to the next pool that is created. @@ -295,6 +285,7 @@ contract MixinStakingPool is returns (address operatorAddress) { operatorAddress = poolById[poolId].operatorAddress; + return operatorAddress; } /// @dev Convenience function for loading information on a pool. diff --git a/contracts/staking/contracts/src/sys/MixinOwnable.sol b/contracts/staking/contracts/src/sys/MixinOwnable.sol index a0b3ebeb9c..964fa49105 100644 --- a/contracts/staking/contracts/src/sys/MixinOwnable.sol +++ b/contracts/staking/contracts/src/sys/MixinOwnable.sol @@ -1,42 +1,22 @@ pragma solidity ^0.5.9; +import "@0x/contracts-utils/contracts/src/Ownable.sol"; import "../interfaces/IStakingEvents.sol"; import "../immutable/MixinStorage.sol"; contract MixinOwnable is + Ownable, IStakingEvents, MixinDeploymentConstants, MixinConstants, MixinStorage { - + /// @dev This mixin contains logic for ownable contracts. /// Note that unlike the standardized `ownable` contract, /// there is no state declared here. It is instead located /// in `immutable/MixinStorage.sol` and its value is set /// by the delegating proxy (StakingProxy.sol) - - /// @dev reverts if called by a sender other than the owner. - modifier onlyOwner() { - require( - msg.sender == owner, - "NOT_OWNER" - ); - _; - } - - /// @dev Transfers the ownership of this contract - /// @param newOwner New owner of contract - function transferOwnership(address newOwner) - external - onlyOwner - { - require( - newOwner != address(0), - "CANNOT_SET_OWNEROT_ADDRESS_ZERO" - ); - owner = newOwner; - emit OwnershipTransferred(newOwner); - } + constructor() public {} } diff --git a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol index b3d0390bd3..53cb4ef059 100644 --- a/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol +++ b/contracts/staking/contracts/src/vaults/StakingPoolRewardVault.sol @@ -49,7 +49,7 @@ contract StakingPoolRewardVault is // mapping from Pool to Reward Balance in ETH mapping (bytes32 => Balance) internal balanceByPoolId; - /// @dev Default constructor. This contract is payable, but only by the staking contract. + /// @dev Fallback function. This contract is payable, but only by the staking contract. function () external payable @@ -59,18 +59,6 @@ contract StakingPoolRewardVault is emit RewardDeposited(UNKNOWN_STAKING_POOL_ID, msg.value); } - /// @dev Deposit a reward in ETH. - /// Note that this is only callable by the staking contract, and when - /// not in catastrophic failure mode. - function deposit() - external - payable - onlyStakingContract - onlyNotInCatostrophicFailure - { - emit RewardDeposited(UNKNOWN_STAKING_POOL_ID, msg.value); - } - /// @dev Deposit a reward in ETH for a specific pool. /// Note that this is only callable by the staking contract, and when /// not in catastrophic failure mode. @@ -124,7 +112,7 @@ contract StakingPoolRewardVault is ); // update balance and transfer `amount` in ETH to staking contract - balanceByPoolId[poolId].operatorBalance -= uint96(amount); + balanceByPoolId[poolId].operatorBalance -= amount._downcastToUint96(); stakingContractAddress.transfer(amount); // notify @@ -147,7 +135,7 @@ contract StakingPoolRewardVault is ); // update balance and transfer `amount` in ETH to staking contract - balanceByPoolId[poolId].membersBalance -= uint96(amount); + balanceByPoolId[poolId].membersBalance -= amount._downcastToUint96(); stakingContractAddress.transfer(amount); // notify diff --git a/contracts/staking/contracts/src/vaults/ZrxVault.sol b/contracts/staking/contracts/src/vaults/ZrxVault.sol index 44d49c1dca..5db7e4b870 100644 --- a/contracts/staking/contracts/src/vaults/ZrxVault.sol +++ b/contracts/staking/contracts/src/vaults/ZrxVault.sol @@ -20,7 +20,7 @@ pragma solidity ^0.5.5; import "../libs/LibSafeMath.sol"; import "../interfaces/IZrxVault.sol"; -import "../interfaces/IAssetProxy.sol"; +import "@0x/contracts-asset-proxy/contracts/src/interfaces/IAssetProxy.sol"; import "@0x/contracts-erc20/contracts/src/interfaces/IERC20Token.sol"; import "./MixinVaultCore.sol"; diff --git a/contracts/staking/package.json b/contracts/staking/package.json index a457509f16..7512c5f4a9 100644 --- a/contracts/staking/package.json +++ b/contracts/staking/package.json @@ -36,7 +36,7 @@ "compile:truffle": "truffle compile" }, "config": { - "abis": "./generated-artifacts/@(IAssetProxy|IStaking|IStakingEvents|IStakingPoolRewardVault|IStakingProxy|IStructs|IVaultCore|IWallet|IZrxVault|LibEIP712Hash|LibFeeMath|LibFeeMathTest|LibRewardMath|LibSafeMath|LibSafeMath64|LibSafeMath96|LibSignatureValidator|MixinConstants|MixinDelegatedStake|MixinDeploymentConstants|MixinExchangeFees|MixinExchangeManager|MixinOwnable|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakingPool|MixinStakingPoolRewardVault|MixinStakingPoolRewards|MixinStorage|MixinTimelockedStake|MixinVaultCore|MixinZrxVault|Staking|StakingPoolRewardVault|StakingProxy|ZrxVault).json", + "abis": "./generated-artifacts/@(IStaking|IStakingEvents|IStakingPoolRewardVault|IStakingProxy|IStructs|IVaultCore|IWallet|IZrxVault|LibEIP712Hash|LibFeeMath|LibFeeMathTest|LibRewardMath|LibSafeMath|LibSafeMath64|LibSafeMath96|LibSignatureValidator|MixinConstants|MixinDelegatedStake|MixinDeploymentConstants|MixinExchangeFees|MixinExchangeManager|MixinOwnable|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakingPool|MixinStakingPoolRewardVault|MixinStakingPoolRewards|MixinStorage|MixinTimelockedStake|MixinVaultCore|MixinZrxVault|Staking|StakingPoolRewardVault|StakingProxy|ZrxVault).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/staking/src/artifacts.ts b/contracts/staking/src/artifacts.ts index f48eebc424..52425fa81a 100644 --- a/contracts/staking/src/artifacts.ts +++ b/contracts/staking/src/artifacts.ts @@ -5,7 +5,6 @@ */ import { ContractArtifact } from 'ethereum-types'; -import * as IAssetProxy from '../generated-artifacts/IAssetProxy.json'; import * as IStaking from '../generated-artifacts/IStaking.json'; import * as IStakingEvents from '../generated-artifacts/IStakingEvents.json'; import * as IStakingPoolRewardVault from '../generated-artifacts/IStakingPoolRewardVault.json'; @@ -32,8 +31,8 @@ import * as MixinScheduler from '../generated-artifacts/MixinScheduler.json'; import * as MixinStake from '../generated-artifacts/MixinStake.json'; import * as MixinStakeBalances from '../generated-artifacts/MixinStakeBalances.json'; import * as MixinStakingPool from '../generated-artifacts/MixinStakingPool.json'; -import * as MixinStakingPoolRewards from '../generated-artifacts/MixinStakingPoolRewards.json'; import * as MixinStakingPoolRewardVault from '../generated-artifacts/MixinStakingPoolRewardVault.json'; +import * as MixinStakingPoolRewards from '../generated-artifacts/MixinStakingPoolRewards.json'; import * as MixinStorage from '../generated-artifacts/MixinStorage.json'; import * as MixinTimelockedStake from '../generated-artifacts/MixinTimelockedStake.json'; import * as MixinVaultCore from '../generated-artifacts/MixinVaultCore.json'; @@ -50,7 +49,6 @@ export const artifacts = { MixinConstants: MixinConstants as ContractArtifact, MixinDeploymentConstants: MixinDeploymentConstants as ContractArtifact, MixinStorage: MixinStorage as ContractArtifact, - IAssetProxy: IAssetProxy as ContractArtifact, IStaking: IStaking as ContractArtifact, IStakingEvents: IStakingEvents as ContractArtifact, IStakingPoolRewardVault: IStakingPoolRewardVault as ContractArtifact, diff --git a/contracts/staking/src/wrappers.ts b/contracts/staking/src/wrappers.ts index d46f317608..931b313c04 100644 --- a/contracts/staking/src/wrappers.ts +++ b/contracts/staking/src/wrappers.ts @@ -3,7 +3,6 @@ * Warning: This file is auto-generated by contracts-gen. Don't edit manually. * ----------------------------------------------------------------------------- */ -export * from '../generated-wrappers/i_asset_proxy'; export * from '../generated-wrappers/i_staking'; export * from '../generated-wrappers/i_staking_events'; export * from '../generated-wrappers/i_staking_pool_reward_vault'; diff --git a/contracts/staking/tsconfig.json b/contracts/staking/tsconfig.json index d82e516af3..bf4014ba65 100644 --- a/contracts/staking/tsconfig.json +++ b/contracts/staking/tsconfig.json @@ -3,7 +3,6 @@ "compilerOptions": { "outDir": "lib", "rootDir": ".", "resolveJsonModule": true }, "include": ["./src/**/*", "./test/**/*", "./generated-wrappers/**/*"], "files": [ - "generated-artifacts/IAssetProxy.json", "generated-artifacts/IStaking.json", "generated-artifacts/IStakingEvents.json", "generated-artifacts/IStakingPoolRewardVault.json",