diff --git a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol index d6da4f3889..569764fee0 100644 --- a/contracts/exchange/contracts/src/MixinWrapperFunctions.sol +++ b/contracts/exchange/contracts/src/MixinWrapperFunctions.sol @@ -189,7 +189,6 @@ contract MixinWrapperFunctions is // The `takerAssetData` must be the same for each order. // Rather than checking equality, we point the `takerAssetData` of each order to the same memory location. // This is less expensive than checking equality. - bytes memory originalTakerAssetData = orders[i].takerAssetData; orders[i].takerAssetData = takerAssetData; // Attempt to sell the remaining amount of takerAsset @@ -199,9 +198,6 @@ contract MixinWrapperFunctions is signatures[i] ); - // Restore the original `takerAssetData` so we're non-destructive. - orders[i].takerAssetData = originalTakerAssetData; - // Update amounts filled and fees paid by maker and taker fillResults = LibFillResults.addFillResults(fillResults, singleFillResults); @@ -245,7 +241,6 @@ contract MixinWrapperFunctions is // The `makerAssetData` must be the same for each order. // Rather than checking equality, we point the `makerAssetData` of each order to the same memory location. // This is less expensive than checking equality. - bytes memory originalMakerAssetData = orders[i].makerAssetData; orders[i].makerAssetData = makerAssetData; // Attempt to sell the remaining amount of takerAsset @@ -255,9 +250,6 @@ contract MixinWrapperFunctions is signatures[i] ); - // Restore the original `makerAssetData` so we're non-destructive. - orders[i].makerAssetData = originalMakerAssetData; - // Update amounts filled and fees paid by maker and taker fillResults = LibFillResults.addFillResults(fillResults, singleFillResults); @@ -284,9 +276,10 @@ contract MixinWrapperFunctions is { fillResults = marketSellOrdersNoThrow(orders, takerAssetFillAmount, signatures); if (fillResults.takerAssetFilledAmount < takerAssetFillAmount) { - LibRichErrors.rrevert(LibExchangeRichErrors.IncompleteMarketSellError( + LibRichErrors.rrevert(LibExchangeRichErrors.IncompleteFillError( + LibExchangeRichErrors.IncompleteFillErrorCode.INCOMPLETE_MARKET_SELL_ORDERS, takerAssetFillAmount, - _getOrderHashes(orders) + fillResults.takerAssetFilledAmount )); } } @@ -306,9 +299,10 @@ contract MixinWrapperFunctions is { fillResults = marketBuyOrdersNoThrow(orders, makerAssetFillAmount, signatures); if (fillResults.makerAssetFilledAmount < makerAssetFillAmount) { - LibRichErrors.rrevert(LibExchangeRichErrors.IncompleteMarketBuyError( + LibRichErrors.rrevert(LibExchangeRichErrors.IncompleteFillError( + LibExchangeRichErrors.IncompleteFillErrorCode.INCOMPLETE_MARKET_BUY_ORDERS, makerAssetFillAmount, - _getOrderHashes(orders) + fillResults.makerAssetFilledAmount )); } } @@ -360,8 +354,9 @@ contract MixinWrapperFunctions is ); if (fillResults.takerAssetFilledAmount != takerAssetFillAmount) { LibRichErrors.rrevert(LibExchangeRichErrors.IncompleteFillError( + LibExchangeRichErrors.IncompleteFillErrorCode.INCOMPLETE_FILL_ORDER, takerAssetFillAmount, - getOrderInfo(order).orderHash + fillResults.takerAssetFilledAmount )); } return fillResults; diff --git a/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol b/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol deleted file mode 100644 index 0d1848f2cb..0000000000 --- a/contracts/exchange/contracts/src/interfaces/IExchangeRichErrors.sol +++ /dev/null @@ -1,39 +0,0 @@ -pragma solidity ^0.5.9; - - -contract IExchangeRichErrors { - - enum AssetProxyDispatchErrorCodes { - INVALID_ASSET_DATA_LENGTH, - UNKNOWN_ASSET_PROXY - } - - enum BatchMatchOrdersErrorCodes { - ZERO_LEFT_ORDERS, - ZERO_RIGHT_ORDERS, - INVALID_LENGTH_LEFT_SIGNATURES, - INVALID_LENGTH_RIGHT_SIGNATURES - } - - enum FillErrorCodes { - INVALID_TAKER_AMOUNT, - TAKER_OVERPAY, - OVERFILL, - INVALID_FILL_PRICE - } - - enum SignatureErrorCodes { - BAD_SIGNATURE, - INVALID_LENGTH, - UNSUPPORTED, - ILLEGAL, - INAPPROPRIATE_SIGNATURE_TYPE, - INVALID_SIGNER - } - - enum TransactionErrorCodes { - NO_REENTRANCY, - ALREADY_EXECUTED, - EXPIRED - } -} diff --git a/contracts/exchange/contracts/src/libs/LibExchangeRichErrorDecoder.sol b/contracts/exchange/contracts/src/libs/LibExchangeRichErrorDecoder.sol index ba998517a2..17a6b63c02 100644 --- a/contracts/exchange/contracts/src/libs/LibExchangeRichErrorDecoder.sol +++ b/contracts/exchange/contracts/src/libs/LibExchangeRichErrorDecoder.sol @@ -382,53 +382,18 @@ contract LibExchangeRichErrorDecoder { public pure returns ( - uint256 fillAmount, - bytes32 orderHash + LibExchangeRichErrors.IncompleteFillErrorCode errorCode, + uint256 expectedAssetFillAmount, + uint256 actualAssetFillAmount ) { _assertSelectorBytes(encoded, LibExchangeRichErrors.IncompleteFillErrorSelector()); - (fillAmount, orderHash) = abi.decode( + uint8 _errorCode; + (_errorCode, expectedAssetFillAmount, actualAssetFillAmount) = abi.decode( encoded.sliceDestructive(4, encoded.length), - (uint256, bytes32) - ); - } - - /// @dev Decompose an ABI-encoded IncompleteMarketSellError. - /// @param encoded ABI-encoded revert error. - /// @return orderHash Hash of the order being filled. - function decodeIncompleteMarketSellError(bytes memory encoded) - public - pure - returns ( - uint256 takerAssetFillAmount, - bytes32[] memory orderHashes - ) - { - _assertSelectorBytes(encoded, LibExchangeRichErrors.IncompleteMarketSellErrorSelector()); - // solhint-disable-next-line - (takerAssetFillAmount, orderHashes) = abi.decode( - encoded.sliceDestructive(4, encoded.length), - (uint256, bytes32[]) - ); - } - - /// @dev Decompose an ABI-encoded IncompleteMarketBuyError. - /// @param encoded ABI-encoded revert error. - /// @return orderHash Hash of the order being filled. - function decodeIncompleteMarketBuyError(bytes memory encoded) - public - pure - returns ( - uint256 makerAssetFillAmount, - bytes32[] memory orderHashes - ) - { - _assertSelectorBytes(encoded, LibExchangeRichErrors.IncompleteMarketBuyErrorSelector()); - // solhint-disable-next-line - (makerAssetFillAmount, orderHashes) = abi.decode( - encoded.sliceDestructive(4, encoded.length), - (uint256, bytes32[]) + (uint8, uint256, uint256) ); + errorCode = LibExchangeRichErrors.IncompleteFillErrorCode(_errorCode); } /// @dev Revert if the leading 4 bytes of `encoded` is not `selector`. diff --git a/contracts/exchange/package.json b/contracts/exchange/package.json index a2cfab8182..8a55693bd6 100644 --- a/contracts/exchange/package.json +++ b/contracts/exchange/package.json @@ -35,7 +35,7 @@ "compile:truffle": "truffle compile" }, "config": { - "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxy|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IExchangeRichErrors|IMatchOrders|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|TestWrapperFunctions|Whitelist).json", + "abis": "./generated-artifacts/@(Exchange|ExchangeWrapper|IAssetProxy|IAssetProxyDispatcher|IEIP1271Wallet|IExchange|IExchangeCore|IMatchOrders|ISignatureValidator|ITransactions|ITransferSimulator|IWallet|IWrapperFunctions|IsolatedExchange|LibExchangeRichErrorDecoder|MixinAssetProxyDispatcher|MixinExchangeCore|MixinMatchOrders|MixinSignatureValidator|MixinTransactions|MixinTransferSimulator|MixinWrapperFunctions|ReentrancyTester|TestAssetProxyDispatcher|TestExchangeInternals|TestLibExchangeRichErrorDecoder|TestSignatureValidator|TestValidatorWallet|TestWrapperFunctions|Whitelist).json", "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually." }, "repository": { diff --git a/contracts/exchange/src/artifacts.ts b/contracts/exchange/src/artifacts.ts index 03f27aafc6..8c1aedae3e 100644 --- a/contracts/exchange/src/artifacts.ts +++ b/contracts/exchange/src/artifacts.ts @@ -12,7 +12,6 @@ import * as IAssetProxyDispatcher from '../generated-artifacts/IAssetProxyDispat import * as IEIP1271Wallet from '../generated-artifacts/IEIP1271Wallet.json'; import * as IExchange from '../generated-artifacts/IExchange.json'; import * as IExchangeCore from '../generated-artifacts/IExchangeCore.json'; -import * as IExchangeRichErrors from '../generated-artifacts/IExchangeRichErrors.json'; import * as IMatchOrders from '../generated-artifacts/IMatchOrders.json'; import * as ISignatureValidator from '../generated-artifacts/ISignatureValidator.json'; import * as ITransactions from '../generated-artifacts/ITransactions.json'; @@ -52,7 +51,6 @@ export const artifacts = { IEIP1271Wallet: IEIP1271Wallet as ContractArtifact, IExchange: IExchange as ContractArtifact, IExchangeCore: IExchangeCore as ContractArtifact, - IExchangeRichErrors: IExchangeRichErrors as ContractArtifact, IMatchOrders: IMatchOrders as ContractArtifact, ISignatureValidator: ISignatureValidator as ContractArtifact, ITransactions: ITransactions as ContractArtifact, diff --git a/contracts/exchange/src/wrappers.ts b/contracts/exchange/src/wrappers.ts index 2efb88a2ed..171f93ba3e 100644 --- a/contracts/exchange/src/wrappers.ts +++ b/contracts/exchange/src/wrappers.ts @@ -10,7 +10,6 @@ export * from '../generated-wrappers/i_asset_proxy_dispatcher'; export * from '../generated-wrappers/i_e_i_p1271_wallet'; export * from '../generated-wrappers/i_exchange'; export * from '../generated-wrappers/i_exchange_core'; -export * from '../generated-wrappers/i_exchange_rich_errors'; export * from '../generated-wrappers/i_match_orders'; export * from '../generated-wrappers/i_signature_validator'; export * from '../generated-wrappers/i_transactions'; diff --git a/contracts/exchange/test/lib_exchange_rich_error_decoder.ts b/contracts/exchange/test/lib_exchange_rich_error_decoder.ts index fd57ff33b2..85ccb85f1d 100644 --- a/contracts/exchange/test/lib_exchange_rich_error_decoder.ts +++ b/contracts/exchange/test/lib_exchange_rich_error_decoder.ts @@ -146,20 +146,9 @@ blockchainTests.resets('LibExchangeRichErrorDecoder', ({ provider, txDefaults }) })(); (() => { - const orderHash = orderUtils.generatePseudoRandomOrderHash(); - const amount = new BigNumber(hexRandom(WORD_LENGTH)); - createDecodeTest(ExchangeRevertErrors.IncompleteFillError, [amount, orderHash]); - })(); - - (() => { - const orderHashes = _.times(3, () => orderUtils.generatePseudoRandomOrderHash()); - const amount = new BigNumber(hexRandom(WORD_LENGTH)); - createDecodeTest(ExchangeRevertErrors.IncompleteMarketSellError, [amount, orderHashes]); - })(); - - (() => { - const orderHashes = _.times(3, () => orderUtils.generatePseudoRandomOrderHash()); - const amount = new BigNumber(hexRandom(WORD_LENGTH)); - createDecodeTest(ExchangeRevertErrors.IncompleteMarketBuyError, [amount, orderHashes]); + const errorCode = ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteMarketSellOrders; + const expectedAmount = new BigNumber(hexRandom(WORD_LENGTH)); + const actualAmount = new BigNumber(hexRandom(WORD_LENGTH)); + createDecodeTest(ExchangeRevertErrors.IncompleteFillError, [errorCode, expectedAmount, actualAmount]); })(); }); diff --git a/contracts/exchange/test/wrapper.ts b/contracts/exchange/test/wrapper.ts index 03bed7b3eb..659ce8f234 100644 --- a/contracts/exchange/test/wrapper.ts +++ b/contracts/exchange/test/wrapper.ts @@ -197,8 +197,11 @@ blockchainTests.resets('Exchange wrappers', env => { takerAssetFillAmount: signedOrder.takerAssetAmount.dividedToIntegerBy(2), }); - const orderHashHex = orderHashUtils.getOrderHashHex(signedOrder); - const expectedError = new ExchangeRevertErrors.IncompleteFillError(takerAssetFillAmount, orderHashHex); + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteFillOrder, + takerAssetFillAmount, + takerAssetFillAmount.dividedToIntegerBy(2), + ); const tx = exchangeWrapper.fillOrKillOrderAsync(signedOrder, takerAddress); return expect(tx).to.revertWith(expectedError); }); diff --git a/contracts/exchange/test/wrapper_unit_tests.ts b/contracts/exchange/test/wrapper_unit_tests.ts index ca1c54dabf..71b835adf1 100644 --- a/contracts/exchange/test/wrapper_unit_tests.ts +++ b/contracts/exchange/test/wrapper_unit_tests.ts @@ -166,7 +166,11 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { takerAssetAmount: fillAmount.minus(1), }); const signature = createOrderSignature(order); - const expectedError = new ExchangeRevertErrors.IncompleteFillError(fillAmount, getExpectedOrderHash(order)); + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteFillOrder, + fillAmount, + fillAmount.minus(1), + ); const tx = testContract.fillOrKillOrder.awaitTransactionSuccessAsync(order, fillAmount, signature); return expect(tx).to.revertWith(expectedError); }); @@ -179,7 +183,11 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { takerAssetAmount: fillAmount.plus(1), }); const signature = createOrderSignature(order); - const expectedError = new ExchangeRevertErrors.IncompleteFillError(fillAmount, getExpectedOrderHash(order)); + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteFillOrder, + fillAmount, + fillAmount.plus(1), + ); const tx = testContract.fillOrKillOrder.awaitTransactionSuccessAsync(order, fillAmount, signature); return expect(tx).to.revertWith(expectedError); }); @@ -394,8 +402,9 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { // the `takerAssetFilledAmount`. failingOrder.takerAssetAmount = failingOrder.takerAssetAmount.minus(1); const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteFillOrder, failingAmount, - getExpectedOrderHash(failingOrder), + failingAmount.minus(1), ); const tx = txHelper.getResultAndReceiptAsync( testContract.batchFillOrKillOrders, @@ -418,8 +427,9 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { // the `takerAssetFilledAmount`. failingOrder.takerAssetAmount = failingOrder.takerAssetAmount.plus(1); const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteFillOrder, failingAmount, - getExpectedOrderHash(failingOrder), + failingAmount.plus(1), ); const tx = txHelper.getResultAndReceiptAsync( testContract.batchFillOrKillOrders, @@ -817,9 +827,10 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { (total, o) => o.takerAssetAmount.plus(total), constants.ZERO_AMOUNT, ).plus(1); - const expectedError = new ExchangeRevertErrors.IncompleteMarketSellError( + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteMarketSellOrders, takerAssetFillAmount, - orders.map(o => getExpectedOrderHash(o)), + takerAssetFillAmount.minus(1), ); const tx = txHelper.getResultAndReceiptAsync( testContract.marketSellOrdersFillOrKill, @@ -869,16 +880,16 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { order.salt = ALWAYS_FAILING_SALT; } const signatures = _.times(COUNT, i => createOrderSignature(orders[i])); + const badOrdersAmount = _.reduce(badOrders, (total, o) => o.takerAssetAmount.plus(total), constants.ZERO_AMOUNT); const takerAssetFillAmount = _.reduce( orders, (total, o) => o.takerAssetAmount.plus(total), constants.ZERO_AMOUNT, - ) - .minus(_.reduce(badOrders, (total, o) => o.takerAssetAmount.plus(total), constants.ZERO_AMOUNT)) - .plus(1); - const expectedError = new ExchangeRevertErrors.IncompleteMarketSellError( + ).minus(badOrdersAmount).plus(1); + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteMarketSellOrders, takerAssetFillAmount, - orders.map(o => getExpectedOrderHash(o)), + takerAssetFillAmount.minus(1), ); const tx = txHelper.getResultAndReceiptAsync( testContract.marketSellOrdersFillOrKill, @@ -1166,9 +1177,10 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { (total, o) => o.makerAssetAmount.plus(total), constants.ZERO_AMOUNT, ).plus(1); - const expectedError = new ExchangeRevertErrors.IncompleteMarketBuyError( + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteMarketBuyOrders, makerAssetFillAmount, - orders.map(o => getExpectedOrderHash(o)), + makerAssetFillAmount.minus(1), ); const tx = txHelper.getResultAndReceiptAsync( testContract.marketBuyOrdersFillOrKill, @@ -1218,16 +1230,16 @@ blockchainTests('Exchange wrapper functions unit tests.', env => { order.salt = ALWAYS_FAILING_SALT; } const signatures = _.times(COUNT, i => createOrderSignature(orders[i])); + const badOrdersAmount = _.reduce(badOrders, (total, o) => o.makerAssetAmount.plus(total), constants.ZERO_AMOUNT); const makerAssetFillAmount = _.reduce( orders, (total, o) => o.makerAssetAmount.plus(total), constants.ZERO_AMOUNT, - ) - .minus(_.reduce(badOrders, (total, o) => o.makerAssetAmount.plus(total), constants.ZERO_AMOUNT)) - .plus(1); - const expectedError = new ExchangeRevertErrors.IncompleteMarketBuyError( + ).minus(badOrdersAmount).plus(1); + const expectedError = new ExchangeRevertErrors.IncompleteFillError( + ExchangeRevertErrors.IncompleteFillErrorCode.IncompleteMarketBuyOrders, makerAssetFillAmount, - orders.map(o => getExpectedOrderHash(o)), + makerAssetFillAmount.minus(1), ); const tx = txHelper.getResultAndReceiptAsync( testContract.marketBuyOrdersFillOrKill, diff --git a/contracts/exchange/tsconfig.json b/contracts/exchange/tsconfig.json index ee982946bd..beff07f648 100644 --- a/contracts/exchange/tsconfig.json +++ b/contracts/exchange/tsconfig.json @@ -10,7 +10,6 @@ "generated-artifacts/IEIP1271Wallet.json", "generated-artifacts/IExchange.json", "generated-artifacts/IExchangeCore.json", - "generated-artifacts/IExchangeRichErrors.json", "generated-artifacts/IMatchOrders.json", "generated-artifacts/ISignatureValidator.json", "generated-artifacts/ITransactions.json",