From 622509c508272790e3e69c09cf1a1f9696815147 Mon Sep 17 00:00:00 2001 From: Jacob Evans Date: Mon, 13 Aug 2018 12:23:29 +1000 Subject: [Order-utils] Order is valid when maker amount is very small Previously our min fillable calculation would throw a rounding error when encountering a valid order (with a small maker amount). This was inconsistent with the on-chain logic which allowed this order to be filled. --- packages/contracts/test/exchange/fill_order.ts | 11 ++ .../test/utils/fill_order_combinatorial_utils.ts | 7 ++ .../test/utils/order_factory_from_scenario.ts | 23 ++++ packages/contracts/test/utils/types.ts | 3 +- packages/order-utils/CHANGELOG.json | 4 + packages/order-utils/src/order_state_utils.ts | 36 +++--- .../order-utils/test/order_state_utils_test.ts | 122 +++++++++++++++++++++ packages/order-watcher/test/order_watcher_test.ts | 20 ++-- 8 files changed, 195 insertions(+), 31 deletions(-) create mode 100644 packages/order-utils/test/order_state_utils_test.ts diff --git a/packages/contracts/test/exchange/fill_order.ts b/packages/contracts/test/exchange/fill_order.ts index 1494fe093..e79e2239e 100644 --- a/packages/contracts/test/exchange/fill_order.ts +++ b/packages/contracts/test/exchange/fill_order.ts @@ -104,6 +104,17 @@ describe('FillOrder Tests', () => { }; await fillOrderCombinatorialUtils.testFillOrderScenarioAsync(provider, fillScenario); }); + it('should transfer the correct amounts when makerAssetAmount < takerAssetAmount with zero decimals', async () => { + const fillScenario = { + ...defaultFillScenario, + orderScenario: { + ...defaultFillScenario.orderScenario, + makerAssetAmountScenario: OrderAssetAmountScenario.Small, + makerAssetDataScenario: AssetDataScenario.ERC20ZeroDecimals, + }, + }; + await fillOrderCombinatorialUtils.testFillOrderScenarioAsync(provider, fillScenario); + }); it('should transfer the correct amounts when taker is specified and order is claimed by taker', async () => { const fillScenario = { ...defaultFillScenario, diff --git a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts index 284c4a2db..f18ad0dd3 100644 --- a/packages/contracts/test/utils/fill_order_combinatorial_utils.ts +++ b/packages/contracts/test/utils/fill_order_combinatorial_utils.ts @@ -81,6 +81,12 @@ export async function fillOrderCombinatorialUtilsFactoryAsync( erc20FiveDecimalTokenCount, fiveDecimals, ); + const zeroDecimals = new BigNumber(0); + const erc20ZeroDecimalTokenCount = 2; + const [erc20ZeroDecimalTokenA, erc20ZeroDecimalTokenB] = await erc20Wrapper.deployDummyTokensAsync( + erc20ZeroDecimalTokenCount, + zeroDecimals, + ); const erc20Proxy = await erc20Wrapper.deployProxyAsync(); await erc20Wrapper.setBalancesAndAllowancesAsync(); @@ -119,6 +125,7 @@ export async function fillOrderCombinatorialUtilsFactoryAsync( zrxToken.address, [erc20EighteenDecimalTokenA.address, erc20EighteenDecimalTokenB.address], [erc20FiveDecimalTokenA.address, erc20FiveDecimalTokenB.address], + [erc20ZeroDecimalTokenA.address, erc20ZeroDecimalTokenB.address], erc721Token, erc721Balances, exchangeContract.address, diff --git a/packages/contracts/test/utils/order_factory_from_scenario.ts b/packages/contracts/test/utils/order_factory_from_scenario.ts index a908140b9..8e04db588 100644 --- a/packages/contracts/test/utils/order_factory_from_scenario.ts +++ b/packages/contracts/test/utils/order_factory_from_scenario.ts @@ -21,6 +21,8 @@ const POINT_ONE_UNITS_EIGHTEEN_DECIMALS = new BigNumber(100_000_000_000_000_000) const POINT_ZERO_FIVE_UNITS_EIGHTEEN_DECIMALS = new BigNumber(50_000_000_000_000_000); const TEN_UNITS_FIVE_DECIMALS = new BigNumber(1_000_000); const FIVE_UNITS_FIVE_DECIMALS = new BigNumber(500_000); +const TEN_UNITS_ZERO_DECIMALS = new BigNumber(10); +const ONE_THOUSAND_UNITS_ZERO_DECIMALS = new BigNumber(1000); const ONE_NFT_UNIT = new BigNumber(1); export class OrderFactoryFromScenario { @@ -28,6 +30,7 @@ export class OrderFactoryFromScenario { private readonly _zrxAddress: string; private readonly _nonZrxERC20EighteenDecimalTokenAddresses: string[]; private readonly _erc20FiveDecimalTokenAddresses: string[]; + private readonly _erc20ZeroDecimalTokenAddresses: string[]; private readonly _erc721Token: DummyERC721TokenContract; private readonly _erc721Balances: ERC721TokenIdsByOwner; private readonly _exchangeAddress: string; @@ -36,6 +39,7 @@ export class OrderFactoryFromScenario { zrxAddress: string, nonZrxERC20EighteenDecimalTokenAddresses: string[], erc20FiveDecimalTokenAddresses: string[], + erc20ZeroDecimalTokenAddresses: string[], erc721Token: DummyERC721TokenContract, erc721Balances: ERC721TokenIdsByOwner, exchangeAddress: string, @@ -44,6 +48,7 @@ export class OrderFactoryFromScenario { this._zrxAddress = zrxAddress; this._nonZrxERC20EighteenDecimalTokenAddresses = nonZrxERC20EighteenDecimalTokenAddresses; this._erc20FiveDecimalTokenAddresses = erc20FiveDecimalTokenAddresses; + this._erc20ZeroDecimalTokenAddresses = erc20ZeroDecimalTokenAddresses; this._erc721Token = erc721Token; this._erc721Balances = erc721Balances; this._exchangeAddress = exchangeAddress; @@ -89,6 +94,9 @@ export class OrderFactoryFromScenario { erc721MakerAssetIds[0], ); break; + case AssetDataScenario.ERC20ZeroDecimals: + makerAssetData = assetDataUtils.encodeERC20AssetData(this._erc20ZeroDecimalTokenAddresses[0]); + break; default: throw errorUtils.spawnSwitchErr('AssetDataScenario', orderScenario.makerAssetDataScenario); } @@ -109,6 +117,9 @@ export class OrderFactoryFromScenario { erc721TakerAssetIds[0], ); break; + case AssetDataScenario.ERC20ZeroDecimals: + takerAssetData = assetDataUtils.encodeERC20AssetData(this._erc20ZeroDecimalTokenAddresses[1]); + break; default: throw errorUtils.spawnSwitchErr('AssetDataScenario', orderScenario.takerAssetDataScenario); } @@ -126,6 +137,9 @@ export class OrderFactoryFromScenario { case AssetDataScenario.ERC721: makerAssetAmount = ONE_NFT_UNIT; break; + case AssetDataScenario.ERC20ZeroDecimals: + makerAssetAmount = ONE_THOUSAND_UNITS_ZERO_DECIMALS; + break; default: throw errorUtils.spawnSwitchErr('AssetDataScenario', orderScenario.makerAssetDataScenario); } @@ -142,6 +156,9 @@ export class OrderFactoryFromScenario { case AssetDataScenario.ERC721: makerAssetAmount = ONE_NFT_UNIT; break; + case AssetDataScenario.ERC20ZeroDecimals: + makerAssetAmount = TEN_UNITS_ZERO_DECIMALS; + break; default: throw errorUtils.spawnSwitchErr('AssetDataScenario', orderScenario.makerAssetDataScenario); } @@ -166,6 +183,9 @@ export class OrderFactoryFromScenario { case AssetDataScenario.ERC721: takerAssetAmount = ONE_NFT_UNIT; break; + case AssetDataScenario.ERC20ZeroDecimals: + takerAssetAmount = ONE_THOUSAND_UNITS_ZERO_DECIMALS; + break; default: throw errorUtils.spawnSwitchErr('AssetDataScenario', orderScenario.takerAssetDataScenario); } @@ -182,6 +202,9 @@ export class OrderFactoryFromScenario { case AssetDataScenario.ERC721: takerAssetAmount = ONE_NFT_UNIT; break; + case AssetDataScenario.ERC20ZeroDecimals: + takerAssetAmount = TEN_UNITS_ZERO_DECIMALS; + break; default: throw errorUtils.spawnSwitchErr('AssetDataScenario', orderScenario.takerAssetDataScenario); } diff --git a/packages/contracts/test/utils/types.ts b/packages/contracts/test/utils/types.ts index 67313b647..481ee87d6 100644 --- a/packages/contracts/test/utils/types.ts +++ b/packages/contracts/test/utils/types.ts @@ -177,10 +177,11 @@ export enum ExpirationTimeSecondsScenario { } export enum AssetDataScenario { - ERC721 = 'ERC721', + ERC20ZeroDecimals = 'ERC20_ZERO_DECIMALS', ZRXFeeToken = 'ZRX_FEE_TOKEN', ERC20FiveDecimals = 'ERC20_FIVE_DECIMALS', ERC20NonZRXEighteenDecimals = 'ERC20_NON_ZRX_EIGHTEEN_DECIMALS', + ERC721 = 'ERC721', } export enum TakerAssetFillAmountScenario { diff --git a/packages/order-utils/CHANGELOG.json b/packages/order-utils/CHANGELOG.json index 6b49d2ee6..86f0da65a 100644 --- a/packages/order-utils/CHANGELOG.json +++ b/packages/order-utils/CHANGELOG.json @@ -2,6 +2,10 @@ { "version": "1.0.1-rc.4", "changes": [ + { + "note": "Remove rounding error being thrown when maker amount is very small", + "pr": 959 + }, { "note": "Added rateUtils and sortingUtils", "pr": 953 diff --git a/packages/order-utils/src/order_state_utils.ts b/packages/order-utils/src/order_state_utils.ts index 189bf4180..7974d5d0b 100644 --- a/packages/order-utils/src/order_state_utils.ts +++ b/packages/order-utils/src/order_state_utils.ts @@ -11,6 +11,7 @@ import { BigNumber } from '@0xproject/utils'; import { AbstractBalanceAndProxyAllowanceFetcher } from './abstract/abstract_balance_and_proxy_allowance_fetcher'; import { AbstractOrderFilledCancelledFetcher } from './abstract/abstract_order_filled_cancelled_fetcher'; import { orderHashUtils } from './order_hash'; +import { OrderValidationUtils } from './order_validation_utils'; import { RemainingFillableCalculator } from './remaining_fillable_calculator'; import { utils } from './utils'; @@ -22,10 +23,9 @@ interface SidedOrderRelevantState { traderFeeProxyAllowance: BigNumber; filledTakerAssetAmount: BigNumber; remainingFillableAssetAmount: BigNumber; + isOrderCancelled: boolean; } -const ACCEPTABLE_RELATIVE_ROUNDING_ERROR = 0.0001; - export class OrderStateUtils { private readonly _balanceAndProxyAllowanceFetcher: AbstractBalanceAndProxyAllowanceFetcher; private readonly _orderFilledCancelledFetcher: AbstractOrderFilledCancelledFetcher; @@ -34,6 +34,9 @@ export class OrderStateUtils { sidedOrderRelevantState: SidedOrderRelevantState, ): void { const isMakerSide = sidedOrderRelevantState.isMakerSide; + if (sidedOrderRelevantState.isOrderCancelled) { + throw new Error(ExchangeContractErrs.OrderAlreadyCancelledOrFilled); + } const availableTakerAssetAmount = signedOrder.takerAssetAmount.minus( sidedOrderRelevantState.filledTakerAssetAmount, ); @@ -71,23 +74,15 @@ export class OrderStateUtils { ); } } - - let minFillableTakerAssetAmountWithinNoRoundingErrorRange; - if (isMakerSide) { - minFillableTakerAssetAmountWithinNoRoundingErrorRange = signedOrder.takerAssetAmount - .dividedBy(ACCEPTABLE_RELATIVE_ROUNDING_ERROR) - .dividedBy(signedOrder.makerAssetAmount); - } else { - minFillableTakerAssetAmountWithinNoRoundingErrorRange = signedOrder.makerAssetAmount - .dividedBy(ACCEPTABLE_RELATIVE_ROUNDING_ERROR) - .dividedBy(signedOrder.takerAssetAmount); - } - - if ( - sidedOrderRelevantState.remainingFillableAssetAmount.lessThan( - minFillableTakerAssetAmountWithinNoRoundingErrorRange, - ) - ) { + const remainingTakerAssetAmount = signedOrder.takerAssetAmount.minus( + sidedOrderRelevantState.filledTakerAssetAmount, + ); + const isRoundingError = OrderValidationUtils.isRoundingError( + remainingTakerAssetAmount, + signedOrder.takerAssetAmount, + signedOrder.makerAssetAmount, + ); + if (isRoundingError) { throw new Error(ExchangeContractErrs.OrderFillRoundingError); } } @@ -101,6 +96,7 @@ export class OrderStateUtils { public async getOpenOrderStateAsync(signedOrder: SignedOrder): Promise { const orderRelevantState = await this.getOpenOrderRelevantStateAsync(signedOrder); const orderHash = orderHashUtils.getOrderHashHex(signedOrder); + const isOrderCancelled = await this._orderFilledCancelledFetcher.isOrderCancelledAsync(orderHash); const sidedOrderRelevantState = { isMakerSide: true, traderBalance: orderRelevantState.makerBalance, @@ -109,6 +105,7 @@ export class OrderStateUtils { traderFeeProxyAllowance: orderRelevantState.makerFeeProxyAllowance, filledTakerAssetAmount: orderRelevantState.filledTakerAssetAmount, remainingFillableAssetAmount: orderRelevantState.remainingFillableMakerAssetAmount, + isOrderCancelled, }; try { OrderStateUtils._validateIfOrderIsValid(signedOrder, sidedOrderRelevantState); @@ -278,6 +275,7 @@ export class OrderStateUtils { traderFeeProxyAllowance, filledTakerAssetAmount, remainingFillableAssetAmount, + isOrderCancelled, }; return sidedOrderRelevantState; } diff --git a/packages/order-utils/test/order_state_utils_test.ts b/packages/order-utils/test/order_state_utils_test.ts new file mode 100644 index 000000000..9292c40a4 --- /dev/null +++ b/packages/order-utils/test/order_state_utils_test.ts @@ -0,0 +1,122 @@ +import { BigNumber } from '@0xproject/utils'; +import * as chai from 'chai'; +import 'mocha'; + +import { AbstractBalanceAndProxyAllowanceFetcher, AbstractOrderFilledCancelledFetcher, OrderStateUtils } from '../src'; + +import { chaiSetup } from './utils/chai_setup'; +import { testOrderFactory } from './utils/test_order_factory'; + +chaiSetup.configure(); +const expect = chai.expect; + +describe('OrderStateUtils', () => { + describe('#getOpenOrderStateAsync', () => { + const buildMockBalanceFetcher = (takerBalance: BigNumber): AbstractBalanceAndProxyAllowanceFetcher => { + const balanceFetcher = { + async getBalanceAsync(_assetData: string, _userAddress: string): Promise { + return takerBalance; + }, + async getProxyAllowanceAsync(_assetData: string, _userAddress: string): Promise { + return takerBalance; + }, + }; + return balanceFetcher; + }; + const buildMockOrderFilledFetcher = ( + filledAmount: BigNumber = new BigNumber(0), + cancelled: boolean = false, + ): AbstractOrderFilledCancelledFetcher => { + const orderFetcher = { + async getFilledTakerAmountAsync(_orderHash: string): Promise { + return filledAmount; + }, + async isOrderCancelledAsync(_orderHash: string): Promise { + return cancelled; + }, + getZRXAssetData(): string { + return ''; + }, + }; + return orderFetcher; + }; + it('should have valid order state if order can be fully filled with small maker amount', async () => { + const makerAssetAmount = new BigNumber(10); + const takerAssetAmount = new BigNumber(10000000000000000); + const takerBalance = takerAssetAmount; + const orderFilledAmount = new BigNumber(0); + const mockBalanceFetcher = buildMockBalanceFetcher(takerBalance); + const mockOrderFilledFetcher = buildMockOrderFilledFetcher(orderFilledAmount); + const [signedOrder] = testOrderFactory.generateTestSignedOrders( + { + makerAssetAmount, + takerAssetAmount, + }, + 1, + ); + + const orderStateUtils = new OrderStateUtils(mockBalanceFetcher, mockOrderFilledFetcher); + const orderState = await orderStateUtils.getOpenOrderStateAsync(signedOrder); + expect(orderState.isValid).to.eq(true); + }); + it('should be invalid when an order is partially filled where only a rounding error remains', async () => { + const makerAssetAmount = new BigNumber(1001); + const takerAssetAmount = new BigNumber(3); + const takerBalance = takerAssetAmount; + const orderFilledAmount = new BigNumber(2); + const mockBalanceFetcher = buildMockBalanceFetcher(takerBalance); + const mockOrderFilledFetcher = buildMockOrderFilledFetcher(orderFilledAmount); + const [signedOrder] = testOrderFactory.generateTestSignedOrders( + { + makerAssetAmount, + takerAssetAmount, + }, + 1, + ); + + const orderStateUtils = new OrderStateUtils(mockBalanceFetcher, mockOrderFilledFetcher); + const orderState = await orderStateUtils.getOpenOrderStateAsync(signedOrder); + expect(orderState.isValid).to.eq(false); + }); + it('should be invalid when an order is cancelled', async () => { + const makerAssetAmount = new BigNumber(1000); + const takerAssetAmount = new BigNumber(2); + const takerBalance = takerAssetAmount; + const orderFilledAmount = new BigNumber(0); + const isCancelled = true; + const mockBalanceFetcher = buildMockBalanceFetcher(takerBalance); + const mockOrderFilledFetcher = buildMockOrderFilledFetcher(orderFilledAmount, isCancelled); + const [signedOrder] = testOrderFactory.generateTestSignedOrders( + { + makerAssetAmount, + takerAssetAmount, + }, + 1, + ); + + const orderStateUtils = new OrderStateUtils(mockBalanceFetcher, mockOrderFilledFetcher); + const orderState = await orderStateUtils.getOpenOrderStateAsync(signedOrder); + expect(orderState.isValid).to.eq(false); + }); + it('should be invalid when an order is fully filled', async () => { + const makerAssetAmount = new BigNumber(1000); + const takerAssetAmount = new BigNumber(2); + const takerBalance = takerAssetAmount; + const orderFilledAmount = takerAssetAmount; + const isCancelled = false; + const mockBalanceFetcher = buildMockBalanceFetcher(takerBalance); + const mockOrderFilledFetcher = buildMockOrderFilledFetcher(orderFilledAmount, isCancelled); + const [signedOrder] = testOrderFactory.generateTestSignedOrders( + { + makerAssetAmount, + takerAssetAmount, + }, + 1, + ); + + const orderStateUtils = new OrderStateUtils(mockBalanceFetcher, mockOrderFilledFetcher); + const orderState = await orderStateUtils.getOpenOrderStateAsync(signedOrder); + expect(orderState.isValid).to.eq(false); + }); + }); +}); diff --git a/packages/order-watcher/test/order_watcher_test.ts b/packages/order-watcher/test/order_watcher_test.ts index 00962bed0..0f4caeb63 100644 --- a/packages/order-watcher/test/order_watcher_test.ts +++ b/packages/order-watcher/test/order_watcher_test.ts @@ -501,25 +501,27 @@ describe('OrderWatcher', () => { expect(orderState.isValid).to.be.false(); const invalidOrderState = orderState as OrderStateInvalid; expect(invalidOrderState.orderHash).to.be.equal(orderHash); - expect(invalidOrderState.error).to.be.equal(ExchangeContractErrs.OrderFillRoundingError); + expect(invalidOrderState.error).to.be.equal(ExchangeContractErrs.OrderAlreadyCancelledOrFilled); }); orderWatcher.subscribe(callback); await contractWrappers.exchange.cancelOrderAsync(signedOrder); })().catch(done); }); - it('should emit orderStateInvalid when within rounding error range', (done: DoneCallback) => { + it('should emit orderStateInvalid when within rounding error range after a partial fill', (done: DoneCallback) => { (async () => { - const remainingFillableAmountInBaseUnits = new BigNumber(100); - signedOrder = await fillScenarios.createFillableSignedOrderAsync( + const fillAmountInBaseUnits = new BigNumber(2); + const makerAssetAmount = new BigNumber(1001); + const takerAssetAmount = new BigNumber(3); + signedOrder = await fillScenarios.createAsymmetricFillableSignedOrderAsync( makerAssetData, takerAssetData, makerAddress, takerAddress, - fillableAmount, + makerAssetAmount, + takerAssetAmount, ); const orderHash = orderHashUtils.getOrderHashHex(signedOrder); await orderWatcher.addOrderAsync(signedOrder); - const callback = callbackErrorReporter.reportNodeCallbackErrors(done)((orderState: OrderState) => { expect(orderState.isValid).to.be.false(); const invalidOrderState = orderState as OrderStateInvalid; @@ -527,11 +529,7 @@ describe('OrderWatcher', () => { expect(invalidOrderState.error).to.be.equal(ExchangeContractErrs.OrderFillRoundingError); }); orderWatcher.subscribe(callback); - await contractWrappers.exchange.fillOrderAsync( - signedOrder, - fillableAmount.minus(remainingFillableAmountInBaseUnits), - takerAddress, - ); + await contractWrappers.exchange.fillOrderAsync(signedOrder, fillAmountInBaseUnits, takerAddress); })().catch(done); }); describe('erc721', () => { -- cgit