From 762a6199b25391bde65ea2f1f91532c3c1d5badf Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Thu, 28 Jun 2018 18:28:09 -0700 Subject: Fix tests --- packages/contracts/compiler.json | 1 + packages/contracts/package.json | 2 +- .../current/tokens/ERC721Token/ERC721Token.sol | 14 +- packages/contracts/src/utils/artifacts.ts | 2 + packages/contracts/src/utils/constants.ts | 1 + packages/contracts/test/asset_proxy/proxies.ts | 205 ++++++++++++++------- 6 files changed, 146 insertions(+), 79 deletions(-) (limited to 'packages') diff --git a/packages/contracts/compiler.json b/packages/contracts/compiler.json index 1ee58081a..56c8b1761 100644 --- a/packages/contracts/compiler.json +++ b/packages/contracts/compiler.json @@ -29,6 +29,7 @@ "Exchange", "ExchangeWrapper", "IAssetData", + "IAssetProxy", "IValidator", "IWallet", "MixinAuthorizable", diff --git a/packages/contracts/package.json b/packages/contracts/package.json index b5a599fe5..1a26f3364 100644 --- a/packages/contracts/package.json +++ b/packages/contracts/package.json @@ -35,7 +35,7 @@ }, "config": { "abis": - "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|ERC20Proxy|ERC721Proxy|Exchange|ExchangeWrapper|IAssetData|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|TestAssetProxyOwner|TestAssetProxyDispatcher|TestLibBytes|TestLibs|TestSignatureValidator|TestValidator|TestWallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" + "../migrations/artifacts/2.0.0/@(AssetProxyOwner|DummyERC20Token|DummyERC721Receiver|DummyERC721Token|ERC20Proxy|ERC721Proxy|Exchange|ExchangeWrapper|IAssetData|IAssetProxy|MixinAuthorizable|MultiSigWallet|MultiSigWalletWithTimeLock|TestAssetProxyOwner|TestAssetProxyDispatcher|TestLibBytes|TestLibs|TestSignatureValidator|TestValidator|TestWallet|TokenRegistry|Whitelist|WETH9|ZRXToken).json" }, "repository": { "type": "git", diff --git a/packages/contracts/src/contracts/current/tokens/ERC721Token/ERC721Token.sol b/packages/contracts/src/contracts/current/tokens/ERC721Token/ERC721Token.sol index cff34e99e..41ba149e3 100644 --- a/packages/contracts/src/contracts/current/tokens/ERC721Token/ERC721Token.sol +++ b/packages/contracts/src/contracts/current/tokens/ERC721Token/ERC721Token.sol @@ -68,7 +68,7 @@ contract ERC721Token is * @param _tokenId uint256 ID of the token to validate */ modifier canTransfer(uint256 _tokenId) { - require(isApprovedOrOwner(msg.sender, _tokenId), "3"); + require(isApprovedOrOwner(msg.sender, _tokenId)); _; } @@ -222,8 +222,8 @@ contract ERC721Token is public canTransfer(_tokenId) { - require(_from != address(0), "1"); - require(_to != address(0), "2"); + require(_from != address(0)); + require(_to != address(0)); clearApproval(_from, _tokenId); removeTokenFrom(_from, _tokenId); @@ -276,7 +276,7 @@ contract ERC721Token is { transferFrom(_from, _to, _tokenId); // solium-disable-next-line arg-overflow - require(checkAndCallSafeTransfer(_from, _to, _tokenId, _data), "7"); + require(checkAndCallSafeTransfer(_from, _to, _tokenId, _data)); } /** @@ -331,7 +331,7 @@ contract ERC721Token is function clearApproval(address _owner, uint256 _tokenId) internal { - require(ownerOf(_tokenId) == _owner, "4"); + require(ownerOf(_tokenId) == _owner); if (tokenApprovals[_tokenId] != address(0)) { tokenApprovals[_tokenId] = address(0); emit Approval(_owner, address(0), _tokenId); @@ -346,7 +346,7 @@ contract ERC721Token is function addTokenTo(address _to, uint256 _tokenId) internal { - require(tokenOwner[_tokenId] == address(0), "6"); + require(tokenOwner[_tokenId] == address(0)); tokenOwner[_tokenId] = _to; ownedTokensCount[_to] = safeAdd(ownedTokensCount[_to], 1); } @@ -359,7 +359,7 @@ contract ERC721Token is function removeTokenFrom(address _from, uint256 _tokenId) internal { - require(ownerOf(_tokenId) == _from, "5"); + require(ownerOf(_tokenId) == _from); ownedTokensCount[_from] = safeSub(ownedTokensCount[_from], 1); tokenOwner[_tokenId] = address(0); } diff --git a/packages/contracts/src/utils/artifacts.ts b/packages/contracts/src/utils/artifacts.ts index d58e7973c..b25266409 100644 --- a/packages/contracts/src/utils/artifacts.ts +++ b/packages/contracts/src/utils/artifacts.ts @@ -8,6 +8,7 @@ import * as ERC20Proxy from '../artifacts/ERC20Proxy.json'; import * as ERC721Proxy from '../artifacts/ERC721Proxy.json'; import * as Exchange from '../artifacts/Exchange.json'; import * as ExchangeWrapper from '../artifacts/ExchangeWrapper.json'; +import * as IAssetProxy from '../artifacts/IAssetProxy.json'; import * as MixinAuthorizable from '../artifacts/MixinAuthorizable.json'; import * as MultiSigWallet from '../artifacts/MultiSigWallet.json'; import * as MultiSigWalletWithTimeLock from '../artifacts/MultiSigWalletWithTimeLock.json'; @@ -33,6 +34,7 @@ export const artifacts = { Exchange: (Exchange as any) as ContractArtifact, ExchangeWrapper: (ExchangeWrapper as any) as ContractArtifact, EtherToken: (EtherToken as any) as ContractArtifact, + IAssetProxy: (IAssetProxy as any) as ContractArtifact, MixinAuthorizable: (MixinAuthorizable as any) as ContractArtifact, MultiSigWallet: (MultiSigWallet as any) as ContractArtifact, MultiSigWalletWithTimeLock: (MultiSigWalletWithTimeLock as any) as ContractArtifact, diff --git a/packages/contracts/src/utils/constants.ts b/packages/contracts/src/utils/constants.ts index 2b2bd2425..8e68f376d 100644 --- a/packages/contracts/src/utils/constants.ts +++ b/packages/contracts/src/utils/constants.ts @@ -27,6 +27,7 @@ export const constants = { MAX_ETHERTOKEN_WITHDRAW_GAS: 43000, MAX_TOKEN_TRANSFERFROM_GAS: 80000, MAX_TOKEN_APPROVE_GAS: 60000, + TRANSFER_FROM_GAS: 150000, DUMMY_TOKEN_NAME: '', DUMMY_TOKEN_SYMBOL: '', DUMMY_TOKEN_DECIMALS: new BigNumber(18), diff --git a/packages/contracts/test/asset_proxy/proxies.ts b/packages/contracts/test/asset_proxy/proxies.ts index 39121d95c..8c9dc9997 100644 --- a/packages/contracts/test/asset_proxy/proxies.ts +++ b/packages/contracts/test/asset_proxy/proxies.ts @@ -15,6 +15,7 @@ import { import { DummyERC721TokenContract } from '../../src/generated_contract_wrappers/dummy_e_r_c721_token'; import { ERC20ProxyContract } from '../../src/generated_contract_wrappers/e_r_c20_proxy'; import { ERC721ProxyContract } from '../../src/generated_contract_wrappers/e_r_c721_proxy'; +import { IAssetProxyContract } from '../../src/generated_contract_wrappers/i_asset_proxy'; import { artifacts } from '../../src/utils/artifacts'; import { expectRevertReasonOrAlwaysFailingTransactionAsync } from '../../src/utils/assertions'; import { chaiSetup } from '../../src/utils/chai_setup'; @@ -41,6 +42,7 @@ describe('Asset Transfer Proxies', () => { let erc721Receiver: DummyERC721ReceiverContract; let erc20Proxy: ERC20ProxyContract; let erc721Proxy: ERC721ProxyContract; + let assetProxyInterface: IAssetProxyContract; let erc20Wrapper: ERC20Wrapper; let erc721Wrapper: ERC721Wrapper; @@ -90,6 +92,11 @@ describe('Asset Transfer Proxies', () => { provider, txDefaults, ); + assetProxyInterface = await IAssetProxyContract.deployFrom0xArtifactAsync( + artifacts.IAssetProxy, + provider, + txDefaults, + ); }); beforeEach(async () => { await blockchainLifecycle.startAsync(); @@ -105,14 +112,18 @@ describe('Asset Transfer Proxies', () => { // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); const amount = new BigNumber(10); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); await web3Wrapper.awaitTransactionSuccessAsync( - await erc20Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: exchangeAddress }, - ), + await web3Wrapper.sendTransactionAsync({ + to: erc20Proxy.address, + data, + from: exchangeAddress, + }), constants.AWAIT_TRANSACTION_MINED_MS, ); // Verify transfer was successful @@ -131,14 +142,18 @@ describe('Asset Transfer Proxies', () => { // Perform a transfer from makerAddress to takerAddress const erc20Balances = await erc20Wrapper.getBalancesAsync(); const amount = new BigNumber(0); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); await web3Wrapper.awaitTransactionSuccessAsync( - await erc20Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: exchangeAddress }, - ), + await web3Wrapper.sendTransactionAsync({ + to: erc20Proxy.address, + data, + from: exchangeAddress, + }), constants.AWAIT_TRANSACTION_MINED_MS, ); // Verify transfer was successful @@ -156,7 +171,13 @@ describe('Asset Transfer Proxies', () => { const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address); // Create allowance less than transfer amount. Set allowance on proxy. const allowance = new BigNumber(0); - const transferAmount = new BigNumber(10); + const amount = new BigNumber(10); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); await web3Wrapper.awaitTransactionSuccessAsync( await zrxToken.approve.sendTransactionAsync(erc20Proxy.address, allowance, { from: makerAddress, @@ -165,13 +186,11 @@ describe('Asset Transfer Proxies', () => { ); // Perform a transfer; expect this to fail. return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc20Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - transferAmount, - { from: exchangeAddress }, - ), + web3Wrapper.sendTransactionAsync({ + to: erc20Proxy.address, + data, + from: exchangeAddress, + }), RevertReason.TransferFailed, ); }); @@ -179,11 +198,18 @@ describe('Asset Transfer Proxies', () => { it('should throw if requesting address is not authorized', async () => { // Construct ERC20 asset data const encodedAssetData = assetProxyUtils.encodeERC20AssetData(zrxToken.address); - // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(10); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc20Proxy.transferFrom.sendTransactionAsync(encodedAssetData, makerAddress, takerAddress, amount, { + web3Wrapper.sendTransactionAsync({ + to: erc20Proxy.address, + data, from: notAuthorized, }), RevertReason.SenderNotAuthorized, @@ -208,14 +234,18 @@ describe('Asset Transfer Proxies', () => { expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(1); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); await web3Wrapper.awaitTransactionSuccessAsync( - await erc721Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: exchangeAddress }, - ), + await web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: exchangeAddress, + }), constants.AWAIT_TRANSACTION_MINED_MS, ); // Verify transfer was successful @@ -231,17 +261,21 @@ describe('Asset Transfer Proxies', () => { expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(1); - const txHash = await erc721Proxy.transferFrom.sendTransactionAsync( + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( encodedAssetData, makerAddress, erc721Receiver.address, amount, - { from: exchangeAddress }, ); - await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS); - // Parse transaction logs const logDecoder = new LogDecoder(web3Wrapper, erc721Receiver.address); - const tx = await logDecoder.getTxWithDecodedLogsAsync(txHash); + const tx = await logDecoder.getTxWithDecodedLogsAsync( + await web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: exchangeAddress, + gas: constants.TRANSFER_FROM_GAS, + }), + ); // Verify that no log was emitted by erc721 receiver expect(tx.logs.length).to.be.equal(1); const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs; @@ -266,17 +300,21 @@ describe('Asset Transfer Proxies', () => { expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(1); - const txHash = await erc721Proxy.transferFrom.sendTransactionAsync( + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( encodedAssetData, makerAddress, erc721Receiver.address, amount, - { from: exchangeAddress }, ); - await web3Wrapper.awaitTransactionSuccessAsync(txHash, constants.AWAIT_TRANSACTION_MINED_MS); - // Parse transaction logs const logDecoder = new LogDecoder(web3Wrapper, erc721Receiver.address); - const tx = await logDecoder.getTxWithDecodedLogsAsync(txHash); + const tx = await logDecoder.getTxWithDecodedLogsAsync( + await web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: exchangeAddress, + gas: constants.TRANSFER_FROM_GAS, + }), + ); // Validate log emitted by erc721 receiver expect(tx.logs.length).to.be.equal(1); const tokenReceivedLog = tx.logs[0] as LogWithDecodedArgs; @@ -301,14 +339,19 @@ describe('Asset Transfer Proxies', () => { expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(1); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + erc20Proxy.address, // the ERC20 proxy does not have an ERC721 receiver + amount, + ); return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc721Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - erc20Proxy.address, // the ERC20 proxy does not have an ERC721 receiver - amount, - { from: exchangeAddress }, - ), + web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: exchangeAddress, + gas: constants.TRANSFER_FROM_GAS, + }), RevertReason.TransferFailed, ); }); @@ -321,14 +364,18 @@ describe('Asset Transfer Proxies', () => { expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(0); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc721Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: exchangeAddress }, - ), + web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: exchangeAddress, + }), RevertReason.InvalidAmount, ); }); @@ -341,14 +388,18 @@ describe('Asset Transfer Proxies', () => { expect(ownerMakerAsset).to.be.bignumber.equal(makerAddress); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(500); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc721Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: exchangeAddress }, - ), + web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: exchangeAddress, + }), RevertReason.InvalidAmount, ); }); @@ -358,15 +409,23 @@ describe('Asset Transfer Proxies', () => { const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Remove transfer approval for makerAddress. await web3Wrapper.awaitTransactionSuccessAsync( - await erc721Token.setApprovalForAll.sendTransactionAsync(erc721Proxy.address, false, { + await erc721Token.approve.sendTransactionAsync(constants.NULL_ADDRESS, erc721MakerTokenId, { from: makerAddress, }), constants.AWAIT_TRANSACTION_MINED_MS, ); // Perform a transfer; expect this to fail. const amount = new BigNumber(1); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc20Proxy.transferFrom.sendTransactionAsync(encodedAssetData, makerAddress, takerAddress, amount, { + web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, from: exchangeAddress, }), RevertReason.TransferFailed, @@ -378,14 +437,18 @@ describe('Asset Transfer Proxies', () => { const encodedAssetData = assetProxyUtils.encodeERC721AssetData(erc721Token.address, erc721MakerTokenId); // Perform a transfer from makerAddress to takerAddress const amount = new BigNumber(1); + const data = assetProxyInterface.transferFrom.getABIEncodedTransactionData( + encodedAssetData, + makerAddress, + takerAddress, + amount, + ); return expectRevertReasonOrAlwaysFailingTransactionAsync( - erc721Proxy.transferFrom.sendTransactionAsync( - encodedAssetData, - makerAddress, - takerAddress, - amount, - { from: notAuthorized }, - ), + web3Wrapper.sendTransactionAsync({ + to: erc721Proxy.address, + data, + from: notAuthorized, + }), RevertReason.SenderNotAuthorized, ); }); -- cgit v1.2.3