Move isFunctionRemoveAuthorizedAddress to test

6 files changed, 70 insertions, 22 deletions

diff --git a/packages/contracts/compiler.json b/packages/contracts/compiler.json
index f1646e79e..6b7b69caa 100644
--- a/packages/contracts/compiler.json
+++ b/packages/contracts/compiler.json
@@ -30,6 +30,7 @@
         "MixinAuthorizable",
         "MultiSigWallet",
         "MultiSigWalletWithTimeLock",
+        "TestAssetProxyOwner",
         "TestAssetDataDecoders",
         "TestAssetProxyDispatcher",
         "TestLibBytes",
diff --git a/packages/contracts/package.json b/packages/contracts/package.json
index 87f88a8eb..8249b9e0c 100644
--- a/packages/contracts/package.json
+++ b/packages/contracts/package.json
@@ -34,7 +34,7 @@
     },
     "config": {
         "abis":
-            "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|ERC20Proxy|ERC721Proxy|Exchange|ExchangeWrapper|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|TestAssetDataDecoders|TestAssetProxyDispatcher|TestLibBytes|TestLibs|TestSignatureValidator|TestValidator|TestWallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json"
+            "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|ERC20Proxy|ERC721Proxy|Exchange|ExchangeWrapper|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|TestAssetProxyOwner|TestAssetDataDecoders|TestAssetProxyDispatcher|TestLibBytes|TestLibs|TestSignatureValidator|TestValidator|TestWallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json"
     },
     "repository": {
         "type": "git",
diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol
index 50c538cea..79f64312f 100644
--- a/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol
+++ b/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol
@@ -39,7 +39,7 @@ contract AssetProxyOwner is
     modifier validRemoveAuthorizedAddressTx(uint256 transactionId) {
         Transaction storage tx = transactions[transactionId];
         require(isAssetProxyRegistered[tx.destination]);
-        require(isFunctionRemoveAuthorizedAddress(tx.data));
+        require(tx.data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_SELECTOR);
         _;
     }
 
@@ -95,17 +95,4 @@ contract AssetProxyOwner is
             tx.executed = false;
         }
     }
-
-    /// @dev Compares first 4 bytes of byte array to removeAuthorizedAddress function selector.
-    /// @param data Transaction data.
-    /// @return Successful if data is a call to removeAuthorizedAddress.
-    function isFunctionRemoveAuthorizedAddress(bytes memory data)
-        public
-        pure
-        returns (bool)
-    {
-        bytes4 first4Bytes = data.readBytes4(0);
-        require(REMOVE_AUTHORIZED_ADDRESS_SELECTOR == first4Bytes);
-        return true;
-    }
 }
diff --git a/packages/contracts/src/contracts/current/test/TestAssetProxyOwner/TestAssetProxyOwner.sol b/packages/contracts/src/contracts/current/test/TestAssetProxyOwner/TestAssetProxyOwner.sol
new file mode 100644
index 000000000..35746d2ad
--- /dev/null
+++ b/packages/contracts/src/contracts/current/test/TestAssetProxyOwner/TestAssetProxyOwner.sol
@@ -0,0 +1,47 @@
+/*
+
+  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.4.24;
+
+import "../../protocol/AssetProxyOwner/AssetProxyOwner.sol";
+
+contract TestAssetProxyOwner is
+    AssetProxyOwner
+{
+    constructor(
+        address[] memory _owners,
+        address[] memory _assetProxyContracts,
+        uint256 _required,
+        uint256 _secondsTimeLocked
+    )
+        public
+        AssetProxyOwner(_owners, _assetProxyContracts, _required, _secondsTimeLocked)
+    {
+    }
+    
+    /// @dev Compares first 4 bytes of byte array to removeAuthorizedAddress function selector.
+    /// @param data Transaction data.
+    /// @return Successful if data is a call to removeAuthorizedAddress.
+    function isFunctionRemoveAuthorizedAddress(bytes memory data)
+        public
+        pure
+        returns (bool)
+    {
+        return data.readBytes4(0) == REMOVE_AUTHORIZED_ADDRESS_SELECTOR;
+    }
+}
diff --git a/packages/contracts/src/utils/artifacts.ts b/packages/contracts/src/utils/artifacts.ts
index fa18cc9d2..a46bab9ae 100644
--- a/packages/contracts/src/utils/artifacts.ts
+++ b/packages/contracts/src/utils/artifacts.ts
@@ -13,6 +13,7 @@ import * as MultiSigWallet from '../artifacts/MultiSigWallet.json';
 import * as MultiSigWalletWithTimeLock from '../artifacts/MultiSigWalletWithTimeLock.json';
 import * as TestAssetDataDecoders from '../artifacts/TestAssetDataDecoders.json';
 import * as TestAssetProxyDispatcher from '../artifacts/TestAssetProxyDispatcher.json';
+import * as TestAssetProxyOwner from '../artifacts/TestAssetProxyOwner.json';
 import * as TestLibBytes from '../artifacts/TestLibBytes.json';
 import * as TestLibs from '../artifacts/TestLibs.json';
 import * as TestSignatureValidator from '../artifacts/TestSignatureValidator.json';
@@ -36,6 +37,7 @@ export const artifacts = {
     MixinAuthorizable: (MixinAuthorizable as any) as ContractArtifact,
     MultiSigWallet: (MultiSigWallet as any) as ContractArtifact,
     MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact,
+    TestAssetProxyOwner: (TestAssetProxyOwner as any) as ContractArtifact,
     TestAssetProxyDispatcher: (TestAssetProxyDispatcher as any) as ContractArtifact,
     TestAssetDataDecoders: (TestAssetDataDecoders as any) as ContractArtifact,
     TestLibBytes: (TestLibBytes as any) as ContractArtifact,
diff --git a/packages/contracts/test/asset_proxy_owner.ts b/packages/contracts/test/asset_proxy_owner.ts
index aa4999e95..a46882828 100644
--- a/packages/contracts/test/asset_proxy_owner.ts
+++ b/packages/contracts/test/asset_proxy_owner.ts
@@ -10,11 +10,11 @@ import {
     ExecutionFailureContractEventArgs,
     SubmissionContractEventArgs,
 } from '../src/generated_contract_wrappers/asset_proxy_owner';
-import { MixinAuthorizableContract } from '../src/generated_contract_wrappers/mixin_authorizable';
+import { MixinAuthorizableContract } from  '../src/generated_contract_wrappers/mixin_authorizable';
+import { TestAssetProxyOwnerContract} from '../src/generated_contract_wrappers/test_asset_proxy_owner';
 import { artifacts } from '../src/utils/artifacts';
 import {
     expectRevertOrAlwaysFailingTransactionAsync,
-    expectRevertOrContractCallFailedAsync,
 } from '../src/utils/assertions';
 import { chaiSetup } from '../src/utils/chai_setup';
 import { constants } from '../src/utils/constants';
@@ -35,6 +35,7 @@ describe('AssetProxyOwner', () => {
     let erc20Proxy: MixinAuthorizableContract;
     let erc721Proxy: MixinAuthorizableContract;
     let multiSig: AssetProxyOwnerContract;
+    let testAssetProxyOwner: TestAssetProxyOwnerContract;
     let multiSigWrapper: MultiSigWrapper;
 
     before(async () => {
@@ -68,6 +69,15 @@ describe('AssetProxyOwner', () => {
             SECONDS_TIME_LOCKED,
         );
         multiSigWrapper = new MultiSigWrapper(multiSig, provider);
+        testAssetProxyOwner = await TestAssetProxyOwnerContract.deployFrom0xArtifactAsync(
+            artifacts.TestAssetProxyOwner,
+            provider,
+            txDefaults,
+            owners,
+            defaultAssetProxyContractAddresses,
+            REQUIRED_APPROVALS,
+            SECONDS_TIME_LOCKED,
+        );
         await web3Wrapper.awaitTransactionSuccessAsync(
             await erc20Proxy.transferOwnership.sendTransactionAsync(multiSig.address, { from: initialOwner }),
             constants.AWAIT_TRANSACTION_MINED_MS,
@@ -118,23 +128,24 @@ describe('AssetProxyOwner', () => {
     });
 
     describe('isFunctionRemoveAuthorizedAddress', () => {
-        it('should throw if data is not for removeAuthorizedAddress', async () => {
+        it('should return false if data is not for removeAuthorizedAddress', async () => {
             const notRemoveAuthorizedAddressData = erc20Proxy.addAuthorizedAddress.getABIEncodedTransactionData(
                 owners[0],
             );
-            return expectRevertOrContractCallFailedAsync(
-                multiSig.isFunctionRemoveAuthorizedAddress.callAsync(notRemoveAuthorizedAddressData),
+            const result = await testAssetProxyOwner.isFunctionRemoveAuthorizedAddress.callAsync(
+                notRemoveAuthorizedAddressData,
             );
+            expect(result).to.be.false();
         });
 
         it('should return true if data is for removeAuthorizedAddress', async () => {
             const removeAuthorizedAddressData = erc20Proxy.removeAuthorizedAddress.getABIEncodedTransactionData(
                 owners[0],
             );
-            const isFunctionRemoveAuthorizedAddress = await multiSig.isFunctionRemoveAuthorizedAddress.callAsync(
+            const result = await testAssetProxyOwner.isFunctionRemoveAuthorizedAddress.callAsync(
                 removeAuthorizedAddressData,
             );
-            expect(isFunctionRemoveAuthorizedAddress).to.be.true();
+            expect(result).to.be.true();
         });
     });