From 918315e89f3408124d2e78bbd1acb58ed42d1766 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 12:40:58 +0200 Subject: Implement fillOrKill & tests --- src/0x.js.ts | 27 ++------ src/contract_wrappers/exchange_wrapper.ts | 105 +++++++++++++++++++++++++----- src/globals.d.ts | 4 +- src/types.ts | 15 +++-- src/utils/constants.ts | 1 + src/utils/utils.ts | 24 +++++++ test/exchange_wrapper_test.ts | 103 ++++++++++++++++++++++++++++- 7 files changed, 233 insertions(+), 46 deletions(-) diff --git a/src/0x.js.ts b/src/0x.js.ts index 0f437e039..6d66c9d86 100644 --- a/src/0x.js.ts +++ b/src/0x.js.ts @@ -4,7 +4,6 @@ import {bigNumberConfigs} from './bignumber_config'; import * as ethUtil from 'ethereumjs-util'; import contract = require('truffle-contract'); import * as Web3 from 'web3'; -import * as ethABI from 'ethereumjs-abi'; import findVersions = require('find-versions'); import compareVersions = require('compare-versions'); import {Web3Wrapper} from './web3_wrapper'; @@ -16,8 +15,7 @@ import {ExchangeWrapper} from './contract_wrappers/exchange_wrapper'; import {TokenRegistryWrapper} from './contract_wrappers/token_registry_wrapper'; import {ecSignatureSchema} from './schemas/ec_signature_schema'; import {TokenWrapper} from './contract_wrappers/token_wrapper'; -import {SolidityTypes, ECSignature, ZeroExError} from './types'; -import {Order, SignedOrder} from './types'; +import {SolidityTypes, ECSignature, ZeroExError, Order, SignedOrder} from './types'; import {orderSchema} from './schemas/order_schemas'; import * as ExchangeArtifacts from './artifacts/Exchange.json'; @@ -132,30 +130,13 @@ export class ZeroEx { * Computes the orderHash for a given order and returns it as a hex encoded string. */ public async getOrderHashHexAsync(order: Order|SignedOrder): Promise { - const exchangeContractAddr = await this.getExchangeAddressAsync(); assert.doesConformToSchema('order', SchemaValidator.convertToJSONSchemaCompatibleObject(order as object), orderSchema); - const orderParts = [ - {value: exchangeContractAddr, type: SolidityTypes.address}, - {value: order.maker, type: SolidityTypes.address}, - {value: order.taker, type: SolidityTypes.address}, - {value: order.makerTokenAddress, type: SolidityTypes.address}, - {value: order.takerTokenAddress, type: SolidityTypes.address}, - {value: order.feeRecipient, type: SolidityTypes.address}, - {value: utils.bigNumberToBN(order.makerTokenAmount), type: SolidityTypes.uint256}, - {value: utils.bigNumberToBN(order.takerTokenAmount), type: SolidityTypes.uint256}, - {value: utils.bigNumberToBN(order.makerFee), type: SolidityTypes.uint256}, - {value: utils.bigNumberToBN(order.takerFee), type: SolidityTypes.uint256}, - {value: utils.bigNumberToBN(order.expirationUnixTimestampSec), type: SolidityTypes.uint256}, - {value: utils.bigNumberToBN(order.salt), type: SolidityTypes.uint256}, - ]; - const types = _.map(orderParts, o => o.type); - const values = _.map(orderParts, o => o.value); - const hashBuff = ethABI.soliditySHA3(types, values); - const hashHex = ethUtil.bufferToHex(hashBuff); - return hashHex; + const exchangeContractAddr = await this.getExchangeAddressAsync(); + const orderHash = utils.getOrderHashHex(order, exchangeContractAddr); + return orderHash; } /** * Signs an orderHash and returns it's elliptic curve signature diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index d3a53a9f7..55ff9068e 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -7,6 +7,7 @@ import { ExchangeContract, ExchangeContractErrCodes, ExchangeContractErrs, + Order, OrderValues, OrderAddresses, SignedOrder, @@ -126,21 +127,9 @@ export class ExchangeWrapper extends ContractWrapper { const exchangeInstance = await this.getExchangeContractAsync(); await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress); - const orderAddresses: OrderAddresses = [ - signedOrder.maker, - signedOrder.taker, - signedOrder.makerTokenAddress, - signedOrder.takerTokenAddress, - signedOrder.feeRecipient, - ]; - const orderValues: OrderValues = [ - signedOrder.makerTokenAmount, - signedOrder.takerTokenAmount, - signedOrder.makerFee, - signedOrder.takerFee, - signedOrder.expirationUnixTimestampSec, - signedOrder.salt, - ]; + const orderAddresses = this.getOrderAddresses(signedOrder); + const orderValues = this.getOrderValues(signedOrder); + const gas = await exchangeInstance.fill.estimateGas( orderAddresses, orderValues, @@ -168,6 +157,67 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } + /** + * Attempts to fill a specific amount of an order. If the entire amount specified cannot be filled, + * the fill order is abandoned. + */ + public async fillOrKillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, + shouldCheckTransfer: boolean, takerAddress: string) { + assert.doesConformToSchema('signedOrder', + SchemaValidator.convertToJSONSchemaCompatibleObject(signedOrder as object), + signedOrderSchema); + assert.isBigNumber('fillTakerAmount', fillTakerAmount); + assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer); + await assert.isSenderAddressAsync('takerAddress', takerAddress, this.web3Wrapper); + + const exchangeInstance = await this.getExchangeContractAsync(); + await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress); + + // Check that fillValue available >= fillTakerAmount + const orderHashHex = utils.getOrderHashHex(signedOrder, exchangeInstance.address); + const unavailableTakerAmount = await this.getUnavailableTakerAmountAsync(orderHashHex); + const remainingTakerAmount = signedOrder.takerTokenAmount.minus(unavailableTakerAmount); + if (remainingTakerAmount < fillTakerAmount) { + throw new Error(ExchangeContractErrs.INSUFFICIENT_REMAINING_FILL_AMOUNT); + } + + const orderAddresses = this.getOrderAddresses(signedOrder); + const orderValues = this.getOrderValues(signedOrder); + + const gas = await exchangeInstance.fillOrKill.estimateGas( + orderAddresses, + orderValues, + fillTakerAmount, + signedOrder.ecSignature.v, + signedOrder.ecSignature.r, + signedOrder.ecSignature.s, + { + from: takerAddress, + }, + ); + try { + const response: ContractResponse = await exchangeInstance.fillOrKill( + orderAddresses, + orderValues, + fillTakerAmount, + signedOrder.ecSignature.v, + signedOrder.ecSignature.r, + signedOrder.ecSignature.s, + { + from: takerAddress, + gas, + }, + ); + this.throwErrorLogsAsErrors(response.logs); + } catch (err) { + // There is a potential race condition where when the cancellation is broadcasted, a sufficient + // fillAmount is available, but by the time the transaction gets mined, it no longer is. Instead of + // throwing an invalid jump exception, we would rather give the user a more helpful error message. + if (_.includes(err, constants.INVALID_JUMP_IDENTIFIER)) { + throw new Error(ZeroExError.INSUFFICIENT_REMAINING_FILL_AMOUNT); + } + } + } /** * Subscribe to an event type emitted by the Exchange smart contract */ @@ -232,8 +282,8 @@ export class ExchangeWrapper extends ContractWrapper { * Handling the edge-cases that arise when this happens would require making sure that the user has sufficient * funds to pay both the fees and the transfer amount. We decided to punt on this for now as the contracts * will throw for these edge-cases. - * TODO: Throw errors before calling the smart contract for these edge-cases - * TODO: in order to minimize the callers gas costs. + * TODO: Throw errors before calling the smart contract for these edge-cases in order to minimize + * the callers gas costs. */ private async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, @@ -316,4 +366,25 @@ export class ExchangeWrapper extends ContractWrapper { const exchangeInstance = await this.getExchangeContractAsync(); return exchangeInstance.ZRX.call(); } + private getOrderAddresses(order: Order|SignedOrder) { + const orderAddresses: OrderAddresses = [ + order.maker, + order.taker, + order.makerTokenAddress, + order.takerTokenAddress, + order.feeRecipient, + ]; + return orderAddresses; + } + private getOrderValues(order: Order|SignedOrder) { + const orderValues: OrderValues = [ + order.makerTokenAmount, + order.takerTokenAmount, + order.makerFee, + order.takerFee, + order.expirationUnixTimestampSec, + order.salt, + ]; + return orderValues; + } } diff --git a/src/globals.d.ts b/src/globals.d.ts index 164fc2386..567ba016d 100644 --- a/src/globals.d.ts +++ b/src/globals.d.ts @@ -47,7 +47,9 @@ declare module 'ethereumjs-util' { } // truffle-contract declarations -declare interface ContractInstance {} +declare interface ContractInstance { + address: string; +} declare interface ContractFactory { setProvider: (providerObj: any) => void; deployed: () => ContractInstance; diff --git a/src/types.ts b/src/types.ts index a02bd0252..49d8365d1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -44,7 +44,8 @@ export interface ContractEventObj { } export type CreateContractEvent = (indexFilterValues: IndexFilterValues, subscriptionOpts: SubscriptionOpts) => ContractEventObj; -export interface ExchangeContract { + +export interface ExchangeContract extends ContractInstance { isValidSignature: { call: (signerAddressHex: string, dataHex: string, v: number, r: string, s: string, txOpts?: TxOpts) => Promise; @@ -68,6 +69,12 @@ export interface ExchangeContract { estimateGas: (orderAddresses: OrderAddresses, orderValues: OrderValues, fillAmount: BigNumber.BigNumber, shouldCheckTransfer: boolean, v: number, r: string, s: string, txOpts: TxOpts) => number; }; + fillOrKill: { + (orderAddresses: OrderAddresses, orderValues: OrderValues, fillAmount: BigNumber.BigNumber, + v: number, r: string, s: string, txOpts: TxOpts): ContractResponse; + estimateGas: (orderAddresses: OrderAddresses, orderValues: OrderValues, fillAmount: BigNumber.BigNumber, + v: number, r: string, s: string, txOpts: TxOpts) => number; + }; filled: { call: (orderHash: string) => BigNumber.BigNumber; }; @@ -76,7 +83,7 @@ export interface ExchangeContract { }; } -export interface TokenContract { +export interface TokenContract extends ContractInstance { balanceOf: { call: (address: string) => Promise; }; @@ -89,7 +96,7 @@ export interface TokenContract { approve: (proxyAddress: string, amountInBaseUnits: BigNumber.BigNumber, txOpts: TxOpts) => void; } -export interface TokenRegistryContract { +export interface TokenRegistryContract extends ContractInstance { getTokenMetaData: { call: (address: string) => Promise; }; @@ -127,7 +134,7 @@ export const ExchangeContractErrs = strEnum([ 'INSUFFICIENT_MAKER_FEE_BALANCE', 'INSUFFICIENT_MAKER_FEE_ALLOWANCE', 'TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER', - + 'INSUFFICIENT_REMAINING_FILL_AMOUNT', ]); export type ExchangeContractErrs = keyof typeof ExchangeContractErrs; diff --git a/src/utils/constants.ts b/src/utils/constants.ts index 5a5ba0e0a..ebc8586f3 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -1,4 +1,5 @@ export const constants = { NULL_ADDRESS: '0x0000000000000000000000000000000000000000', TESTRPC_NETWORK_ID: 50, + INVALID_JUMP_IDENTIFIER: 'invalid JUMP at', }; diff --git a/src/utils/utils.ts b/src/utils/utils.ts index 114b46f6c..1d2e2f908 100644 --- a/src/utils/utils.ts +++ b/src/utils/utils.ts @@ -1,5 +1,8 @@ import * as _ from 'lodash'; import * as BN from 'bn.js'; +import * as ethUtil from 'ethereumjs-util'; +import * as ethABI from 'ethereumjs-abi'; +import {SolidityTypes, Order} from '../types'; export const utils = { /** @@ -22,6 +25,27 @@ export const utils = { const isValid = /^0x[0-9A-F]{64}$/i.test(orderHashHex); return isValid; }, + getOrderHashHex(order: Order, exchangeContractAddr: string) { + const orderParts = [ + {value: exchangeContractAddr, type: SolidityTypes.address}, + {value: order.maker, type: SolidityTypes.address}, + {value: order.taker, type: SolidityTypes.address}, + {value: order.makerTokenAddress, type: SolidityTypes.address}, + {value: order.takerTokenAddress, type: SolidityTypes.address}, + {value: order.feeRecipient, type: SolidityTypes.address}, + {value: utils.bigNumberToBN(order.makerTokenAmount), type: SolidityTypes.uint256}, + {value: utils.bigNumberToBN(order.takerTokenAmount), type: SolidityTypes.uint256}, + {value: utils.bigNumberToBN(order.makerFee), type: SolidityTypes.uint256}, + {value: utils.bigNumberToBN(order.takerFee), type: SolidityTypes.uint256}, + {value: utils.bigNumberToBN(order.expirationUnixTimestampSec), type: SolidityTypes.uint256}, + {value: utils.bigNumberToBN(order.salt), type: SolidityTypes.uint256}, + ]; + const types = _.map(orderParts, o => o.type); + const values = _.map(orderParts, o => o.value); + const hashBuff = ethABI.soliditySHA3(types, values); + const hashHex = ethUtil.bufferToHex(hashBuff); + return hashHex; + }, spawnSwitchErr(name: string, value: any) { return new Error(`Unexpected switch value: ${value} encountered for ${name}`); }, diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index 370e741b4..9ef20736f 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -120,6 +120,84 @@ describe('ExchangeWrapper', () => { expect(isValid).to.be.true(); }); }); + describe('#fillOrKillOrderAsync', () => { + let makerTokenAddress: string; + let takerTokenAddress: string; + let coinbase: string; + let makerAddress: string; + let takerAddress: string; + let feeRecipient: string; + const fillTakerAmount = new BigNumber(5); + const shouldCheckTransfer = false; + before(async () => { + [coinbase, makerAddress, takerAddress, feeRecipient] = userAddresses; + tokens = await zeroEx.tokenRegistry.getTokensAsync(); + const [makerToken, takerToken] = tokenUtils.getNonProtocolTokens(); + makerTokenAddress = makerToken.address; + takerTokenAddress = takerToken.address; + }); + describe('failed fillOrKill', () => { + it('should throw if remaining fillAmount is less then the desired fillAmount', async () => { + const fillableAmount = new BigNumber(5); + const signedOrder = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, + ); + const tooLargeFillAmount = new BigNumber(7); + const fillAmountDifference = tooLargeFillAmount.minus(fillableAmount); + await zeroEx.token.transferAsync(takerTokenAddress, coinbase, takerAddress, fillAmountDifference); + await zeroEx.token.setProxyAllowanceAsync(takerTokenAddress, takerAddress, tooLargeFillAmount); + await zeroEx.token.transferAsync(makerTokenAddress, coinbase, makerAddress, fillAmountDifference); + await zeroEx.token.setProxyAllowanceAsync(makerTokenAddress, makerAddress, tooLargeFillAmount); + + return expect(zeroEx.exchange.fillOrKillOrderAsync( + signedOrder, tooLargeFillAmount, shouldCheckTransfer, takerAddress, + )).to.be.rejectedWith(ExchangeContractErrs.INSUFFICIENT_REMAINING_FILL_AMOUNT); + }); + }); + describe('successful fills', () => { + it('should fill a valid order', async () => { + const fillableAmount = new BigNumber(5); + const signedOrder = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, + ); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, makerAddress)) + .to.be.bignumber.equal(fillableAmount); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, makerAddress)) + .to.be.bignumber.equal(0); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, takerAddress)) + .to.be.bignumber.equal(0); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, takerAddress)) + .to.be.bignumber.equal(fillableAmount); + await zeroEx.exchange.fillOrKillOrderAsync(signedOrder, fillTakerAmount, + shouldCheckTransfer, takerAddress); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, makerAddress)) + .to.be.bignumber.equal(fillableAmount.minus(fillTakerAmount)); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, makerAddress)) + .to.be.bignumber.equal(fillTakerAmount); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, takerAddress)) + .to.be.bignumber.equal(fillTakerAmount); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, takerAddress)) + .to.be.bignumber.equal(fillableAmount.minus(fillTakerAmount)); + }); + it('should partially fill a valid order', async () => { + const fillableAmount = new BigNumber(5); + const signedOrder = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, + ); + const partialFillAmount = new BigNumber(3); + await zeroEx.exchange.fillOrderAsync(signedOrder, partialFillAmount, + shouldCheckTransfer, takerAddress); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, makerAddress)) + .to.be.bignumber.equal(fillableAmount.minus(partialFillAmount)); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, makerAddress)) + .to.be.bignumber.equal(partialFillAmount); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, takerAddress)) + .to.be.bignumber.equal(partialFillAmount); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, takerAddress)) + .to.be.bignumber.equal(fillableAmount.minus(partialFillAmount)); + }); + }); + }); describe('#fillOrderAsync', () => { let makerTokenAddress: string; let takerTokenAddress: string; @@ -210,7 +288,7 @@ describe('ExchangeWrapper', () => { )).to.be.rejectedWith(ExchangeContractErrs.INSUFFICIENT_MAKER_ALLOWANCE); }); }); - it('should throw when there a rounding error would have occurred', async () => { + it('should throw when a rounding error would have occurred', async () => { const makerAmount = new BigNumber(3); const takerAmount = new BigNumber(5); const signedOrder = await fillScenarios.createAsymmetricFillableSignedOrderAsync( @@ -309,6 +387,29 @@ describe('ExchangeWrapper', () => { expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, takerAddress)) .to.be.bignumber.equal(fillableAmount.minus(partialFillAmount)); }); + it('should fill up to remaining amount if desired fillAmount greater than available amount', async () => { + const fillableAmount = new BigNumber(5); + const signedOrder = await fillScenarios.createFillableSignedOrderAsync( + makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, + ); + const tooLargeFillAmount = new BigNumber(7); + const fillAmountDifference = tooLargeFillAmount.minus(fillableAmount); + await zeroEx.token.transferAsync(takerTokenAddress, coinbase, takerAddress, fillAmountDifference); + await zeroEx.token.setProxyAllowanceAsync(takerTokenAddress, takerAddress, tooLargeFillAmount); + await zeroEx.token.transferAsync(makerTokenAddress, coinbase, makerAddress, fillAmountDifference); + await zeroEx.token.setProxyAllowanceAsync(makerTokenAddress, makerAddress, tooLargeFillAmount); + + await zeroEx.exchange.fillOrderAsync(signedOrder, tooLargeFillAmount, shouldCheckTransfer, + takerAddress); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, makerAddress)) + .to.be.bignumber.equal(fillAmountDifference); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, makerAddress)) + .to.be.bignumber.equal(fillableAmount); + expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, takerAddress)) + .to.be.bignumber.equal(fillableAmount); + expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, takerAddress)) + .to.be.bignumber.equal(fillAmountDifference); + }); it('should fill the valid orders with fees', async () => { const fillableAmount = new BigNumber(5); const makerFee = new BigNumber(1); -- cgit v1.2.3 From 8c81fab695230f0dba8db62d11908faa7a82cd2c Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 13:05:52 +0200 Subject: remove space --- src/types.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/types.ts b/src/types.ts index 1ee8a5bd6..92e9dfb9c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -44,7 +44,6 @@ export interface ContractEventObj { } export type CreateContractEvent = (indexFilterValues: IndexFilterValues, subscriptionOpts: SubscriptionOpts) => ContractEventObj; - export interface ExchangeContract extends ContractInstance { isValidSignature: { call: (signerAddressHex: string, dataHex: string, v: number, r: string, s: string, -- cgit v1.2.3 From 9ae2e0c1aec5ac989eca652aa8c329114bcffccf Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 13:06:01 +0200 Subject: remove unused arg --- src/contract_wrappers/exchange_wrapper.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index c24a518a4..f6451a3cc 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -178,12 +178,11 @@ export class ExchangeWrapper extends ContractWrapper { * the fill order is abandoned. */ public async fillOrKillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, - shouldCheckTransfer: boolean, takerAddress: string) { + takerAddress: string) { assert.doesConformToSchema('signedOrder', SchemaValidator.convertToJSONSchemaCompatibleObject(signedOrder as object), signedOrderSchema); assert.isBigNumber('fillTakerAmount', fillTakerAmount); - assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer); await assert.isSenderAddressAsync('takerAddress', takerAddress, this.web3Wrapper); const exchangeInstance = await this.getExchangeContractAsync(); -- cgit v1.2.3 From d3a74e51e37ab8618516a52e9161ea5dd055479a Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 13:27:36 +0200 Subject: Fix tests with removed shouldCheckTransfer --- test/exchange_wrapper_test.ts | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/exchange_wrapper_test.ts b/test/exchange_wrapper_test.ts index ff2121de1..1186345be 100644 --- a/test/exchange_wrapper_test.ts +++ b/test/exchange_wrapper_test.ts @@ -128,7 +128,6 @@ describe('ExchangeWrapper', () => { let takerAddress: string; let feeRecipient: string; const fillTakerAmount = new BigNumber(5); - const shouldCheckTransfer = false; before(async () => { [coinbase, makerAddress, takerAddress, feeRecipient] = userAddresses; tokens = await zeroEx.tokenRegistry.getTokensAsync(); @@ -150,7 +149,7 @@ describe('ExchangeWrapper', () => { await zeroEx.token.setProxyAllowanceAsync(makerTokenAddress, makerAddress, tooLargeFillAmount); return expect(zeroEx.exchange.fillOrKillOrderAsync( - signedOrder, tooLargeFillAmount, shouldCheckTransfer, takerAddress, + signedOrder, tooLargeFillAmount, takerAddress, )).to.be.rejectedWith(ExchangeContractErrs.INSUFFICIENT_REMAINING_FILL_AMOUNT); }); }); @@ -168,8 +167,7 @@ describe('ExchangeWrapper', () => { .to.be.bignumber.equal(0); expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, takerAddress)) .to.be.bignumber.equal(fillableAmount); - await zeroEx.exchange.fillOrKillOrderAsync(signedOrder, fillTakerAmount, - shouldCheckTransfer, takerAddress); + await zeroEx.exchange.fillOrKillOrderAsync(signedOrder, fillTakerAmount, takerAddress); expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, makerAddress)) .to.be.bignumber.equal(fillableAmount.minus(fillTakerAmount)); expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, makerAddress)) @@ -185,8 +183,7 @@ describe('ExchangeWrapper', () => { makerTokenAddress, takerTokenAddress, makerAddress, takerAddress, fillableAmount, ); const partialFillAmount = new BigNumber(3); - await zeroEx.exchange.fillOrderAsync(signedOrder, partialFillAmount, - shouldCheckTransfer, takerAddress); + await zeroEx.exchange.fillOrKillOrderAsync(signedOrder, partialFillAmount, takerAddress); expect(await zeroEx.token.getBalanceAsync(makerTokenAddress, makerAddress)) .to.be.bignumber.equal(fillableAmount.minus(partialFillAmount)); expect(await zeroEx.token.getBalanceAsync(takerTokenAddress, makerAddress)) -- cgit v1.2.3 From 951d15b3cad954d995402e9379547fd2de90227f Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 17:22:30 +0200 Subject: Spacing fix --- src/0x.js.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/0x.js.ts b/src/0x.js.ts index 7be6922fc..20bd2c985 100644 --- a/src/0x.js.ts +++ b/src/0x.js.ts @@ -130,8 +130,7 @@ export class ZeroEx { * Computes the orderHash for a given order and returns it as a hex encoded string. */ public async getOrderHashHexAsync(order: Order|SignedOrder): Promise { - assert.doesConformToSchema('order', - SchemaValidator.convertToJSONSchemaCompatibleObject(order as object), + assert.doesConformToSchema('order', SchemaValidator.convertToJSONSchemaCompatibleObject(order as object), orderSchema); const exchangeContractAddr = await this.getExchangeAddressAsync(); -- cgit v1.2.3 From bc441015b672c310bd4b4a67fcf9a98e28793883 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 17:23:59 +0200 Subject: add `hex` to function and variable name for clarity --- src/0x.js.ts | 4 ++-- src/contract_wrappers/exchange_wrapper.ts | 8 ++++---- test/0x.js_test.ts | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/0x.js.ts b/src/0x.js.ts index 20bd2c985..8f1178b2a 100644 --- a/src/0x.js.ts +++ b/src/0x.js.ts @@ -134,8 +134,8 @@ export class ZeroEx { orderSchema); const exchangeContractAddr = await this.getExchangeAddressAsync(); - const orderHash = utils.getOrderHashHex(order, exchangeContractAddr); - return orderHash; + const orderHashHex = utils.getOrderHashHex(order, exchangeContractAddr); + return orderHashHex; } /** * Signs an orderHash and returns it's elliptic curve signature diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index f6451a3cc..d25b8aa29 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -292,11 +292,11 @@ export class ExchangeWrapper extends ContractWrapper { logEventObj.watch(callback); this.exchangeLogEventObjs.push(logEventObj); } - private async getOrderHashAsync(order: Order|SignedOrder): Promise { + private async getOrderHashHexAsync(order: Order|SignedOrder): Promise { const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order); const exchangeInstance = await this.getExchangeContractAsync(); - const orderHash = utils.getOrderHashHex(order, exchangeInstance.address); - return orderHash; + const orderHashHex = utils.getOrderHashHex(order, exchangeInstance.address); + return orderHashHex; } private async stopWatchingExchangeLogEventsAsync() { const stopWatchingPromises = _.map(this.exchangeLogEventObjs, logEventObj => { @@ -334,7 +334,7 @@ export class ExchangeWrapper extends ContractWrapper { if (takerTokenCancelAmount.eq(0)) { throw new Error(ExchangeContractErrs.ORDER_CANCEL_AMOUNT_ZERO); } - const orderHash = await this.getOrderHashAsync(order); + const orderHash = await this.getOrderHashHexAsync(order); const unavailableAmount = await this.getUnavailableTakerAmountAsync(orderHash); if (order.takerTokenAmount.minus(unavailableAmount).eq(0)) { throw new Error(ExchangeContractErrs.ORDER_ALREADY_CANCELLED_OR_FILLED); diff --git a/test/0x.js_test.ts b/test/0x.js_test.ts index 5096d5df2..58f259a11 100644 --- a/test/0x.js_test.ts +++ b/test/0x.js_test.ts @@ -151,7 +151,7 @@ describe('ZeroEx library', () => { expect(baseUnitAmount).to.be.bignumber.equal(expectedUnitAmount); }); }); - describe('#getOrderHashAsync', () => { + describe('#getOrderHashHexAsync', () => { const exchangeContractAddress = constants.NULL_ADDRESS; const expectedOrderHash = '0x103a5e97dab5dbeb8f385636f86a7d1e458a7ccbe1bd194727f0b2f85ab116c7'; const order: Order = { -- cgit v1.2.3 From 9e855b4c6ad3baef8be3a725ca26b679c2542c00 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 17:53:52 +0200 Subject: Remove catch of invalid jump throws since there are many reasons the contracts could throw this error --- src/contract_wrappers/exchange_wrapper.ts | 38 ++++++++++++------------------- src/utils/constants.ts | 1 - 2 files changed, 14 insertions(+), 25 deletions(-) diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index d25b8aa29..4f132656e 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -189,7 +189,7 @@ export class ExchangeWrapper extends ContractWrapper { await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, takerAddress); // Check that fillValue available >= fillTakerAmount - const orderHashHex = utils.getOrderHashHex(signedOrder, exchangeInstance.address); + const orderHashHex = await this.getOrderHashHexAsync(signedOrder); const unavailableTakerAmount = await this.getUnavailableTakerAmountAsync(orderHashHex); const remainingTakerAmount = signedOrder.takerTokenAmount.minus(unavailableTakerAmount); if (remainingTakerAmount < fillTakerAmount) { @@ -209,28 +209,19 @@ export class ExchangeWrapper extends ContractWrapper { from: takerAddress, }, ); - try { - const response: ContractResponse = await exchangeInstance.fillOrKill( - orderAddresses, - orderValues, - fillTakerAmount, - signedOrder.ecSignature.v, - signedOrder.ecSignature.r, - signedOrder.ecSignature.s, - { - from: takerAddress, - gas, - }, - ); - this.throwErrorLogsAsErrors(response.logs); - } catch (err) { - // There is a potential race condition where when the cancellation is broadcasted, a sufficient - // fillAmount is available, but by the time the transaction gets mined, it no longer is. Instead of - // throwing an invalid jump exception, we would rather give the user a more helpful error message. - if (_.includes(err, constants.INVALID_JUMP_IDENTIFIER)) { - throw new Error(ExchangeContractErrs.INSUFFICIENT_REMAINING_FILL_AMOUNT); - } - } + const response: ContractResponse = await exchangeInstance.fillOrKill( + orderAddresses, + orderValues, + fillTakerAmount, + signedOrder.ecSignature.v, + signedOrder.ecSignature.r, + signedOrder.ecSignature.s, + { + from: takerAddress, + gas, + }, + ); + this.throwErrorLogsAsErrors(response.logs); } /** * Cancel a given fill amount of an order. Cancellations are cumulative. @@ -293,7 +284,6 @@ export class ExchangeWrapper extends ContractWrapper { this.exchangeLogEventObjs.push(logEventObj); } private async getOrderHashHexAsync(order: Order|SignedOrder): Promise { - const [orderAddresses, orderValues] = ExchangeWrapper.getOrderAddressesAndValues(order); const exchangeInstance = await this.getExchangeContractAsync(); const orderHashHex = utils.getOrderHashHex(order, exchangeInstance.address); return orderHashHex; diff --git a/src/utils/constants.ts b/src/utils/constants.ts index ebc8586f3..5a5ba0e0a 100644 --- a/src/utils/constants.ts +++ b/src/utils/constants.ts @@ -1,5 +1,4 @@ export const constants = { NULL_ADDRESS: '0x0000000000000000000000000000000000000000', TESTRPC_NETWORK_ID: 50, - INVALID_JUMP_IDENTIFIER: 'invalid JUMP at', }; -- cgit v1.2.3 From ca308354880e81425043cd586da6c781013bea9d Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Wed, 7 Jun 2017 17:54:27 +0200 Subject: make txOpts optional --- src/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/types.ts b/src/types.ts index 92e9dfb9c..cc145dc2e 100644 --- a/src/types.ts +++ b/src/types.ts @@ -76,9 +76,9 @@ export interface ExchangeContract extends ContractInstance { }; fillOrKill: { (orderAddresses: OrderAddresses, orderValues: OrderValues, fillAmount: BigNumber.BigNumber, - v: number, r: string, s: string, txOpts: TxOpts): ContractResponse; + v: number, r: string, s: string, txOpts?: TxOpts): ContractResponse; estimateGas: (orderAddresses: OrderAddresses, orderValues: OrderValues, fillAmount: BigNumber.BigNumber, - v: number, r: string, s: string, txOpts: TxOpts) => number; + v: number, r: string, s: string, txOpts?: TxOpts) => number; }; filled: { call: (orderHash: string) => BigNumber.BigNumber; -- cgit v1.2.3