From 703e890918a4b93fea36982e2f6a254ac8195eb4 Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Tue, 22 Oct 2019 16:22:21 -0400 Subject: [PATCH 1/2] `@0x/contracts-staking`: Call `StakingProxy.assertValidStorageParams()` in `MixinParams.setParams()` --- contracts/staking/CHANGELOG.json | 4 ++ .../staking/contracts/src/StakingProxy.sol | 6 +-- .../src/interfaces/IStakingProxy.sol | 9 ++++ .../staking/contracts/src/sys/MixinParams.sol | 5 ++ .../test/TestAssertStorageParams.sol | 2 +- .../contracts/test/TestMixinParams.sol | 47 +++++++++++++++++++ .../contracts/test/TestStakingProxy.sol | 4 +- contracts/staking/package.json | 2 +- contracts/staking/src/artifacts.ts | 2 + contracts/staking/src/wrappers.ts | 1 + contracts/staking/test/migration_test.ts | 20 ++++---- .../staking/test/unit_tests/params_test.ts | 14 ++++-- contracts/staking/tsconfig.json | 1 + 13 files changed, 96 insertions(+), 21 deletions(-) create mode 100644 contracts/staking/contracts/test/TestMixinParams.sol diff --git a/contracts/staking/CHANGELOG.json b/contracts/staking/CHANGELOG.json index 4cdc721b0d..953a1773c8 100644 --- a/contracts/staking/CHANGELOG.json +++ b/contracts/staking/CHANGELOG.json @@ -13,6 +13,10 @@ { "note": "Removed protocol fee != 0 assertion.", "pr": 2278 + }, + { + "note": "Call `StakingProxy.assertValidStorageParams()` in `MixinParams.setParams()`", + "pr": "TODO" } ] }, diff --git a/contracts/staking/contracts/src/StakingProxy.sol b/contracts/staking/contracts/src/StakingProxy.sol index cae4fff6af..7ab8c3381c 100644 --- a/contracts/staking/contracts/src/StakingProxy.sol +++ b/contracts/staking/contracts/src/StakingProxy.sol @@ -157,8 +157,8 @@ contract StakingProxy is // Asserts that a stake weight is <= 100%. // Asserts that pools allow >= 1 maker. // Asserts that all addresses are initialized. - function _assertValidStorageParams() - internal + function assertValidStorageParams() + public view { // Epoch length must be between 5 and 30 days long @@ -216,6 +216,6 @@ contract StakingProxy is } // Assert initialized storage values are valid - _assertValidStorageParams(); + assertValidStorageParams(); } } diff --git a/contracts/staking/contracts/src/interfaces/IStakingProxy.sol b/contracts/staking/contracts/src/interfaces/IStakingProxy.sol index c29560cfdf..03dac64c01 100644 --- a/contracts/staking/contracts/src/interfaces/IStakingProxy.sol +++ b/contracts/staking/contracts/src/interfaces/IStakingProxy.sol @@ -55,4 +55,13 @@ contract IStakingProxy { external view returns (IStructs.ReadOnlyState memory); + + /// @dev Asserts that an epoch is between 5 and 30 days long. + // Asserts that 0 < cobb douglas alpha value <= 1. + // Asserts that a stake weight is <= 100%. + // Asserts that pools allow >= 1 maker. + // Asserts that all addresses are initialized. + function assertValidStorageParams() + external + view; } diff --git a/contracts/staking/contracts/src/sys/MixinParams.sol b/contracts/staking/contracts/src/sys/MixinParams.sol index 169578f209..4fdb210401 100644 --- a/contracts/staking/contracts/src/sys/MixinParams.sol +++ b/contracts/staking/contracts/src/sys/MixinParams.sol @@ -22,6 +22,7 @@ import "@0x/contracts-utils/contracts/src/LibRichErrors.sol"; import "../immutable/MixinStorage.sol"; import "../immutable/MixinConstants.sol"; import "../interfaces/IStakingEvents.sol"; +import "../interfaces/IStakingProxy.sol"; import "../libs/LibStakingRichErrors.sol"; @@ -53,6 +54,10 @@ contract MixinParams is _cobbDouglasAlphaNumerator, _cobbDouglasAlphaDenominator ); + + // Let the staking proxy enforce that these parameters are within + // acceptable ranges. + IStakingProxy(address(this)).assertValidStorageParams(); } /// @dev Retrieves all configurable parameter values. diff --git a/contracts/staking/contracts/test/TestAssertStorageParams.sol b/contracts/staking/contracts/test/TestAssertStorageParams.sol index aa5c5c62b0..efe5355b89 100644 --- a/contracts/staking/contracts/test/TestAssertStorageParams.sol +++ b/contracts/staking/contracts/test/TestAssertStorageParams.sol @@ -49,7 +49,7 @@ contract TestAssertStorageParams is minimumPoolStake = params.minimumPoolStake; cobbDouglasAlphaNumerator = params.cobbDouglasAlphaNumerator; cobbDouglasAlphaDenominator = params.cobbDouglasAlphaDenominator; - _assertValidStorageParams(); + assertValidStorageParams(); } function _attachStakingContract(address) diff --git a/contracts/staking/contracts/test/TestMixinParams.sol b/contracts/staking/contracts/test/TestMixinParams.sol new file mode 100644 index 0000000000..6c51c584a7 --- /dev/null +++ b/contracts/staking/contracts/test/TestMixinParams.sol @@ -0,0 +1,47 @@ +/* + + Copyright 2019 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.9; +pragma experimental ABIEncoderV2; + +import "../src/sys/MixinParams.sol"; + + +contract TestMixinParams is + MixinParams +{ + bool public shouldFailAssertValidStorageParams; + + /// @dev Set `shouldFailAssertValidStorageParams` + function setShouldFailAssertValidStorageParams(bool shouldFail) + external + { + shouldFailAssertValidStorageParams = shouldFail; + } + + /// @dev `IStakingProxy.assertValidStorageParams()` that reverts if + /// `shouldFailAssertValidStorageParams` is true. + function assertValidStorageParams() + public + view + { + if (shouldFailAssertValidStorageParams) { + revert("ASSERT_VALID_STORAGE_PARAMS_FAILED"); + } + } +} diff --git a/contracts/staking/contracts/test/TestStakingProxy.sol b/contracts/staking/contracts/test/TestStakingProxy.sol index e1619f173a..b6c9ae1999 100644 --- a/contracts/staking/contracts/test/TestStakingProxy.sol +++ b/contracts/staking/contracts/test/TestStakingProxy.sol @@ -35,8 +35,8 @@ contract TestStakingProxy is ) {} - function _assertValidStorageParams() - internal + function assertValidStorageParams() + public view { require( diff --git a/contracts/staking/package.json b/contracts/staking/package.json index 0052b78911..a9f31bcc25 100644 --- a/contracts/staking/package.json +++ b/contracts/staking/package.json @@ -37,7 +37,7 @@ }, "config": { "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.", - "abis": "./generated-artifacts/@(IStaking|IStakingEvents|IStakingProxy|IStorage|IStorageInit|IStructs|IZrxVault|LibCobbDouglas|LibFixedMath|LibFixedMathRichErrors|LibProxy|LibSafeDowncast|LibStakingRichErrors|MixinAbstract|MixinConstants|MixinCumulativeRewards|MixinDeploymentConstants|MixinExchangeFees|MixinExchangeManager|MixinFinalizer|MixinParams|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakeStorage|MixinStakingPool|MixinStakingPoolRewards|MixinStorage|ReadOnlyProxy|Staking|StakingProxy|TestAssertStorageParams|TestCobbDouglas|TestCumulativeRewardTracking|TestDelegatorRewards|TestExchangeManager|TestFinalizer|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestMixinStake|TestMixinStakeStorage|TestProtocolFees|TestStaking|TestStakingNoWETH|TestStakingProxy|TestStorageLayoutAndConstants|ZrxVault|ZrxVaultBackstop).json" + "abis": "./generated-artifacts/@(IStaking|IStakingEvents|IStakingProxy|IStorage|IStorageInit|IStructs|IZrxVault|LibCobbDouglas|LibFixedMath|LibFixedMathRichErrors|LibProxy|LibSafeDowncast|LibStakingRichErrors|MixinAbstract|MixinConstants|MixinCumulativeRewards|MixinDeploymentConstants|MixinExchangeFees|MixinExchangeManager|MixinFinalizer|MixinParams|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakeStorage|MixinStakingPool|MixinStakingPoolRewards|MixinStorage|ReadOnlyProxy|Staking|StakingProxy|TestAssertStorageParams|TestCobbDouglas|TestCumulativeRewardTracking|TestDelegatorRewards|TestExchangeManager|TestFinalizer|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestMixinParams|TestMixinStake|TestMixinStakeStorage|TestProtocolFees|TestStaking|TestStakingNoWETH|TestStakingProxy|TestStorageLayoutAndConstants|ZrxVault|ZrxVaultBackstop).json" }, "repository": { "type": "git", diff --git a/contracts/staking/src/artifacts.ts b/contracts/staking/src/artifacts.ts index 9e31c1564e..797b33e51c 100644 --- a/contracts/staking/src/artifacts.ts +++ b/contracts/staking/src/artifacts.ts @@ -47,6 +47,7 @@ import * as TestLibFixedMath from '../generated-artifacts/TestLibFixedMath.json' import * as TestLibProxy from '../generated-artifacts/TestLibProxy.json'; import * as TestLibProxyReceiver from '../generated-artifacts/TestLibProxyReceiver.json'; import * as TestLibSafeDowncast from '../generated-artifacts/TestLibSafeDowncast.json'; +import * as TestMixinParams from '../generated-artifacts/TestMixinParams.json'; import * as TestMixinStake from '../generated-artifacts/TestMixinStake.json'; import * as TestMixinStakeStorage from '../generated-artifacts/TestMixinStakeStorage.json'; import * as TestProtocolFees from '../generated-artifacts/TestProtocolFees.json'; @@ -101,6 +102,7 @@ export const artifacts = { TestLibProxy: TestLibProxy as ContractArtifact, TestLibProxyReceiver: TestLibProxyReceiver as ContractArtifact, TestLibSafeDowncast: TestLibSafeDowncast as ContractArtifact, + TestMixinParams: TestMixinParams as ContractArtifact, TestMixinStake: TestMixinStake as ContractArtifact, TestMixinStakeStorage: TestMixinStakeStorage as ContractArtifact, TestProtocolFees: TestProtocolFees as ContractArtifact, diff --git a/contracts/staking/src/wrappers.ts b/contracts/staking/src/wrappers.ts index 0ce6576d60..70d0530291 100644 --- a/contracts/staking/src/wrappers.ts +++ b/contracts/staking/src/wrappers.ts @@ -45,6 +45,7 @@ export * from '../generated-wrappers/test_lib_fixed_math'; export * from '../generated-wrappers/test_lib_proxy'; export * from '../generated-wrappers/test_lib_proxy_receiver'; export * from '../generated-wrappers/test_lib_safe_downcast'; +export * from '../generated-wrappers/test_mixin_params'; export * from '../generated-wrappers/test_mixin_stake'; export * from '../generated-wrappers/test_mixin_stake_storage'; export * from '../generated-wrappers/test_protocol_fees'; diff --git a/contracts/staking/test/migration_test.ts b/contracts/staking/test/migration_test.ts index 7438bdeb7c..890e002c93 100644 --- a/contracts/staking/test/migration_test.ts +++ b/contracts/staking/test/migration_test.ts @@ -228,7 +228,7 @@ blockchainTests('Migration tests', env => { const expectedError = new StakingRevertErrors.InvalidParamValueError( StakingRevertErrors.InvalidParamValueErrorCodes.InvalidEpochDuration, ); - expect(tx).to.revertWith(expectedError); + return expect(tx).to.revertWith(expectedError); }); it('reverts if epoch duration is > 30 days', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ @@ -238,21 +238,21 @@ blockchainTests('Migration tests', env => { const expectedError = new StakingRevertErrors.InvalidParamValueError( StakingRevertErrors.InvalidParamValueErrorCodes.InvalidEpochDuration, ); - expect(tx).to.revertWith(expectedError); + return expect(tx).to.revertWith(expectedError); }); it('succeeds if epoch duration is 5 days', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ ...stakingConstants.DEFAULT_PARAMS, epochDurationInSeconds: fiveDays, }); - expect(tx).to.be.fulfilled(''); + return expect(tx).to.be.fulfilled(''); }); it('succeeds if epoch duration is 30 days', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ ...stakingConstants.DEFAULT_PARAMS, epochDurationInSeconds: thirtyDays, }); - expect(tx).to.be.fulfilled(''); + return expect(tx).to.be.fulfilled(''); }); it('reverts if alpha denominator is 0', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ @@ -262,7 +262,7 @@ blockchainTests('Migration tests', env => { const expectedError = new StakingRevertErrors.InvalidParamValueError( StakingRevertErrors.InvalidParamValueErrorCodes.InvalidCobbDouglasAlpha, ); - expect(tx).to.revertWith(expectedError); + return expect(tx).to.revertWith(expectedError); }); it('reverts if alpha > 1', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ @@ -273,7 +273,7 @@ blockchainTests('Migration tests', env => { const expectedError = new StakingRevertErrors.InvalidParamValueError( StakingRevertErrors.InvalidParamValueErrorCodes.InvalidCobbDouglasAlpha, ); - expect(tx).to.revertWith(expectedError); + return expect(tx).to.revertWith(expectedError); }); it('succeeds if alpha == 1', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ @@ -281,7 +281,7 @@ blockchainTests('Migration tests', env => { cobbDouglasAlphaNumerator: new BigNumber(1), cobbDouglasAlphaDenominator: new BigNumber(1), }); - expect(tx).to.be.fulfilled(''); + return expect(tx).to.be.fulfilled(''); }); it('succeeds if alpha == 0', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ @@ -289,7 +289,7 @@ blockchainTests('Migration tests', env => { cobbDouglasAlphaNumerator: constants.ZERO_AMOUNT, cobbDouglasAlphaDenominator: new BigNumber(1), }); - expect(tx).to.be.fulfilled(''); + return expect(tx).to.be.fulfilled(''); }); it('reverts if delegation weight is > 100%', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ @@ -299,14 +299,14 @@ blockchainTests('Migration tests', env => { const expectedError = new StakingRevertErrors.InvalidParamValueError( StakingRevertErrors.InvalidParamValueErrorCodes.InvalidRewardDelegatedStakeWeight, ); - expect(tx).to.revertWith(expectedError); + return expect(tx).to.revertWith(expectedError); }); it('succeeds if delegation weight is 100%', async () => { const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({ ...stakingConstants.DEFAULT_PARAMS, rewardDelegatedStakeWeight: new BigNumber(stakingConstants.PPM), }); - expect(tx).to.be.fulfilled(''); + return expect(tx).to.be.fulfilled(''); }); }); }); diff --git a/contracts/staking/test/unit_tests/params_test.ts b/contracts/staking/test/unit_tests/params_test.ts index f24c6ec983..e49a46a141 100644 --- a/contracts/staking/test/unit_tests/params_test.ts +++ b/contracts/staking/test/unit_tests/params_test.ts @@ -3,20 +3,20 @@ import { AuthorizableRevertErrors, BigNumber } from '@0x/utils'; import { TransactionReceiptWithDecodedLogs } from 'ethereum-types'; import * as _ from 'lodash'; -import { artifacts, IStakingEventsParamsSetEventArgs, MixinParamsContract } from '../../src/'; +import { artifacts, IStakingEventsParamsSetEventArgs, TestMixinParamsContract } from '../../src/'; import { constants as stakingConstants } from '../utils/constants'; import { StakingParams } from '../utils/types'; blockchainTests('Configurable Parameters unit tests', env => { - let testContract: MixinParamsContract; + let testContract: TestMixinParamsContract; let authorizedAddress: string; let notAuthorizedAddress: string; before(async () => { [authorizedAddress, notAuthorizedAddress] = await env.getAccountAddressesAsync(); - testContract = await MixinParamsContract.deployFrom0xArtifactAsync( - artifacts.MixinParams, + testContract = await TestMixinParamsContract.deployFrom0xArtifactAsync( + artifacts.TestMixinParams, env.provider, env.txDefaults, artifacts, @@ -66,6 +66,12 @@ blockchainTests('Configurable Parameters unit tests', env => { return expect(tx).to.revertWith(expectedError); }); + it('throws if `assertValidStorageParams()` throws`', async () => { + await testContract.setShouldFailAssertValidStorageParams.awaitTransactionSuccessAsync(true); + const tx = setParamsAndAssertAsync({}); + return expect(tx).to.revertWith('ASSERT_VALID_STORAGE_PARAMS_FAILED'); + }); + it('works if called by owner', async () => { return setParamsAndAssertAsync({}); }); diff --git a/contracts/staking/tsconfig.json b/contracts/staking/tsconfig.json index b8371bcb1b..69b05735ad 100644 --- a/contracts/staking/tsconfig.json +++ b/contracts/staking/tsconfig.json @@ -45,6 +45,7 @@ "generated-artifacts/TestLibProxy.json", "generated-artifacts/TestLibProxyReceiver.json", "generated-artifacts/TestLibSafeDowncast.json", + "generated-artifacts/TestMixinParams.json", "generated-artifacts/TestMixinStake.json", "generated-artifacts/TestMixinStakeStorage.json", "generated-artifacts/TestProtocolFees.json", From 07e1d502e7feae5c7784dd9331111a816fbdb55e Mon Sep 17 00:00:00 2001 From: Lawrence Forman Date: Tue, 22 Oct 2019 16:29:34 -0400 Subject: [PATCH 2/2] `@0x/contracts-staking`: Update changelog. --- contracts/staking/CHANGELOG.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/staking/CHANGELOG.json b/contracts/staking/CHANGELOG.json index 953a1773c8..bcdb6fd73a 100644 --- a/contracts/staking/CHANGELOG.json +++ b/contracts/staking/CHANGELOG.json @@ -16,7 +16,7 @@ }, { "note": "Call `StakingProxy.assertValidStorageParams()` in `MixinParams.setParams()`", - "pr": "TODO" + "pr": 2279 } ] },