From d4aacd218a4fdb1876ac656e04fcccdb892a9395 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 22 May 2018 07:39:21 -0700 Subject: Move readFirst4 to LibBytes --- .../protocol/AssetProxyOwner/AssetProxyOwner.sol | 31 ++++++++-------------- .../current/test/TestLibBytes/TestLibBytes.sol | 12 +++++++++ .../contracts/current/utils/LibBytes/LibBytes.sol | 19 +++++++++++++ packages/contracts/test/asset_proxy_owner.ts | 22 +++------------ packages/contracts/test/libraries/lib_bytes.ts | 8 ++++++ 5 files changed, 53 insertions(+), 39 deletions(-) diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol b/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol index 1061d0b9a..1b74dfc44 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxyOwner/AssetProxyOwner.sol @@ -19,8 +19,10 @@ pragma solidity ^0.4.10; import "../../multisig/MultiSigWalletWithTimeLock.sol"; +import "../../utils/LibBytes/LibBytes.sol"; contract AssetProxyOwner is + LibBytes, MultiSigWalletWithTimeLock { @@ -31,6 +33,8 @@ contract AssetProxyOwner is bytes4 constant REMOVE_AUTHORIZED_ADDRESS_SELECTOR = bytes4(keccak256("removeAuthorizedAddress(address)")); + /// @dev Function will revert if the transaction does not call `removeAuthorizedAddress` + /// on an approved AssetProxy contract. modifier validRemoveAuthorizedAddressTx(uint256 transactionId) { Transaction storage tx = transactions[transactionId]; require(isAssetProxyRegistered[tx.destination]); @@ -41,20 +45,22 @@ contract AssetProxyOwner is /// @dev Contract constructor sets initial owners, required number of confirmations, /// time lock, and list of AssetProxy addresses. /// @param _owners List of initial owners. + /// @param _assetProxyContracts Array of AssetProxy contract addresses. /// @param _required Number of required confirmations. /// @param _secondsTimeLocked Duration needed after a transaction is confirmed and before it becomes executable, in seconds. - /// @param _assetProxyContracts Array of AssetProxy contract addresses. function AssetProxyOwner( address[] memory _owners, + address[] memory _assetProxyContracts, uint256 _required, - uint256 _secondsTimeLocked, - address[] memory _assetProxyContracts) + uint256 _secondsTimeLocked + ) public MultiSigWalletWithTimeLock(_owners, _required, _secondsTimeLocked) { for (uint256 i = 0; i < _assetProxyContracts.length; i++) { - require(_assetProxyContracts[i] != address(0)); - isAssetProxyRegistered[_assetProxyContracts[i]] = true; + address assetProxy = _assetProxyContracts[i]; + require(assetProxy != address(0)); + isAssetProxyRegistered[assetProxy] = true; } } @@ -101,19 +107,4 @@ contract AssetProxyOwner is require(REMOVE_AUTHORIZED_ADDRESS_SELECTOR == first4Bytes); return true; } - - /// @dev Reads the first 4 bytes from a byte array of arbitrary length. - /// @param data Byte array to read first 4 bytes from. - /// @return First 4 bytes of data. - function readFirst4(bytes memory data) - public - pure - returns (bytes4 result) - { - require(data.length >= 4); - assembly { - result := mload(add(data, 32)) - } - return result; - } } diff --git a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol index ac4602933..5a6801262 100644 --- a/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol +++ b/packages/contracts/src/contracts/current/test/TestLibBytes/TestLibBytes.sol @@ -130,4 +130,16 @@ contract TestLibBytes is writeUint256(b, index, input); return b; } + + /// @dev Reads the first 4 bytes from a byte array of arbitrary length. + /// @param b Byte array to read first 4 bytes from. + /// @return First 4 bytes of data. + function publicReadFirst4(bytes memory b) + public + pure + returns (bytes4 result) + { + result = readFirst4(b); + return result; + } } diff --git a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol index 2c5d9e756..975565773 100644 --- a/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol +++ b/packages/contracts/src/contracts/current/utils/LibBytes/LibBytes.sol @@ -21,6 +21,7 @@ pragma solidity ^0.4.24; contract LibBytes { // Revert reasons + string constant GTE_4_LENGTH_REQUIRED = "Length must be greater than or equal to 4."; string constant GTE_20_LENGTH_REQUIRED = "Length must be greater than or equal to 20."; string constant GTE_32_LENGTH_REQUIRED = "Length must be greater than or equal to 32."; @@ -203,4 +204,22 @@ contract LibBytes { { writeBytes32(b, index, bytes32(input)); } + + /// @dev Reads the first 4 bytes from a byte array of arbitrary length. + /// @param b Byte array to read first 4 bytes from. + /// @return First 4 bytes of data. + function readFirst4(bytes memory b) + internal + pure + returns (bytes4 result) + { + require( + b.length >= 4, + GTE_4_LENGTH_REQUIRED + ); + assembly { + result := mload(add(b, 32)) + } + return result; + } } diff --git a/packages/contracts/test/asset_proxy_owner.ts b/packages/contracts/test/asset_proxy_owner.ts index 6e999dd99..e3c6a5324 100644 --- a/packages/contracts/test/asset_proxy_owner.ts +++ b/packages/contracts/test/asset_proxy_owner.ts @@ -56,9 +56,9 @@ describe('AssetProxyOwner', () => { provider, txDefaults, owners, + defaultAssetProxyContractAddresses, REQUIRED_APPROVALS, SECONDS_TIME_LOCKED, - defaultAssetProxyContractAddresses, ); multiSigWrapper = new MultiSigWrapper(multiSig, zeroEx); await erc20Proxy.transferOwnership.sendTransactionAsync(multiSig.address, { from: initialOwner }); @@ -79,9 +79,9 @@ describe('AssetProxyOwner', () => { provider, txDefaults, owners, + assetProxyContractAddresses, REQUIRED_APPROVALS, SECONDS_TIME_LOCKED, - assetProxyContractAddresses, ); const isErc20ProxyRegistered = await newMultiSig.isAssetProxyRegistered.callAsync(erc20Proxy.address); const isErc721ProxyRegistered = await newMultiSig.isAssetProxyRegistered.callAsync(erc721Proxy.address); @@ -96,29 +96,13 @@ describe('AssetProxyOwner', () => { provider, txDefaults, owners, + assetProxyContractAddresses, REQUIRED_APPROVALS, SECONDS_TIME_LOCKED, - assetProxyContractAddresses, ), ).to.be.rejectedWith(constants.REVERT); }); }); - describe('readFirst4', () => { - it('should return the first 4 bytes of a byte array of arbitrary length', async () => { - const addAuthorizedAddressData = erc20Proxy.addAuthorizedAddress.getABIEncodedTransactionData(owners[0]); - const removeAuthorizedAddressData = erc20Proxy.removeAuthorizedAddress.getABIEncodedTransactionData( - owners[0], - ); - const expectedAddAuthorizedAddressSelector = addAuthorizedAddressData.slice(0, 10); - const expectedRemoveAuthorizedAddressSelector = removeAuthorizedAddressData.slice(0, 10); - const [addAuthorizedAddressSelector, removeAuthorizedAddressSelector] = await Promise.all([ - multiSig.readFirst4.callAsync(addAuthorizedAddressData), - multiSig.readFirst4.callAsync(removeAuthorizedAddressData), - ]); - expect(expectedAddAuthorizedAddressSelector).to.equal(addAuthorizedAddressSelector); - expect(expectedRemoveAuthorizedAddressSelector).to.equal(removeAuthorizedAddressSelector); - }); - }); describe('isFunctionRemoveAuthorizedAddress', () => { it('should throw if data is not for removeAuthorizedAddress', async () => { diff --git a/packages/contracts/test/libraries/lib_bytes.ts b/packages/contracts/test/libraries/lib_bytes.ts index ce3adbdae..fc28c363b 100644 --- a/packages/contracts/test/libraries/lib_bytes.ts +++ b/packages/contracts/test/libraries/lib_bytes.ts @@ -248,4 +248,12 @@ describe('LibBytes', () => { it('should fail if the length between the offset and end of the byte array is too short to hold a uint256)', async () => {}); }); */ + + describe('readFirst4', () => { + it('should return the first 4 bytes of a byte array of arbitrary length', async () => { + const first4Bytes = libBytes.publicReadFirst4.callAsync(byteArrayLongerThan32Bytes); + const expectedFirst4Bytes = byteArrayLongerThan32Bytes.slice(0, 10); + expect(first4Bytes).to.equal(expectedFirst4Bytes); + }); + }); }); -- cgit v1.2.3