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: show ARB token on all Orbit chains #1475

Draft
wants to merge 49 commits into
base: master
Choose a base branch
from
Draft

Conversation

fionnachan
Copy link
Member

@fionnachan fionnachan commented Jan 24, 2024

image image

closes FS-327

@cla-bot cla-bot bot added the cla-signed label Jan 24, 2024
Copy link

vercel bot commented Jan 24, 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 Apr 22, 2024 4:13pm

Comment on lines +204 to +205
parentChainId,
childChainId
Copy link
Member Author

Choose a reason for hiding this comment

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

passing chain id fixes the following issue:

user's source chain was an Arbitrum Chain, e.g. Arb One, and destination chain is L3 Orbit
then user switched destination to L1
the L1<->L2 Arb token was not added to bridgeTokens because the chain ids in useArbTokenBridge was still the old pair (Arb One & L3 Orbit) instead of L1 & L2

Comment on lines +250 to +252
if (isArbitrumToken) {
return true
}
Copy link
Member Author

Choose a reason for hiding this comment

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

never show the "Import" text for Arb token

Comment on lines 116 to 133
const [networks] = useNetworks()
const { childChainProvider, parentChainProvider } =
useNetworksRelationship(networks)

const { addPendingTransaction } = useTransactionHistory(walletAddress)

const {
eth: [, updateEthL1Balance],
erc20: [, updateErc20L1Balance]
} = useBalance({
provider: l1.provider,
provider: parentChainProvider,
walletAddress
})
const {
eth: [, updateEthL2Balance],
erc20: [, updateErc20L2Balance]
} = useBalance({
provider: l2.provider,
provider: childChainProvider,
Copy link
Member Author

Choose a reason for hiding this comment

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

l1 & l2 from params are stale so had to do this after switching chain

might need to check if that causes issues with transfers

Copy link
Contributor

@brtkx brtkx Jan 25, 2024

Choose a reason for hiding this comment

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

We should probably use that across the entire hook and get rid of l1/l2 in params

Copy link
Member Author

Choose a reason for hiding this comment

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

100%
@chrstph-dvx is working on that 😁

Comment on lines -27 to -29
if (!walletAddress) {
return
}
Copy link
Member Author

Choose a reason for hiding this comment

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

didn't use it at all

Comment on lines 116 to 133
const [networks] = useNetworks()
const { childChainProvider, parentChainProvider } =
useNetworksRelationship(networks)

const { addPendingTransaction } = useTransactionHistory(walletAddress)

const {
eth: [, updateEthL1Balance],
erc20: [, updateErc20L1Balance]
} = useBalance({
provider: l1.provider,
provider: parentChainProvider,
walletAddress
})
const {
eth: [, updateEthL2Balance],
erc20: [, updateErc20L2Balance]
} = useBalance({
provider: l2.provider,
provider: childChainProvider,
Copy link
Contributor

@brtkx brtkx Jan 25, 2024

Choose a reason for hiding this comment

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

We should probably use that across the entire hook and get rid of l1/l2 in params

const isArbitrumTokenAndIsChildChainOrbit =
isArbitrumTokenList(listId) && isChildChainOrbit
const parentChainTokenAddress = isArbitrumTokenAndIsChildChainOrbit
? tokenData.address.toLowerCase()
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
? tokenData.address.toLowerCase()
? address.toLowerCase()

chrstph-dvx
chrstph-dvx previously approved these changes Mar 19, 2024
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.

None yet

3 participants