From 732202fe8e4f3eb1847515a0c3f8f2d068d35baf Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 24 Apr 2018 11:24:28 -0700 Subject: Implement sender abstraction --- .../current/protocol/Exchange/Exchange.sol | 5 +- .../current/protocol/Exchange/LibOrder.sol | 3 + .../protocol/Exchange/MixinExchangeCore.sol | 61 ++++++++++++---- .../protocol/Exchange/MixinTransactions.sol | 85 ++++++++++++++++++++++ .../protocol/Exchange/MixinWrapperFunctions.sol | 62 ++++++++-------- .../protocol/Exchange/mixins/MTransactions.sol | 41 +++++++++++ 6 files changed, 210 insertions(+), 47 deletions(-) create mode 100644 packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol create mode 100644 packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol (limited to 'packages/contracts') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/Exchange.sol b/packages/contracts/src/contracts/current/protocol/Exchange/Exchange.sol index 5ccb06b88..caf48bfca 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/Exchange.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/Exchange.sol @@ -24,13 +24,15 @@ import "./MixinSignatureValidator.sol"; import "./MixinSettlement.sol"; import "./MixinWrapperFunctions.sol"; import "./MixinAssetProxyDispatcher.sol"; +import "./MixinTransactions.sol"; contract Exchange is MixinExchangeCore, MixinSignatureValidator, MixinSettlement, MixinWrapperFunctions, - MixinAssetProxyDispatcher + MixinAssetProxyDispatcher, + MixinTransactions { string constant public VERSION = "2.0.1-alpha"; @@ -42,5 +44,6 @@ contract Exchange is MixinSettlement(_zrxProxyData) MixinWrapperFunctions() MixinAssetProxyDispatcher() + MixinTransactions() {} } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/LibOrder.sol b/packages/contracts/src/contracts/current/protocol/Exchange/LibOrder.sol index 61a4a382d..a98d7cbeb 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/LibOrder.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/LibOrder.sol @@ -26,6 +26,7 @@ contract LibOrder { "address makerAddress", "address takerAddress", "address feeRecipientAddress", + "address senderAddress", "uint256 makerAssetAmount", "uint256 takerAssetAmount", "uint256 makerFee", @@ -40,6 +41,7 @@ contract LibOrder { address makerAddress; address takerAddress; address feeRecipientAddress; + address senderAddress; uint256 makerAssetAmount; uint256 takerAssetAmount; uint256 makerFee; @@ -66,6 +68,7 @@ contract LibOrder { order.makerAddress, order.takerAddress, order.feeRecipientAddress, + order.senderAddress, order.makerAssetAmount, order.takerAssetAmount, order.makerFee, diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol index 710315515..97c1f1541 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol @@ -18,10 +18,12 @@ pragma solidity ^0.4.21; pragma experimental ABIEncoderV2; +pragma experimental "v0.5.0"; import "./mixins/MExchangeCore.sol"; import "./mixins/MSettlement.sol"; import "./mixins/MSignatureValidator.sol"; +import "./mixins/MTransactions.sol"; import "./LibOrder.sol"; import "./LibErrors.sol"; import "./LibPartialAmount.sol"; @@ -35,6 +37,7 @@ contract MixinExchangeCore is MExchangeCore, MSettlement, MSignatureValidator, + MTransactions, SafeMath, LibErrors, LibPartialAmount @@ -113,10 +116,16 @@ contract MixinExchangeCore is require(order.takerAssetAmount > 0); require(isValidSignature(orderHash, order.makerAddress, signature)); } + + // Validate sender + if (order.senderAddress != address(0)) { + require(order.senderAddress == msg.sender); + } - // Validate taker + // Validate transaction signed by taker + address takerAddress = getSignerAddress(); if (order.takerAddress != address(0)) { - require(order.takerAddress == msg.sender); + require(order.takerAddress == takerAddress); } require(takerAssetFillAmount > 0); @@ -146,21 +155,10 @@ contract MixinExchangeCore is // Settle order (fillResults.makerAssetFilledAmount, fillResults.makerFeePaid, fillResults.takerFeePaid) = - settleOrder(order, msg.sender, fillResults.takerAssetFilledAmount); + settleOrder(order, takerAddress, fillResults.takerAssetFilledAmount); // Log order - emit Fill( - order.makerAddress, - msg.sender, - order.feeRecipientAddress, - fillResults.makerAssetFilledAmount, - fillResults.takerAssetFilledAmount, - fillResults.makerFeePaid, - fillResults.takerFeePaid, - orderHash, - order.makerAssetData, - order.takerAssetData - ); + emitFillEvent(order, takerAddress, orderHash, fillResults); return fillResults; } @@ -178,8 +176,16 @@ contract MixinExchangeCore is // Validate the order require(order.makerAssetAmount > 0); require(order.takerAssetAmount > 0); - require(order.makerAddress == msg.sender); + // Validate sender + if (order.senderAddress != address(0)) { + require(order.senderAddress == msg.sender); + } + + // Validate transaction signed by maker + address makerAddress = getSignerAddress(); + require(order.makerAddress == makerAddress); + if (block.timestamp >= order.expirationTimeSeconds) { emit ExchangeError(uint8(Errors.ORDER_EXPIRED), orderHash); return false; @@ -233,4 +239,27 @@ contract MixinExchangeCore is isError = errPercentageTimes1000000 > 1000; return isError; } + + /// @dev Logs a Fill event with the given arguments. + /// The sole purpose of this function is to get around the stack variable limit. + function emitFillEvent( + Order memory order, + address takerAddress, + bytes32 orderHash, + FillResults memory fillResults) + internal + { + emit Fill( + order.makerAddress, + takerAddress, + order.feeRecipientAddress, + fillResults.makerAssetFilledAmount, + fillResults.takerAssetFilledAmount, + fillResults.makerFeePaid, + fillResults.takerFeePaid, + orderHash, + order.makerAssetData, + order.takerAssetData + ); + } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol new file mode 100644 index 000000000..e4fc4767b --- /dev/null +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol @@ -0,0 +1,85 @@ +/* + + 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.21; +pragma experimental ABIEncoderV2; + +import "./mixins/MSignatureValidator.sol"; +import "./mixins/MTransactions.sol"; + +contract MixinTransactions is + MSignatureValidator, + MTransactions +{ + + // Mapping of transaction hash => executed + mapping (bytes32 => bool) public transactions; + + // Address of current transaction signer + address currentSigner; + + /// @dev Executes an exchange method call in the context of signer. + /// @param salt Arbitrary number to ensure uniqueness of transaction hash. + /// @param signer Address of transaction signer. + /// @param data AbiV2 encoded calldata. + /// @param signature Proof of signer transaction by signer. + function executeTransaction( + uint256 salt, + address signer, + bytes data, + bytes signature) + external + { + // Prevent reentrancy + require(currentSigner == address(0)); + + // Calculate transaction hash + bytes32 transactionHash = keccak256( + address(this), + salt, + data + ); + + // Validate transaction has not been executed + require(!transactions[transactionHash]); + + // TODO: is SignatureType.Caller necessary if we make this check? + if (signer != msg.sender) { + // Validate signature + require(isValidSignature(transactionHash, signer, signature)); + + // Set the current transaction signer + currentSigner = signer; + } + + // Execute transaction + transactions[transactionHash] = true; + require(address(this).delegatecall(data)); + + // Reset current transaction signer + currentSigner = address(0); + } + + function getSignerAddress() + internal + view + returns (address) + { + address signerAddress = currentSigner == address(0) ? msg.sender : currentSigner; + return signerAddress; + } +} diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol index 2c0fd0751..4d6ba17dd 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinWrapperFunctions.sol @@ -75,24 +75,25 @@ contract MixinWrapperFunctions is // | | 0x00 | | 1. offset to order (*) | // | | 0x20 | | 2. takerAssetFillAmount | // | | 0x40 | | 3. offset to signature (*) | - // | Data | | 11 * 32 | order: | - // | | 0x000 | | 1. makerAddress | - // | | 0x020 | | 2. takerAddress | - // | | 0x040 | | 3. feeRecipientAddress | - // | | 0x060 | | 4. makerAssetAmount | - // | | 0x080 | | 5. takerAssetAmount | - // | | 0x0A0 | | 6. makerFeeAmount | - // | | 0x0C0 | | 7. takerFeeAmount | - // | | 0x0E0 | | 8. expirationTimeSeconds | - // | | 0x100 | | 9. salt | - // | | 0x120 | | 10. Offset to makerAssetProxyMetadata (*) | - // | | 0x140 | | 11. Offset to takerAssetProxyMetadata (*) | - // | | 0x160 | 32 | makerAssetProxyMetadata Length | - // | | 0x180 | ** | makerAssetProxyMetadata Contents | - // | | 0x1A0 | 32 | takerAssetProxyMetadata Length | - // | | 0x1C0 | ** | takerAssetProxyMetadata Contents | - // | | 0x1E0 | 32 | signature Length | - // | | 0x200 | ** | signature Contents | + // | Data | | 12 * 32 | order: | + // | | 0x000 | | 1. senderAddress | + // | | 0x020 | | 2. makerAddress | + // | | 0x040 | | 3. takerAddress | + // | | 0x060 | | 4. feeRecipientAddress | + // | | 0x080 | | 5. makerAssetAmount | + // | | 0x0A0 | | 6. takerAssetAmount | + // | | 0x0C0 | | 7. makerFeeAmount | + // | | 0x0E0 | | 8. takerFeeAmount | + // | | 0x100 | | 9. expirationTimeSeconds | + // | | 0x120 | | 10. salt | + // | | 0x140 | | 11. Offset to makerAssetProxyMetadata (*) | + // | | 0x160 | | 12. Offset to takerAssetProxyMetadata (*) | + // | | 0x180 | 32 | makerAssetProxyMetadata Length | + // | | 0x1A0 | ** | makerAssetProxyMetadata Contents | + // | | 0x1C0 | 32 | takerAssetProxyMetadata Length | + // | | 0x1E0 | ** | takerAssetProxyMetadata Contents | + // | | 0x200 | 32 | signature Length | + // | | 0x220 | ** | signature Contents | // * Offsets are calculated from the beginning of the current area: Header, Params, Data: // An offset stored in the Params area is calculated from the beginning of the Params section. @@ -150,19 +151,20 @@ contract MixinWrapperFunctions is mstore(dataAreaEnd, mload(sourceOffset)) // makerAddress mstore(add(dataAreaEnd, 0x20), mload(add(sourceOffset, 0x20))) // takerAddress mstore(add(dataAreaEnd, 0x40), mload(add(sourceOffset, 0x40))) // feeRecipientAddress - mstore(add(dataAreaEnd, 0x60), mload(add(sourceOffset, 0x60))) // makerAssetAmount - mstore(add(dataAreaEnd, 0x80), mload(add(sourceOffset, 0x80))) // takerAssetAmount - mstore(add(dataAreaEnd, 0xA0), mload(add(sourceOffset, 0xA0))) // makerFeeAmount - mstore(add(dataAreaEnd, 0xC0), mload(add(sourceOffset, 0xC0))) // takerFeeAmount - mstore(add(dataAreaEnd, 0xE0), mload(add(sourceOffset, 0xE0))) // expirationTimeSeconds - mstore(add(dataAreaEnd, 0x100), mload(add(sourceOffset, 0x100))) // salt - mstore(add(dataAreaEnd, 0x120), mload(add(sourceOffset, 0x120))) // Offset to makerAssetProxyMetadata - mstore(add(dataAreaEnd, 0x140), mload(add(sourceOffset, 0x140))) // Offset to takerAssetProxyMetadata - dataAreaEnd := add(dataAreaEnd, 0x160) - sourceOffset := add(sourceOffset, 0x160) + mstore(add(dataAreaEnd, 0x60), mload(add(sourceOffset, 0x60))) // senderAddress + mstore(add(dataAreaEnd, 0x80), mload(add(sourceOffset, 0x80))) // makerAssetAmount + mstore(add(dataAreaEnd, 0xA0), mload(add(sourceOffset, 0xA0))) // takerAssetAmount + mstore(add(dataAreaEnd, 0xC0), mload(add(sourceOffset, 0xC0))) // makerFeeAmount + mstore(add(dataAreaEnd, 0xE0), mload(add(sourceOffset, 0xE0))) // takerFeeAmount + mstore(add(dataAreaEnd, 0x100), mload(add(sourceOffset, 0x100))) // expirationTimeSeconds + mstore(add(dataAreaEnd, 0x120), mload(add(sourceOffset, 0x120))) // salt + mstore(add(dataAreaEnd, 0x140), mload(add(sourceOffset, 0x140))) // Offset to makerAssetProxyMetadata + mstore(add(dataAreaEnd, 0x160), mload(add(sourceOffset, 0x160))) // Offset to takerAssetProxyMetadata + dataAreaEnd := add(dataAreaEnd, 0x180) + sourceOffset := add(sourceOffset, 0x180) // Write offset to - mstore(add(dataAreaStart, mul(9, 0x20)), sub(dataAreaEnd, dataAreaStart)) + mstore(add(dataAreaStart, mul(10, 0x20)), sub(dataAreaEnd, dataAreaStart)) // Calculate length of arrayLenBytes := mload(sourceOffset) @@ -181,7 +183,7 @@ contract MixinWrapperFunctions is } // Write offset to - mstore(add(dataAreaStart, mul(10, 0x20)), sub(dataAreaEnd, dataAreaStart)) + mstore(add(dataAreaStart, mul(11, 0x20)), sub(dataAreaEnd, dataAreaStart)) // Calculate length of arrayLenBytes := mload(sourceOffset) diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol new file mode 100644 index 000000000..31209de02 --- /dev/null +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol @@ -0,0 +1,41 @@ +/* + + 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.21; +pragma experimental ABIEncoderV2; + +import "./MSignatureValidator.sol"; + +contract MTransactions is MSignatureValidator { + + /// @dev Executes an exchange method call in the context of signer. + /// @param salt Arbitrary number to ensure uniqueness of transaction hash. + /// @param signer Address of transaction signer. + /// @param data AbiV2 encoded calldata. + /// @param signature Proof of signer transaction by signer. + function executeTransaction( + uint256 salt, + address signer, + bytes data, + bytes signature) + external; + + function getSignerAddress() + internal + view + returns (address); +} -- cgit v1.2.3 From 0e0a46f373e076843fd4118cdafb325fb754141a Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 24 Apr 2018 11:24:55 -0700 Subject: Update tests and utils --- packages/contracts/src/utils/crypto.ts | 2 + packages/contracts/src/utils/exchange_wrapper.ts | 16 +- packages/contracts/src/utils/order_factory.ts | 1 + packages/contracts/src/utils/order_utils.ts | 3 + .../contracts/src/utils/transaction_factory.ts | 35 +++++ packages/contracts/src/utils/types.ts | 9 ++ packages/contracts/test/exchange/transactions.ts | 161 +++++++++++++++++++++ 7 files changed, 226 insertions(+), 1 deletion(-) create mode 100644 packages/contracts/src/utils/transaction_factory.ts create mode 100644 packages/contracts/test/exchange/transactions.ts (limited to 'packages/contracts') diff --git a/packages/contracts/src/utils/crypto.ts b/packages/contracts/src/utils/crypto.ts index 222cb7cca..85a37122a 100644 --- a/packages/contracts/src/utils/crypto.ts +++ b/packages/contracts/src/utils/crypto.ts @@ -29,6 +29,8 @@ export const crypto = { args[i] = new BN(arg.toString(10), 10); } else if (ethUtil.isValidAddress(arg)) { argTypes.push('address'); + } else if (arg instanceof Buffer) { + argTypes.push('bytes'); } else if (_.isString(arg)) { argTypes.push('string'); } else if (_.isBuffer(arg)) { diff --git a/packages/contracts/src/utils/exchange_wrapper.ts b/packages/contracts/src/utils/exchange_wrapper.ts index 7d06c321b..000905854 100644 --- a/packages/contracts/src/utils/exchange_wrapper.ts +++ b/packages/contracts/src/utils/exchange_wrapper.ts @@ -9,7 +9,7 @@ import { constants } from './constants'; import { formatters } from './formatters'; import { LogDecoder } from './log_decoder'; import { orderUtils } from './order_utils'; -import { AssetProxyId, SignedOrder } from './types'; +import { AssetProxyId, SignedOrder, SignedTransaction } from './types'; export class ExchangeWrapper { private _exchange: ExchangeContract; @@ -207,6 +207,20 @@ export class ExchangeWrapper { const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); return tx; } + public async executeTransactionAsync( + signedTx: SignedTransaction, + from: string, + ): Promise { + const txHash = await this._exchange.executeTransaction.sendTransactionAsync( + signedTx.salt, + signedTx.signer, + signedTx.data, + signedTx.signature, + { from }, + ); + const tx = await this._getTxWithDecodedExchangeLogsAsync(txHash); + return tx; + } public async getOrderHashAsync(signedOrder: SignedOrder): Promise { const order = orderUtils.getOrderStruct(signedOrder); const orderHash = await this._exchange.getOrderHash.callAsync(order); diff --git a/packages/contracts/src/utils/order_factory.ts b/packages/contracts/src/utils/order_factory.ts index 08853054d..044e9b865 100644 --- a/packages/contracts/src/utils/order_factory.ts +++ b/packages/contracts/src/utils/order_factory.ts @@ -19,6 +19,7 @@ export class OrderFactory { ): SignedOrder { const randomExpiration = new BigNumber(Math.floor((Date.now() + Math.random() * 100000000000) / 1000)); const order = ({ + senderAddress: ZeroEx.NULL_ADDRESS, expirationTimeSeconds: randomExpiration, salt: ZeroEx.generatePseudoRandomSalt(), takerAddress: ZeroEx.NULL_ADDRESS, diff --git a/packages/contracts/src/utils/order_utils.ts b/packages/contracts/src/utils/order_utils.ts index 52eb44941..10bbf4f7c 100644 --- a/packages/contracts/src/utils/order_utils.ts +++ b/packages/contracts/src/utils/order_utils.ts @@ -24,6 +24,7 @@ export const orderUtils = { }, getOrderStruct(signedOrder: SignedOrder): OrderStruct { const orderStruct = { + senderAddress: signedOrder.senderAddress, makerAddress: signedOrder.makerAddress, takerAddress: signedOrder.takerAddress, feeRecipientAddress: signedOrder.feeRecipientAddress, @@ -44,6 +45,7 @@ export const orderUtils = { 'address makerAddress', 'address takerAddress', 'address feeRecipientAddress', + 'address senderAddress', 'uint256 makerAssetAmount', 'uint256 takerAssetAmount', 'uint256 makerFee', @@ -58,6 +60,7 @@ export const orderUtils = { order.makerAddress, order.takerAddress, order.feeRecipientAddress, + order.senderAddress, order.makerAssetAmount, order.takerAssetAmount, order.makerFee, diff --git a/packages/contracts/src/utils/transaction_factory.ts b/packages/contracts/src/utils/transaction_factory.ts new file mode 100644 index 000000000..3a4f48330 --- /dev/null +++ b/packages/contracts/src/utils/transaction_factory.ts @@ -0,0 +1,35 @@ +import { ZeroEx } from '0x.js'; +import { BigNumber } from '@0xproject/utils'; +import * as ethUtil from 'ethereumjs-util'; + +import { crypto } from './crypto'; +import { signingUtils } from './signing_utils'; +import { SignatureType, SignedTransaction } from './types'; + +export class TransactionFactory { + private _signer: string; + private _exchangeAddress: string; + private _privateKey: Buffer; + constructor(privateKey: Buffer, exchangeAddress: string) { + this._privateKey = privateKey; + this._exchangeAddress = exchangeAddress; + const signerBuff = ethUtil.privateToAddress(this._privateKey); + this._signer = `0x${signerBuff.toString('hex')}`; + } + public newSignedTransaction( + data: string, + signatureType: SignatureType = SignatureType.Ecrecover, + ): SignedTransaction { + const salt = ZeroEx.generatePseudoRandomSalt(); + const txHash = crypto.solSHA3([this._exchangeAddress, salt, ethUtil.toBuffer(data)]); + const signature = signingUtils.signMessage(txHash, this._privateKey, signatureType); + const signedTx = { + exchangeAddress: this._exchangeAddress, + salt, + signer: this._signer, + data, + signature: `0x${signature.toString('hex')}`, + }; + return signedTx; + } +} diff --git a/packages/contracts/src/utils/types.ts b/packages/contracts/src/utils/types.ts index a23b6b876..934dc52fa 100644 --- a/packages/contracts/src/utils/types.ts +++ b/packages/contracts/src/utils/types.ts @@ -122,6 +122,7 @@ export interface SignedOrder extends UnsignedOrder { } export interface OrderStruct { + senderAddress: string; makerAddress: string; takerAddress: string; feeRecipientAddress: string; @@ -148,3 +149,11 @@ export enum SignatureType { Trezor, Contract, } + +export interface SignedTransaction { + exchangeAddress: string; + salt: BigNumber; + signer: string; + data: string; + signature: string; +} diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts new file mode 100644 index 000000000..697fb70e2 --- /dev/null +++ b/packages/contracts/test/exchange/transactions.ts @@ -0,0 +1,161 @@ +import { ZeroEx } from '0x.js'; + +import { BlockchainLifecycle } from '@0xproject/dev-utils'; +import { BigNumber } from '@0xproject/utils'; +import * as chai from 'chai'; +import * as ethUtil from 'ethereumjs-util'; +import * as Web3 from 'web3'; + +import { DummyERC20TokenContract } from '../../src/contract_wrappers/generated/dummy_e_r_c20_token'; +import { ERC20ProxyContract } from '../../src/contract_wrappers/generated/e_r_c20_proxy'; +import { ExchangeContract } from '../../src/contract_wrappers/generated/exchange'; +import { assetProxyUtils } from '../../src/utils/asset_proxy_utils'; +import { constants } from '../../src/utils/constants'; +import { ERC20Wrapper } from '../../src/utils/erc20_wrapper'; +import { ExchangeWrapper } from '../../src/utils/exchange_wrapper'; +import { OrderFactory } from '../../src/utils/order_factory'; +import { orderUtils } from '../../src/utils/order_utils'; +import { TransactionFactory } from '../../src/utils/transaction_factory'; +import { + AssetProxyId, + ContractName, + ERC20BalancesByOwner, + ExchangeContractErrs, + OrderStruct, + SignatureType, + SignedOrder, + SignedTransaction, +} from '../../src/utils/types'; +import { chaiSetup } from '../utils/chai_setup'; +import { deployer } from '../utils/deployer'; +import { provider, web3Wrapper } from '../utils/web3_wrapper'; + +chaiSetup.configure(); +const expect = chai.expect; +const blockchainLifecycle = new BlockchainLifecycle(web3Wrapper); + +describe('Exchange transactions', () => { + let senderAddress: string; + let owner: string; + let makerAddress: string; + let takerAddress: string; + let feeRecipientAddress: string; + + let erc20TokenA: DummyERC20TokenContract; + let erc20TokenB: DummyERC20TokenContract; + let zrxToken: DummyERC20TokenContract; + let exchange: ExchangeContract; + let erc20Proxy: ERC20ProxyContract; + + let erc20Balances: ERC20BalancesByOwner; + let signedOrder: SignedOrder; + let order: OrderStruct; + let orderFactory: OrderFactory; + let transactionFactory: TransactionFactory; + let exchangeWrapper: ExchangeWrapper; + let erc20Wrapper: ERC20Wrapper; + + let defaultMakerTokenAddress: string; + let defaultTakerTokenAddress: string; + + let zeroEx: ZeroEx; + + before(async () => { + const accounts = await web3Wrapper.getAvailableAddressesAsync(); + const usedAddresses = ([owner, senderAddress, makerAddress, takerAddress, feeRecipientAddress] = accounts); + + erc20Wrapper = new ERC20Wrapper(deployer, provider, usedAddresses, owner); + + [erc20TokenA, erc20TokenB, zrxToken] = await erc20Wrapper.deployDummyTokensAsync(); + erc20Proxy = await erc20Wrapper.deployProxyAsync(); + await erc20Wrapper.setBalancesAndAllowancesAsync(); + + const exchangeInstance = await deployer.deployAsync(ContractName.Exchange, [ + assetProxyUtils.encodeERC20ProxyData(zrxToken.address), + ]); + exchange = new ExchangeContract(exchangeInstance.abi, exchangeInstance.address, provider); + zeroEx = new ZeroEx(provider, { + exchangeContractAddress: exchange.address, + networkId: constants.TESTRPC_NETWORK_ID, + }); + exchangeWrapper = new ExchangeWrapper(exchange, zeroEx); + await exchangeWrapper.registerAssetProxyAsync(AssetProxyId.ERC20, erc20Proxy.address, owner); + + await erc20Proxy.addAuthorizedAddress.sendTransactionAsync(exchange.address, { from: owner }); + + defaultMakerTokenAddress = erc20TokenA.address; + defaultTakerTokenAddress = erc20TokenB.address; + + const defaultOrderParams = { + ...constants.STATIC_ORDER_PARAMS, + senderAddress, + exchangeAddress: exchange.address, + makerAddress, + feeRecipientAddress, + makerAssetData: assetProxyUtils.encodeERC20ProxyData(defaultMakerTokenAddress), + takerAssetData: assetProxyUtils.encodeERC20ProxyData(defaultTakerTokenAddress), + }; + const makerPrivateKey = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddress)]; + const takerPrivateKey = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(takerAddress)]; + orderFactory = new OrderFactory(makerPrivateKey, defaultOrderParams); + transactionFactory = new TransactionFactory(takerPrivateKey, exchange.address); + }); + beforeEach(async () => { + await blockchainLifecycle.startAsync(); + }); + afterEach(async () => { + await blockchainLifecycle.revertAsync(); + }); + + describe('executeTransaction', () => { + describe('fillOrder', () => { + beforeEach(async () => { + erc20Balances = await erc20Wrapper.getBalancesAsync(); + signedOrder = orderFactory.newSignedOrder(); + order = orderUtils.getOrderStruct(signedOrder); + }); + + it('should transfer the correct amounts when signed by taker and called by sender', async () => { + const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); + const data = exchange.fillOrder.getABIEncodedTransactionData( + order, + takerAssetFillAmount, + signedOrder.signature, + ); + const signedTx = transactionFactory.newSignedTransaction(data); + const res = await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + const makerAssetFillAmount = takerAssetFillAmount + .times(signedOrder.makerAssetAmount) + .dividedToIntegerBy(signedOrder.takerAssetAmount); + const makerFeePaid = signedOrder.makerFee + .times(makerAssetFillAmount) + .dividedToIntegerBy(signedOrder.makerAssetAmount); + const takerFeePaid = signedOrder.takerFee + .times(makerAssetFillAmount) + .dividedToIntegerBy(signedOrder.makerAssetAmount); + expect(newBalances[makerAddress][defaultMakerTokenAddress]).to.be.bignumber.equal( + erc20Balances[makerAddress][defaultMakerTokenAddress].minus(makerAssetFillAmount), + ); + expect(newBalances[makerAddress][defaultTakerTokenAddress]).to.be.bignumber.equal( + erc20Balances[makerAddress][defaultTakerTokenAddress].add(takerAssetFillAmount), + ); + expect(newBalances[makerAddress][zrxToken.address]).to.be.bignumber.equal( + erc20Balances[makerAddress][zrxToken.address].minus(makerFeePaid), + ); + expect(newBalances[takerAddress][defaultTakerTokenAddress]).to.be.bignumber.equal( + erc20Balances[takerAddress][defaultTakerTokenAddress].minus(takerAssetFillAmount), + ); + expect(newBalances[takerAddress][defaultMakerTokenAddress]).to.be.bignumber.equal( + erc20Balances[takerAddress][defaultMakerTokenAddress].add(makerAssetFillAmount), + ); + expect(newBalances[takerAddress][zrxToken.address]).to.be.bignumber.equal( + erc20Balances[takerAddress][zrxToken.address].minus(takerFeePaid), + ); + expect(newBalances[feeRecipientAddress][zrxToken.address]).to.be.bignumber.equal( + erc20Balances[feeRecipientAddress][zrxToken.address].add(makerFeePaid.add(takerFeePaid)), + ); + }); + }); + }); +}); -- cgit v1.2.3 From 185e7d43fbf308cd6b2c6032adb5e8160a206d89 Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Tue, 24 Apr 2018 16:35:57 -0700 Subject: Add tests --- packages/contracts/test/exchange/transactions.ts | 42 +++++++++++++++++++++--- 1 file changed, 38 insertions(+), 4 deletions(-) (limited to 'packages/contracts') diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 697fb70e2..62bba0ab8 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -51,7 +51,8 @@ describe('Exchange transactions', () => { let signedOrder: SignedOrder; let order: OrderStruct; let orderFactory: OrderFactory; - let transactionFactory: TransactionFactory; + let makerTransactionFactory: TransactionFactory; + let takerTransactionFactory: TransactionFactory; let exchangeWrapper: ExchangeWrapper; let erc20Wrapper: ERC20Wrapper; @@ -98,7 +99,8 @@ describe('Exchange transactions', () => { const makerPrivateKey = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(makerAddress)]; const takerPrivateKey = constants.TESTRPC_PRIVATE_KEYS[accounts.indexOf(takerAddress)]; orderFactory = new OrderFactory(makerPrivateKey, defaultOrderParams); - transactionFactory = new TransactionFactory(takerPrivateKey, exchange.address); + makerTransactionFactory = new TransactionFactory(makerPrivateKey, exchange.address); + takerTransactionFactory = new TransactionFactory(takerPrivateKey, exchange.address); }); beforeEach(async () => { await blockchainLifecycle.startAsync(); @@ -115,6 +117,19 @@ describe('Exchange transactions', () => { order = orderUtils.getOrderStruct(signedOrder); }); + it('should throw if not called by specified sender', async () => { + const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); + const data = exchange.fillOrder.getABIEncodedTransactionData( + order, + takerAssetFillAmount, + signedOrder.signature, + ); + const signedTx = takerTransactionFactory.newSignedTransaction(data); + return expect(exchangeWrapper.executeTransactionAsync(signedTx, takerAddress)).to.be.rejectedWith( + constants.REVERT, + ); + }); + it('should transfer the correct amounts when signed by taker and called by sender', async () => { const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const data = exchange.fillOrder.getABIEncodedTransactionData( @@ -122,8 +137,8 @@ describe('Exchange transactions', () => { takerAssetFillAmount, signedOrder.signature, ); - const signedTx = transactionFactory.newSignedTransaction(data); - const res = await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + const signedTx = takerTransactionFactory.newSignedTransaction(data); + await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAssetFillAmount = takerAssetFillAmount .times(signedOrder.makerAssetAmount) @@ -157,5 +172,24 @@ describe('Exchange transactions', () => { ); }); }); + + describe('cancelOrder', () => { + it('should throw if not called by specified sender', async () => { + const data = exchange.cancelOrder.getABIEncodedTransactionData(order); + const signedTx = makerTransactionFactory.newSignedTransaction(data); + return expect(exchangeWrapper.executeTransactionAsync(signedTx, makerAddress)).to.be.rejectedWith( + constants.REVERT, + ); + }); + + it('should cancel the order when signed by maker and called by sender', async () => { + const data = exchange.cancelOrder.getABIEncodedTransactionData(order); + const signedTx = makerTransactionFactory.newSignedTransaction(data); + await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); + const newBalances = await erc20Wrapper.getBalancesAsync(); + expect(newBalances).to.deep.equal(erc20Balances); + }); + }); }); }); -- cgit v1.2.3 From 9ddec32260981e714c332651ae292221e06ebc1e Mon Sep 17 00:00:00 2001 From: Amir Bandeali Date: Wed, 25 Apr 2018 16:21:55 -0700 Subject: Add tests and comments --- .../protocol/Exchange/MixinExchangeCore.sol | 11 +++--- .../protocol/Exchange/MixinTransactions.sol | 21 ++++++++---- .../protocol/Exchange/mixins/MTransactions.sol | 7 +++- packages/contracts/src/utils/crypto.ts | 2 -- packages/contracts/test/exchange/transactions.ts | 40 ++++++++++++++-------- 5 files changed, 50 insertions(+), 31 deletions(-) (limited to 'packages/contracts') diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol index 97c1f1541..80120bc74 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinExchangeCore.sol @@ -18,7 +18,6 @@ pragma solidity ^0.4.21; pragma experimental ABIEncoderV2; -pragma experimental "v0.5.0"; import "./mixins/MExchangeCore.sol"; import "./mixins/MSettlement.sol"; @@ -117,13 +116,13 @@ contract MixinExchangeCore is require(isValidSignature(orderHash, order.makerAddress, signature)); } - // Validate sender + // Validate sender is allowed to fill this order if (order.senderAddress != address(0)) { require(order.senderAddress == msg.sender); } - // Validate transaction signed by taker - address takerAddress = getSignerAddress(); + // Validate taker is allowed to fill this order + address takerAddress = getCurrentContextAddress(); if (order.takerAddress != address(0)) { require(order.takerAddress == takerAddress); } @@ -177,13 +176,13 @@ contract MixinExchangeCore is require(order.makerAssetAmount > 0); require(order.takerAssetAmount > 0); - // Validate sender + // Validate sender is allowed to cancel this order if (order.senderAddress != address(0)) { require(order.senderAddress == msg.sender); } // Validate transaction signed by maker - address makerAddress = getSignerAddress(); + address makerAddress = getCurrentContextAddress(); require(order.makerAddress == makerAddress); if (block.timestamp >= order.expirationTimeSeconds) { diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol index e4fc4767b..9edb1694f 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/MixinTransactions.sol @@ -27,10 +27,11 @@ contract MixinTransactions is { // Mapping of transaction hash => executed + // This prevents transactions from being executed more than once. mapping (bytes32 => bool) public transactions; // Address of current transaction signer - address currentSigner; + address public currentContextAddress; /// @dev Executes an exchange method call in the context of signer. /// @param salt Arbitrary number to ensure uniqueness of transaction hash. @@ -45,7 +46,7 @@ contract MixinTransactions is external { // Prevent reentrancy - require(currentSigner == address(0)); + require(currentContextAddress == address(0)); // Calculate transaction hash bytes32 transactionHash = keccak256( @@ -63,7 +64,7 @@ contract MixinTransactions is require(isValidSignature(transactionHash, signer, signature)); // Set the current transaction signer - currentSigner = signer; + currentContextAddress = signer; } // Execute transaction @@ -71,15 +72,21 @@ contract MixinTransactions is require(address(this).delegatecall(data)); // Reset current transaction signer - currentSigner = address(0); + // TODO: Check if gas is paid when currentContextAddress is already 0. + currentContextAddress = address(0); } - function getSignerAddress() + /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`). + /// If calling a fill function, this address will represent the taker. + /// If calling a cancel function, this address will represent the maker. + /// @return Signer of 0x transaction if entry point is `executeTransaction`. + /// `msg.sender` if entry point is any other function. + function getCurrentContextAddress() internal view returns (address) { - address signerAddress = currentSigner == address(0) ? msg.sender : currentSigner; - return signerAddress; + address contextAddress = currentContextAddress == address(0) ? msg.sender : currentContextAddress; + return contextAddress; } } diff --git a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol index 31209de02..10bfcb035 100644 --- a/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol +++ b/packages/contracts/src/contracts/current/protocol/Exchange/mixins/MTransactions.sol @@ -34,7 +34,12 @@ contract MTransactions is MSignatureValidator { bytes signature) external; - function getSignerAddress() + /// @dev The current function will be called in the context of this address (either 0x transaction signer or `msg.sender`). + /// If calling a fill function, this address will represent the taker. + /// If calling a cancel function, this address will represent the maker. + /// @return Signer of 0x transaction if entry point is `executeTransaction`. + /// `msg.sender` if entry point is any other function. + function getCurrentContextAddress() internal view returns (address); diff --git a/packages/contracts/src/utils/crypto.ts b/packages/contracts/src/utils/crypto.ts index 85a37122a..222cb7cca 100644 --- a/packages/contracts/src/utils/crypto.ts +++ b/packages/contracts/src/utils/crypto.ts @@ -29,8 +29,6 @@ export const crypto = { args[i] = new BN(arg.toString(10), 10); } else if (ethUtil.isValidAddress(arg)) { argTypes.push('address'); - } else if (arg instanceof Buffer) { - argTypes.push('bytes'); } else if (_.isString(arg)) { argTypes.push('string'); } else if (_.isBuffer(arg)) { diff --git a/packages/contracts/test/exchange/transactions.ts b/packages/contracts/test/exchange/transactions.ts index 62bba0ab8..2bc078153 100644 --- a/packages/contracts/test/exchange/transactions.ts +++ b/packages/contracts/test/exchange/transactions.ts @@ -49,6 +49,7 @@ describe('Exchange transactions', () => { let erc20Balances: ERC20BalancesByOwner; let signedOrder: SignedOrder; + let signedTx: SignedTransaction; let order: OrderStruct; let orderFactory: OrderFactory; let makerTransactionFactory: TransactionFactory; @@ -111,33 +112,28 @@ describe('Exchange transactions', () => { describe('executeTransaction', () => { describe('fillOrder', () => { + let takerAssetFillAmount: BigNumber; beforeEach(async () => { erc20Balances = await erc20Wrapper.getBalancesAsync(); signedOrder = orderFactory.newSignedOrder(); order = orderUtils.getOrderStruct(signedOrder); - }); - it('should throw if not called by specified sender', async () => { - const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); + takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); const data = exchange.fillOrder.getABIEncodedTransactionData( order, takerAssetFillAmount, signedOrder.signature, ); - const signedTx = takerTransactionFactory.newSignedTransaction(data); + signedTx = takerTransactionFactory.newSignedTransaction(data); + }); + + it('should throw if not called by specified sender', async () => { return expect(exchangeWrapper.executeTransactionAsync(signedTx, takerAddress)).to.be.rejectedWith( constants.REVERT, ); }); it('should transfer the correct amounts when signed by taker and called by sender', async () => { - const takerAssetFillAmount = signedOrder.takerAssetAmount.div(2); - const data = exchange.fillOrder.getABIEncodedTransactionData( - order, - takerAssetFillAmount, - signedOrder.signature, - ); - const signedTx = takerTransactionFactory.newSignedTransaction(data); await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); const makerAssetFillAmount = takerAssetFillAmount @@ -171,20 +167,34 @@ describe('Exchange transactions', () => { erc20Balances[feeRecipientAddress][zrxToken.address].add(makerFeePaid.add(takerFeePaid)), ); }); + + it('should throw if the a 0x transaction with the same transactionHash has already been executed', async () => { + await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + return expect(exchangeWrapper.executeTransactionAsync(signedTx, senderAddress)).to.be.rejectedWith( + constants.REVERT, + ); + }); + + it('should reset the currentContextAddress', async () => { + await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); + const currentContextAddress = await exchange.currentContextAddress.callAsync(); + expect(currentContextAddress).to.equal(ZeroEx.NULL_ADDRESS); + }); }); describe('cancelOrder', () => { - it('should throw if not called by specified sender', async () => { + beforeEach(async () => { const data = exchange.cancelOrder.getABIEncodedTransactionData(order); - const signedTx = makerTransactionFactory.newSignedTransaction(data); + signedTx = makerTransactionFactory.newSignedTransaction(data); + }); + + it('should throw if not called by specified sender', async () => { return expect(exchangeWrapper.executeTransactionAsync(signedTx, makerAddress)).to.be.rejectedWith( constants.REVERT, ); }); it('should cancel the order when signed by maker and called by sender', async () => { - const data = exchange.cancelOrder.getABIEncodedTransactionData(order); - const signedTx = makerTransactionFactory.newSignedTransaction(data); await exchangeWrapper.executeTransactionAsync(signedTx, senderAddress); const res = await exchangeWrapper.fillOrderAsync(signedOrder, takerAddress); const newBalances = await erc20Wrapper.getBalancesAsync(); -- cgit v1.2.3