-
Notifications
You must be signed in to change notification settings - Fork 192
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
refactor: abstract hooks from transfer panel #1838
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
const networkConnectionWarningToast = () => | ||
warningToast( | ||
<> | ||
Network connection issue. Please contact{' '} | ||
<ExternalLink href={GET_HELP_LINK} className="underline"> | ||
support | ||
</ExternalLink> | ||
. | ||
</>, | ||
{ autoClose: false } | ||
) |
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.
we prompt users to switch to the correct chain so this is no longer needed
errorMessage?: TransferReadinessRichErrorMessage | string | ||
}) { | ||
const [{ amount }] = useArbQueryParams() |
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.
minor clean up to not pass this in as prop
|
||
return false | ||
}, [isArbitrumOne, isArbitrumSepolia, isDepositMode, selectedToken]) | ||
const isCctpTransfer = useIsCctpTransfer() |
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.
logic is unchanged but moved into the hook
const connectedChainId = networks.sourceChain.id | ||
const sourceChainEqualsConnectedChain = sourceChainId === connectedChainId | ||
|
||
// Transfer is invalid if the connected chain doesn't mismatches source-destination chain requirements | ||
const depositNetworkConnectionWarning = | ||
isDepositMode && | ||
(!sourceChainEqualsConnectedChain || isConnectedToOrbitChain.current) | ||
const withdrawalNetworkConnectionWarning = | ||
!isDepositMode && !sourceChainEqualsConnectedChain | ||
if ( | ||
depositNetworkConnectionWarning || | ||
withdrawalNetworkConnectionWarning | ||
) { | ||
return networkConnectionWarningToast() | ||
} |
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 no longer needed and because we prompt users to switch to the correct chain at transfer
import { useIsConnectedToOrbitChain } from '../../../hooks/useIsConnectedToOrbitChain' | ||
import { useIsConnectedToArbitrum } from '../../../hooks/useIsConnectedToArbitrum' | ||
|
||
export function useTransferRequiresChainSwitch() { |
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.
what if this was a return value from useTransferReadiness
?
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 deleted the hook and used a simple condition const isConnectedToTheWrongChain = chainId !== latestNetworks.current.sourceChain.id
instead
also abstracted the relevant code from transferCctp
and transfer
into a reusable function
selectedToken && warningTokens[selectedToken.address.toLowerCase()] | ||
if (warningToken) { | ||
const description = getWarningTokenDescription(warningToken.type) | ||
warningToast( |
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 don't think we should be showing toasts from this method, it should only tell us if it's warning token or not.
if (isWarningToken()) {
showWarningTokenToast(warningToken: WarningToken) {
// here use warningToken.address and warningToken.type
}
}
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.
With this we can just refactor this method to:
function isWarningToken(token: ERC20BridgeToken | null) {
if (!token) {
return false
}
return typeof warningTokens[token.address.toLowerCase()] !== 'undefined'
}
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.
Also this could probably be a part of TokenUtils
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.
warningTokens
is from useAppState()
so unless further refactoring is done, it can't be moved out of a hook or component, so i wouldn't move it to the utils file
|
||
async function isTransferAllowed(isCctp: boolean) { |
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 just get isCctp
directly instead of passing the prop
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.
Also this method uses a lot of hook values directly, are we sure we are not running into stale values or race conditions here?
const hasBothSigners = parentSigner && childSigner | ||
if (isEOA && !hasBothSigners) { | ||
throw signerUndefinedError | ||
if (isConnectedToTheWrongChain) { |
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 don't know about this entire code block, I'd expect isTransferAllowed
to be pure. I'd have the network switching logic in its own method.
isTeleportMode | ||
}) | ||
if (destinationAddressError) { | ||
console.error(destinationAddressError) |
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 don't think this is needed, function like this shouldn't produce side effects like this imho
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.
We can just
return typeof destinationAddressError !== null
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.
But also not sure if we need a helper, we could just use useState
for this, and then we can use useMemo
for isTransferAllowed
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 refactored the code for destination address error across files
throw 'Signer is undefined' | ||
const signer = sourceChainSigner | ||
|
||
if (!(await isTransferAllowed(true))) { |
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 boolean prop is not very clear, but maybe it will be gone if we refactor the method, I don't think we need props here at all, this could probably be done with useMemo
Closes FS-744