From 07cdfa655be81862f7ff40fe5ac6fdb38d780370 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 1 Jun 2017 15:12:27 +0200 Subject: Add FILL_AMOUNT_IS_ZERO check --- src/contract_wrappers/exchange_wrapper.ts | 9 +++++++++ src/types.ts | 5 +++++ test/exchange_wrapper_test.ts | 4 ++-- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index d1763e307..7a5b6ac8c 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -4,6 +4,7 @@ import { ECSignature, ExchangeContract, ExchangeContractErrs, + FillOrderValidationErrs, OrderValues, OrderAddresses, SignedOrder, @@ -66,6 +67,8 @@ export class ExchangeWrapper extends ContractWrapper { const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); const exchangeInstance = await this.getExchangeInstanceOrThrowAsync(); + this.validateFillOrder(signedOrder, fillAmount, senderAddress, shouldCheckTransfer); + const orderAddresses: OrderAddresses = [ signedOrder.maker, signedOrder.taker, @@ -108,6 +111,12 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } + private validateFillOrder(signedOrder: SignedOrder, fillAmount: BigNumber.BigNumber, senderAddress: string, + shouldCheckTransfer: boolean = true) { + if (fillAmount.eq(0)) { + throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); + } + } private async getExchangeInstanceOrThrowAsync(): Promise { const contractInstance = await this.instantiateContractIfExistsAsync((ExchangeArtifacts as any)); return contractInstance as ExchangeContract; diff --git a/src/types.ts b/src/types.ts index 7d668e78a..d9ed0b6bf 100644 --- a/src/types.ts +++ b/src/types.ts @@ -79,6 +79,11 @@ export enum ExchangeContractErrs { ERROR_CANCEL_NO_VALUE, // Order has already been fully filled or cancelled } +export const FillOrderValidationErrs = strEnum([ + 'FILL_AMOUNT_IS_ZERO', +]); +export type FillOrderValidationErrs = keyof typeof FillOrderValidationErrs; + export interface ContractResponse { logs: ContractEvent[]; } diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index b2fb1894b..4867427ae 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -7,7 +7,7 @@ import * as _ from 'lodash'; import {BlockchainLifecycle} from './utils/blockchain_lifecycle'; import * as BigNumber from 'bignumber.js'; import {orderFactory} from './utils/order_factory'; -import {Token} from '../src/types'; +import {FillOrderValidationErrs, Token} from '../src/types'; import * as Web3 from 'web3'; import * as dirtyChai from 'dirty-chai'; import ChaiBigNumber = require('chai-bignumber'); @@ -134,7 +134,7 @@ describe('ExchangeWrapper', () => { 5, addressBySymbol.MLN, 5, addressBySymbol.GNT); const fillAmount = new BigNumber(0); expect(zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount)) - .to.be.rejectedWith('This order has already been filled or cancelled'); + .to.be.rejectedWith(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); }); }); describe('successful fills', () => { -- cgit v1.2.3 From d5d20db439a4a6fe913462bd216a776cc4300450 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 1 Jun 2017 15:18:13 +0200 Subject: Add NOT_A_TAKER check --- src/contract_wrappers/exchange_wrapper.ts | 3 +++ src/types.ts | 1 + test/exchange_wrapper_test.ts | 11 ++++++++++- 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 7a5b6ac8c..e53754e07 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -116,6 +116,9 @@ export class ExchangeWrapper extends ContractWrapper { if (fillAmount.eq(0)) { throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); } + if (signedOrder.taker !== constants.NULL_ADDRESS && signedOrder.taker !== senderAddress) { + throw new Error(FillOrderValidationErrs.NOT_A_TAKER); + } } private async getExchangeInstanceOrThrowAsync(): Promise { const contractInstance = await this.instantiateContractIfExistsAsync((ExchangeArtifacts as any)); diff --git a/src/types.ts b/src/types.ts index d9ed0b6bf..d6970bc0b 100644 --- a/src/types.ts +++ b/src/types.ts @@ -81,6 +81,7 @@ export enum ExchangeContractErrs { export const FillOrderValidationErrs = strEnum([ 'FILL_AMOUNT_IS_ZERO', + 'NOT_A_TAKER', ]); export type FillOrderValidationErrs = keyof typeof FillOrderValidationErrs; diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index 4867427ae..c5cbc58be 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -129,13 +129,22 @@ describe('ExchangeWrapper', () => { describe('failed fills', () => { it('should throw when the fill amount is zero', async () => { const maker = userAddresses[0]; - const taker = userAddresses[1]; + const taker = userAddresses[0]; const signedOrder = await orderFactory.createSignedOrderAsync(zeroEx, networkId, maker, taker, 5, addressBySymbol.MLN, 5, addressBySymbol.GNT); const fillAmount = new BigNumber(0); expect(zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount)) .to.be.rejectedWith(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); }); + it('should throw when sender is not a taker', async () => { + const maker = userAddresses[0]; + const taker = userAddresses[1]; + const signedOrder = await orderFactory.createSignedOrderAsync(zeroEx, networkId, maker, taker, + 5, addressBySymbol.MLN, 5, addressBySymbol.GNT); + const fillAmount = new BigNumber(5); + expect(zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount)) + .to.be.rejectedWith(FillOrderValidationErrs.NOT_A_TAKER); + }); }); describe('successful fills', () => { afterEach('reset default account', () => { -- cgit v1.2.3 From 21d00f04d46a8067801ff539fb21cbbed416c8b3 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 1 Jun 2017 15:21:24 +0200 Subject: Fix tests --- test/0x.js_test.ts | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/test/0x.js_test.ts b/test/0x.js_test.ts index 32040bd33..a548d8268 100644 --- a/test/0x.js_test.ts +++ b/test/0x.js_test.ts @@ -48,6 +48,7 @@ describe('ZeroEx library', () => { const expectedOrderHash = '0x103a5e97dab5dbeb8f385636f86a7d1e458a7ccbe1bd194727f0b2f85ab116c7'; const order: Order = { maker: constants.NULL_ADDRESS, + taker: constants.NULL_ADDRESS, feeRecipient: constants.NULL_ADDRESS, makerTokenAddress: constants.NULL_ADDRESS, takerTokenAddress: constants.NULL_ADDRESS, @@ -59,15 +60,8 @@ describe('ZeroEx library', () => { expirationUnixTimestampSec: new BigNumber(0), }; const exchangeAddress = constants.NULL_ADDRESS; - it('defaults takerAddress to NULL address', () => { - const orderHash = ZeroEx.getOrderHashHex(exchangeAddress, order); - expect(orderHash).to.be.equal(expectedOrderHash); - }); it('calculates the order hash', () => { - const orderWithZeroTaker = _.assign(order, { - taker: constants.NULL_ADDRESS, - }); - const orderHash = ZeroEx.getOrderHashHex(exchangeAddress, orderWithZeroTaker); + const orderHash = ZeroEx.getOrderHashHex(exchangeAddress, order); expect(orderHash).to.be.equal(expectedOrderHash); }); }); -- cgit v1.2.3 From d8e35c364ea94b606810b340fb02d8706e257c3c Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Thu, 1 Jun 2017 15:46:27 +0200 Subject: Add EXPIRED test --- package.json | 2 +- src/contract_wrappers/exchange_wrapper.ts | 3 +++ src/types.ts | 3 ++- test/exchange_wrapper_test.ts | 38 ++++++++++++++++++------------- test/utils/order_factory.ts | 10 +++++--- 5 files changed, 35 insertions(+), 21 deletions(-) diff --git a/package.json b/package.json index 8cef48c95..8b472f2a9 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,7 @@ "pretest:umd": "run-s clean build:*:dev", "substitute_umd_bundle": "npm run remove_src_files_not_used_by_tests; shx mv _bundles/* lib/src", "remove_src_files_not_used_by_tests": "find ./lib/src \\( -path ./lib/src/utils -o -path ./lib/src/schemas -o -path \"./lib/src/types.*\" \\) -prune -o -type f -print | xargs rm", - "run_mocha": "mocha lib/test/**/*_test.js" + "run_mocha": "mocha lib/test/**/*_test.js --timeout 3000" }, "config": { "artifacts": "Proxy Exchange TokenRegistry Token Mintable EtherToken", diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index e53754e07..88b0fa913 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -119,6 +119,9 @@ export class ExchangeWrapper extends ContractWrapper { if (signedOrder.taker !== constants.NULL_ADDRESS && signedOrder.taker !== senderAddress) { throw new Error(FillOrderValidationErrs.NOT_A_TAKER); } + if (signedOrder.expirationUnixTimestampSec.lessThan(Date.now() / 1000)) { + throw new Error(FillOrderValidationErrs.EXPIRED); + } } private async getExchangeInstanceOrThrowAsync(): Promise { const contractInstance = await this.instantiateContractIfExistsAsync((ExchangeArtifacts as any)); diff --git a/src/types.ts b/src/types.ts index d6970bc0b..6d4166013 100644 --- a/src/types.ts +++ b/src/types.ts @@ -31,7 +31,7 @@ export type OrderAddresses = [string, string, string, string, string]; export type OrderValues = [ BigNumber.BigNumber, BigNumber.BigNumber, BigNumber.BigNumber, - BigNumber.BigNumber, BigNumber.BigNumber, BigNumber.BigNumber, + BigNumber.BigNumber, BigNumber.BigNumber, BigNumber.BigNumber ]; export interface ExchangeContract { @@ -82,6 +82,7 @@ export enum ExchangeContractErrs { export const FillOrderValidationErrs = strEnum([ 'FILL_AMOUNT_IS_ZERO', 'NOT_A_TAKER', + 'EXPIRED', ]); export type FillOrderValidationErrs = keyof typeof FillOrderValidationErrs; diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index c5cbc58be..f929df149 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -107,6 +107,9 @@ describe('ExchangeWrapper', () => { let tokens: Token[]; const addressBySymbol: {[symbol: string]: string} = {}; let networkId: number; + let maker: string; + let taker: string; + const fillAmount = new BigNumber(5); const setBalance = async (toAddress: string, amountInBaseUnits: BigNumber.BigNumber|number, tokenAddress: string) => { @@ -126,40 +129,43 @@ describe('ExchangeWrapper', () => { }); networkId = await promisify(web3.version.getNetwork)(); }); + beforeEach('get ready for fill', async () => { + [maker, taker] = userAddresses; + zeroEx.setDefaultAccount(taker); + await setAllowance(maker, 5, addressBySymbol.MLN); + await setBalance(taker, 5, addressBySymbol.GNT); + await setAllowance(taker, 5, addressBySymbol.GNT); + }); + afterEach('reset sender', () => { + zeroEx.setDefaultAccount(userAddresses[0]); + }); describe('failed fills', () => { it('should throw when the fill amount is zero', async () => { - const maker = userAddresses[0]; - const taker = userAddresses[0]; const signedOrder = await orderFactory.createSignedOrderAsync(zeroEx, networkId, maker, taker, 5, addressBySymbol.MLN, 5, addressBySymbol.GNT); - const fillAmount = new BigNumber(0); expect(zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount)) .to.be.rejectedWith(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); }); it('should throw when sender is not a taker', async () => { - const maker = userAddresses[0]; - const taker = userAddresses[1]; const signedOrder = await orderFactory.createSignedOrderAsync(zeroEx, networkId, maker, taker, 5, addressBySymbol.MLN, 5, addressBySymbol.GNT); - const fillAmount = new BigNumber(5); + const notTaker = userAddresses[2]; + zeroEx.setDefaultAccount(notTaker); expect(zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount)) .to.be.rejectedWith(FillOrderValidationErrs.NOT_A_TAKER); }); + it('should throw when order is expired', async () => { + const OLD_TIMESTAMP = new BigNumber(42); + const signedOrder = await orderFactory.createSignedOrderAsync(zeroEx, networkId, maker, taker, + 5, addressBySymbol.MLN, 5, addressBySymbol.GNT, OLD_TIMESTAMP); + expect(zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount)) + .to.be.rejectedWith(FillOrderValidationErrs.EXPIRED); + }); }); describe('successful fills', () => { - afterEach('reset default account', () => { - zeroEx.setDefaultAccount(userAddresses[0]); - }); it('should fill the valid order', async () => { - const maker = userAddresses[0]; - const taker = userAddresses[1]; - await setAllowance(maker, 5, addressBySymbol.MLN); - await setBalance(taker, 5, addressBySymbol.GNT); - await setAllowance(taker, 5, addressBySymbol.GNT); const signedOrder = await orderFactory.createSignedOrderAsync(zeroEx, networkId, maker, taker, 5, addressBySymbol.MLN, 5, addressBySymbol.GNT); - const fillAmount = new BigNumber(5); - zeroEx.setDefaultAccount(taker); await zeroEx.exchange.fillOrderAsync(signedOrder, fillAmount); expect(await zeroEx.token.getBalanceAsync(addressBySymbol.MLN, taker)).to.be.bignumber.equal(5); expect(await zeroEx.token.getBalanceAsync(addressBySymbol.GNT, taker)).to.be.bignumber.equal(0); diff --git a/test/utils/order_factory.ts b/test/utils/order_factory.ts index e41e973ee..c6c6ed927 100644 --- a/test/utils/order_factory.ts +++ b/test/utils/order_factory.ts @@ -14,10 +14,14 @@ export const orderFactory = { makerTokenAmount: BigNumber.BigNumber|number, makerTokenAddress: string, takerTokenAmount: BigNumber.BigNumber|number, - takerTokenAddress: string): Promise { + takerTokenAddress: string, + expirationUnixTimestampSec?: BigNumber.BigNumber): Promise { // TODO refactor and check const exchangeAddress: string = (ExchangeArtifacts as any).networks[networkId].address; - const INF_TIMESTAMP = 2524604400; + const INF_TIMESTAMP = new BigNumber(2524604400); + expirationUnixTimestampSec = _.isUndefined(expirationUnixTimestampSec) ? + INF_TIMESTAMP : + expirationUnixTimestampSec; const order = { maker, taker, @@ -29,7 +33,7 @@ export const orderFactory = { takerTokenAddress, salt: ZeroEx.generatePseudoRandomSalt(), feeRecipient: constants.NULL_ADDRESS, - expirationUnixTimestampSec: new BigNumber(INF_TIMESTAMP), + expirationUnixTimestampSec, }; const orderHash = ZeroEx.getOrderHashHex(exchangeAddress, order); const ecSignature = await zeroEx.signOrderHashAsync(orderHash); -- cgit v1.2.3