diff --git a/contracts/staking/contracts/test/TestExchangeManager.sol b/contracts/staking/contracts/test/TestExchangeManager.sol new file mode 100644 index 0000000000..5a47aae84b --- /dev/null +++ b/contracts/staking/contracts/test/TestExchangeManager.sol @@ -0,0 +1,42 @@ +/* + + 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/Staking.sol"; + + +contract TestExchangeManager is + Staking +{ + function setValidExchange(address exchange) + external + { + validExchanges[exchange] = true; + } + + function onlyExchangeFunction() + external + view + onlyExchange + returns (bool) + { + return true; + } +} diff --git a/contracts/staking/package.json b/contracts/staking/package.json index 0889a633e4..5784ca33b4 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|TestFinalizer|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestProtocolFees|TestStaking|TestStakingNoWETH|TestStakingProxy|TestStorageLayoutAndConstants|ZrxVault).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|TestProtocolFees|TestStaking|TestStakingNoWETH|TestStakingProxy|TestStorageLayoutAndConstants|ZrxVault).json" }, "repository": { "type": "git", diff --git a/contracts/staking/src/artifacts.ts b/contracts/staking/src/artifacts.ts index c7c7930e16..effc576939 100644 --- a/contracts/staking/src/artifacts.ts +++ b/contracts/staking/src/artifacts.ts @@ -40,6 +40,7 @@ import * as TestAssertStorageParams from '../generated-artifacts/TestAssertStora import * as TestCobbDouglas from '../generated-artifacts/TestCobbDouglas.json'; import * as TestCumulativeRewardTracking from '../generated-artifacts/TestCumulativeRewardTracking.json'; import * as TestDelegatorRewards from '../generated-artifacts/TestDelegatorRewards.json'; +import * as TestExchangeManager from '../generated-artifacts/TestExchangeManager.json'; import * as TestFinalizer from '../generated-artifacts/TestFinalizer.json'; import * as TestInitTarget from '../generated-artifacts/TestInitTarget.json'; import * as TestLibFixedMath from '../generated-artifacts/TestLibFixedMath.json'; @@ -89,6 +90,7 @@ export const artifacts = { TestCobbDouglas: TestCobbDouglas as ContractArtifact, TestCumulativeRewardTracking: TestCumulativeRewardTracking as ContractArtifact, TestDelegatorRewards: TestDelegatorRewards as ContractArtifact, + TestExchangeManager: TestExchangeManager as ContractArtifact, TestFinalizer: TestFinalizer as ContractArtifact, TestInitTarget: TestInitTarget as ContractArtifact, TestLibFixedMath: TestLibFixedMath as ContractArtifact, diff --git a/contracts/staking/src/wrappers.ts b/contracts/staking/src/wrappers.ts index a36e5b2651..684ab66a60 100644 --- a/contracts/staking/src/wrappers.ts +++ b/contracts/staking/src/wrappers.ts @@ -38,6 +38,7 @@ export * from '../generated-wrappers/test_assert_storage_params'; export * from '../generated-wrappers/test_cobb_douglas'; export * from '../generated-wrappers/test_cumulative_reward_tracking'; export * from '../generated-wrappers/test_delegator_rewards'; +export * from '../generated-wrappers/test_exchange_manager'; export * from '../generated-wrappers/test_finalizer'; export * from '../generated-wrappers/test_init_target'; export * from '../generated-wrappers/test_lib_fixed_math'; diff --git a/contracts/staking/test/exchange_test.ts b/contracts/staking/test/exchange_test.ts deleted file mode 100644 index 2530a483d7..0000000000 --- a/contracts/staking/test/exchange_test.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { ERC20Wrapper } from '@0x/contracts-asset-proxy'; -import { blockchainTests, expect } from '@0x/contracts-test-utils'; -import { StakingRevertErrors } from '@0x/order-utils'; -import * as _ from 'lodash'; - -import { deployAndConfigureContractsAsync, StakingApiWrapper } from './utils/api_wrapper'; - -// tslint:disable:no-unnecessary-type-assertion -blockchainTests('Exchange Integrations', env => { - // tokens & addresses - let accounts: string[]; - let owner: string; - let exchange: string; - // wrappers - let stakingApiWrapper: StakingApiWrapper; - let erc20Wrapper: ERC20Wrapper; - // tests - before(async () => { - // create accounts - accounts = await env.getAccountAddressesAsync(); - owner = accounts[0]; - exchange = accounts[1]; - // set up ERC20Wrapper - erc20Wrapper = new ERC20Wrapper(env.provider, accounts, owner); - // deploy staking contracts - stakingApiWrapper = await deployAndConfigureContractsAsync(env, owner, erc20Wrapper); - }); - blockchainTests.resets('Exchange Tracking in Staking Contract', () => { - it('basic exchange tracking', async () => { - const { validExchanges, addExchangeAddress, removeExchangeAddress } = stakingApiWrapper.stakingContract; - // 1 try querying an invalid addresses - const invalidAddress = '0x0000000000000000000000000000000000000001'; - const isInvalidAddressValid = await validExchanges.callAsync(invalidAddress); - expect(isInvalidAddressValid).to.be.false(); - // 2 add valid address - await addExchangeAddress.awaitTransactionSuccessAsync(exchange); - const isValidAddressValid = await validExchanges.callAsync(exchange); - expect(isValidAddressValid).to.be.true(); - // 3 try adding valid address again - let revertError = new StakingRevertErrors.ExchangeManagerError( - StakingRevertErrors.ExchangeManagerErrorCodes.ExchangeAlreadyRegistered, - exchange, - ); - let tx = addExchangeAddress.awaitTransactionSuccessAsync(exchange); - await expect(tx).to.revertWith(revertError); - // 4 remove valid address - await removeExchangeAddress.awaitTransactionSuccessAsync(exchange); - const isValidAddressStillValid = await validExchanges.callAsync(exchange); - expect(isValidAddressStillValid).to.be.false(); - // 5 try removing valid address again - revertError = new StakingRevertErrors.ExchangeManagerError( - StakingRevertErrors.ExchangeManagerErrorCodes.ExchangeNotRegistered, - exchange, - ); - tx = removeExchangeAddress.awaitTransactionSuccessAsync(exchange); - await expect(tx).to.revertWith(revertError); - // @todo should not be able to add / remove an exchange if not contract owner. - }); - }); -}); -// tslint:enable:no-unnecessary-type-assertion diff --git a/contracts/staking/test/unit_tests/exchange_test.ts b/contracts/staking/test/unit_tests/exchange_test.ts new file mode 100644 index 0000000000..da9bb4d2aa --- /dev/null +++ b/contracts/staking/test/unit_tests/exchange_test.ts @@ -0,0 +1,131 @@ +import { blockchainTests, expect } from '@0x/contracts-test-utils'; +import { StakingRevertErrors } from '@0x/order-utils'; +import { AuthorizableRevertErrors } from '@0x/utils'; +import * as _ from 'lodash'; + +import { artifacts, TestExchangeManagerContract } from '../../src'; + +blockchainTests.resets.only('Exchange Unit Tests', env => { + // Addresses + let nonOwner: string; + let owner: string; + let nonExchange: string; + let exchange: string; + let nonAuthority: string; + let authority: string; + + // Exchange Manager + let exchangeManager: TestExchangeManagerContract; + + before(async () => { + // Set up addresses for testing. + [nonOwner, owner, nonExchange, exchange, nonAuthority, authority] = await env.getAccountAddressesAsync(); + + // Deploy the Exchange Manager contract. + exchangeManager = await TestExchangeManagerContract.deployFrom0xArtifactAsync( + artifacts.TestExchangeManager, + env.provider, + { + ...env.txDefaults, + from: owner, + }, + artifacts, + ); + + // Register the exchange. + await exchangeManager.setValidExchange.awaitTransactionSuccessAsync(exchange); + + // Register an authority. + await exchangeManager.addAuthorizedAddress.awaitTransactionSuccessAsync(authority, { from: owner }); + }); + + describe('onlyExchange', () => { + it('should revert if called by an unregistered exchange', async () => { + const expectedError = new StakingRevertErrors.OnlyCallableByExchangeError(nonExchange); + return expect(exchangeManager.onlyExchangeFunction.callAsync({ from: nonExchange })).to.revertWith( + expectedError, + ); + }); + + it('should succeed if called by a registered exchange', async () => { + const didSucceed = await exchangeManager.onlyExchangeFunction.callAsync({ from: exchange }); + expect(didSucceed).to.be.true(); + }); + }); + + describe('addExchangeAddress', () => { + it('should revert if called by an unauthorized address', async () => { + const expectedError = new AuthorizableRevertErrors.SenderNotAuthorizedError(nonAuthority); + const tx = exchangeManager.addExchangeAddress.awaitTransactionSuccessAsync(nonExchange, { + from: nonAuthority, + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should successfully add an exchange if called by the (authorized) owner', async () => { + // Register a new exchange. + await exchangeManager.addExchangeAddress.awaitTransactionSuccessAsync(nonExchange, { from: owner }); + + // Ensure that the exchange was successfully registered. + const isValidExchange = await exchangeManager.validExchanges.callAsync(nonExchange); + expect(isValidExchange).to.be.true(); + }); + + it('should successfully add an exchange if called by an authorized address', async () => { + // Register a new exchange. + await exchangeManager.addExchangeAddress.awaitTransactionSuccessAsync(nonExchange, { from: authority }); + + // Ensure that the exchange was successfully registered. + const isValidExchange = await exchangeManager.validExchanges.callAsync(nonExchange); + expect(isValidExchange).to.be.true(); + }); + + it('should fail to add an exchange redundantly', async () => { + const expectedError = new StakingRevertErrors.ExchangeManagerError( + StakingRevertErrors.ExchangeManagerErrorCodes.ExchangeAlreadyRegistered, + exchange, + ); + const tx = exchangeManager.addExchangeAddress.awaitTransactionSuccessAsync(exchange, { from: authority }); + return expect(tx).to.revertWith(expectedError); + }); + }); + + describe('removeExchangeAddress', () => { + it('should revert if called by an unauthorized address', async () => { + const expectedError = new AuthorizableRevertErrors.SenderNotAuthorizedError(nonAuthority); + const tx = exchangeManager.removeExchangeAddress.awaitTransactionSuccessAsync(exchange, { + from: nonAuthority, + }); + return expect(tx).to.revertWith(expectedError); + }); + + it('should successfully remove an exchange if called by the (authorized) owner', async () => { + // Remove the registered exchange. + await exchangeManager.removeExchangeAddress.awaitTransactionSuccessAsync(exchange, { from: owner }); + + // Ensure that the exchange was removed. + const isValidExchange = await exchangeManager.validExchanges.callAsync(exchange); + expect(isValidExchange).to.be.false(); + }); + + it('should successfully remove a registered exchange if called by an authorized address', async () => { + // Remove the registered exchange. + await exchangeManager.removeExchangeAddress.awaitTransactionSuccessAsync(exchange, { from: authority }); + + // Ensure that the exchange was removed. + const isValidExchange = await exchangeManager.validExchanges.callAsync(exchange); + expect(isValidExchange).to.be.false(); + }); + + it('should fail to remove an exchange redundantly', async () => { + const expectedError = new StakingRevertErrors.ExchangeManagerError( + StakingRevertErrors.ExchangeManagerErrorCodes.ExchangeNotRegistered, + nonExchange, + ); + const tx = exchangeManager.removeExchangeAddress.awaitTransactionSuccessAsync(nonExchange, { + from: authority, + }); + return expect(tx).to.revertWith(expectedError); + }); + }); +}); diff --git a/contracts/staking/tsconfig.json b/contracts/staking/tsconfig.json index 466ca080b3..2e0a90ed6d 100644 --- a/contracts/staking/tsconfig.json +++ b/contracts/staking/tsconfig.json @@ -38,6 +38,7 @@ "generated-artifacts/TestCobbDouglas.json", "generated-artifacts/TestCumulativeRewardTracking.json", "generated-artifacts/TestDelegatorRewards.json", + "generated-artifacts/TestExchangeManager.json", "generated-artifacts/TestFinalizer.json", "generated-artifacts/TestInitTarget.json", "generated-artifacts/TestLibFixedMath.json", diff --git a/contracts/utils/contracts/src/Authorizable.sol b/contracts/utils/contracts/src/Authorizable.sol index 38a7a3eed2..e0c6fbfea0 100644 --- a/contracts/utils/contracts/src/Authorizable.sol +++ b/contracts/utils/contracts/src/Authorizable.sol @@ -151,6 +151,6 @@ contract Authorizable is delete authorized[target]; authorities[index] = authorities[authorities.length - 1]; authorities.length -= 1; - emit AuthorizedAddressRemoved(target, msg.sender); + emit AuthorizedAddressRemoved(target, msg.sender); } }