From e1ee6b84945e729d894f6535be02f3541e43dbf0 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 12:57:21 +0200 Subject: Add check for ROUNDING_ERROR and test for it --- src/contract_wrappers/exchange_wrapper.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 3fb187de2..fe5fc3d78 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -154,6 +154,10 @@ export class ExchangeWrapper extends ContractWrapper { if (fillMakerAmountInBaseUnits.greaterThan(makerAllowance)) { throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_ALLOWANCE); } + if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmountInBaseUnits, + signedOrder.makerTokenAmount)) { + throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); + } } private throwErrorLogsAsErrors(logs: ContractEvent[]): void { const errEvent = _.find(logs, {event: 'LogError'}); @@ -163,6 +167,18 @@ export class ExchangeWrapper extends ContractWrapper { throw new Error(errMessage); } } + private async isRoundingErrorAsync(takerTokenAmount: BigNumber.BigNumber, + fillTakerAmountInBaseUnits: BigNumber.BigNumber, + makerTokenAmount: BigNumber.BigNumber): Promise { + const exchangeInstance = await this.getExchangeContractAsync(); + const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); + const isRoundingError = await exchangeInstance.isRoundingError.call( + takerTokenAmount, fillTakerAmountInBaseUnits, makerTokenAmount, { + from: senderAddress, + }, + ); + return isRoundingError; + } private async getExchangeContractAsync(): Promise { if (!_.isUndefined(this.exchangeContractIfExists)) { return this.exchangeContractIfExists; -- cgit v1.2.3 From c650d1ba204a862de81b225118d9bcd1a38ad25d Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 16:18:54 +0200 Subject: Add tests and checks for fees balances and allowances --- src/contract_wrappers/exchange_wrapper.ts | 54 +++++++++++++++++++++++++------ 1 file changed, 45 insertions(+), 9 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index fe5fc3d78..fa5c16485 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -74,9 +74,9 @@ export class ExchangeWrapper extends ContractWrapper { assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer); const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); - await this.validateFillOrderAsync(signedOrder, fillTakerAmountInBaseUnits, senderAddress); - const exchangeInstance = await this.getExchangeContractAsync(); + const zrxTokenAddress = await exchangeInstance.ZRX.call(); + await this.validateFillOrderAsync(signedOrder, fillTakerAmountInBaseUnits, senderAddress, zrxTokenAddress); const orderAddresses: OrderAddresses = [ signedOrder.maker, @@ -121,7 +121,7 @@ export class ExchangeWrapper extends ContractWrapper { this.throwErrorLogsAsErrors(response.logs); } private async validateFillOrderAsync(signedOrder: SignedOrder, fillTakerAmountInBaseUnits: BigNumber.BigNumber, - senderAddress: string) { + senderAddress: string, zrxTokenAddress: string): Promise { if (fillTakerAmountInBaseUnits.eq(0)) { throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); } @@ -131,13 +131,32 @@ export class ExchangeWrapper extends ContractWrapper { if (signedOrder.expirationUnixTimestampSec.lessThan(Date.now() / 1000)) { throw new Error(FillOrderValidationErrs.EXPIRED); } + + await this.validateFillOrderBalancesAndAllowancesAsync(signedOrder, fillTakerAmountInBaseUnits, + senderAddress, zrxTokenAddress); + + if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmountInBaseUnits, + signedOrder.makerTokenAmount)) { + throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); + } + } + private async validateFillOrderBalancesAndAllowancesAsync(signedOrder: SignedOrder, + fillTakerAmountInBaseUnits: BigNumber.BigNumber, + senderAddress: string, + zrxTokenAddress: string): Promise { + // TODO: There is a possibility that the user might have enough funds + // to fulfill the order or pay fees but not both. This will happen if + // makerToken === zrxToken || makerToken === zrxToken + // We don't check it for now. The contract checks it and throws. + const makerBalance = await this.tokenWrapper.getBalanceAsync(signedOrder.makerTokenAddress, - signedOrder.maker); + signedOrder.maker); const takerBalance = await this.tokenWrapper.getBalanceAsync(signedOrder.takerTokenAddress, senderAddress); const makerAllowance = await this.tokenWrapper.getProxyAllowanceAsync(signedOrder.makerTokenAddress, - signedOrder.maker); + signedOrder.maker); const takerAllowance = await this.tokenWrapper.getProxyAllowanceAsync(signedOrder.takerTokenAddress, - senderAddress); + senderAddress); + // How many taker tokens would you get for 1 maker token; const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount); const fillMakerAmountInBaseUnits = fillTakerAmountInBaseUnits.div(exchangeRate); @@ -154,9 +173,26 @@ export class ExchangeWrapper extends ContractWrapper { if (fillMakerAmountInBaseUnits.greaterThan(makerAllowance)) { throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_ALLOWANCE); } - if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmountInBaseUnits, - signedOrder.makerTokenAmount)) { - throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); + + const makerFeeBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, + signedOrder.maker); + const takerFeeBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress); + const makerFeeAllowance = await this.tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, + signedOrder.maker); + const takerFeeAllowance = await this.tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, + senderAddress); + + if (signedOrder.takerFee.greaterThan(takerFeeBalance)) { + throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_FEE_BALANCE); + } + if (signedOrder.takerFee.greaterThan(takerFeeAllowance)) { + throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_FEE_ALLOWANCE); + } + if (signedOrder.makerFee.greaterThan(makerFeeBalance)) { + throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_FEE_BALANCE); + } + if (signedOrder.makerFee.greaterThan(makerFeeAllowance)) { + throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_FEE_ALLOWANCE); } } private throwErrorLogsAsErrors(logs: ContractEvent[]): void { -- cgit v1.2.3 From ade42047436f8f81bcc158fdfdc297dc6a2b1f66 Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 2 Jun 2017 16:54:21 +0200 Subject: Improve error names and remove duplicate import --- src/contract_wrappers/exchange_wrapper.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index fa5c16485..93e49cf33 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -10,6 +10,7 @@ import { OrderAddresses, SignedOrder, ContractEvent, + ContractResponse, } from '../types'; import {assert} from '../utils/assert'; import {ContractWrapper} from './contract_wrapper'; @@ -17,18 +18,17 @@ import * as ExchangeArtifacts from '../artifacts/Exchange.json'; import {ecSignatureSchema} from '../schemas/ec_signature_schema'; import {signedOrderSchema} from '../schemas/order_schemas'; import {SchemaValidator} from '../utils/schema_validator'; -import {ContractResponse} from '../types'; import {constants} from '../utils/constants'; import {TokenWrapper} from './token_wrapper'; export class ExchangeWrapper extends ContractWrapper { private exchangeContractErrCodesToMsg = { - [ExchangeContractErrCodes.ERROR_FILL_EXPIRED]: ExchangeContractErrs.ORDER_EXPIRED, - [ExchangeContractErrCodes.ERROR_CANCEL_EXPIRED]: ExchangeContractErrs.ORDER_EXPIRED, + [ExchangeContractErrCodes.ERROR_FILL_EXPIRED]: ExchangeContractErrs.ORDER_FILL_EXPIRED, + [ExchangeContractErrCodes.ERROR_CANCEL_EXPIRED]: ExchangeContractErrs.ORDER_FILL_EXPIRED, [ExchangeContractErrCodes.ERROR_FILL_NO_VALUE]: ExchangeContractErrs.ORDER_REMAINING_FILL_AMOUNT_ZERO, [ExchangeContractErrCodes.ERROR_CANCEL_NO_VALUE]: ExchangeContractErrs.ORDER_REMAINING_FILL_AMOUNT_ZERO, - [ExchangeContractErrCodes.ERROR_FILL_TRUNCATION]: ExchangeContractErrs.ORDER_ROUNDING_ERROR, - [ExchangeContractErrCodes.ERROR_FILL_BALANCE_ALLOWANCE]: ExchangeContractErrs.ORDER_BALANCE_ALLOWANCE_ERROR, + [ExchangeContractErrCodes.ERROR_FILL_TRUNCATION]: ExchangeContractErrs.ORDER_FILL_ROUNDING_ERROR, + [ExchangeContractErrCodes.ERROR_FILL_BALANCE_ALLOWANCE]: ExchangeContractErrs.FILL_BALANCE_ALLOWANCE_ERROR, }; private exchangeContractIfExists?: ExchangeContract; private tokenWrapper: TokenWrapper; -- cgit v1.2.3 From 38c48eb2266632a10fee75ecea7bbce4c343c1cb Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 2 Jun 2017 17:02:16 +0200 Subject: Improve fillOrderAsync comment --- src/contract_wrappers/exchange_wrapper.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 93e49cf33..131211771 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -61,9 +61,12 @@ export class ExchangeWrapper extends ContractWrapper { return isValidSignature; } /** - * Fills a signed order with a fillAmount denominated in baseUnits of the taker token. The caller can - * decide whether they want the call to throw if the balance/allowance checks fail by setting - * shouldCheckTransfer to false. If set to true, the call will fail without throwing, preserving gas costs. + * Fills a signed order with a fillAmount denominated in baseUnits of the taker token. + * Since the order in which transactions are included in the next block is indeterminate, race-conditions + * could arise where a users balance or allowance changes before the fillOrder executes. Because of this, + * we allow you to specify `shouldCheckTransfer`. If true, the smart contract will not throw if while + * executing, the parties do not have sufficient balances/allowances, preserving gas costs. Setting it to + * false foregoes this check and causes the smart contract to throw instead. */ public async fillOrderAsync(signedOrder: SignedOrder, fillTakerAmountInBaseUnits: BigNumber.BigNumber, shouldCheckTransfer: boolean): Promise { -- cgit v1.2.3 From 827a0d4e91169e338cbdc8042d158aaf7fdcf96c Mon Sep 17 00:00:00 2001 From: Fabio Berger Date: Fri, 2 Jun 2017 17:03:36 +0200 Subject: rename fillTakerAmountInBaseUnits to fillTakerAmount --- src/contract_wrappers/exchange_wrapper.ts | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 131211771..f0f6e79f7 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -68,18 +68,18 @@ export class ExchangeWrapper extends ContractWrapper { * executing, the parties do not have sufficient balances/allowances, preserving gas costs. Setting it to * false foregoes this check and causes the smart contract to throw instead. */ - public async fillOrderAsync(signedOrder: SignedOrder, fillTakerAmountInBaseUnits: BigNumber.BigNumber, + public async fillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, shouldCheckTransfer: boolean): Promise { assert.doesConformToSchema('signedOrder', SchemaValidator.convertToJSONSchemaCompatibleObject(signedOrder as object), signedOrderSchema); - assert.isBigNumber('fillTakerAmountInBaseUnits', fillTakerAmountInBaseUnits); + assert.isBigNumber('fillTakerAmount', fillTakerAmount); assert.isBoolean('shouldCheckTransfer', shouldCheckTransfer); const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); const exchangeInstance = await this.getExchangeContractAsync(); const zrxTokenAddress = await exchangeInstance.ZRX.call(); - await this.validateFillOrderAsync(signedOrder, fillTakerAmountInBaseUnits, senderAddress, zrxTokenAddress); + await this.validateFillOrderAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); const orderAddresses: OrderAddresses = [ signedOrder.maker, @@ -99,7 +99,7 @@ export class ExchangeWrapper extends ContractWrapper { const gas = await exchangeInstance.fill.estimateGas( orderAddresses, orderValues, - fillTakerAmountInBaseUnits, + fillTakerAmount, shouldCheckTransfer, signedOrder.ecSignature.v, signedOrder.ecSignature.r, @@ -111,7 +111,7 @@ export class ExchangeWrapper extends ContractWrapper { const response: ContractResponse = await exchangeInstance.fill( orderAddresses, orderValues, - fillTakerAmountInBaseUnits, + fillTakerAmount, shouldCheckTransfer, signedOrder.ecSignature.v, signedOrder.ecSignature.r, @@ -123,9 +123,9 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } - private async validateFillOrderAsync(signedOrder: SignedOrder, fillTakerAmountInBaseUnits: BigNumber.BigNumber, + private async validateFillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, senderAddress: string, zrxTokenAddress: string): Promise { - if (fillTakerAmountInBaseUnits.eq(0)) { + if (fillTakerAmount.eq(0)) { throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); } if (signedOrder.taker !== constants.NULL_ADDRESS && signedOrder.taker !== senderAddress) { @@ -135,16 +135,16 @@ export class ExchangeWrapper extends ContractWrapper { throw new Error(FillOrderValidationErrs.EXPIRED); } - await this.validateFillOrderBalancesAndAllowancesAsync(signedOrder, fillTakerAmountInBaseUnits, + await this.validateFillOrderBalancesAndAllowancesAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); - if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmountInBaseUnits, + if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmount, signedOrder.makerTokenAmount)) { throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); } } private async validateFillOrderBalancesAndAllowancesAsync(signedOrder: SignedOrder, - fillTakerAmountInBaseUnits: BigNumber.BigNumber, + fillTakerAmount: BigNumber.BigNumber, senderAddress: string, zrxTokenAddress: string): Promise { // TODO: There is a possibility that the user might have enough funds @@ -162,12 +162,12 @@ export class ExchangeWrapper extends ContractWrapper { // How many taker tokens would you get for 1 maker token; const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount); - const fillMakerAmountInBaseUnits = fillTakerAmountInBaseUnits.div(exchangeRate); + const fillMakerAmountInBaseUnits = fillTakerAmount.div(exchangeRate); - if (fillTakerAmountInBaseUnits.greaterThan(takerBalance)) { + if (fillTakerAmount.greaterThan(takerBalance)) { throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_BALANCE); } - if (fillTakerAmountInBaseUnits.greaterThan(takerAllowance)) { + if (fillTakerAmount.greaterThan(takerAllowance)) { throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_ALLOWANCE); } if (fillMakerAmountInBaseUnits.greaterThan(makerBalance)) { @@ -207,12 +207,12 @@ export class ExchangeWrapper extends ContractWrapper { } } private async isRoundingErrorAsync(takerTokenAmount: BigNumber.BigNumber, - fillTakerAmountInBaseUnits: BigNumber.BigNumber, + fillTakerAmount: BigNumber.BigNumber, makerTokenAmount: BigNumber.BigNumber): Promise { const exchangeInstance = await this.getExchangeContractAsync(); const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); const isRoundingError = await exchangeInstance.isRoundingError.call( - takerTokenAmount, fillTakerAmountInBaseUnits, makerTokenAmount, { + takerTokenAmount, fillTakerAmount, makerTokenAmount, { from: senderAddress, }, ); -- cgit v1.2.3 From 9756aa86b051940b88f87fbecb313bf07590eca3 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:34:01 +0200 Subject: Add getZRXTokenAddressAsync --- src/contract_wrappers/exchange_wrapper.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index f0f6e79f7..0c7f27507 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -78,7 +78,7 @@ export class ExchangeWrapper extends ContractWrapper { const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); const exchangeInstance = await this.getExchangeContractAsync(); - const zrxTokenAddress = await exchangeInstance.ZRX.call(); + const zrxTokenAddress = await this.getZRXTokenAddressAsync(exchangeInstance); await this.validateFillOrderAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); const orderAddresses: OrderAddresses = [ @@ -226,4 +226,7 @@ export class ExchangeWrapper extends ContractWrapper { this.exchangeContractIfExists = contractInstance as ExchangeContract; return this.exchangeContractIfExists; } + private async getZRXTokenAddressAsync(exchangeInstance: ExchangeContract): Promise { + return exchangeInstance.ZRX.call(); + } } -- cgit v1.2.3 From ef3a27ed0545e2e30e965b928ae13c8a53e16e8d Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:35:05 +0200 Subject: Rename validation functions --- src/contract_wrappers/exchange_wrapper.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 0c7f27507..965b31710 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -79,7 +79,7 @@ export class ExchangeWrapper extends ContractWrapper { const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); const exchangeInstance = await this.getExchangeContractAsync(); const zrxTokenAddress = await this.getZRXTokenAddressAsync(exchangeInstance); - await this.validateFillOrderAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); + await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); const orderAddresses: OrderAddresses = [ signedOrder.maker, @@ -123,8 +123,8 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } - private async validateFillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, - senderAddress: string, zrxTokenAddress: string): Promise { + private async validateFillOrderAndThrowIfInvalidAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, + senderAddress: string, zrxTokenAddress: string): Promise { if (fillTakerAmount.eq(0)) { throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); } @@ -135,7 +135,7 @@ export class ExchangeWrapper extends ContractWrapper { throw new Error(FillOrderValidationErrs.EXPIRED); } - await this.validateFillOrderBalancesAndAllowancesAsync(signedOrder, fillTakerAmount, + await this.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmount, @@ -143,10 +143,10 @@ export class ExchangeWrapper extends ContractWrapper { throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); } } - private async validateFillOrderBalancesAndAllowancesAsync(signedOrder: SignedOrder, - fillTakerAmount: BigNumber.BigNumber, - senderAddress: string, - zrxTokenAddress: string): Promise { + private async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder: SignedOrder, + fillTakerAmount: BigNumber.BigNumber, + senderAddress: string, + zrxTokenAddress: string): Promise { // TODO: There is a possibility that the user might have enough funds // to fulfill the order or pay fees but not both. This will happen if // makerToken === zrxToken || makerToken === zrxToken -- cgit v1.2.3 From 0f7bd0597234dbeafcee31e6f715f3c2004696df Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:37:28 +0200 Subject: Don't pass zrxTokenAsign --- src/contract_wrappers/exchange_wrapper.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 965b31710..d7d56c276 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -78,8 +78,7 @@ export class ExchangeWrapper extends ContractWrapper { const senderAddress = await this.web3Wrapper.getSenderAddressOrThrowAsync(); const exchangeInstance = await this.getExchangeContractAsync(); - const zrxTokenAddress = await this.getZRXTokenAddressAsync(exchangeInstance); - await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); + await this.validateFillOrderAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, senderAddress); const orderAddresses: OrderAddresses = [ signedOrder.maker, @@ -123,8 +122,9 @@ export class ExchangeWrapper extends ContractWrapper { ); this.throwErrorLogsAsErrors(response.logs); } - private async validateFillOrderAndThrowIfInvalidAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, - senderAddress: string, zrxTokenAddress: string): Promise { + private async validateFillOrderAndThrowIfInvalidAsync(signedOrder: SignedOrder, + fillTakerAmount: BigNumber.BigNumber, + senderAddress: string): Promise { if (fillTakerAmount.eq(0)) { throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); } @@ -134,7 +134,7 @@ export class ExchangeWrapper extends ContractWrapper { if (signedOrder.expirationUnixTimestampSec.lessThan(Date.now() / 1000)) { throw new Error(FillOrderValidationErrs.EXPIRED); } - + const zrxTokenAddress = await this.getZRXTokenAddressAsync(); await this.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); @@ -226,7 +226,8 @@ export class ExchangeWrapper extends ContractWrapper { this.exchangeContractIfExists = contractInstance as ExchangeContract; return this.exchangeContractIfExists; } - private async getZRXTokenAddressAsync(exchangeInstance: ExchangeContract): Promise { + private async getZRXTokenAddressAsync(): Promise { + const exchangeInstance = await this.getExchangeContractAsync(); return exchangeInstance.ZRX.call(); } } -- cgit v1.2.3 From e5cc0e0562eccf67eff9cf0521c4884ffc3b0f3b Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:38:47 +0200 Subject: Rename NOT_A_TAKER to TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER --- src/contract_wrappers/exchange_wrapper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index d7d56c276..ac15faff4 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -129,7 +129,7 @@ export class ExchangeWrapper extends ContractWrapper { throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); } if (signedOrder.taker !== constants.NULL_ADDRESS && signedOrder.taker !== senderAddress) { - throw new Error(FillOrderValidationErrs.NOT_A_TAKER); + throw new Error(FillOrderValidationErrs.TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER); } if (signedOrder.expirationUnixTimestampSec.lessThan(Date.now() / 1000)) { throw new Error(FillOrderValidationErrs.EXPIRED); -- cgit v1.2.3 From f5158eebf335c9fbcfb7cfb6f32a0ecd1161391d Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:39:21 +0200 Subject: Assign timestamp to a variable --- src/contract_wrappers/exchange_wrapper.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index ac15faff4..4c46404bb 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -131,7 +131,8 @@ export class ExchangeWrapper extends ContractWrapper { if (signedOrder.taker !== constants.NULL_ADDRESS && signedOrder.taker !== senderAddress) { throw new Error(FillOrderValidationErrs.TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER); } - if (signedOrder.expirationUnixTimestampSec.lessThan(Date.now() / 1000)) { + const currentUnixTimestampSec = Date.now() / 1000; + if (signedOrder.expirationUnixTimestampSec.lessThan(currentUnixTimestampSec)) { throw new Error(FillOrderValidationErrs.EXPIRED); } const zrxTokenAddress = await this.getZRXTokenAddressAsync(); -- cgit v1.2.3 From 7fd84e29ab8c27cb872cf8cd0f631d4b78abba0e Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:40:06 +0200 Subject: Rename EXPIRED to FILL_ORDER_EXPIRED --- src/contract_wrappers/exchange_wrapper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 4c46404bb..e957530ae 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -133,7 +133,7 @@ export class ExchangeWrapper extends ContractWrapper { } const currentUnixTimestampSec = Date.now() / 1000; if (signedOrder.expirationUnixTimestampSec.lessThan(currentUnixTimestampSec)) { - throw new Error(FillOrderValidationErrs.EXPIRED); + throw new Error(FillOrderValidationErrs.FILL_ORDER_EXPIRED); } const zrxTokenAddress = await this.getZRXTokenAddressAsync(); await this.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, -- cgit v1.2.3 From 54e8bf773091eb2c8587c72e50892afec42abb9d Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 17:41:23 +0200 Subject: Assign wouldRoundingErrorOccur to a variable --- src/contract_wrappers/exchange_wrapper.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index e957530ae..828994829 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -139,8 +139,10 @@ export class ExchangeWrapper extends ContractWrapper { await this.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, senderAddress, zrxTokenAddress); - if (await this.isRoundingErrorAsync(signedOrder.takerTokenAmount, fillTakerAmount, - signedOrder.makerTokenAmount)) { + const wouldRoundingErrorOccur = await this.isRoundingErrorAsync( + signedOrder.takerTokenAmount, fillTakerAmount, signedOrder.makerTokenAmount, + ); + if (wouldRoundingErrorOccur) { throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); } } -- cgit v1.2.3 From abf2cf4c5e2ea6d09d862871adfa16ca108907e2 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 18:00:37 +0200 Subject: Move FillOrderValidatinErrs to ExchangeContractErrs --- src/contract_wrappers/exchange_wrapper.ts | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 828994829..716f9c77a 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -5,7 +5,6 @@ import { ExchangeContract, ExchangeContractErrCodes, ExchangeContractErrs, - FillOrderValidationErrs, OrderValues, OrderAddresses, SignedOrder, @@ -126,14 +125,14 @@ export class ExchangeWrapper extends ContractWrapper { fillTakerAmount: BigNumber.BigNumber, senderAddress: string): Promise { if (fillTakerAmount.eq(0)) { - throw new Error(FillOrderValidationErrs.FILL_AMOUNT_IS_ZERO); + throw new Error(ExchangeContractErrs.ORDER_REMAINING_FILL_AMOUNT_ZERO); } if (signedOrder.taker !== constants.NULL_ADDRESS && signedOrder.taker !== senderAddress) { - throw new Error(FillOrderValidationErrs.TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER); + throw new Error(ExchangeContractErrs.TRANSACTION_SENDER_IS_NOT_FILL_ORDER_TAKER); } const currentUnixTimestampSec = Date.now() / 1000; if (signedOrder.expirationUnixTimestampSec.lessThan(currentUnixTimestampSec)) { - throw new Error(FillOrderValidationErrs.FILL_ORDER_EXPIRED); + throw new Error(ExchangeContractErrs.ORDER_FILL_EXPIRED); } const zrxTokenAddress = await this.getZRXTokenAddressAsync(); await this.validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder, fillTakerAmount, @@ -143,7 +142,7 @@ export class ExchangeWrapper extends ContractWrapper { signedOrder.takerTokenAmount, fillTakerAmount, signedOrder.makerTokenAmount, ); if (wouldRoundingErrorOccur) { - throw new Error(FillOrderValidationErrs.ROUNDING_ERROR); + throw new Error(ExchangeContractErrs.ORDER_FILL_ROUNDING_ERROR); } } private async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder: SignedOrder, @@ -168,16 +167,16 @@ export class ExchangeWrapper extends ContractWrapper { const fillMakerAmountInBaseUnits = fillTakerAmount.div(exchangeRate); if (fillTakerAmount.greaterThan(takerBalance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_BALANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_BALANCE); } if (fillTakerAmount.greaterThan(takerAllowance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_ALLOWANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_ALLOWANCE); } if (fillMakerAmountInBaseUnits.greaterThan(makerBalance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_BALANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_BALANCE); } if (fillMakerAmountInBaseUnits.greaterThan(makerAllowance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_ALLOWANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_ALLOWANCE); } const makerFeeBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, @@ -189,16 +188,16 @@ export class ExchangeWrapper extends ContractWrapper { senderAddress); if (signedOrder.takerFee.greaterThan(takerFeeBalance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_FEE_BALANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_FEE_BALANCE); } if (signedOrder.takerFee.greaterThan(takerFeeAllowance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_TAKER_FEE_ALLOWANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_FEE_ALLOWANCE); } if (signedOrder.makerFee.greaterThan(makerFeeBalance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_FEE_BALANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_FEE_BALANCE); } if (signedOrder.makerFee.greaterThan(makerFeeAllowance)) { - throw new Error(FillOrderValidationErrs.NOT_ENOUGH_MAKER_FEE_ALLOWANCE); + throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_FEE_ALLOWANCE); } } private throwErrorLogsAsErrors(logs: ContractEvent[]): void { -- cgit v1.2.3 From 89d93494788f06d227628dfcfbd712e26ef02b77 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 18:05:27 +0200 Subject: Rewrite comment --- src/contract_wrappers/exchange_wrapper.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 716f9c77a..ba3b05758 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -65,7 +65,7 @@ export class ExchangeWrapper extends ContractWrapper { * could arise where a users balance or allowance changes before the fillOrder executes. Because of this, * we allow you to specify `shouldCheckTransfer`. If true, the smart contract will not throw if while * executing, the parties do not have sufficient balances/allowances, preserving gas costs. Setting it to - * false foregoes this check and causes the smart contract to throw instead. + * false forgoes this check and causes the smart contract to throw instead. */ public async fillOrderAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, shouldCheckTransfer: boolean): Promise { @@ -145,14 +145,20 @@ export class ExchangeWrapper extends ContractWrapper { throw new Error(ExchangeContractErrs.ORDER_FILL_ROUNDING_ERROR); } } + + /** + * This method does not currently validate the edge-case where the makerToken or takerToken is also the token used + * to pay fees (ZRX). It is possible for them to have enough for fees and the transfer but not both. + * 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. + */ private async validateFillOrderBalancesAndAllowancesAndThrowIfInvalidAsync(signedOrder: SignedOrder, fillTakerAmount: BigNumber.BigNumber, senderAddress: string, zrxTokenAddress: string): Promise { - // TODO: There is a possibility that the user might have enough funds - // to fulfill the order or pay fees but not both. This will happen if - // makerToken === zrxToken || makerToken === zrxToken - // We don't check it for now. The contract checks it and throws. const makerBalance = await this.tokenWrapper.getBalanceAsync(signedOrder.makerTokenAddress, signedOrder.maker); -- cgit v1.2.3 From 45fadeb690e27acccc776515ada8d39e3633194a Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 18:06:06 +0200 Subject: Allign arguments --- src/contract_wrappers/exchange_wrapper.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index ba3b05758..55ddcc46a 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -161,12 +161,12 @@ export class ExchangeWrapper extends ContractWrapper { zrxTokenAddress: string): Promise { const makerBalance = await this.tokenWrapper.getBalanceAsync(signedOrder.makerTokenAddress, - signedOrder.maker); + signedOrder.maker); const takerBalance = await this.tokenWrapper.getBalanceAsync(signedOrder.takerTokenAddress, senderAddress); const makerAllowance = await this.tokenWrapper.getProxyAllowanceAsync(signedOrder.makerTokenAddress, - signedOrder.maker); + signedOrder.maker); const takerAllowance = await this.tokenWrapper.getProxyAllowanceAsync(signedOrder.takerTokenAddress, - senderAddress); + senderAddress); // How many taker tokens would you get for 1 maker token; const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount); -- cgit v1.2.3 From 76280dd5dbc5a5a2fb5f7230fa73d594f822c7e6 Mon Sep 17 00:00:00 2001 From: Leonid Logvinov Date: Fri, 2 Jun 2017 18:10:06 +0200 Subject: Address feedback --- src/contract_wrappers/exchange_wrapper.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) (limited to 'src/contract_wrappers') diff --git a/src/contract_wrappers/exchange_wrapper.ts b/src/contract_wrappers/exchange_wrapper.ts index 55ddcc46a..4aa532bdd 100644 --- a/src/contract_wrappers/exchange_wrapper.ts +++ b/src/contract_wrappers/exchange_wrapper.ts @@ -168,42 +168,42 @@ export class ExchangeWrapper extends ContractWrapper { const takerAllowance = await this.tokenWrapper.getProxyAllowanceAsync(signedOrder.takerTokenAddress, senderAddress); - // How many taker tokens would you get for 1 maker token; + // exchangeRate is the price of one maker token denominated in taker tokens const exchangeRate = signedOrder.takerTokenAmount.div(signedOrder.makerTokenAmount); const fillMakerAmountInBaseUnits = fillTakerAmount.div(exchangeRate); if (fillTakerAmount.greaterThan(takerBalance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_BALANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_TAKER_BALANCE); } if (fillTakerAmount.greaterThan(takerAllowance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_ALLOWANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_TAKER_ALLOWANCE); } if (fillMakerAmountInBaseUnits.greaterThan(makerBalance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_BALANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_MAKER_BALANCE); } if (fillMakerAmountInBaseUnits.greaterThan(makerAllowance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_ALLOWANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_MAKER_ALLOWANCE); } const makerFeeBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, - signedOrder.maker); + signedOrder.maker); const takerFeeBalance = await this.tokenWrapper.getBalanceAsync(zrxTokenAddress, senderAddress); const makerFeeAllowance = await this.tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, - signedOrder.maker); + signedOrder.maker); const takerFeeAllowance = await this.tokenWrapper.getProxyAllowanceAsync(zrxTokenAddress, - senderAddress); + senderAddress); if (signedOrder.takerFee.greaterThan(takerFeeBalance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_FEE_BALANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_TAKER_FEE_BALANCE); } if (signedOrder.takerFee.greaterThan(takerFeeAllowance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_TAKER_FEE_ALLOWANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_TAKER_FEE_ALLOWANCE); } if (signedOrder.makerFee.greaterThan(makerFeeBalance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_FEE_BALANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_MAKER_FEE_BALANCE); } if (signedOrder.makerFee.greaterThan(makerFeeAllowance)) { - throw new Error(ExchangeContractErrs.NOT_ENOUGH_MAKER_FEE_ALLOWANCE); + throw new Error(ExchangeContractErrs.INSUFFICIENT_MAKER_FEE_ALLOWANCE); } } private throwErrorLogsAsErrors(logs: ContractEvent[]): void { -- cgit v1.2.3