Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: validation gas limit #260

Merged
merged 2 commits into from
Jun 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/services/meta_transaction_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
RFQT_SKIP_BUY_REQUESTS,
} from '../config';
import {
DEFAULT_VALIDATION_GAS_LIMIT,
ONE_GWEI,
ONE_MINUTE_MS,
ONE_SECOND_MS,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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({
Expand Down
46 changes: 32 additions & 14 deletions src/services/swap_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
RFQT_SKIP_BUY_REQUESTS,
} from '../config';
import {
DEFAULT_VALIDATION_GAS_LIMIT,
GAS_LIMIT_BUFFER_MULTIPLIER,
GST2_WALLET_ADDRESSES,
ONE,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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))
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion test/rfqt_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion test/swap_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
{
Expand Down