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: Add new routes components #2279

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

Conversation

chrstph-dvx
Copy link
Contributor

@chrstph-dvx chrstph-dvx commented Feb 24, 2025

BREAKING CHANGES: Removed TransferPanelSummary

Summary

Steps to test

image

@chrstph-dvx chrstph-dvx requested review from dewanshparashar and brtkx and removed request for dewanshparashar February 24, 2025 16:13
@cla-bot cla-bot bot added the cla-signed label Feb 24, 2025
Copy link

vercel bot commented Feb 24, 2025

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

Name Status Preview Updated (UTC)
arbitrum-token-bridge ✅ Ready (Inspect) Visit Preview Mar 11, 2025 4:09pm


/**
* Display CCTP routes:
* - Mainnet/Arb1, Sepolia/ArbSepolia
Copy link
Contributor

Choose a reason for hiding this comment

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

For USDC transfers, I don't think now we need the dialog for selection of Canonical vs CCTP bridge after choosing the route in summary panel.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Also should we have a permanent route option for opening 3rd party bridge's dialog?

* - Mainnet/Arb1, Sepolia/ArbSepolia
* - Native USDC on Arbitrum, USDC on L1
* Display layerzero:
* - Mainnet/Arb1, Sepolia/ArbSepolia
Copy link
Contributor

Choose a reason for hiding this comment

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

LayerZero OFT v2 is only available for mainnets at the moment, not Sepolia/ArbSepolia.

if (isOftV2Transfer) {
return (
<Wrapper>
<LayerZeroRoute onRouteSelected={onRouteSelected} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend this be named as OftV2Route and not layerzero since OFT v1 is quite different from this, and it will be confusing if we even plan on integrating it.

<div
className={twMerge(
'group cursor-pointer rounded border border-[#ffffff33] text-white transition-colors',
selected && 'border-white'
Copy link
Contributor

Choose a reason for hiding this comment

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

Design nit: I think the selected route option should stand out a bit more because we want user to select the absolute correct route before transferring. It should maybe have a selected/checked badge and a slightly different color and a thicker border.

image

@@ -1188,11 +1197,9 @@ export function TransferPanel() {
)}
>
<TransferPanelMain />
<Routes onRouteSelected={setSelectedRoute} />
Copy link
Contributor

Choose a reason for hiding this comment

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

I think when there is only 1 route option, it should show selected by default (currently it isn't showing which route is selected on load), and if we're not selecting any route by default then the move funds button should be disabled.

Also, if multiple routes are an option then are we selecting the first route by default?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By default no route are selected

Copy link
Contributor

@brtkx brtkx Feb 25, 2025

Choose a reason for hiding this comment

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

That was my thought as well, seems like an UX issue. No routes are selected by default, but the transfer button is enabled so there is a default route. IMHO a route should be always selected in that case, or the transfer button should be disabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, some logic was missing, it's fixed now. Button is disabled until we select a route. But it make sense to auto select a route if we have only one, good point

@@ -0,0 +1,22 @@
import { create } from 'zustand'

export type RouteType = 'arbitrum' | 'layerzero' | 'cctp'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be an enum, and this should come from bridge-sdk and should be linked with transfer-type otherwise we're creating a lot of disjointed types trying to express the same information?

Eg. in bridge-sdk, we have TransferType = ${Asset}_${TxType} | 'cctp' | 'oftV2'.
At the very least the Route names and Transfer type names should be consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TransferType carries a lot of information that we don't need, we have only 3 types here. And TransferType has 8. Also in ArbitrumRoute for example we would have a transferType that might be different than the actual transfer

{isLoadingGasEstimate ? (
<Loader size="small" color="white" />
) : gasCost ? (
formatAmount(BigNumber.from(gasCost), {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we okay with removing the feature of showing the fiat/dollar value of the gas fee? IMHO it's a pretty useful feature.

* If source and destination chains are using the same currency, we display combined cost for both child and parent chain.
* If they use a different currency, we display cost for child chain only
*/
const gasCost =
Copy link
Contributor

Choose a reason for hiding this comment

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

Might need to revisit the gas logic here because I get N/A being displayed intermittently.
(This was while transferring 0.00001 XAI from ArbOne to XAI chain, but I saw this issue in other cases as well)

image

Original
image

Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, we should also cover existing cases where ETH + native gas token gas is shown together

height={15}
alt="duration"
/>
<span className="ml-1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also add the old tooltip and the bolt icon for fast withdrawals?

Old source:

{fastWithdrawalActive && (
<div className="flex items-center">
<Tooltip
content={
'Fast Withdrawals relies on a committee of validators. In the event of a committee outage, your withdrawal falls back to the 7 day challenge period secured by Arbitrum Fraud Proofs.'
}
>
<InformationCircleIcon className="h-3 w-3 sm:ml-1" />
</Tooltip>
<div className="ml-1 flex space-x-0.5 text-[#FFD000]">
<Image src={LightningIcon} alt="Lightning Icon" />
<span className="font-normal">FAST</span>
</div>
</div>
)}

Ref image:
image

* If source and destination chains are using the same currency, we display combined cost for both child and parent chain.
* If they use a different currency, we display cost for child chain only
*/
const gasCost =
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed offline, we should also cover existing cases where ETH + native gas token gas is shown together

*/
const isUsdcTransfer = isTokenNativeUSDC(token?.address)
const overrideToken = isDepositMode ? bridgedUsdcToken : nativeUsdcToken
const duration =
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could we rename duration to durationMs so it's easier to understand right away

<span className="ml-1">{bridge}</span>
</div>
</div>
{tag && (
Copy link
Contributor

Choose a reason for hiding this comment

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

<div className="flex flex-col">
<span>You will receive:</span>
<div className="flex items-center text-lg">
{'logoURI' in token ? (
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 use this instead? It also handles native token logos. You don't have to specify logoURI with this one, by default it will use the selected token logo.

https://github.com/OffchainLabs/arbitrum-token-bridge/blob/9ab310208b722331fc81756d6bfb0ed767538c13/packages/arb-token-bridge-ui/src/components/TransferPanel/TokenLogo.tsx

@@ -135,7 +135,9 @@ export function getWithdrawalConfirmationDate({
return dayjs(createdAt).add(confirmationTimeInSeconds, 'second')
}

function getWithdrawalDuration(tx: MergedTransaction) {
export function getWithdrawalDuration(
tx: Pick<MergedTransaction, 'createdAt' | 'sourceChainId'>
Copy link
Contributor

Choose a reason for hiding this comment

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

We could also change tx to { createdAt, sourceChainId }

@brtkx
Copy link
Contributor

brtkx commented Feb 26, 2025

Showing routes has a delay after typing amount. Maybe we could have some loading state?

Screen.Recording.2025-02-26.at.16.50.12.mov

@fionnachan
Copy link
Member

  • did we intentionally take out the ability to search a token by its child chain address on the token search panel on this PR?
  • if a user types a figure to the amount input when it's not USDC or ETH, the app crashes

Comment on lines +80 to +96
function getGasCostAndToken({
childChainNativeCurrency,
parentChainNativeCurrency,
gasSummaryStatus,
estimatedChildChainGasFees,
estimatedParentChainGasFees,
isDepositMode,
selectedToken
}: {
childChainNativeCurrency: NativeCurrency
parentChainNativeCurrency: NativeCurrency
gasSummaryStatus: UseGasSummaryResult['status']
estimatedChildChainGasFees: UseGasSummaryResult['estimatedChildChainGasFees']
estimatedParentChainGasFees: UseGasSummaryResult['estimatedParentChainGasFees']
isDepositMode: boolean
selectedToken: ERC20BridgeToken | null
}): {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is accepting lots of parameters rather than being a hook with lots of internal calls to hook to make it easier to test

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.

4 participants