-
Notifications
You must be signed in to change notification settings - Fork 17
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(tangle-dapp): Bridge revamp & Router integration #2677
Conversation
✅ Deploy Preview for tangle-dapp ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for tangle-cloud ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
aebc553
to
17b31d7
Compare
tokenType: EVMTokenEnum; | ||
bridgeType: EVMTokenBridgeEnum; | ||
address: AddressType; | ||
abi: any; |
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 think we have a type for ABI: Abi
from viem
. Should avoid using any
type whenever possible to reduce future logic bugs
@@ -81,7 +82,9 @@ export type LocalStorageValueOf<T extends LocalStorageKey> = | |||
? LiquidStakingTableData | |||
: T extends LocalStorageKey.ONBOARDING_MODALS_SEEN | |||
? OnboardingPageKey[] | |||
: never; | |||
: T extends LocalStorageKey.EVM_TOKEN_BALANCES | |||
? Record<string, any> |
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.
Is there a concrete structure for the EVM token balances? Should avoid using any
as much as possible
{/* <head> | ||
<script src="https://unpkg.com/react-scan/dist/auto.global.js" async /> | ||
</head> */} |
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.
Could you tell me more about this and why it was removed/commented out?
sourceAmount: sendingAmount?.toString() ?? '', | ||
destinationAmount: receivingAmount?.toString() ?? '', |
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 avoid defaulting to empty strings to circumvent the type system, this will lead to logic bugs. Instead I think it may be better to handle the undefined
case directly
notificationApi({ | ||
message: 'Transfer failed', | ||
variant: 'error', | ||
}); |
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.
Would be better if the toast shows the error message as a description/secondary message, otherwise the user will be forced to look on the console to debug the issue
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.
You can use the ensureError
utility function to convert the unknown error into Error type then get its .message
/> | ||
|
||
<div className="flex flex-col gap-1"> | ||
<Typography | ||
variant="h5" | ||
fw="bold" | ||
className="block cursor-default text-mono-200 dark:text-mono-0" | ||
className="cursor-default text-mono-200 dark:text-mono-0 flex items-center gap-1" |
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.
Why flex and gap here? Is asset.symbol
composed of various parts or components?
const actionButtonLoadingText = useMemo( | ||
() => (isRouterQuoteLoading ? 'Fetching bridge fee...' : ''), | ||
[isRouterQuoteLoading], | ||
); |
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.
No useMemo
required here 👍🏼
const actionButtonIsLoading = useMemo( | ||
() => isRouterQuoteLoading, | ||
[isRouterQuoteLoading], | ||
); |
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.
No useMemo
required here 👍🏼
const tokenExplorerUrl = makeExplorerUrl( | ||
selectedChainExplorerUrl?.url ?? '', | ||
token.address, | ||
'address', | ||
'web3', | ||
); |
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.
Will this call work as intended with an empty string for the first argument?
Co-authored-by: Yurixander <[email protected]>
New PR here - #2698 |
Summary of changes
Proposed area of change
apps/tangle-dapp
apps/tangle-cloud
libs/tangle-shared-ui
libs/webb-ui-components
Associated issue(s)
Screenshots
Screen Recording
CleanShot.2024-12-11.at.10.38.29.mp4