From 89117beda26ae400fe12ed5a157aac405d872049 Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Tue, 18 Aug 2020 13:10:51 -0700 Subject: [PATCH 1/3] `@0x/utils`: Add support for nested rich revert decoding --- packages/utils/src/revert_error.ts | 14 ++++++++++++++ packages/utils/test/revert_error_test.ts | 13 +++++++++++++ 2 files changed, 27 insertions(+) diff --git a/packages/utils/src/revert_error.ts b/packages/utils/src/revert_error.ts index 04051f03a3..3fa2c6213b 100644 --- a/packages/utils/src/revert_error.ts +++ b/packages/utils/src/revert_error.ts @@ -14,6 +14,7 @@ type ArgTypes = | BigNumber | number | boolean + | RevertError | BigNumber[] | string[] | number[] @@ -122,6 +123,19 @@ export abstract class RevertError extends Error { const instance = new type(); try { const values = decoder(_bytes); + _.transform( + values, + (result, value, key) => { + const { type: argType } = instance._getArgumentByName(key); + if (argType === 'bytes') { + try { + const nestedRevert = RevertError.decode(value as string, coerce); + result[key] = nestedRevert.toString(); + } catch (err) {} // tslint:disable-line:no-empty + } + }, + values, + ); _.assign(instance, { values }); instance.message = instance.toString(); return instance; diff --git a/packages/utils/test/revert_error_test.ts b/packages/utils/test/revert_error_test.ts index 2a2f47e37b..b6618e7651 100644 --- a/packages/utils/test/revert_error_test.ts +++ b/packages/utils/test/revert_error_test.ts @@ -39,7 +39,14 @@ class FixedSizeArrayRevertError extends RevertError { } } +class ParentRevertError extends RevertError { + public constructor(nestedError?: string) { + super('ParentRevertError', 'ParentRevertError(bytes nestedError)', { nestedError }); + } +} + RevertError.registerType(CustomRevertError); +RevertError.registerType(ParentRevertError); describe('RevertError', () => { describe('equality', () => { @@ -159,6 +166,12 @@ describe('RevertError', () => { const decode = () => RevertError.decode(_encoded); expect(decode).to.throw(); }); + it('should decode a nested revert error', () => { + const nested = new StringRevertError(message); + const parent = new ParentRevertError(nested.encode()); + const decoded = RevertError.decode(parent.encode()); + expect(decoded.toString()).to.equal(new ParentRevertError(nested.toString()).toString()); + }); }); describe('getThrownErrorRevertErrorBytes', () => { it('should decode Parity revert errors', () => { From e078f3d4799c571b1d9f85f7e966860a153e772d Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Tue, 18 Aug 2020 16:56:38 -0700 Subject: [PATCH 2/3] Appease CI --- contracts/exchange/test/dispatcher.ts | 8 ++++---- contracts/exchange/test/signature_validator.ts | 10 +++++----- .../exchange/test/transactions_unit_tests.ts | 16 ++++++++-------- .../integrations/test/exchange/core_test.ts | 12 ++++++------ .../test/exchange/fill_dydx_order_test.ts | 6 +++--- .../exchange/transaction_protocol_fee_test.ts | 4 ++-- .../test/exchange/transaction_test.ts | 10 +++++----- .../test/stop-limit/chainlink_stop_limit_test.ts | 4 ++-- .../test/features/meta_transactions_test.ts | 4 ++-- contracts/zero-ex/test/features/ownable_test.ts | 2 +- .../zero-ex/test/features/token_spender_test.ts | 2 +- contracts/zero-ex/test/flash_wallet_test.ts | 4 ++-- 12 files changed, 41 insertions(+), 41 deletions(-) diff --git a/contracts/exchange/test/dispatcher.ts b/contracts/exchange/test/dispatcher.ts index ef9f58d528..7d36aca270 100644 --- a/contracts/exchange/test/dispatcher.ts +++ b/contracts/exchange/test/dispatcher.ts @@ -286,11 +286,11 @@ describe('AssetProxyDispatcher', () => { }); const encodedAssetData = encodeERC20AssetData(erc20TokenA.address); const amount = new BigNumber(1); - const nestedError = new StringRevertError(RevertReason.TransferFailed).encode(); + const nestedError = new StringRevertError(RevertReason.TransferFailed); const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHash, encodedAssetData, - nestedError, + nestedError.toString(), ); const tx = assetProxyDispatcher .dispatchTransferFrom(orderHash, encodedAssetData, makerAddress, takerAddress, amount) @@ -309,11 +309,11 @@ describe('AssetProxyDispatcher', () => { from: makerAddress, }); const transferIndexAsBytes32 = '0x0000000000000000000000000000000000000000000000000000000000000001'; - const nestedError = new StringRevertError(RevertReason.TransferFailed).encode(); + const nestedError = new StringRevertError(RevertReason.TransferFailed); const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( transferIndexAsBytes32, assetDataB, - nestedError, + nestedError.toString(), ); const tx = assetProxyDispatcher .simulateDispatchTransferFromCalls( diff --git a/contracts/exchange/test/signature_validator.ts b/contracts/exchange/test/signature_validator.ts index 07d66760d2..8dce19f20c 100644 --- a/contracts/exchange/test/signature_validator.ts +++ b/contracts/exchange/test/signature_validator.ts @@ -300,7 +300,7 @@ blockchainTests.resets('MixinSignatureValidator', env => { hashHex, validatorWallet.address, signatureHex, - new StringRevertError(validatorWalletRevertReason).encode(), + new StringRevertError(validatorWalletRevertReason).toString(), ); const tx = validateAsync(hashHex, validatorWallet.address, signatureHex, ValidatorWalletAction.Revert); return expect(tx).to.revertWith(expectedError); @@ -562,7 +562,7 @@ blockchainTests.resets('MixinSignatureValidator', env => { validatorWallet.address, data, signatureHex, - new StringRevertError(validatorWalletRevertReason).encode(), + new StringRevertError(validatorWalletRevertReason).toString(), ); const tx = validateAsync(signedOrder, signatureHex, ValidatorWalletAction.Revert); return expect(tx).to.revertWith(expectedError); @@ -693,7 +693,7 @@ blockchainTests.resets('MixinSignatureValidator', env => { validatorWallet.address, data, signatureHex, - new StringRevertError(validatorWalletRevertReason).encode(), + new StringRevertError(validatorWalletRevertReason).toString(), ); const tx = validateAsync(signedOrder, signatureHex, ValidatorWalletAction.Revert); return expect(tx).to.revertWith(expectedError); @@ -916,7 +916,7 @@ blockchainTests.resets('MixinSignatureValidator', env => { validatorWallet.address, data, signatureHex, - new StringRevertError(validatorWalletRevertReason).encode(), + new StringRevertError(validatorWalletRevertReason).toString(), ); const tx = validateAsync(signedTransaction, signatureHex, ValidatorWalletAction.Revert); return expect(tx).to.revertWith(expectedError); @@ -1041,7 +1041,7 @@ blockchainTests.resets('MixinSignatureValidator', env => { validatorWallet.address, data, signatureHex, - new StringRevertError(validatorWalletRevertReason).encode(), + new StringRevertError(validatorWalletRevertReason).toString(), ); const tx = validateAsync(signedTransaction, signatureHex, ValidatorWalletAction.Revert); return expect(tx).to.revertWith(expectedError); diff --git a/contracts/exchange/test/transactions_unit_tests.ts b/contracts/exchange/test/transactions_unit_tests.ts index a237bf3853..318e9c472d 100644 --- a/contracts/exchange/test/transactions_unit_tests.ts +++ b/contracts/exchange/test/transactions_unit_tests.ts @@ -101,7 +101,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - executableError.encode(), + executableError.toString(), ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. @@ -123,7 +123,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - executableError.encode(), + executableError.toString(), ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. @@ -145,7 +145,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - executableError.encode(), + executableError.toString(), ); // Call the `batchExecuteTransactions()` function and ensure that it reverts with the expected revert error. @@ -280,7 +280,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const outerExecuteTransactionHash = transactionHashUtils.getTransactionHashHex(outerExecuteTransaction); const outerExpectedError = new ExchangeRevertErrors.TransactionExecutionError( outerExecuteTransactionHash, - innerExpectedError.encode(), + innerExpectedError.toString(), ); const tx = transactionsContract .batchExecuteTransactions([outerExecuteTransaction], [randomSignature()]) @@ -363,7 +363,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const errorData = new ExchangeRevertErrors.TransactionInvalidContextError( innerTransactionHash, accounts[0], - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData); const tx = transactionsContract .executeTransaction(outerTransaction, validSignature) @@ -385,7 +385,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const errorData = new ExchangeRevertErrors.TransactionInvalidContextError( innerTransactionHash, accounts[0], - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError(outerTransactionHash, errorData); const tx = transactionsContract .executeTransaction(outerTransaction, validSignature) @@ -466,7 +466,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - executableError.encode(), + executableError.toString(), ); const tx = transactionsContract .executeTransaction(transaction, randomSignature()) @@ -486,7 +486,7 @@ blockchainTests.resets('Transaction Unit Tests', ({ provider, web3Wrapper, txDef const executableError = new StringRevertError('EXECUTABLE_FAILED'); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHash, - executableError.encode(), + executableError.toString(), ); const tx = transactionsContract .executeTransaction(transaction, validSignature) diff --git a/contracts/integrations/test/exchange/core_test.ts b/contracts/integrations/test/exchange/core_test.ts index 5bdfa76ec7..0dda0fde5c 100644 --- a/contracts/integrations/test/exchange/core_test.ts +++ b/contracts/integrations/test/exchange/core_test.ts @@ -558,7 +558,7 @@ blockchainTests.resets('Exchange core', () => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashHex, signedOrder.makerAssetData, - new StringRevertError(RevertReason.TransferFailed).encode(), + new StringRevertError(RevertReason.TransferFailed).toString(), ); const tx = exchange .fillOrder(signedOrder, takerAssetFillAmount, signedOrder.signature) @@ -587,7 +587,7 @@ blockchainTests.resets('Exchange core', () => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashHex, signedOrder.takerAssetData, - new StringRevertError(RevertReason.TransferFailed).encode(), + new StringRevertError(RevertReason.TransferFailed).toString(), ); const tx = exchange .fillOrder(signedOrder, takerAssetFillAmount, signedOrder.signature) @@ -616,7 +616,7 @@ blockchainTests.resets('Exchange core', () => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashHex, signedOrder.makerAssetData, - new StringRevertError(RevertReason.InvalidAmount).encode(), + new StringRevertError(RevertReason.InvalidAmount).toString(), ); const tx = exchange .fillOrder(signedOrder, takerAssetFillAmount, signedOrder.signature) @@ -645,7 +645,7 @@ blockchainTests.resets('Exchange core', () => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashHex, signedOrder.takerAssetData, - new StringRevertError(RevertReason.InvalidAmount).encode(), + new StringRevertError(RevertReason.InvalidAmount).toString(), ); const tx = exchange .fillOrder(signedOrder, takerAssetFillAmount, signedOrder.signature) @@ -980,7 +980,7 @@ blockchainTests.resets('Exchange core', () => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashHex, assetData, - new StringRevertError(RevertReason.TargetNotEven).encode(), + new StringRevertError(RevertReason.TargetNotEven).toString(), ); const tx = exchange .fillOrder(signedOrder, signedOrder.takerAssetAmount, signedOrder.signature) @@ -1015,7 +1015,7 @@ blockchainTests.resets('Exchange core', () => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashHex, assetData, - new StringRevertError(RevertReason.TargetNotEven).encode(), + new StringRevertError(RevertReason.TargetNotEven).toString(), ); const tx = exchange .fillOrder(signedOrder, signedOrder.takerAssetAmount, signedOrder.signature) diff --git a/contracts/integrations/test/exchange/fill_dydx_order_test.ts b/contracts/integrations/test/exchange/fill_dydx_order_test.ts index 5b410a906f..6cfa83c34f 100644 --- a/contracts/integrations/test/exchange/fill_dydx_order_test.ts +++ b/contracts/integrations/test/exchange/fill_dydx_order_test.ts @@ -9,7 +9,7 @@ import { DummyERC20TokenContract } from '@0x/contracts-erc20'; import { LibMathRevertErrors } from '@0x/contracts-exchange-libs'; import { blockchainTests, constants, describe, expect, toBaseUnitAmount } from '@0x/contracts-test-utils'; import { SignedOrder } from '@0x/order-utils'; -import { BigNumber, RevertError } from '@0x/utils'; +import { BigNumber, decodeThrownErrorAsRevertError, StringRevertError } from '@0x/utils'; import { DecodedLogArgs, LogWithDecodedArgs } from 'ethereum-types'; import * as _ from 'lodash'; @@ -276,9 +276,9 @@ blockchainTests.resets('Exchange fills dydx orders', env => { try { await txPromise.catch(); } catch (e) { - assetProxyError = RevertError.decode(e.values.errorData); + assetProxyError = decodeThrownErrorAsRevertError(e).values.errorData; } - expect(assetProxyError).to.deep.equal(expectedAssetProxyError); + expect(assetProxyError).to.deep.equal(new StringRevertError(expectedAssetProxyError.toString())); }); }); }); diff --git a/contracts/integrations/test/exchange/transaction_protocol_fee_test.ts b/contracts/integrations/test/exchange/transaction_protocol_fee_test.ts index 09c794a73e..c4c1097252 100644 --- a/contracts/integrations/test/exchange/transaction_protocol_fee_test.ts +++ b/contracts/integrations/test/exchange/transaction_protocol_fee_test.ts @@ -121,7 +121,7 @@ blockchainTests.resets('Transaction <> protocol fee integration tests', env => { maker.address, wethless.address, '0x', - ).encode(); + ).toString(); return new ExchangeRevertErrors.TransactionExecutionError( transactionHashUtils.getTransactionHashHex(failedTransaction), nestedError, @@ -252,7 +252,7 @@ blockchainTests.resets('Transaction <> protocol fee integration tests', env => { .awaitTransactionSuccessAsync({ from: alice.address, value: REFUND_AMOUNT }); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHashUtils.getTransactionHashHex(recursiveTransaction), - protocolFeeError(order, transaction).encode(), + protocolFeeError(order, transaction).toString(), ); return expect(tx).to.revertWith(expectedError); }); diff --git a/contracts/integrations/test/exchange/transaction_test.ts b/contracts/integrations/test/exchange/transaction_test.ts index 749ea812e3..77266aea7a 100644 --- a/contracts/integrations/test/exchange/transaction_test.ts +++ b/contracts/integrations/test/exchange/transaction_test.ts @@ -280,7 +280,7 @@ blockchainTests.resets('Transaction integration tests', env => { const noReentrancyError = new ExchangeRevertErrors.TransactionInvalidContextError( transactionHashHex, transaction.signerAddress, - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( recursiveTransactionHashHex, noReentrancyError, @@ -330,7 +330,7 @@ blockchainTests.resets('Transaction integration tests', env => { orderHashUtils.getOrderHashHex(order), order.makerAddress, order.signature, - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHashHex, nestedError, @@ -353,7 +353,7 @@ blockchainTests.resets('Transaction integration tests', env => { ExchangeRevertErrors.ExchangeContextErrorCodes.InvalidMaker, orderHashUtils.getOrderHashHex(order), takers[0].address, - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHashHex, nestedError, @@ -403,7 +403,7 @@ blockchainTests.resets('Transaction integration tests', env => { ExchangeRevertErrors.ExchangeContextErrorCodes.InvalidMaker, orderHashUtils.getOrderHashHex(orders[0]), takers[0].address, - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHashHex, nestedError, @@ -771,7 +771,7 @@ blockchainTests.resets('Transaction integration tests', env => { const nestedError = new ExchangeRevertErrors.OrderStatusError( orderHashUtils.getOrderHashHex(order), OrderStatus.Cancelled, - ).encode(); + ).toString(); const expectedError = new ExchangeRevertErrors.TransactionExecutionError( transactionHashUtils.getTransactionHashHex(transaction2), nestedError, diff --git a/contracts/integrations/test/stop-limit/chainlink_stop_limit_test.ts b/contracts/integrations/test/stop-limit/chainlink_stop_limit_test.ts index cb3928fddf..a76a1e46e3 100644 --- a/contracts/integrations/test/stop-limit/chainlink_stop_limit_test.ts +++ b/contracts/integrations/test/stop-limit/chainlink_stop_limit_test.ts @@ -123,7 +123,7 @@ blockchainTests.resets('Chainlink stop-limit order tests', env => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashUtils.getOrderHashHex(order), order.makerAssetData, - new StringRevertError('ChainlinkStopLimit/OUT_OF_PRICE_RANGE').encode(), + new StringRevertError('ChainlinkStopLimit/OUT_OF_PRICE_RANGE').toString(), ); return expect(tx).to.revertWith(expectedError); }); @@ -133,7 +133,7 @@ blockchainTests.resets('Chainlink stop-limit order tests', env => { const expectedError = new ExchangeRevertErrors.AssetProxyTransferError( orderHashUtils.getOrderHashHex(order), order.makerAssetData, - new StringRevertError('ChainlinkStopLimit/OUT_OF_PRICE_RANGE').encode(), + new StringRevertError('ChainlinkStopLimit/OUT_OF_PRICE_RANGE').toString(), ); return expect(tx).to.revertWith(expectedError); }); diff --git a/contracts/zero-ex/test/features/meta_transactions_test.ts b/contracts/zero-ex/test/features/meta_transactions_test.ts index f5f30fda7f..943da28a5a 100644 --- a/contracts/zero-ex/test/features/meta_transactions_test.ts +++ b/contracts/zero-ex/test/features/meta_transactions_test.ts @@ -297,7 +297,7 @@ blockchainTests.resets('MetaTransactions feature', env => { new ZeroExRevertErrors.MetaTransactions.MetaTransactionCallFailedError( mtxHash, actualCallData, - new StringRevertError('FAIL').encode(), + new StringRevertError('FAIL').toString(), ), ); }); @@ -465,7 +465,7 @@ blockchainTests.resets('MetaTransactions feature', env => { mtxHash, signers[0], signature, - ).encode(), + ).toString(), ), ); }); diff --git a/contracts/zero-ex/test/features/ownable_test.ts b/contracts/zero-ex/test/features/ownable_test.ts index 428e6f0c9b..4c4f4d1708 100644 --- a/contracts/zero-ex/test/features/ownable_test.ts +++ b/contracts/zero-ex/test/features/ownable_test.ts @@ -102,7 +102,7 @@ blockchainTests.resets('Ownable feature', env => { return expect(tx).to.revertWith( new ZeroExRevertErrors.Ownable.MigrateCallFailedError( testMigrator.address, - new StringRevertError('OOPSIE').encode(), + new StringRevertError('OOPSIE').toString(), ), ); }); diff --git a/contracts/zero-ex/test/features/token_spender_test.ts b/contracts/zero-ex/test/features/token_spender_test.ts index dcb6592cde..825ed49478 100644 --- a/contracts/zero-ex/test/features/token_spender_test.ts +++ b/contracts/zero-ex/test/features/token_spender_test.ts @@ -98,7 +98,7 @@ blockchainTests.resets('TokenSpender feature', env => { tokenFrom, tokenTo, tokenAmount, - new StringRevertError('TestTokenSpenderERC20Token/Revert').encode(), + new StringRevertError('TestTokenSpenderERC20Token/Revert').toString(), ); return expect(tx).to.revertWith(expectedError); }); diff --git a/contracts/zero-ex/test/flash_wallet_test.ts b/contracts/zero-ex/test/flash_wallet_test.ts index 97df5cf51e..51079aeaee 100644 --- a/contracts/zero-ex/test/flash_wallet_test.ts +++ b/contracts/zero-ex/test/flash_wallet_test.ts @@ -127,7 +127,7 @@ blockchainTests.resets('FlashWallet', env => { callTarget.address, REVERTING_DATA, constants.ZERO_AMOUNT, - new StringRevertError('TestCallTarget/REVERT').encode(), + new StringRevertError('TestCallTarget/REVERT').toString(), ), ); }); @@ -203,7 +203,7 @@ blockchainTests.resets('FlashWallet', env => { wallet.address, callTarget.address, REVERTING_DATA, - new StringRevertError('TestCallTarget/REVERT').encode(), + new StringRevertError('TestCallTarget/REVERT').toString(), ), ); }); From 270d7a3f196a5864692f3d7af8a6784945e548b7 Mon Sep 17 00:00:00 2001 From: Michael Zhu Date: Wed, 19 Aug 2020 10:11:25 -0700 Subject: [PATCH 3/3] Update changelog --- packages/utils/CHANGELOG.json | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/utils/CHANGELOG.json b/packages/utils/CHANGELOG.json index d7e95fd53c..1fa4f2738f 100644 --- a/packages/utils/CHANGELOG.json +++ b/packages/utils/CHANGELOG.json @@ -1,4 +1,13 @@ [ + { + "version": "5.6.0", + "changes": [ + { + "note": "Added support for nested rich revert decoding", + "pr": 2668 + } + ] + }, { "timestamp": 1594788383, "version": "5.5.1",