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.

Sign up for GitHub

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: use pre-computed gas usage for deposit max amounts #873

Merged
merged 2 commits into from
Oct 17, 2023
Merged

fix: use pre-computed gas usage for deposit max amounts #873

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
de11ae3
fix: use pre-computed gas usage for deposit max amounts
dohaki Oct 16, 2023
f20b1af
review requests
dohaki Oct 16, 2023
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
6 changes: 3 additions & 3 deletions src/utils/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import assert from "assert";
import { ethers, providers } from "ethers";
import { BigNumber, ethers, providers } from "ethers";
import { utils } from "@across-protocol/sdk-v2";
import { CHAIN_IDs, TOKEN_SYMBOLS_MAP } from "@across-protocol/constants-v2";
import * as superstruct from "superstruct";
Expand Down Expand Up @@ -697,5 +697,5 @@ export const walletBlacklist = (process.env.REACT_APP_WALLET_BLACKLIST || "")
.split(",")
.map((address) => address.toLowerCase());

// Fallback gas costs for when the gas estimation fails
export const fallbackEstimatedGasCosts = ethers.utils.parseEther("0.01");
// Pre-computed gas expenditure for deposits used for estimations
export const gasExpenditureDeposit = BigNumber.from(90_000);
55 changes: 8 additions & 47 deletions src/views/Bridge/hooks/useMaxBalance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,8 @@
import { useQuery } from "react-query";
import { BigNumber, providers, constants, utils } from "ethers";
import { BigNumber, constants } from "ethers";

import { useBalanceBySymbol, useConnection } from "hooks";
import {
getConfig,
Route,
max,
getProvider,
fallbackEstimatedGasCosts,
} from "utils";
import { getPaddedGasEstimation } from "utils/transactions";

const config = getConfig();
import { Route, max, getProvider, gasExpenditureDeposit } from "utils";

export function useMaxBalance(selectedRoute: Route) {
const { balance } = useBalanceBySymbol(
Expand All @@ -35,14 +26,9 @@ export function useMaxBalance(selectedRoute: Route) {
selectedRoute.fromTokenSymbol !== "ETH"
? balance
: // For ETH, we need to take the gas costs into account before setting the max. bridgable amount
await estimateGasCostsForDeposit(selectedRoute, signer)
.then((estimatedGasCosts) =>
max(balance.sub(estimatedGasCosts), 0)
)
.catch((err) => {
console.error(err);
return max(balance.sub(fallbackEstimatedGasCosts), 0);
});
await estimateGasCostsForDeposit(selectedRoute).then(
Copy link
Contributor

Choose a reason for hiding this comment

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

ooc, how regularly does this run? I wonder if we want some smoothing logic so that the estimated gas cost doesn't bounce around and cause the deposit amount to flip-flop between being valid and invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default is 5 minutes... But we can easily customize the refresh interval for this value

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow - I'd expected something like 2 - 10 seconds. 5 minutes is actually really long; we might want to revisit that separately, because it might lead to issues if gas becomes volatile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah wait, this actually updates more frequently I noticed. Because it is dependent on the useBalance query which queries the balance on every new block.

(estimatedGasCosts) => max(balance.sub(estimatedGasCosts), 0)
);
} else {
maxBridgeAmount = constants.Zero;
}
Expand All @@ -51,38 +37,13 @@ export function useMaxBalance(selectedRoute: Route) {
},
{
enabled: Boolean(account && balance && signer),
retry: true,
}
);
}

async function estimateGasCostsForDeposit(
selectedRoute: Route,
signer: providers.JsonRpcSigner
) {
async function estimateGasCostsForDeposit(selectedRoute: Route) {
pxrl marked this conversation as resolved.
Show resolved Hide resolved
const provider = getProvider(selectedRoute.fromChain);
const spokePool = config.getSpokePool(selectedRoute.fromChain, signer);
const tokenInfo = config.getTokenInfoByAddress(
selectedRoute.fromChain,
selectedRoute.fromTokenAddress
);
const amount = utils.parseUnits("0.000001", tokenInfo.decimals);
const argsForEstimation = {
recipient: await signer.getAddress(),
originToken: tokenInfo.address,
amount,
destinationChain: selectedRoute.toChain,
relayerFeePct: 0,
quoteTimestamp: BigNumber.from(Math.floor(Date.now() / 1000)).sub(60 * 60),
message: "0x",
maxCount: constants.MaxUint256,
};
const paddedGasEstimation = await getPaddedGasEstimation(
selectedRoute.fromChain,
spokePool,
"deposit",
...Object.values(argsForEstimation),
{ value: selectedRoute.isNative ? amount : 0 }
);
const gasPrice = await provider.getGasPrice();
return gasPrice.mul(paddedGasEstimation);
return gasPrice.mul(gasExpenditureDeposit);
Copy link
Contributor

Choose a reason for hiding this comment

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

getGasPrice() can be unreliable because it's mostly ignorant of the gas pricing strategies of each chain. It's probably fine as an approximation, but should we apply a multiplier? I think something really coarse would be fine - i.e. 3x or so. Given that the deposit cost is so low it will then reserve only a small amount of the native token, and that should be fine by users.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it makes sense to me. Changed here f20b1af

}
Loading