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.

Sign up for GitHub

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

[Multichain] fix: Check all fallbackHandler deployments [SW-236] #4281

Merged
merged 4 commits into from
Oct 1, 2024
Merged

[Multichain] fix: Check all fallbackHandler deployments [SW-236] #4281

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e98677a
fix: Check all fallbackHandler deployments
usame-algan Sep 30, 2024
82803f4
fix: Add list of networks the twap fallback handler is deployed at an…
usame-algan Sep 30, 2024
328f991
fix: Add tests for twap fallback handler
usame-algan Sep 30, 2024
371afbd
Merge remote-tracking branch 'origin/epic/multichain-safes' into offi…
usame-algan Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/components/settings/FallbackHandler/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { TWAP_FALLBACK_HANDLER } from '@/features/swap/helpers/utils'
import { getCompatibilityFallbackHandlerDeployments } from '@safe-global/safe-deployments'
import NextLink from 'next/link'
import { Typography, Box, Grid, Paper, Link, Alert } from '@mui/material'
import semverSatisfies from 'semver/functions/satisfies'
Expand All @@ -7,7 +8,6 @@ import type { ReactElement } from 'react'

import EthHashInfo from '@/components/common/EthHashInfo'
import useSafeInfo from '@/hooks/useSafeInfo'
import { getFallbackHandlerContractDeployment } from '@/services/contracts/deployments'
import { HelpCenterArticle } from '@/config/constants'
import ExternalLink from '@/components/common/ExternalLink'
import { useTxBuilderApp } from '@/hooks/safe-apps/useTxBuilderApp'
Expand All @@ -22,11 +22,12 @@ export const FallbackHandler = (): ReactElement | null => {

const supportsFallbackHandler = !!safe.version && semverSatisfies(safe.version, FALLBACK_HANDLER_VERSION)

const fallbackHandlerDeployment = useMemo(() => {
if (!chain) {
const fallbackHandlerDeployments = useMemo(() => {
if (!chain || !safe.version) {
return undefined
}
return getFallbackHandlerContractDeployment(chain, safe.version)

return getCompatibilityFallbackHandlerDeployments({ network: chain?.chainId, version: safe.version })
}, [safe.version, chain])

if (!supportsFallbackHandler) {
Expand All @@ -35,7 +36,8 @@ export const FallbackHandler = (): ReactElement | null => {

const hasFallbackHandler = !!safe.fallbackHandler
const isOfficial =
hasFallbackHandler && safe.fallbackHandler?.value === fallbackHandlerDeployment?.networkAddresses[safe.chainId]
safe.fallbackHandler &&
fallbackHandlerDeployments?.networkAddresses[safe.chainId].includes(safe.fallbackHandler.value)
const isTWAPFallbackHandler = safe.fallbackHandler?.value === TWAP_FALLBACK_HANDLER
Copy link
Member

Choose a reason for hiding this comment

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

I know you did not introduce this. But this check is too simplified. There are many chains where no Extensible Fallback handler is deployed e.g. Base.
Maybe we should only allow this on Mainnet, Gnosis Chain, Sepolia and Arbitrum for now as those are the chains where CoW exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't isTWAPFallbackHandler be false on those other chains where its not deployed which is the expected behaviour?

Copy link
Member

Choose a reason for hiding this comment

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

But it just compares an hard coded address:

export const TWAP_FALLBACK_HANDLER = '0x2f55e8b20D0B9FEFA187AA7d00B6Cbe563605bF5'

There are no checks that this address is a contract on a chain so this will always be true when you replay a twap Safe for instance in case someone sets one up with the twap fallback handler.

Copy link
Member

Choose a reason for hiding this comment

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

These are the networks where that fallback handler is deployed:
https://github.com/cowprotocol/composable-cow/blob/main/networks.json

Copy link
Member Author

@usame-algan usame-algan Sep 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ah I see. Then lets harden the condition by also checking that the chainId matches one of the deployed networks. I hard-coded the list of those networks provided by the link.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also added some tests for the twap fallback handler message


const warning = !hasFallbackHandler ? (
Expand Down Expand Up @@ -101,7 +103,7 @@ export const FallbackHandler = (): ReactElement | null => {
{safe.fallbackHandler && (
<EthHashInfo
shortAddress={false}
name={safe.fallbackHandler.name || fallbackHandlerDeployment?.contractName}
name={safe.fallbackHandler.name || fallbackHandlerDeployments?.contractName}
address={safe.fallbackHandler.value}
customAvatar={safe.fallbackHandler.logoUri}
showCopyButton
Expand Down
Loading