From 96c90e6295315802aaad1b80e5a31e4cef886675 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Mon, 18 Jun 2018 16:19:45 +1000 Subject: Rebase with latest removing PROXY_ID from transfer --- .../current/protocol/AssetProxy/ERC721Proxy.sol | 30 -------- .../protocol/AssetProxy/MixinERC20Transfer.sol | 62 +++++++++------- .../protocol/AssetProxy/MixinERC721Transfer.sol | 85 +++++++++++++--------- .../AssetProxy/libs/LibAssetProxyErrors.sol | 4 - .../protocol/AssetProxy/libs/LibTransferErrors.sol | 25 +++++++ 5 files changed, 111 insertions(+), 95 deletions(-) create mode 100644 packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol (limited to 'packages/contracts') diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol index 2b280add4..7ff25aea3 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/ERC721Proxy.sol @@ -41,34 +41,4 @@ contract ERC721Proxy is { return PROXY_ID; } - - /// @dev Decodes ERC721 Asset data. - /// @param assetData Encoded byte array. - /// @return proxyId Intended ERC721 proxy id. - /// @return token ERC721 token address. - /// @return tokenId ERC721 token id. - /// @return receiverData Additional data with no specific format, which - /// is passed to the receiving contract's onERC721Received. - function decodeERC721AssetData(bytes memory assetData) - internal - pure - returns ( - address token, - uint256 tokenId, - bytes memory receiverData - ) - { - // Decode asset data. - token = readAddress(assetData, 0); - tokenId = readUint256(assetData, 20); - if (assetData.length > 52) { - receiverData = readBytes(assetData, 52); - } - - return ( - token, - tokenId, - receiverData - ); - } } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol index b0c32db59..4af39a00b 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC20Transfer.sol @@ -21,50 +21,60 @@ pragma experimental ABIEncoderV2; import "../../utils/LibBytes/LibBytes.sol"; import "../../tokens/ERC20Token/IERC20Token.sol"; +import "./libs/LibTransferErrors.sol"; contract MixinERC20Transfer is - LibBytes + LibBytes, + LibTransferErrors { - // Id of this proxy. - uint8 constant PROXY_ID = 1; - // Revert reasons - string constant INVALID_METADATA_LENGTH = "Metadata must have a length of 21."; - string constant TRANSFER_FAILED = "Transfer failed."; - string constant PROXY_ID_MISMATCH = "Proxy id in metadata does not match this proxy id."; - /// @dev Internal version of `transferFrom`. - /// @param assetMetadata Encoded byte array. + /// @param assetData Encoded byte array. /// @param from Address to transfer asset from. /// @param to Address to transfer asset to. /// @param amount Amount of asset to transfer. function transferFromInternal( - bytes memory assetMetadata, + bytes memory assetData, address from, address to, uint256 amount ) internal { - // Data must be intended for this proxy. - uint256 length = assetMetadata.length; - - require( - length == 21, - INVALID_METADATA_LENGTH - ); + // Decode asset data. + address token = readAddress(assetData, 0); + // Transfer tokens. + // We do a raw call so we can check the success separate + // from the return data. + bool success = token.call(abi.encodeWithSelector( + IERC20Token(token).transferFrom.selector, + from, + to, + amount + )); require( - uint8(assetMetadata[length - 1]) == PROXY_ID, - PROXY_ID_MISMATCH + success, + TRANSFER_FAILED ); - - // Decode metadata. - address token = readAddress(assetMetadata, 0); - - // Transfer tokens. - bool success = IERC20Token(token).transferFrom(from, to, amount); + + // Check return data. + // If there is no return data, we assume the token incorrectly + // does not return a bool. In this case we expect it to revert + // on failure, which was handled above. + // If the token does return data, we require that it is a single + // value that evaluates to true. + assembly { + if returndatasize { + success := 0 + if eq(returndatasize, 32) { + // First 64 bytes of memory are reserved scratch space + returndatacopy(0, 0, 32) + success := mload(0) + } + } + } require( - success == true, + success, TRANSFER_FAILED ); } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol index bbb807956..d09aba599 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/MixinERC721Transfer.sol @@ -21,59 +21,74 @@ pragma experimental ABIEncoderV2; import "../../utils/LibBytes/LibBytes.sol"; import "../../tokens/ERC721Token/ERC721Token.sol"; +import "./libs/LibTransferErrors.sol"; contract MixinERC721Transfer is - LibBytes + LibBytes, + LibTransferErrors { - - // Id of this proxy. - uint8 constant PROXY_ID = 2; - - // Revert reasons - string constant INVALID_TRANSFER_AMOUNT = "Transfer amount must equal 1."; - string constant INVALID_METADATA_LENGTH = "Metadata must have a length of 53."; - string constant PROXY_ID_MISMATCH = "Proxy id in metadata does not match this proxy id."; - /// @dev Internal version of `transferFrom`. - /// @param assetMetadata Encoded byte array. + /// @param assetData Encoded byte array. /// @param from Address to transfer asset from. /// @param to Address to transfer asset to. /// @param amount Amount of asset to transfer. function transferFromInternal( - bytes memory assetMetadata, + bytes memory assetData, address from, address to, uint256 amount ) internal { - // Data must be intended for this proxy. - uint256 length = assetMetadata.length; - - require( - length == 53, - INVALID_METADATA_LENGTH - ); - - require( - uint8(assetMetadata[length - 1]) == PROXY_ID, - PROXY_ID_MISMATCH - ); - // There exists only 1 of each token. require( amount == 1, - INVALID_TRANSFER_AMOUNT + INVALID_AMOUNT ); + + // Decode asset data. + ( + address token, + uint256 tokenId, + bytes memory receiverData + ) = decodeERC721AssetData(assetData); + + // Transfer token. Saves gas by calling safeTransferFrom only + // when there is receiverData present. Either succeeds or throws. + if (receiverData.length > 0) { + ERC721Token(token).safeTransferFrom(from, to, tokenId, receiverData); + } else { + ERC721Token(token).transferFrom(from, to, tokenId); + } + } - // Decode metadata - address token = readAddress(assetMetadata, 0); - uint256 tokenId = readUint256(assetMetadata, 20); - - // Transfer token. - // Either succeeds or throws. - // @TODO: Call safeTransferFrom if there is additional - // data stored in `assetMetadata`. - ERC721Token(token).transferFrom(from, to, tokenId); + /// @dev Decodes ERC721 Asset data. + /// @param assetData Encoded byte array. + /// @return proxyId Intended ERC721 proxy id. + /// @return token ERC721 token address. + /// @return tokenId ERC721 token id. + /// @return receiverData Additional data with no specific format, which + /// is passed to the receiving contract's onERC721Received. + function decodeERC721AssetData(bytes memory assetData) + internal + pure + returns ( + address token, + uint256 tokenId, + bytes memory receiverData + ) + { + // Decode asset data. + token = readAddress(assetData, 0); + tokenId = readUint256(assetData, 20); + if (assetData.length > 52) { + receiverData = readBytes(assetData, 52); + } + + return ( + token, + tokenId, + receiverData + ); } } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol index 65bdacdb7..dca4f400f 100644 --- a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibAssetProxyErrors.sol @@ -25,8 +25,4 @@ contract LibAssetProxyErrors { string constant TARGET_ALREADY_AUTHORIZED = "TARGET_ALREADY_AUTHORIZED"; // Target address must not already be authorized. string constant INDEX_OUT_OF_BOUNDS = "INDEX_OUT_OF_BOUNDS"; // Specified array index is out of bounds. string constant AUTHORIZED_ADDRESS_MISMATCH = "AUTHORIZED_ADDRESS_MISMATCH"; // Address at index does not match given target address. - - /// AssetProxy errors /// - string constant INVALID_AMOUNT = "INVALID_AMOUNT"; // Transfer amount must equal 1. - string constant TRANSFER_FAILED = "TRANSFER_FAILED"; // Transfer failed. } diff --git a/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol new file mode 100644 index 000000000..ba784ab22 --- /dev/null +++ b/packages/contracts/src/contracts/current/protocol/AssetProxy/libs/LibTransferErrors.sol @@ -0,0 +1,25 @@ +/* + + 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; + +contract LibTransferErrors { + /// Transfer errors /// + string constant INVALID_AMOUNT = "INVALID_AMOUNT"; // Transfer amount must equal 1. + string constant TRANSFER_FAILED = "TRANSFER_FAILED"; // Transfer failed. +} -- cgit