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 crash due to failed estimated gas limit #6055

Merged
merged 9 commits into from
Sep 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
4 changes: 4 additions & 0 deletions src/handlers/web3.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,10 @@ export async function estimateGasWithPadding(
? contractCallEstimateGas(...(callArguments ?? []), txPayloadToEstimate)
: p.estimateGas(cleanTxPayload));

if (!BigNumber.isBigNumber(estimatedGas)) {
throw new Error('Invalid gas limit type');
}

const lastBlockGasLimit = addBuffer(gasLimit.toString(), 0.9);
const paddedGas = addBuffer(estimatedGas.toString(), paddingFactor.toString());
logger.debug('[web3]: ⛽ GAS CALCULATIONS!', {
Expand Down
8 changes: 6 additions & 2 deletions src/raps/actions/crosschainSwap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,13 @@ export const estimateCrosschainSwapGasLimit = async ({
SWAP_GAS_PADDING
);

return gasLimit || getCrosschainSwapDefaultGasLimit(quote);
if (gasLimit === null || gasLimit === undefined || isNaN(Number(gasLimit))) {
return getCrosschainSwapDefaultGasLimit(quote) || getDefaultGasLimitForTrade(quote, chainId);
}

return gasLimit;
} catch (error) {
return getCrosschainSwapDefaultGasLimit(quote);
return getCrosschainSwapDefaultGasLimit(quote) || getDefaultGasLimitForTrade(quote, chainId);
}
};

Expand Down
13 changes: 10 additions & 3 deletions src/raps/actions/swap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,13 @@ export const estimateSwapGasLimit = async ({
WRAP_GAS_PADDING
);

return gasLimit || String(quote?.defaultGasLimit) || String(default_estimate);
if (gasLimit === null || gasLimit === undefined || isNaN(Number(gasLimit))) {
return quote?.defaultGasLimit || default_estimate;
}

return gasLimit;
} catch (e) {
return String(quote?.defaultGasLimit) || String(default_estimate);
return quote?.defaultGasLimit || default_estimate;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my guess was this was causing an issue with the string case on quote?.defaultGasLimit when undefined

}
// Swap
} else {
Expand All @@ -116,8 +120,11 @@ export const estimateSwapGasLimit = async ({
}

const gasLimit = await estimateGasWithPadding(params, method, methodArgs, provider, SWAP_GAS_PADDING);
if (gasLimit === null || gasLimit === undefined || isNaN(Number(gasLimit))) {
return getDefaultGasLimitForTrade(quote, chainId);
}

return gasLimit || getDefaultGasLimitForTrade(quote, chainId);
return gasLimit;
} catch (error) {
return getDefaultGasLimitForTrade(quote, chainId);
}
Expand Down
7 changes: 6 additions & 1 deletion src/raps/actions/unlock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,12 @@ export const estimateApprove = async ({
const gasLimit = await tokenContract.estimateGas.approve(spender, MaxUint256, {
from: owner,
});
return gasLimit ? gasLimit.toString() : `${gasUnits.basic_approval}`;

if (gasLimit === null || gasLimit === undefined || isNaN(Number(gasLimit))) {
return `${gasUnits.basic_approval}`;
}
walmat marked this conversation as resolved.
Show resolved Hide resolved

return gasLimit.toString();
} catch (error) {
logger.error(new RainbowError('[raps/unlock]: error estimateApprove'), {
message: (error as Error)?.message,
Expand Down
11 changes: 8 additions & 3 deletions src/raps/unlockAndCrosschainSwap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,8 @@ export const estimateUnlockAndCrosschainSwap = async ({
});
}

let unlockGasLimit;

if (swapAssetNeedsUnlocking) {
unlockGasLimit = await estimateApprove({
const unlockGasLimit = await estimateApprove({
owner: accountAddress,
tokenAddress: sellTokenAddress,
spender: allowanceTarget,
Expand All @@ -71,7 +69,14 @@ export const estimateUnlockAndCrosschainSwap = async ({
quote,
});

if (swapGasLimit === null || swapGasLimit === undefined || isNaN(Number(swapGasLimit))) {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will bubble up to the estimation and use the basic_swap defaults for the chain

}

const gasLimit = gasLimits.concat(swapGasLimit).reduce((acc, limit) => add(acc, limit), '0');
if (isNaN(Number(gasLimit))) {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}

return gasLimit.toString();
};
Expand Down
14 changes: 8 additions & 6 deletions src/raps/unlockAndSwap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ export const estimateUnlockAndSwap = async ({
let swapAssetNeedsUnlocking = false;

const nativeAsset = isLowerCaseMatch(ETH_ADDRESS_AGGREGATOR, sellTokenAddress) || isNativeAsset(sellTokenAddress, chainId);

if (!isNativeAssetUnwrapping && !nativeAsset) {
swapAssetNeedsUnlocking = await assetNeedsUnlocking({
owner: accountAddress,
Expand All @@ -65,12 +64,8 @@ export const estimateUnlockAndSwap = async ({
if (gasLimitFromMetadata) {
return gasLimitFromMetadata;
}
}

let unlockGasLimit;

if (swapAssetNeedsUnlocking) {
unlockGasLimit = await estimateApprove({
const unlockGasLimit = await estimateApprove({
owner: accountAddress,
tokenAddress: sellTokenAddress,
spender: getRainbowRouterContractAddress(chainId),
Expand All @@ -85,7 +80,14 @@ export const estimateUnlockAndSwap = async ({
quote,
});

if (swapGasLimit === null || swapGasLimit === undefined || isNaN(Number(swapGasLimit))) {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

}

const gasLimit = gasLimits.concat(swapGasLimit).reduce((acc, limit) => add(acc, limit), '0');
if (isNaN(Number(gasLimit))) {
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

}

return gasLimit.toString();
};
Expand Down
Loading