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

feat: check custom gateway tokens withdrawal approval #2144

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

fionnachan
Copy link
Member

@fionnachan fionnachan commented Dec 16, 2024

This covers USDC custom custom gateway (USDC bridge) but also other custom & custom custom gateway tokens.

This PR removes the hardcoded list we maintained for tokens that require approval at withdrawal.

Closes FS-973

Copy link

vercel bot commented Dec 16, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
arbitrum-token-bridge ✅ Ready (Inspect) Visit Preview Jan 14, 2025 6:37pm

@cla-bot cla-bot bot added the cla-signed label Dec 16, 2024
@fionnachan fionnachan changed the title feat: check custom gateway tokens for withdrawal approval feat: check custom gateway tokens withdrawal approval Dec 16, 2024
Copy link
Member Author

Choose a reason for hiding this comment

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

Tested that after removal, the approval dialog still shows up for withdrawals with https://sepolia.arbiscan.io/token/0xdEFB0b365C718D1F9677E6232494B927c91284B6 (an xErc20 token on Sepolia)

gzeoneth
gzeoneth previously approved these changes Jan 2, 2025
Copy link
Member

@gzeoneth gzeoneth left a comment

Choose a reason for hiding this comment

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

logic lgtm

Comment on lines 90 to 92
* Most tokens inherently allows the token gateway to burn whichever amount
* on the child chain for withdrawal because they inherited the
* IArbToken interface that allows the gateway to burn without allowance approval
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Most tokens inherently allows the token gateway to burn whichever amount
* on the child chain for withdrawal because they inherited the
* IArbToken interface that allows the gateway to burn without allowance approval
* Most tokens inherently allow the token gateway to burn the withdrawal amount
* on the child chain because they inherit the
* IArbToken interface that allows the gateway to burn without allowance approval


return allowance.lt(amount)
// tokens that use the standard gateways do not require approval on child chain
const standardGateways = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simply check if the token contract has a valid and working bridgeBurn method to validate the same without needing to fetch the gateway address and checking standardGatways and introducing isError in gas estimation?

Copy link
Member Author

Choose a reason for hiding this comment

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

we need the gateway address for getting allowanceForSourceChainGateway anyway

we can do an eth_call (provider.call) to check if the bridgeBurn method reverts with a message (means the method exists), but if the token contract implemented the method to revert without a message, then we might wrongly think that the method doesn't exist when it does

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants