fix: validation gas limit (#260)

* Revert "fix: Disable quote validation temporarily (#259)"

This reverts commit 6064f3e.

* fix: Provide default gas estimate for validation calls

src/constants.ts

@@ -17,6 +17,7 @@ export const DEFAULT_LOGGER_INCLUDE_TIMESTAMP = true;
 export const ONE_SECOND_MS = 1000;
 export const ONE_MINUTE_MS = ONE_SECOND_MS * 60;
 export const TEN_MINUTES_MS = ONE_MINUTE_MS * 10;
+export const DEFAULT_VALIDATION_GAS_LIMIT = 10e6;
 
 // The number of orders to post to Mesh at one time
 export const MESH_ORDERS_BATCH_SIZE = 200;

src/services/meta_transaction_service.ts

@@ -21,6 +21,7 @@ import {
     RFQT_SKIP_BUY_REQUESTS,
 } from '../config';
 import {
+    DEFAULT_VALIDATION_GAS_LIMIT,
     ONE_GWEI,
     ONE_MINUTE_MS,
     ONE_SECOND_MS,
@@ -265,6 +266,7 @@ export class MetaTransactionService {
                 from: PUBLIC_ADDRESS_FOR_ETH_CALLS,
                 gasPrice,
                 value: protocolFee,
+                gas: DEFAULT_VALIDATION_GAS_LIMIT,
             });
         } catch (err) {
             // we reach into the underlying revert and throw it instead of
@@ -296,7 +298,6 @@ export class MetaTransactionService {
         protocolFee: BigNumber,
     ): Promise<PartialTxParams> {
         const gasPrice = zeroExTransaction.gasPrice;
-        // TODO(dekz): our pattern is to eth_call and estimateGas in parallel and return the result of eth_call validations
         const gas = await this._contractWrappers.exchange
             .executeTransaction(zeroExTransaction, signature)
             .estimateGasAsync({

src/services/swap_service.ts

@@ -24,6 +24,7 @@ import {
     RFQT_SKIP_BUY_REQUESTS,
 } from '../config';
 import {
+    DEFAULT_VALIDATION_GAS_LIMIT,
     GAS_LIMIT_BUFFER_MULTIPLIER,
     GST2_WALLET_ADDRESSES,
     ONE,
@@ -86,7 +87,16 @@ export class SwapService {
     }
 
     public async calculateSwapQuoteAsync(params: CalculateSwapQuoteParams): Promise<GetSwapQuoteResponse> {
-        const { buyAmount, buyTokenAddress, sellTokenAddress, isETHSell, from, affiliateAddress } = params;
+        const {
+            buyAmount,
+            buyTokenAddress,
+            sellTokenAddress,
+            isETHSell,
+            from,
+            affiliateAddress,
+            // tslint:disable-next-line:boolean-naming
+            skipValidation,
+        } = params;
         const swapQuote = await this._getMarketBuyOrSellQuoteAsync(params);
 
         const attributedSwapQuote = serviceUtils.attributeSwapQuoteOrders(swapQuote);
@@ -115,9 +125,7 @@ export class SwapService {
         );
 
         let conservativeBestCaseGasEstimate = new BigNumber(worstCaseGas).plus(gasTokenGasCost);
-        // Temporarily disable validation
-        // if (!skipValidation && from) {
-        if (false) {
+        if (!skipValidation && from) {
             // Force a revert error if the takerAddress does not have enough ETH.
             const txDataValue = isETHSell
                 ? BigNumber.min(value, await this._web3Wrapper.getBalanceInWeiAsync(from))
@@ -135,7 +143,10 @@ export class SwapService {
         // Add a buffer to get the worst case gas estimate
         const worstCaseGasEstimate = conservativeBestCaseGasEstimate.times(GAS_LIMIT_BUFFER_MULTIPLIER).integerValue();
         // Cap the refund at 50% our best estimate
-        const estimatedGasTokenRefund = BigNumber.min(conservativeBestCaseGasEstimate.div(2), gasTokenRefund);
+        const estimatedGasTokenRefund = BigNumber.min(
+            conservativeBestCaseGasEstimate.div(2),
+            gasTokenRefund,
+        ).decimalPlaces(0);
         const { price, guaranteedPrice } = await this._getSwapQuotePriceAsync(
             buyAmount,
             buyTokenAddress,
@@ -275,12 +286,8 @@ export class SwapService {
     }
 
     private async _estimateGasOrThrowRevertErrorAsync(txData: Partial<TxData>): Promise<BigNumber> {
-        // Perform this concurrently
-        // if the call fails the gas estimation will also fail, we can throw a more helpful
-        // error message than gas estimation failure
-        const estimateGasPromise = this._web3Wrapper.estimateGasAsync(txData).catch(_e => 0);
-        await this._throwIfCallIsRevertErrorAsync(txData);
-        const gas = await estimateGasPromise;
+        const gas = await this._web3Wrapper.estimateGasAsync(txData).catch(_e => DEFAULT_VALIDATION_GAS_LIMIT);
+        await this._throwIfCallIsRevertErrorAsync({ ...txData, gas });
         return new BigNumber(gas);
     }
 
@@ -291,9 +298,20 @@ export class SwapService {
             callResult = await this._web3Wrapper.callAsync(txData);
         } catch (e) {
             // RPCSubprovider can throw if .error exists on the response payload
-            // This `error` response occurs from Parity nodes (incl Alchemy) but not on INFURA (geth)
-            revertError = decodeThrownErrorAsRevertError(e);
-            throw revertError;
+            // This `error` response occurs from Parity nodes (incl Alchemy) and Geth nodes >= 1.9.14
+            // Geth 1.9.15
+            if (e.message && /execution reverted/.test(e.message) && e.data) {
+                try {
+                    revertError = RevertError.decode(e.data, false);
+                } catch (e) {
+                    // No revert error
+                }
+            } else {
+                revertError = decodeThrownErrorAsRevertError(e);
+            }
+            if (revertError) {
+                throw revertError;
+            }
         }
         try {
             revertError = RevertError.decode(callResult, false);

test/rfqt_test.ts

@@ -285,7 +285,7 @@ describe(SUITE_NAME, () => {
                     },
                 );
             });
-            it.skip('should fail validation when taker can not actually fill', async () => {
+            it('should fail validation when taker can not actually fill', async () => {
                 const wethContract = new WETH9Contract(contractAddresses.etherToken, provider);
                 await wethContract
                     .approve(contractAddresses.erc20Proxy, new BigNumber(0))

test/swap_test.ts

@@ -217,7 +217,7 @@ describe(SUITE_NAME, () => {
                 },
             );
         });
-        it.skip('should throw a validation error if takerAddress cannot complete the quote', async () => {
+        it('should throw a validation error if takerAddress cannot complete the quote', async () => {
             // The taker does not have an allowance
             await quoteAndExpectAsync(
                 {