-
Notifications
You must be signed in to change notification settings - Fork 218
feat: show banner for ETH deposits and bump gas #2578
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
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// the 30% gas limit increase should cover the difference | ||
gasLimit: { percentIncrease: BigNumber.from(30) } | ||
gasLimit: { | ||
percentIncrease: BigNumber.from(DEFAULT_GAS_PRICE_PERCENT_INCREASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to be used on gas price, not gas limit, so this ain't right. Looks like we already do the same gas price increase in EthDepositStarter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking if maxFeePerGas
to be bumped, and should we bump it in the sdk instead?
…OffchainLabs/arbitrum-token-bridge into eth-deposit-banner-and-gas-bump
// the 30% gas limit increase should cover the difference | ||
gasLimit: { percentIncrease: BigNumber.from(30) } | ||
gasLimit: { | ||
percentIncrease: BigNumber.from(DEFAULT_GAS_PRICE_PERCENT_INCREASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is supposed to be used on gas price, not gas limit, so this ain't right. Looks like we already do the same gas price increase in EthDepositStarter
.
const hasPendingArbitrumBridgeDeposits = useMemo(() => { | ||
return transactions.some( | ||
tx => | ||
!tx.isWithdrawal && | ||
isTxPending(tx) && | ||
getTransactionType(tx) === 'arbitrum' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just simplify this and show it at all times, and maybe alter the message to include something like "If you have pending deposits..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this, because we already have information overload with different banners in transaction history. I think we should only display them to the relevant users.
No description provided.