From 857a35d4f71c1c954033c3b0505250e18be21cfb Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 9 Nov 2018 00:45:48 +0100 Subject: Fix validateOrderFillableOrThrowAsync method so it also checks order signature, cancelled, cancelledUpTo, and throws helpful error messages --- .../src/contract_wrappers/exchange_wrapper.ts | 17 +++++++++++ .../src/fetchers/order_filled_cancelled_fetcher.ts | 16 ++++++++-- .../schemas/validate_order_fillable_opts_schema.ts | 7 +++++ .../test/exchange_wrapper_test.ts | 12 ++++++++ packages/contracts/test/utils/exchange_wrapper.ts | 4 +++ .../utils/simple_order_filled_cancelled_fetcher.ts | 13 ++++++-- .../abstract_order_filled_cancelled_fetcher.ts | 3 +- .../abstract_order_filled_cancelled_lazy_store.ts | 3 +- packages/order-utils/src/order_state_utils.ts | 4 +-- packages/order-utils/src/order_validation_utils.ts | 35 +++++++++++----------- .../src/store/order_filled_cancelled_lazy_store.ts | 7 +++-- .../order-utils/test/order_state_utils_test.ts | 3 +- 12 files changed, 94 insertions(+), 30 deletions(-) create mode 100644 packages/contract-wrappers/src/schemas/validate_order_fillable_opts_schema.ts diff --git a/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts b/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts index 2e978f35b..902fc755c 100644 --- a/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts +++ b/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts @@ -5,7 +5,9 @@ import { assetDataUtils, BalanceAndProxyAllowanceLazyStore, ExchangeTransferSimulator, + orderHashUtils, OrderValidationUtils, + signatureUtils, } from '@0x/order-utils'; import { AssetProxyId, Order, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; @@ -18,6 +20,7 @@ import { OrderFilledCancelledFetcher } from '../fetchers/order_filled_cancelled_ import { methodOptsSchema } from '../schemas/method_opts_schema'; import { orderTxOptsSchema } from '../schemas/order_tx_opts_schema'; import { txOptsSchema } from '../schemas/tx_opts_schema'; +import { validateOrderFillableOptsSchema } from '../schemas/validate_order_fillable_opts_schema'; import { BlockRange, EventCallback, @@ -1114,6 +1117,20 @@ export class ExchangeWrapper extends ContractWrapper { signedOrder: SignedOrder, opts: ValidateOrderFillableOpts = {}, ): Promise { + assert.doesConformToSchema('signedOrder', signedOrder, schemas.signedOrderSchema); + assert.doesConformToSchema('opts', opts, validateOrderFillableOptsSchema); + + const orderHash = await orderHashUtils.getOrderHashHex(signedOrder); + const isValidSignature = await signatureUtils.isValidSignatureAsync( + this._web3Wrapper.getProvider(), + orderHash, + signedOrder.signature, + signedOrder.makerAddress, + ); + if (!isValidSignature) { + throw new Error('INVALID_ORDER_SIGNATURE'); + } + const balanceAllowanceFetcher = new AssetBalanceAndProxyAllowanceFetcher( this._erc20TokenWrapper, this._erc721TokenWrapper, diff --git a/packages/contract-wrappers/src/fetchers/order_filled_cancelled_fetcher.ts b/packages/contract-wrappers/src/fetchers/order_filled_cancelled_fetcher.ts index acf7038fa..5d350916c 100644 --- a/packages/contract-wrappers/src/fetchers/order_filled_cancelled_fetcher.ts +++ b/packages/contract-wrappers/src/fetchers/order_filled_cancelled_fetcher.ts @@ -1,5 +1,6 @@ // tslint:disable:no-unnecessary-type-assertion -import { AbstractOrderFilledCancelledFetcher } from '@0x/order-utils'; +import { AbstractOrderFilledCancelledFetcher, orderHashUtils } from '@0x/order-utils'; +import { SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { BlockParamLiteral } from 'ethereum-types'; @@ -18,9 +19,18 @@ export class OrderFilledCancelledFetcher implements AbstractOrderFilledCancelled }); return filledTakerAmount; } - public async isOrderCancelledAsync(orderHash: string): Promise { + public async isOrderCancelledAsync(signedOrder: SignedOrder): Promise { + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); const isCancelled = await this._exchange.isCancelledAsync(orderHash); - return isCancelled; + const orderEpoch = await this._exchange.getOrderEpochAsync( + signedOrder.makerAddress, + signedOrder.senderAddress, + { + defaultBlock: this._stateLayer, + }, + ); + const isCancelledByOrderEpoch = orderEpoch > signedOrder.salt; + return isCancelled || isCancelledByOrderEpoch; } public getZRXAssetData(): string { const zrxAssetData = this._exchange.getZRXAssetData(); diff --git a/packages/contract-wrappers/src/schemas/validate_order_fillable_opts_schema.ts b/packages/contract-wrappers/src/schemas/validate_order_fillable_opts_schema.ts new file mode 100644 index 000000000..2e111af04 --- /dev/null +++ b/packages/contract-wrappers/src/schemas/validate_order_fillable_opts_schema.ts @@ -0,0 +1,7 @@ +export const validateOrderFillableOptsSchema = { + id: '/ValidateOrderFillableOpts', + properties: { + expectedFillTakerTokenAmount: { $ref: '/wholeNumberSchema' }, + }, + type: 'object', +}; diff --git a/packages/contract-wrappers/test/exchange_wrapper_test.ts b/packages/contract-wrappers/test/exchange_wrapper_test.ts index 0e537bd83..8058be873 100644 --- a/packages/contract-wrappers/test/exchange_wrapper_test.ts +++ b/packages/contract-wrappers/test/exchange_wrapper_test.ts @@ -282,6 +282,18 @@ describe('ExchangeWrapper', () => { expect(ordersInfo[1].orderHash).to.be.equal(anotherOrderHash); }); }); + describe('#validateOrderFillableOrThrowAsync', () => { + it('should throw if signature is invalid', async () => { + const signedOrderWithInvalidSignature = { + ...signedOrder, + signature: '0xdeadbeef', + }; + + expect( + contractWrappers.exchange.validateOrderFillableOrThrowAsync(signedOrderWithInvalidSignature), + ).to.eventually.to.be.rejected(); + }); + }); describe('#isValidSignature', () => { it('should check if the signature is valid', async () => { const orderHash = orderHashUtils.getOrderHashHex(signedOrder); diff --git a/packages/contracts/test/utils/exchange_wrapper.ts b/packages/contracts/test/utils/exchange_wrapper.ts index 29dba690a..cfff682e1 100644 --- a/packages/contracts/test/utils/exchange_wrapper.ts +++ b/packages/contracts/test/utils/exchange_wrapper.ts @@ -219,6 +219,10 @@ export class ExchangeWrapper { const isCancelled = await this._exchange.cancelled.callAsync(orderHashHex); return isCancelled; } + public async getOrderEpochAsync(makerAddress: string, takerAddress: string): Promise { + const orderEpoch = new BigNumber(await this._exchange.orderEpoch.callAsync(makerAddress, takerAddress)); + return orderEpoch; + } public async getOrderInfoAsync(signedOrder: SignedOrder): Promise { const orderInfo = (await this._exchange.getOrderInfo.callAsync(signedOrder)) as OrderInfo; return orderInfo; diff --git a/packages/contracts/test/utils/simple_order_filled_cancelled_fetcher.ts b/packages/contracts/test/utils/simple_order_filled_cancelled_fetcher.ts index 5f5575c7b..af959e00e 100644 --- a/packages/contracts/test/utils/simple_order_filled_cancelled_fetcher.ts +++ b/packages/contracts/test/utils/simple_order_filled_cancelled_fetcher.ts @@ -1,4 +1,5 @@ -import { AbstractOrderFilledCancelledFetcher } from '@0x/order-utils'; +import { AbstractOrderFilledCancelledFetcher, orderHashUtils } from '@0x/order-utils'; +import { SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { ExchangeWrapper } from './exchange_wrapper'; @@ -14,9 +15,15 @@ export class SimpleOrderFilledCancelledFetcher implements AbstractOrderFilledCan const filledTakerAmount = new BigNumber(await this._exchangeWrapper.getTakerAssetFilledAmountAsync(orderHash)); return filledTakerAmount; } - public async isOrderCancelledAsync(orderHash: string): Promise { + public async isOrderCancelledAsync(signedOrder: SignedOrder): Promise { + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); const isCancelled = await this._exchangeWrapper.isCancelledAsync(orderHash); - return isCancelled; + const orderEpoch = await this._exchangeWrapper.getOrderEpochAsync( + signedOrder.makerAddress, + signedOrder.senderAddress, + ); + const isCancelledByOrderEpoch = orderEpoch > signedOrder.salt; + return isCancelled || isCancelledByOrderEpoch; } public getZRXAssetData(): string { return this._zrxAssetData; diff --git a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts index de096b7d9..9e240f9ef 100644 --- a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts +++ b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_fetcher.ts @@ -1,3 +1,4 @@ +import { SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; /** @@ -17,6 +18,6 @@ export abstract class AbstractOrderFilledCancelledFetcher { * @param orderHash OrderHash of order we are interested in * @return Whether or not the order is cancelled */ - public abstract async isOrderCancelledAsync(orderHash: string): Promise; + public abstract async isOrderCancelledAsync(signedOrder: SignedOrder): Promise; public abstract getZRXAssetData(): string; } diff --git a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts index d9e66db06..186521401 100644 --- a/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts +++ b/packages/order-utils/src/abstract/abstract_order_filled_cancelled_lazy_store.ts @@ -1,8 +1,9 @@ +import { SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; export abstract class AbstractOrderFilledCancelledLazyStore { public abstract async getFilledTakerAmountAsync(orderHash: string): Promise; - public abstract async getIsCancelledAsync(orderHash: string): Promise; + public abstract async getIsCancelledAsync(signedOrder: SignedOrder): Promise; public abstract setFilledTakerAmount(orderHash: string, balance: BigNumber): void; public abstract deleteFilledTakerAmount(orderHash: string): void; public abstract setIsCancelled(orderHash: string, isCancelled: boolean): void; diff --git a/packages/order-utils/src/order_state_utils.ts b/packages/order-utils/src/order_state_utils.ts index 159aeeb09..fe0d6c773 100644 --- a/packages/order-utils/src/order_state_utils.ts +++ b/packages/order-utils/src/order_state_utils.ts @@ -117,7 +117,7 @@ export class OrderStateUtils { public async getOpenOrderStateAsync(signedOrder: SignedOrder, transactionHash?: string): Promise { const orderRelevantState = await this.getOpenOrderRelevantStateAsync(signedOrder); const orderHash = orderHashUtils.getOrderHashHex(signedOrder); - const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash); + const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder); const sidedOrderRelevantState = { isMakerSide: true, traderBalance: orderRelevantState.makerBalance, @@ -256,7 +256,7 @@ export class OrderStateUtils { const filledTakerAssetAmount = await this._orderFilledCancelledFetcher.getFilledTakerAmountAsync(orderHash); const totalMakerAssetAmount = signedOrder.makerAssetAmount; const totalTakerAssetAmount = signedOrder.takerAssetAmount; - const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash); + const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder); const remainingTakerAssetAmount = isOrderCancelled ? new BigNumber(0) : totalTakerAssetAmount.minus(filledTakerAssetAmount); diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts index a40069f63..766fbaf0f 100644 --- a/packages/order-utils/src/order_validation_utils.ts +++ b/packages/order-utils/src/order_validation_utils.ts @@ -109,14 +109,6 @@ export class OrderValidationUtils { throw new Error(RevertReason.TransferFailed); } } - private static _validateRemainingFillAmountNotZeroOrThrow( - takerAssetAmount: BigNumber, - filledTakerTokenAmount: BigNumber, - ): void { - if (takerAssetAmount.eq(filledTakerTokenAmount)) { - throw new Error(RevertReason.OrderUnfillable); - } - } private static _validateOrderNotExpiredOrThrow(expirationTimeSeconds: BigNumber): void { const currentUnixTimestampSec = utils.getCurrentUnixTimestampSec(); if (expirationTimeSeconds.lessThan(currentUnixTimestampSec)) { @@ -125,12 +117,15 @@ export class OrderValidationUtils { } /** * Instantiate OrderValidationUtils - * @param orderFilledCancelledFetcher A module that implements the AbstractOrderFilledCancelledFetcher + * @param orderFilledCancelledFetcher A module that implements the AbstractOrderInfoFetcher * @return An instance of OrderValidationUtils */ constructor(orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher) { this._orderFilledCancelledFetcher = orderFilledCancelledFetcher; } + // TODO(fabio): remove this method once the smart contracts have been refactored + // to return helpful revert reasons instead of ORDER_UNFILLABLE. Instruct devs + // to make "calls" to validate order fillability + getOrderInfo for fillable amount. /** * Validate if the supplied order is fillable, and throw if it isn't * @param exchangeTradeEmulator ExchangeTradeEmulator instance @@ -146,12 +141,19 @@ export class OrderValidationUtils { expectedFillTakerTokenAmount?: BigNumber, ): Promise { const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder); + if (isCancelled) { + throw new Error('CANCELLED'); + } const filledTakerTokenAmount = await this._orderFilledCancelledFetcher.getFilledTakerAmountAsync(orderHash); - OrderValidationUtils._validateRemainingFillAmountNotZeroOrThrow( - signedOrder.takerAssetAmount, - filledTakerTokenAmount, - ); + if (signedOrder.takerAssetAmount.eq(filledTakerTokenAmount)) { + throw new Error('FULLY_FILLED'); + } + try { OrderValidationUtils._validateOrderNotExpiredOrThrow(signedOrder.expirationTimeSeconds); + } catch (err) { + throw new Error('EXPIRED'); + } let fillTakerAssetAmount = signedOrder.takerAssetAmount.minus(filledTakerTokenAmount); if (!_.isUndefined(expectedFillTakerTokenAmount)) { fillTakerAssetAmount = expectedFillTakerTokenAmount; @@ -198,10 +200,9 @@ export class OrderValidationUtils { throw new Error(OrderError.InvalidSignature); } const filledTakerTokenAmount = await this._orderFilledCancelledFetcher.getFilledTakerAmountAsync(orderHash); - OrderValidationUtils._validateRemainingFillAmountNotZeroOrThrow( - signedOrder.takerAssetAmount, - filledTakerTokenAmount, - ); + if (signedOrder.takerAssetAmount.eq(filledTakerTokenAmount)) { + throw new Error(RevertReason.OrderUnfillable); + } if (signedOrder.takerAddress !== constants.NULL_ADDRESS && signedOrder.takerAddress !== takerAddress) { throw new Error(RevertReason.InvalidTaker); } diff --git a/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts b/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts index 1d84ffdaa..afd6f1108 100644 --- a/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts +++ b/packages/order-utils/src/store/order_filled_cancelled_lazy_store.ts @@ -1,8 +1,10 @@ +import { SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import * as _ from 'lodash'; import { AbstractOrderFilledCancelledFetcher } from '../abstract/abstract_order_filled_cancelled_fetcher'; import { AbstractOrderFilledCancelledLazyStore } from '../abstract/abstract_order_filled_cancelled_lazy_store'; +import { orderHashUtils } from '../order_hash'; /** * Copy on read store for balances/proxyAllowances of tokens/accounts @@ -58,9 +60,10 @@ export class OrderFilledCancelledLazyStore implements AbstractOrderFilledCancell * @param orderHash OrderHash from order of interest * @return Whether the order has been cancelled */ - public async getIsCancelledAsync(orderHash: string): Promise { + public async getIsCancelledAsync(signedOrder: SignedOrder): Promise { + const orderHash = orderHashUtils.getOrderHashHex(signedOrder); if (_.isUndefined(this._isCancelled[orderHash])) { - const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash); + const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder); this.setIsCancelled(orderHash, isCancelled); } const cachedIsCancelled = this._isCancelled[orderHash]; // tslint:disable-line:boolean-naming diff --git a/packages/order-utils/test/order_state_utils_test.ts b/packages/order-utils/test/order_state_utils_test.ts index 39c4c362f..42acd54c6 100644 --- a/packages/order-utils/test/order_state_utils_test.ts +++ b/packages/order-utils/test/order_state_utils_test.ts @@ -1,3 +1,4 @@ +import { SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import * as chai from 'chai'; import 'mocha'; @@ -33,7 +34,7 @@ describe('OrderStateUtils', () => { async getFilledTakerAmountAsync(_orderHash: string): Promise { return filledAmount; }, - async isOrderCancelledAsync(_orderHash: string): Promise { + async isOrderCancelledAsync(_signedOrder: SignedOrder): Promise { return cancelled; }, getZRXAssetData(): string { -- cgit v1.2.3 From 57318a6ef22876a8c73321b7cc281302bbd67a3b Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 9 Nov 2018 00:46:07 +0100 Subject: Remove unused validateFillOrKill method --- packages/order-utils/src/order_validation_utils.ts | 31 +--------------------- 1 file changed, 1 insertion(+), 30 deletions(-) diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts index 766fbaf0f..0af82f5cc 100644 --- a/packages/order-utils/src/order_validation_utils.ts +++ b/packages/order-utils/src/order_validation_utils.ts @@ -150,7 +150,7 @@ export class OrderValidationUtils { throw new Error('FULLY_FILLED'); } try { - OrderValidationUtils._validateOrderNotExpiredOrThrow(signedOrder.expirationTimeSeconds); + OrderValidationUtils._validateOrderNotExpiredOrThrow(signedOrder.expirationTimeSeconds); } catch (err) { throw new Error('EXPIRED'); } @@ -229,33 +229,4 @@ export class OrderValidationUtils { } return filledTakerTokenAmount; } - /** - * Validate a call to fillOrKillOrder and throw if it would fail - * @param exchangeTradeEmulator ExchangeTradeEmulator to use - * @param provider Web3 provider to use for JSON RPC requests - * @param signedOrder SignedOrder of interest - * @param fillTakerAssetAmount Amount we'd like to fill the order for - * @param takerAddress The taker of the order - * @param zrxAssetData ZRX asset data - */ - public async validateFillOrKillOrderThrowIfInvalidAsync( - exchangeTradeEmulator: ExchangeTransferSimulator, - provider: Provider, - signedOrder: SignedOrder, - fillTakerAssetAmount: BigNumber, - takerAddress: string, - zrxAssetData: string, - ): Promise { - const filledTakerTokenAmount = await this.validateFillOrderThrowIfInvalidAsync( - exchangeTradeEmulator, - provider, - signedOrder, - fillTakerAssetAmount, - takerAddress, - zrxAssetData, - ); - if (filledTakerTokenAmount !== fillTakerAssetAmount) { - throw new Error(RevertReason.OrderUnfillable); - } - } } -- cgit v1.2.3 From 3980bf39a9ae8cb5b5cc3cbc33b6e84d1bcf6c28 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 9 Nov 2018 18:50:25 +0100 Subject: Keep more helpful error messages, and stop swallowing errors when returning contract error --- packages/order-utils/src/order_validation_utils.ts | 131 +++++++++++---------- 1 file changed, 72 insertions(+), 59 deletions(-) diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts index 0af82f5cc..345e9099e 100644 --- a/packages/order-utils/src/order_validation_utils.ts +++ b/packages/order-utils/src/order_validation_utils.ts @@ -1,4 +1,4 @@ -import { RevertReason, SignedOrder } from '@0x/types'; +import { ExchangeContractErrs, RevertReason, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import { Provider } from 'ethereum-types'; import * as _ from 'lodash'; @@ -57,57 +57,53 @@ export class OrderValidationUtils { senderAddress: string, zrxAssetData: string, ): Promise { - try { - const fillMakerTokenAmount = utils.getPartialAmountFloor( - fillTakerAssetAmount, - signedOrder.takerAssetAmount, - signedOrder.makerAssetAmount, - ); - await exchangeTradeEmulator.transferFromAsync( - signedOrder.makerAssetData, - signedOrder.makerAddress, - senderAddress, - fillMakerTokenAmount, - TradeSide.Maker, - TransferType.Trade, - ); - await exchangeTradeEmulator.transferFromAsync( - signedOrder.takerAssetData, - senderAddress, - signedOrder.makerAddress, - fillTakerAssetAmount, - TradeSide.Taker, - TransferType.Trade, - ); - const makerFeeAmount = utils.getPartialAmountFloor( - fillTakerAssetAmount, - signedOrder.takerAssetAmount, - signedOrder.makerFee, - ); - await exchangeTradeEmulator.transferFromAsync( - zrxAssetData, - signedOrder.makerAddress, - signedOrder.feeRecipientAddress, - makerFeeAmount, - TradeSide.Maker, - TransferType.Fee, - ); - const takerFeeAmount = utils.getPartialAmountFloor( - fillTakerAssetAmount, - signedOrder.takerAssetAmount, - signedOrder.takerFee, - ); - await exchangeTradeEmulator.transferFromAsync( - zrxAssetData, - senderAddress, - signedOrder.feeRecipientAddress, - takerFeeAmount, - TradeSide.Taker, - TransferType.Fee, - ); - } catch (err) { - throw new Error(RevertReason.TransferFailed); - } + const fillMakerTokenAmount = utils.getPartialAmountFloor( + fillTakerAssetAmount, + signedOrder.takerAssetAmount, + signedOrder.makerAssetAmount, + ); + await exchangeTradeEmulator.transferFromAsync( + signedOrder.makerAssetData, + signedOrder.makerAddress, + senderAddress, + fillMakerTokenAmount, + TradeSide.Maker, + TransferType.Trade, + ); + await exchangeTradeEmulator.transferFromAsync( + signedOrder.takerAssetData, + senderAddress, + signedOrder.makerAddress, + fillTakerAssetAmount, + TradeSide.Taker, + TransferType.Trade, + ); + const makerFeeAmount = utils.getPartialAmountFloor( + fillTakerAssetAmount, + signedOrder.takerAssetAmount, + signedOrder.makerFee, + ); + await exchangeTradeEmulator.transferFromAsync( + zrxAssetData, + signedOrder.makerAddress, + signedOrder.feeRecipientAddress, + makerFeeAmount, + TradeSide.Maker, + TransferType.Fee, + ); + const takerFeeAmount = utils.getPartialAmountFloor( + fillTakerAssetAmount, + signedOrder.takerAssetAmount, + signedOrder.takerFee, + ); + await exchangeTradeEmulator.transferFromAsync( + zrxAssetData, + senderAddress, + signedOrder.feeRecipientAddress, + takerFeeAmount, + TradeSide.Taker, + TransferType.Fee, + ); } private static _validateOrderNotExpiredOrThrow(expirationTimeSeconds: BigNumber): void { const currentUnixTimestampSec = utils.getCurrentUnixTimestampSec(); @@ -211,13 +207,30 @@ export class OrderValidationUtils { const desiredFillTakerTokenAmount = remainingTakerTokenAmount.lessThan(fillTakerAssetAmount) ? remainingTakerTokenAmount : fillTakerAssetAmount; - await OrderValidationUtils.validateFillOrderBalancesAllowancesThrowIfInvalidAsync( - exchangeTradeEmulator, - signedOrder, - desiredFillTakerTokenAmount, - takerAddress, - zrxAssetData, - ); + try { + await OrderValidationUtils.validateFillOrderBalancesAllowancesThrowIfInvalidAsync( + exchangeTradeEmulator, + signedOrder, + desiredFillTakerTokenAmount, + takerAddress, + zrxAssetData, + ); + } catch (err) { + const transferFailedErrorMessages = [ + ExchangeContractErrs.InsufficientMakerBalance, + ExchangeContractErrs.InsufficientMakerFeeBalance, + ExchangeContractErrs.InsufficientTakerBalance, + ExchangeContractErrs.InsufficientTakerFeeBalance, + ExchangeContractErrs.InsufficientMakerAllowance, + ExchangeContractErrs.InsufficientMakerFeeAllowance, + ExchangeContractErrs.InsufficientTakerAllowance, + ExchangeContractErrs.InsufficientTakerFeeAllowance, + ]; + if (_.includes(transferFailedErrorMessages, err.message)) { + throw new Error(RevertReason.TransferFailed); + } + throw err; + } const wouldRoundingErrorOccur = OrderValidationUtils.isRoundingErrorFloor( desiredFillTakerTokenAmount, -- cgit v1.2.3 From 53d0f5b98ebd132d51b1fdf463fe57481c895ec4 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 9 Nov 2018 23:15:34 +0100 Subject: Revert comment change --- packages/order-utils/src/order_validation_utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts index 345e9099e..2150e643f 100644 --- a/packages/order-utils/src/order_validation_utils.ts +++ b/packages/order-utils/src/order_validation_utils.ts @@ -113,7 +113,7 @@ export class OrderValidationUtils { } /** * Instantiate OrderValidationUtils - * @param orderFilledCancelledFetcher A module that implements the AbstractOrderInfoFetcher + * @param orderFilledCancelledFetcher A module that implements the AbstractOrderFilledCancelledFetcher * @return An instance of OrderValidationUtils */ constructor(orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher) { -- cgit v1.2.3 From 1f0ac47bd97b88071a6380a728cfdb1457c2590c Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Sat, 10 Nov 2018 00:14:48 +0100 Subject: Move signature validation into OrderValidationUtils.validateOrderFillableOrThrowAsync --- .../src/contract_wrappers/exchange_wrapper.ts | 15 ++------------- .../test/utils/fill_order_combinatorial_utils.ts | 2 +- packages/order-utils/src/order_validation_utils.ts | 14 +++++++++++++- 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts b/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts index 902fc755c..d3a4e9098 100644 --- a/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts +++ b/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts @@ -1120,17 +1120,6 @@ export class ExchangeWrapper extends ContractWrapper { assert.doesConformToSchema('signedOrder', signedOrder, schemas.signedOrderSchema); assert.doesConformToSchema('opts', opts, validateOrderFillableOptsSchema); - const orderHash = await orderHashUtils.getOrderHashHex(signedOrder); - const isValidSignature = await signatureUtils.isValidSignatureAsync( - this._web3Wrapper.getProvider(), - orderHash, - signedOrder.signature, - signedOrder.makerAddress, - ); - if (!isValidSignature) { - throw new Error('INVALID_ORDER_SIGNATURE'); - } - const balanceAllowanceFetcher = new AssetBalanceAndProxyAllowanceFetcher( this._erc20TokenWrapper, this._erc721TokenWrapper, @@ -1141,7 +1130,7 @@ export class ExchangeWrapper extends ContractWrapper { const expectedFillTakerTokenAmountIfExists = opts.expectedFillTakerTokenAmount; const filledCancelledFetcher = new OrderFilledCancelledFetcher(this, BlockParamLiteral.Latest); - const orderValidationUtils = new OrderValidationUtils(filledCancelledFetcher); + const orderValidationUtils = new OrderValidationUtils(filledCancelledFetcher, this._web3Wrapper.getProvider()); await orderValidationUtils.validateOrderFillableOrThrowAsync( exchangeTradeSimulator, signedOrder, @@ -1169,7 +1158,7 @@ export class ExchangeWrapper extends ContractWrapper { const exchangeTradeSimulator = new ExchangeTransferSimulator(balanceAllowanceStore); const filledCancelledFetcher = new OrderFilledCancelledFetcher(this, BlockParamLiteral.Latest); - const orderValidationUtils = new OrderValidationUtils(filledCancelledFetcher); + const orderValidationUtils = new OrderValidationUtils(filledCancelledFetcher, this._web3Wrapper.getProvider()); await orderValidationUtils.validateFillOrderThrowIfInvalidAsync( exchangeTradeSimulator, this._web3Wrapper.getProvider(), diff --git a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts index 81bb33318..8046771f9 100644 --- a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts +++ b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts @@ -392,7 +392,7 @@ export class FillOrderCombinatorialUtils { ); // 5. If I fill it by X, what are the resulting balances/allowances/filled amounts expected? - const orderValidationUtils = new OrderValidationUtils(orderFilledCancelledFetcher); + const orderValidationUtils = new OrderValidationUtils(orderFilledCancelledFetcher, provider); const lazyStore = new BalanceAndProxyAllowanceLazyStore(balanceAndProxyAllowanceFetcher); const exchangeTransferSimulator = new ExchangeTransferSimulator(lazyStore); diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts index 2150e643f..ca4c32543 100644 --- a/packages/order-utils/src/order_validation_utils.ts +++ b/packages/order-utils/src/order_validation_utils.ts @@ -17,6 +17,7 @@ import { utils } from './utils'; */ export class OrderValidationUtils { private readonly _orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher; + private readonly _provider: Provider; /** * A Typescript implementation mirroring the implementation of isRoundingError in the * Exchange smart contract @@ -116,8 +117,9 @@ export class OrderValidationUtils { * @param orderFilledCancelledFetcher A module that implements the AbstractOrderFilledCancelledFetcher * @return An instance of OrderValidationUtils */ - constructor(orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher) { + constructor(orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher, provider: Provider) { this._orderFilledCancelledFetcher = orderFilledCancelledFetcher; + this._provider = provider; } // TODO(fabio): remove this method once the smart contracts have been refactored // to return helpful revert reasons instead of ORDER_UNFILLABLE. Instruct devs @@ -137,6 +139,16 @@ export class OrderValidationUtils { expectedFillTakerTokenAmount?: BigNumber, ): Promise { const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const isValidSignature = await signatureUtils.isValidSignatureAsync( + this._provider, + orderHash, + signedOrder.signature, + signedOrder.makerAddress, + ); + if (!isValidSignature) { + throw new Error('INVALID_ORDER_SIGNATURE'); + } + const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder); if (isCancelled) { throw new Error('CANCELLED'); -- cgit v1.2.3 From d3592d362e243fb5c320125ed18df34858b6db72 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Sun, 11 Nov 2018 13:02:45 +0100 Subject: address linter errors --- packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts b/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts index d3a4e9098..c76e51eee 100644 --- a/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts +++ b/packages/contract-wrappers/src/contract_wrappers/exchange_wrapper.ts @@ -5,9 +5,7 @@ import { assetDataUtils, BalanceAndProxyAllowanceLazyStore, ExchangeTransferSimulator, - orderHashUtils, OrderValidationUtils, - signatureUtils, } from '@0x/order-utils'; import { AssetProxyId, Order, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; -- cgit v1.2.3 From 8aeb18bcc3d10dbc7610d597d98a4ad26131cf78 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Mon, 12 Nov 2018 10:22:03 +0100 Subject: rename param --- packages/contracts/test/utils/exchange_wrapper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contracts/test/utils/exchange_wrapper.ts b/packages/contracts/test/utils/exchange_wrapper.ts index cfff682e1..52322d5c4 100644 --- a/packages/contracts/test/utils/exchange_wrapper.ts +++ b/packages/contracts/test/utils/exchange_wrapper.ts @@ -219,8 +219,8 @@ export class ExchangeWrapper { const isCancelled = await this._exchange.cancelled.callAsync(orderHashHex); return isCancelled; } - public async getOrderEpochAsync(makerAddress: string, takerAddress: string): Promise { - const orderEpoch = new BigNumber(await this._exchange.orderEpoch.callAsync(makerAddress, takerAddress)); + public async getOrderEpochAsync(makerAddress: string, senderAddress: string): Promise { + const orderEpoch = new BigNumber(await this._exchange.orderEpoch.callAsync(makerAddress, senderAddress)); return orderEpoch; } public async getOrderInfoAsync(signedOrder: SignedOrder): Promise { -- cgit v1.2.3 From fd7ba3ecea72487ecbec428c31b5547f8691ca23 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Mon, 12 Nov 2018 10:22:22 +0100 Subject: Use RevertReason when possible --- packages/order-utils/src/order_validation_utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/order-utils/src/order_validation_utils.ts b/packages/order-utils/src/order_validation_utils.ts index ca4c32543..ae4291ea8 100644 --- a/packages/order-utils/src/order_validation_utils.ts +++ b/packages/order-utils/src/order_validation_utils.ts @@ -146,7 +146,7 @@ export class OrderValidationUtils { signedOrder.makerAddress, ); if (!isValidSignature) { - throw new Error('INVALID_ORDER_SIGNATURE'); + throw new Error(RevertReason.InvalidOrderSignature); } const isCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(signedOrder); -- cgit v1.2.3 From b21c1bea46a9fdd43680daf8b241c54d0483770b Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Mon, 12 Nov 2018 10:23:42 +0100 Subject: Use rejectedWith --- packages/contract-wrappers/test/exchange_wrapper_test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contract-wrappers/test/exchange_wrapper_test.ts b/packages/contract-wrappers/test/exchange_wrapper_test.ts index 8058be873..1bf3005cc 100644 --- a/packages/contract-wrappers/test/exchange_wrapper_test.ts +++ b/packages/contract-wrappers/test/exchange_wrapper_test.ts @@ -1,7 +1,7 @@ import { BlockchainLifecycle, callbackErrorReporter } from '@0x/dev-utils'; import { FillScenarios } from '@0x/fill-scenarios'; import { assetDataUtils, orderHashUtils } from '@0x/order-utils'; -import { DoneCallback, SignedOrder } from '@0x/types'; +import { DoneCallback, RevertReason, SignedOrder } from '@0x/types'; import { BigNumber } from '@0x/utils'; import * as chai from 'chai'; import { BlockParamLiteral } from 'ethereum-types'; @@ -291,7 +291,7 @@ describe('ExchangeWrapper', () => { expect( contractWrappers.exchange.validateOrderFillableOrThrowAsync(signedOrderWithInvalidSignature), - ).to.eventually.to.be.rejected(); + ).to.eventually.to.be.rejectedWith(RevertReason.InvalidOrderSignature); }); }); describe('#isValidSignature', () => { -- cgit v1.2.3 From 8efc6c211246ec59a70d25754ccf1986944a5976 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Mon, 12 Nov 2018 10:25:33 +0100 Subject: Remove unnecessary conversion to BigNumber --- packages/contracts/test/utils/exchange_wrapper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contracts/test/utils/exchange_wrapper.ts b/packages/contracts/test/utils/exchange_wrapper.ts index 52322d5c4..c28989d3f 100644 --- a/packages/contracts/test/utils/exchange_wrapper.ts +++ b/packages/contracts/test/utils/exchange_wrapper.ts @@ -212,7 +212,7 @@ export class ExchangeWrapper { return tx; } public async getTakerAssetFilledAmountAsync(orderHashHex: string): Promise { - const filledAmount = new BigNumber(await this._exchange.filled.callAsync(orderHashHex)); + const filledAmount = await this._exchange.filled.callAsync(orderHashHex); return filledAmount; } public async isCancelledAsync(orderHashHex: string): Promise { @@ -220,7 +220,7 @@ export class ExchangeWrapper { return isCancelled; } public async getOrderEpochAsync(makerAddress: string, senderAddress: string): Promise { - const orderEpoch = new BigNumber(await this._exchange.orderEpoch.callAsync(makerAddress, senderAddress)); + const orderEpoch = await this._exchange.orderEpoch.callAsync(makerAddress, senderAddress); return orderEpoch; } public async getOrderInfoAsync(signedOrder: SignedOrder): Promise { -- cgit v1.2.3 From 6fa6579c312295974bff9f6f2f78a4443dcb145e Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Mon, 12 Nov 2018 11:57:54 +0100 Subject: Use correctly formatted signature so that it rejects with the expected reason and not because the signature is mal-formatted --- packages/contract-wrappers/test/exchange_wrapper_test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/contract-wrappers/test/exchange_wrapper_test.ts b/packages/contract-wrappers/test/exchange_wrapper_test.ts index 1bf3005cc..73ce6c743 100644 --- a/packages/contract-wrappers/test/exchange_wrapper_test.ts +++ b/packages/contract-wrappers/test/exchange_wrapper_test.ts @@ -286,7 +286,8 @@ describe('ExchangeWrapper', () => { it('should throw if signature is invalid', async () => { const signedOrderWithInvalidSignature = { ...signedOrder, - signature: '0xdeadbeef', + signature: + '0x1b61a3ed31b43c8780e905a260a35faefcc527be7516aa11c0256729b5b351bc3340349190569279751135161d22529dc25add4f6069af05be04cacbda2ace225403', }; expect( -- cgit v1.2.3 From 348556a54442fc93d76536735b8e8e4d76ed5ce6 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Mon, 12 Nov 2018 12:07:19 +0100 Subject: Add fixes affecting the public interface to the CHANGELOGs --- packages/contract-wrappers/CHANGELOG.json | 20 ++++++++++++++++++++ packages/order-utils/CHANGELOG.json | 25 +++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/packages/contract-wrappers/CHANGELOG.json b/packages/contract-wrappers/CHANGELOG.json index 0346b79b2..0ab5e5d08 100644 --- a/packages/contract-wrappers/CHANGELOG.json +++ b/packages/contract-wrappers/CHANGELOG.json @@ -1,4 +1,24 @@ [ + { + "version": "4.0.0", + "changes": [ + { + "note": + "Add signature validation, regular cancellation and `cancelledUpTo` checks to `validateOrderFillableOrThrowAsync`", + "pr": 1235 + }, + { + "note": + "Improved the errors thrown by `validateOrderFillableOrThrowAsync` by making them more descriptive", + "pr": 1235 + }, + { + "note": + "Throw previously swallowed network errors when calling `validateOrderFillableOrThrowAsync` (see issue: #1218)", + "pr": 1235 + } + ] + }, { "version": "3.0.1", "changes": [ diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index abcf3d392..0ed6117df 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -1,4 +1,29 @@ [ + { + "version": "3.0.0", + "changes": [ + { + "note": + "Add signature validation, regular cancellation and `cancelledUpTo` checks to `validateOrderFillableOrThrowAsync`", + "pr": 1235 + }, + { + "note": + "Improved the errors thrown by `validateOrderFillableOrThrowAsync` by making them more descriptive", + "pr": 1235 + }, + { + "note": + "Throw previously swallowed network errors when calling `validateOrderFillableOrThrowAsync` (see issue: #1218)", + "pr": 1235 + }, + { + "note": + "Modified the `AbstractOrderFilledCancelledFetcher` interface slightly such that `isOrderCancelledAsync` accepts a `signedOrder` instead of an `orderHash` param", + "pr": 1235 + } + ] + }, { "version": "2.0.1", "changes": [ -- cgit v1.2.3