fix: missing gas padding on native gas token deposits #849
Conversation
ACX-1546 Missing FE gas padding on native/gas token deposits
The FE used to impose a "gas buffer", such that a user was not able to deposit the "max" amount of their native token. This recognised that some residual amount would be needed for gas. The FE currently permits the user to max-out their deposit amount, leaving 0 for gas, and produces a deposit transaction that can not be submitted. This can be seen in the screenshot below, where the dev wallet (0x9a8…) has a balance of 0.16498… ETH, and that amount is auto-filled when MAX is selected. |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dohaki
requested review from
nicholaspai,
amateima,
james-a-morris and
pxrl
…-nativegas-token-deposits
…-nativegas-token-deposits
| const gasEstimation = await contract.estimateGas[method](...args); | ||
| // Factor in the padding | ||
| const gasToRecommend = gasEstimation | ||
| .mul(parseEther(String(gasMultiplier))) |
There was a problem hiding this comment.
Could we precompute this in the constants? We could theoretically define
export const gasMultiplierPerChain: Record<string, number> = process.env
.REACT_APP_GAS_ESTIMATION_MULTIPLIER_PER_CHAIN
? JSON.parse(process.env.REACT_APP_GAS_ESTIMATION_MULTIPLIER_PER_CHAIN).map((mul) => parseEther(String(mul)))
: {};
in constants so that we only call parseEther once.
There was a problem hiding this comment.
Yea, we could. But I am not sure if this makes things more predictable or intuitive tbh. So if I import this constant somewhere, I would not expect this to be a BigNumber.
| ) { | ||
| const provider = getProvider(selectedRoute.fromChain); | ||
| const spokePool = config.getSpokePool(selectedRoute.fromChain); | ||
| const mockedArgs = { |
There was a problem hiding this comment.
Nit: should we call this mocked? I feel like that term is usually reserved for tests.
There was a problem hiding this comment.
Hmm, yea feel free to provide an alternative name :)
There was a problem hiding this comment.
Maybe: argsForEstimation
| const gasEstimation = await contract.estimateGas[method](...args); | ||
| // Factor in the padding | ||
| const gasToRecommend = gasEstimation | ||
| .mul(parseEther(String(gasMultiplier))) |
| ) | ||
| .catch((err) => { | ||
| console.error(err); | ||
| return max(balance.sub(utils.parseEther("0.01")), 0); |
There was a problem hiding this comment.
Should we encode this somewhere as a constant? This way, we can modify 0.01 without having to grep for it?
There was a problem hiding this comment.
Good point! Will change
| ) { | ||
| const provider = getProvider(selectedRoute.fromChain); | ||
| const spokePool = config.getSpokePool(selectedRoute.fromChain); | ||
| const mockedArgs = { |
There was a problem hiding this comment.
Maybe: argsForEstimation
| return max(balance.sub(utils.parseEther("0.01")), 0); | ||
| }); | ||
| } else { | ||
| maxBridgeAmount = BigNumber.from(0); |
There was a problem hiding this comment.
You can save a little bit by using ethers.contracts.Zero for this. We also export it as bnZero from the sdk: https://github.com/across-protocol/sdk-v2/blob/5a69b7ce77f0b2e75956786308156b657d7b88e2/src/utils/BigNumberUtils.ts#L12
There was a problem hiding this comment.
One high-level comment - we also have gas padding that occurs in the API for estimating the fill cost of the relayer, and there's some potential for them to be confused. The main hint here is that this new env var is prefixed with REACT_APP.
Not sure exactly what to do about it, but I just wanted to note it.
@pxrl Thanks for flagging. Yea I also noticed that. You are referring to |
…-nativegas-token-deposits
dohaki
deleted the
dong-ha/acx-1546-missing-fe-gas-padding-on-nativegas-token-deposits
branch
This PR does two main things:
gasMultiplierfor a single chain withgasMultiplierPerChainwhich is a mappingchainId -> gasMultiplier. This allows setting different buffers for different chains.