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

usame-algan · 2024-09-30T09:32:21Z

What it solves

Resolves SW-236

How this PR fixes it

Uses getCompatibilityFallbackHandlerDeployments to get all deployment addresses in case there is a canonical and eip155 deployment and checks those addresses to decide whether its an official deployment or not

How to test it

Open a Safe on a network with multiple deployments e.g. oeth:0xa4Fea8eF4a5dc35C5627Da7784b065934a5693a7
Go to the settings page and observe no unofficial fallback handler warning

Screenshots

Checklist

I've tested the branch on mobile 📱
I've documented how it affects the analytics (if at all) 📊
I've written a unit/e2e test for it (if applicable) 🧑‍💻

notion-workspace · 2024-09-30T09:32:26Z

Unofficial fallback handler message for cloned safe

github-actions · 2024-09-30T09:32:36Z

Branch preview

✅ Deploy successful!

Website:
https://official_fallback_handler--walletweb.review.5afe.dev/home?safe=eth:0xA77DE01e157f9f57C7c4A326eeE9C4874D0598b6

Storybook:
https://official_fallback_handler--walletweb.review.5afe.dev/storybook/

github-actions · 2024-09-30T09:38:32Z

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

⚠️ Global Bundle Size Increased

Page	Size (compressed)
`global`	`1004.22 KB` (🟡 +49.42 KB)

Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Fifteen Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page	Size (compressed)	First Load
`/`	`511 B` (🟢 -24.37 KB)	1004.72 KB
`/address-book`	`26.09 KB` (🟡 +5 B)	1.01 MB
`/apps/open`	`54.7 KB` (-1 B)	1.03 MB
`/balances`	`30.95 KB` (🟡 +14 B)	1.01 MB
`/balances/nfts`	`19.18 KB` (-1 B)	1023.4 KB
`/home`	`60.54 KB` (🟡 +839 B)	1.04 MB
`/new-safe/advanced-create`	`36.49 KB` (🟡 +1.36 KB)	1.02 MB
`/new-safe/create`	`35.75 KB` (🟡 +1.36 KB)	1.02 MB
`/new-safe/load`	`16.41 KB` (🟡 +10 B)	1020.63 KB
`/settings/modules`	`9.82 KB` (🟡 +38 B)	1014.04 KB
`/settings/notifications`	`27.03 KB` (-1 B)	1.01 MB
`/settings/setup`	`35.98 KB` (🟡 +5 B)	1.02 MB
`/transactions/tx`	`21.07 KB` (-2 B)	1 MB
`/welcome`	`6.8 KB` (🟢 -1 B)	1011.02 KB
`/welcome/accounts`	`379 B` (🟢 -1 B)	1004.59 KB

Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

github-actions · 2024-09-30T09:41:26Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	78.07% (-0% 🔻)	12416/15904
🔴	Branches	58.18% (+0.03% 🔼)	3231/5553
🟡	Functions	65.48%	1958/2990
🟡	Lines	79.56% (-0.01% 🔻)	11205/14084

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / useTxPendingStatuses.ts	83.64% (-0.91% 🔻)	68.33% (-1.67% 🔻)	75%	92.63% (-1.05% 🔻)

Test suite run success

1550 tests passing in 206 suites.

Report generated by 🧪jest coverage report action from 371afbd

schmanu · 2024-09-30T09:49:42Z

src/components/settings/FallbackHandler/index.tsx

@@ -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


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?

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

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.

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

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.

Also added some tests for the twap fallback handler message

…d adjust check

…cial-fallback-handler

schmanu · 2024-10-01T08:29:43Z

src/features/swap/helpers/utils.ts

@@ -24,6 +24,9 @@ function asDecimal(amount: number | bigint, decimals: number): number {

 export const TWAP_FALLBACK_HANDLER = '0x2f55e8b20D0B9FEFA187AA7d00B6Cbe563605bF5'

+// https://github.com/cowprotocol/composable-cow/blob/main/networks.json
+export const TWAP_FALLBACK_HANDLER_NETWORKS = ['1', '100', '11155111', '42161']


Nit: We could also use a mapping chainId -> address here. But maybe we can change it if there are ever multiple addresses.

usame-algan requested a review from schmanu September 30, 2024 09:32

fix: Check all fallbackHandler deployments

e98677a

usame-algan force-pushed the official-fallback-handler branch from eadaab2 to e98677a Compare September 30, 2024 09:32

schmanu reviewed Sep 30, 2024

View reviewed changes

fix: Add list of networks the twap fallback handler is deployed at an…

82803f4

…d adjust check

usame-algan requested a review from schmanu September 30, 2024 13:12

usame-algan added 2 commits September 30, 2024 15:17

fix: Add tests for twap fallback handler

328f991

Merge remote-tracking branch 'origin/epic/multichain-safes' into offi…

371afbd

…cial-fallback-handler

schmanu approved these changes Oct 1, 2024

View reviewed changes

usame-algan merged commit e8e0f67 into epic/multichain-safes Oct 1, 2024
11 of 13 checks passed

usame-algan deleted the official-fallback-handler branch October 1, 2024 09:45

github-actions bot locked and limited conversation to collaborators Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

usame-algan commented Sep 30, 2024

notion-workspace bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024 •

edited

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading

schmanu Sep 30, 2024

usame-algan Sep 30, 2024

schmanu Sep 30, 2024

schmanu Sep 30, 2024

usame-algan Sep 30, 2024 •

edited

Loading

usame-algan Sep 30, 2024

schmanu Oct 1, 2024

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

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

Conversation

usame-algan commented Sep 30, 2024

What it solves

How this PR fixes it

How to test it

Screenshots

Checklist

notion-workspace bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024 • edited Loading

Branch preview

github-actions bot commented Sep 30, 2024 • edited Loading

📦 Next.js Bundle Analysis for safe-wallet-web

⚠️ Global Bundle Size Increased

Fifteen Pages Changed Size

github-actions bot commented Sep 30, 2024 • edited Loading

Coverage report

Test suite run success

schmanu Sep 30, 2024

Choose a reason for hiding this comment

usame-algan Sep 30, 2024

Choose a reason for hiding this comment

schmanu Sep 30, 2024

Choose a reason for hiding this comment

schmanu Sep 30, 2024

Choose a reason for hiding this comment

usame-algan Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

usame-algan Sep 30, 2024

Choose a reason for hiding this comment

schmanu Oct 1, 2024

Choose a reason for hiding this comment

github-actions bot commented Sep 30, 2024 •

edited

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading

usame-algan Sep 30, 2024 •

edited

Loading